GH-1781: Configure ObjectMapper in JsonSerializer/JsonDeserializer #1782

cnygardtw · 2021-04-22T16:23:23Z

Add ObjectMapperCustomizer interface for customizing local ObjectMapper
Implement ObjectMapperCustomizer in JsonSerializer / JsonDeserializer
call customizeObjectMapper() in ctor so ObjectWriter is properly constructed
added tests / docs

…Deserializer Fixes spring-projectsGH-1781 (spring-projects#1781) * Add ObjectMapperCustomizer interface for customizing local ObjectMapper * Implement ObjectMapperCustomizer in JsonSerializer / JsonDeserializer * call customizeObjectMapper() in ctor so ObjectWriter is properly constructed * added tests / docs

pivotal-issuemaster · 2021-04-22T16:23:26Z

@cnygardtw Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster · 2021-04-22T16:26:02Z

@cnygardtw Thank you for signing the Contributor License Agreement!

artembilan

Thank you for contribution!

Please, find some review on code lines: I really have some doubts about proposed solution...

artembilan · 2021-04-22T17:02:13Z

spring-kafka-docs/src/main/asciidoc/kafka.adoc

+====== Customize the ObjectMapper
+
+Occasionally, the only way to configure the way information is serialized is by customizing the ObjectMapper used in (de)serialization.
+Because Kafka uses its own ObjectMapper separate from the normal Spring-Boot ObjectMapper, customizing the ObjectMapper typically involved (re)implementing various Kafka factories.


Please, call the trade mark as Apache Kafka.

artembilan · 2021-04-22T17:03:41Z

spring-kafka-docs/src/main/asciidoc/kafka.adoc

+[[serdes-customize-object-mapper]]
+====== Customize the ObjectMapper
+
+Occasionally, the only way to configure the way information is serialized is by customizing the ObjectMapper used in (de)serialization.


The ObjectMapper should be wrapped into a code snippet using backtick symbol

artembilan · 2021-04-22T17:05:13Z

spring-kafka-docs/src/main/asciidoc/kafka.adoc

+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.SerializationFeature;
+
+import org.springframework.kafka.support.serializer.JsonSerializer;


I think we can live without imports and package declarations in the samples shown in the docs...

artembilan · 2021-04-22T17:07:41Z

spring-kafka/src/main/java/org/springframework/kafka/support/serializer/JsonDeserializer.java

+	 * @return configured (or replaced) ObjectMapper instance
+	 * @since 2.7.1
+	 */
+	public ObjectMapper customizeObjectMapper(ObjectMapper objectMapper) {


I don't think this is what @garyrussell has meant in the issue.
We can't call non-final methods from the ctors.
Probably the point was to inject an ObjectMapperCustomizer into such a serialize impl...

Good catch @artembilan ; yes a Function<ObjectMapper, ObjectMapper> setter would be safer.

The alternative is to stop creating the ObjectWriter in the ctor and instead construct in the serialize() function (which is what JsonDeserializer does currently). If this were done, then there's probably a better way to just hook into the configure() function. The root cause was that the ObjectWriter was private, and there seemed no other way that wasn't brittle and likely to break in future versions. If that's the preferred change, I can re-look at the implementation.

As an alternative, I added Map<String, ?> to the customize function, and call it first thing from the configure() function instead of calling in the constructor. I'm assuming that everywhere a serializer is instantiated, the configure() function will get called appropriately. The other bit is to create the writer in serialize(). updated patch shortly.

Thanks; we have a day off tomorrow so it will probably be next week before we continue.

Thanks again.

garyrussell

Thanks for the contribution!

Just some minor comments.

garyrussell · 2021-04-22T17:14:55Z

spring-kafka/src/main/java/org/springframework/kafka/support/serializer/JsonSerializer.java

+	 * @return configured (or replaced) ObjectMapper instance
+	 * @since 2.7.1
+	 */
+	public ObjectMapper customizeObjectMapper(ObjectMapper objectMapper) {


This can be a default method on the interface.

Please change all sinces to 2.5.13; we will be backporting this to 2.6, 2.5.

That was my original thought, but I took guidance from JsonTypeResolver. If we provide default implementation, then @FunctionalInterface goes away. Is that what you intend?

I don't think this needs to be a FunctionalInterface the type resolver can be used as lambda expression, or method reference:

spring-kafka/spring-kafka/src/test/java/org/springframework/kafka/support/serializer/JsonSerializationTests.java

Line 371 in bbc5003

.typeResolver(JsonSerializationTests::fooBarJavaTypeForTopic);

This is just a super interface for the (de)serializer.

garyrussell · 2021-04-22T17:25:08Z

spring-kafka/src/main/java/org/springframework/kafka/support/serializer/JsonDeserializer.java

+	 * @return configured (or replaced) ObjectMapper instance
+	 * @since 2.7.1
+	 */
+	public ObjectMapper customizeObjectMapper(ObjectMapper objectMapper) {


See comment in serializer - pull up to interface.

garyrussell · 2021-04-22T17:27:20Z

spring-kafka-docs/src/main/asciidoc/kafka.adoc

+Because Kafka uses its own ObjectMapper separate from the normal Spring-Boot ObjectMapper, customizing the ObjectMapper typically involved (re)implementing various Kafka factories.
+Since 2.7.1, a new interface `ObjectMapperCustomizer` has been introduced and implemented by the `JsonSerializer<T>` and `JsonDeserializer<T>` classes.
+This makes it simpler to configure the ObjectMapper without losing the convenience of the Spring property configuration capabilities.
+


Suggested rewording - the Apache Foundation require us to mention Apache whenever we reference Kafka.

$ git diff diff --git a/spring-kafka-docs/src/main/asciidoc/kafka.adoc b/spring-kafka-docs/src/main/asciidoc/kafka.adoc index 2b1cd6a2..faa0a1e1 100644 --- a/spring-kafka-docs/src/main/asciidoc/kafka.adoc +++ b/spring-kafka-docs/src/main/asciidoc/kafka.adoc @@ -3795,6 +3795,7 @@ Also since version 2.7 `ConsumerPartitionPausedEvent` and `ConsumerPartitionResu [[serdes]] ==== Serialization, Deserialization, and Message Conversion +[[serdes-overview]] ===== Overview Apache Kafka provides a high-level API for serializing and deserializing record values as well as their keys. @@ -3924,8 +3925,9 @@ You can revert to the previous behavior by setting the `removeTypeHeaders` prope ====== Customize the ObjectMapper Occasionally, the only way to configure the way information is serialized is by customizing the ObjectMapper used in (de)serialization. -Because Kafka uses its own ObjectMapper separate from the normal Spring-Boot ObjectMapper, customizing the ObjectMapper typically involved (re)implementing various Kafka factories. -Since 2.7.1, a new interface `ObjectMapperCustomizer` has been introduced and implemented by the `JsonSerializer<T>` and `JsonDeserializer<T>` classes. +When instantiated by Apache Kafka, the Spring JSON Serializer and Deserializer use a locally declared `ObjectMapper`. +The only way to customize the `ObjectMapper` was to inject instances into the producer and consumer factories as discussed <<serdes-overview,here>> rather than by using properties. +Since version 2.5.13, a new interface `ObjectMapperCustomizer` has been introduced and implemented by the `JsonSerializer<T>` and `JsonDeserializer<T>` classes. This makes it simpler to configure the `ObjectMapper` without losing the convenience of the Spring property configuration capabilities. The following is an example of how to provide a simple customized serializer class (derived from `JsonSerializer<T>`) to do the configuration. @@ -3958,7 +3960,7 @@ public class CustomJsonSerializer<T> extends JsonSerializer<T> { ---- ==== -Then in your properties file, configure your `CustomJsonSerializer<T>` class as the `value-serializer`: +Then in your Spring Boot properties file, for example, configure your `CustomJsonSerializer<T>` class as the `value-serializer`: ==== [source, properties]

- delay creation of objectreader/writer - add local cache object to minimize construction - shift objectmapper configuration to configure() function - pass configuration map to objectmapper configuration

artembilan · 2021-04-26T15:00:53Z

spring-kafka/src/main/java/org/springframework/kafka/support/serializer/JsonDeserializer.java

@@ -718,4 +717,27 @@ private JsonTypeResolver buildTypeResolver(String methodProperty) {
 		};
 	}

+	protected class CachedObjectReader {


Why do we need this?
Why just regular volatile property which we set first time call in that deserialize() not enough for us?
Looks like a this.objectMapper and this.targetType always produce the same state, so we even don't need to worry about synchronization.

The problems are:

can't create ObjectWriter in ctor, because there's no way to provide customization of ObjectMapper in ctor

can't create ObjectWriter in configure() function, because unit tests indicate it is not guaranteed that configure() will get called

construct ObjectWriter at point of need, and possibly suffer performance issues

Therefore making no assumptions about how costly it is to construct an ObjectWriter. Considering that the current serializer constructs the ObjectWriter just once in the constructor, I took the conservative approach and tried to at least cache the object so that if conditions are right it would still only get constructed once. On the other hand, hashcode does have a cost, so if you don't think it matters to performance, I can pull it out.

OK. Probably you just didn't see through my suggestion:

private volatile ObjectReader objectReader; ... public T deserialize(...) { if (this.objectReader == null) { this.objectReader = this.objectMapper.readerFor(this.targetType); } }

Does it make sense?
Technically your caching is just the same, but without volatile.
And that's why I don't see a reason in that extra instance if we just can deal with volatile and plain if().

Sorry for not being clear enough from the beginning.

That is certainly simpler, but I'm not familiar enough with all the supported code flows to decide if it's sufficient or not.
Are there any flows where a serializer might get reconfigured? With this code, once the ObjectReader is constructed, changes to the ObjectMapper will not have any effect on the serialization (which was the original reason for the PR). If that's an acceptable constraint, I'll make the change.

I don't think so: you call customizeObjectMapper() from the configure(Map<String, ?> configs, boolean isKey) which is a Kafka Client API, which is called once from Kafka Client instance, e.g.:

if (valueSerializer == null) { this.valueSerializer = config.getConfiguredInstance(ProducerConfig.VALUE_SERIALIZER_CLASS_CONFIG, Serializer.class); this.valueSerializer.configure(config.originals(Collections.singletonMap(ProducerConfig.CLIENT_ID_CONFIG, clientId)), false); } else { config.ignore(ProducerConfig.VALUE_SERIALIZER_CLASS_CONFIG); this.valueSerializer = valueSerializer; }

And you see: they also set config.ignore(ProducerConfig.VALUE_SERIALIZER_CLASS_CONFIG);, which means do not look into that property any more. So, any changes to the ObjectMapper at runtime are not going to be visible anyway.

Sounds reasonable?

On the flip side, changes to the ObjectMapper may or may not be reflected in the hashcode. If not, then the CachedObjectReader won't work any better (or worse) than a volatile.

ok, will update.

artembilan

Looking closely to an issue I would say that it is probably better just to reject all of these changes in favor of plain Java inheritance:

public class CustomJsonSerializer extends JsonSerializer<Object> {

	public CustomJsonSerializer() {
		super(customizedObjectMapper());
	}

	private static ObjectMapper customizedObjectMapper() {
		ObjectMapper mapper = JacksonUtils.enhancedObjectMapper();
		mapper.disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS);
		return mapper;
	}

}

and this is going to work as expected for both Java configuration and Apache Kafka properties setting.

What am I missing?

cnygardtw · 2021-04-26T18:11:09Z

not sure, I'll try that also.

cnygardtw · 2021-04-26T19:08:01Z

That worked as far as my tests required it. Reject the PR. Do you perhaps want a different PR with just documentation updates?

artembilan · 2021-04-26T19:13:46Z

Do you perhaps want a different PR with just documentation updates?

I wonder why, since we talk just about a regular Java inheritance.
But that still probably worth it if you have a time to contribute: with that in docs we may not have similar discussions in the future...

Thanks for understanding!

cnygardtw · 2021-04-26T19:18:26Z

Point me to the docs, and I'll reward you with a facepalm ;) As for docs, I was thinking something in the tips section. There's quite a lot of related SO questions/answers, and the answers are all over the place, so this might help resolve some confusion.

And no worries, a solution that doesn't require a PR is a solution I'm happy with.

artembilan · 2021-04-26T19:24:19Z

Sure thing!

This chapter should be a good one for such a simple example: https://docs.spring.io/spring-kafka/docs/current/reference/html/#tips-n-tricks.

Also we won't mind if you add something for custom (de)serialization use-case into this directory: https://github.com/spring-projects/spring-kafka/tree/main/samples

But yeah: different story - deserves different PR!

garyrussell · 2021-04-30T13:34:11Z

@cnygardtw Closing this; I opened this issue #1784

Thanks for your efforts anyway.

artembilan requested changes Apr 22, 2021

View reviewed changes

garyrussell suggested changes Apr 22, 2021

View reviewed changes

updates per code review comments

1de6873

- delay creation of objectreader/writer - add local cache object to minimize construction - shift objectmapper configuration to configure() function - pass configuration map to objectmapper configuration

cnygardtw requested review from garyrussell and artembilan April 24, 2021 17:24

artembilan requested changes Apr 26, 2021

View reviewed changes

garyrussell closed this Apr 30, 2021

GH-1781: Configure ObjectMapper in JsonSerializer/JsonDeserializer #1782

GH-1781: Configure ObjectMapper in JsonSerializer/JsonDeserializer #1782

Uh oh!

Conversation

cnygardtw commented Apr 22, 2021

Uh oh!

pivotal-issuemaster commented Apr 22, 2021

Uh oh!

pivotal-issuemaster commented Apr 22, 2021

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garyrussell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garyrussell Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

cnygardtw commented Apr 26, 2021

Uh oh!

cnygardtw commented Apr 26, 2021

Uh oh!

artembilan commented Apr 26, 2021

Uh oh!

cnygardtw commented Apr 26, 2021

Uh oh!

artembilan commented Apr 26, 2021

Uh oh!

garyrussell commented Apr 30, 2021

Uh oh!

Uh oh!

garyrussell Apr 22, 2021 •

edited

Loading