minor improvement to DefaultKafkaHeaderMapper #2940
Conversation
| @@ -286,20 +290,19 @@ public void fromHeaders(MessageHeaders headers, Headers target) { | |||
| } | |||
| if (!encodeToJson && valueToAdd instanceof String) { | |||
| target.add(new RecordHeader(key, ((String) valueToAdd).getBytes(getCharset()))); | |||
| className = JAVA_LANG_STRING; | |||
There was a problem hiding this comment.
seems the previous logic has ensured the className is assigned JAVA_LANG_STRING if valueToAdd has been String.
| private Map<String, String> decodeJsonTypes(Headers source) { | ||
| Map<String, String> types = null; | ||
| Map<String, String> types = Collections.emptyMap(); |
There was a problem hiding this comment.
it is common good practise to return empty colletion as opposed to null to avoid NPE as per Effective Java. There is exception, but in this scenario it seems perfectly valid
| } | ||
| catch (IOException e) { | ||
| logger.error(e, () -> "Could not decode json types: " + new String(jsonTypes.value())); | ||
| logger.error(e, () -> "Could not decode json types: " + new String(jsonTypes.value(), StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
json serialization result is always of UTF-8 charset
| @@ -355,8 +358,7 @@ private void populateJsonValueHeader(Header header, String requestedType, Map<St | |||
| } | |||
| catch (IOException e) { | |||
| logger.error(e, () -> | |||
| "Could not decode json type: " + new String(header.value()) + " for key: " | |||
| + header.key()); | |||
| "Could not decode json type: " + requestedType + " for key: " + header.key()); | |||
There was a problem hiding this comment.
seems the intention was type, not header value
| @@ -321,7 +324,7 @@ else if (headerName.equals(KafkaHeaders.LISTENER_INFO) && matchesForInbound(head | |||
| headers.put(headerName, new String(header.value(), getCharset())); | |||
| } | |||
| else if (!(headerName.equals(JSON_TYPES)) && matchesForInbound(headerName)) { | |||
| if (jsonTypes != null && jsonTypes.containsKey(headerName)) { | |||
| if (jsonTypes.containsKey(headerName)) { | |||
| String requestedType = jsonTypes.get(headerName); | |||
There was a problem hiding this comment.
we might well improve perf by using the following pattern:
String jsonType = jsonTypes.get(headerName);
if (jsonType != null) {
... ...
}
for we end up one map query, rather than twice (containsKey() and get()).
| @@ -142,7 +139,7 @@ public DefaultKafkaHeaderMapper(ObjectMapper objectMapper) { | |||
| * @see org.springframework.util.PatternMatchUtils#simpleMatch(String, String) | |||
| */ | |||
| public DefaultKafkaHeaderMapper(String... patterns) { | |||
| this(new ObjectMapper(), patterns); | |||
| this(JacksonUtils.enhancedObjectMapper(), patterns); | |||
There was a problem hiding this comment.
seems we should change above to maintain consistency in the class
|
Thanks, @NathanQingyangXu, for the PR. It is now merged upstream. |
see self-comments for the rationale behind the changes. Feel free to modify and revert back inappropriate change.