Introduce distinct ObjectMappers for RestEasy Reactive / RestEasy Reactive Client #23995

chonton · 2022-02-27T17:44:17Z

Provide three ObjectMappers: unqualified, client, and server that can be independently produced, customized, and injected. This allows different behaviors for the different uses.

geoand · 2022-02-28T06:26:48Z

Thanks a lot for this!

To be honest, I would like to limit the scope of the change.

What I had in mind was to have these separate ObjectMappers only for RESTEasy Reactive and also (and probably more importantly), only leverage the client and server ObjectMapper objects if they have been created. Otherwise, RESTEasy Reactive should use the default ObjectMapper.
The reason is that ObjectMapper is very very heavy (both in terms of heap space used and of time taken to create it), so the common use case where users don't need per-case instances should not suffer any performance penalty just to support the less common case.

Would you be interested in making these changes or do you want to open a new PR?

gsmet · 2022-02-28T10:47:01Z

Yes, we need to be extremely careful about that.

kdubb · 2022-02-28T20:20:58Z

What do you guys think about using JsonMapper instead of ObjectMapper during this change? Or maybe in another PR? As of 2.10, JsonMapper appears to be the preferred system.

gsmet · 2022-02-28T20:32:11Z

Another thing we need to be careful about is backwards compatibility. You might have people relying on the existing behavior so we need to be careful.

chonton · 2022-02-28T21:39:14Z

I've create #24019 to track using JsonMapper

chonton · 2022-03-02T01:12:33Z

Limited the scope of the change per @geoand 's comments

geoand · 2022-03-02T06:14:46Z

Thanks, I'll take a closer look soon.

...steasy/reactive/jackson/runtime/serialisers/FullyFeaturedServerJacksonMessageBodyWriter.java

extensions/jackson/runtime/src/main/java/io/quarkus/jackson/runtime/ClientObjectMapper.java

quarkus-bot · 2022-03-03T01:50:12Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `65a4a35`

Status	Name	Step	Failures	Logs	Raw logs
✖	JVM Tests - JDK 11	`Build`	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 17

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: extensions/smallrye-reactive-messaging-kafka/deployment 
! Skipped: integration-tests/kafka-oauth-keycloak integration-tests/kafka-sasl-elytron integration-tests/kubernetes/quarkus-standard-way-kafka and 2 more

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingWorkingAppPropsTestCase.testContinuousTestingScenario3 - More details - Source on GitHub

java.lang.RuntimeException: 
java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.kafka.client.deployment.DevServicesKafkaProcessor#startKafkaDevService threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed

chonton · 2022-03-03T02:52:20Z

@geoand, Rebased and addressed concerns. I had to move annotations to quarkus-resteasy-reactive-jackson-common module rather than quarkus-resteasy-reactive-jackson module.

Are the qualifier annotations in the package that you expect?

...me/src/main/java/io/quarkus/resteasy/reactive/jackson/common/runtime/ClientObjectMapper.java

...me/src/main/java/io/quarkus/resteasy/reactive/jackson/common/runtime/ServerObjectMapper.java

docs/src/main/asciidoc/rest-json.adoc

geoand

I added a final round of comments.

We'll also need the commits squashed.

Thanks!

...me/src/main/java/io/quarkus/resteasy/reactive/jackson/common/runtime/ClientObjectMapper.java

...me/src/main/java/io/quarkus/resteasy/reactive/jackson/common/runtime/ServerObjectMapper.java

docs/src/main/asciidoc/rest-json.adoc

quarkus-bot · 2022-03-05T20:02:38Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `54ef22d`

Status	Name	Step	Failures	Logs	Raw logs
✖	Initial JDK 11 Build	`Build`	Failures	Logs	Raw logs

Failures

⚙️ Initial JDK 11 Build #

- Failing: extensions/resteasy-reactive/quarkus-resteasy-reactive-jackson-common/runtime 
! Skipped: devtools/bom-descriptor-json docs extensions/oidc-client-reactive-filter/deployment and 31 more

📦 extensions/resteasy-reactive/quarkus-resteasy-reactive-jackson-common/runtime

✖ Failed to execute goal net.revelc.code.formatter:formatter-maven-plugin:2.18.0:validate (default) on project quarkus-resteasy-reactive-jackson-common: File '/home/runner/work/quarkus/quarkus/extensions/resteasy-reactive/quarkus-resteasy-reactive-jackson-common/runtime/src/main/java/io/quarkus/resteasy/reactive/jackson/common/ObjectMapperAudience.java' has not been previously formatted. Please format file (for example by invoking `mvn -f extensions/resteasy-reactive/quarkus-resteasy-reactive-jackson-common/runtime net.revelc.code.formatter:formatter-maven-plugin:2.18.0:format`) and commit before running validation!

…ve Client

geoand · 2022-03-08T07:42:39Z

LGTM, thanks a lot!

@gsmet any more comments on your side?

chonton · 2022-03-14T16:17:43Z

@geoand @gsmet Ready to merge?

geoand · 2022-03-14T16:18:40Z

Let's wait for Guillaume's comments

gsmet

I added some minor suggestions but I have also one major concern we need to sort out. See inline.

...untime/src/main/java/io/quarkus/resteasy/reactive/jackson/common/RestClientObjectMapper.java

docs/src/main/asciidoc/rest-json.adoc

gsmet · 2022-03-21T20:17:23Z

docs/src/main/asciidoc/rest-json.adoc

+[source,java]
+----
+import com.fasterxml.jackson.databind.ObjectMapper;
+import io.quarkus.resteasy.reactive.jackson.common.RestClientObjectMapper;
+import io.quarkus.resteasy.reactive.jackson.common.RestServerObjectMapper;
+
+import javax.inject.Inject;
+
+public class UseFallback {
+
+    private final ObjectMapper serializer;
+
+    @Inject
+    public UseFallback(ObjectMapper mapper,
+            @RestClientObjectMapper Instance<ObjectMapper> clientInstance) {
+        serializer = clientInstance.isUnsatisfied() ? mapper : clientInstance.get();
+    }
+
+}
+----


I'm not sure we really want to document this. These mappers are supposed to be used for a very specific purposes by the RESTEasy Reactive extension so I'm not sure I would document anything except the customization of them.

In any case, if we want to keep that (I recommend we don't), I would put it at the end, not at the start.

Makes sense to remove the fallback in this example. When would someone wish to inject the alternate ObjectMappers into non-quarkus code? Possibly/Probably an edge case.

...untime/src/main/java/io/quarkus/resteasy/reactive/jackson/common/RestServerObjectMapper.java

gsmet · 2022-03-21T20:28:49Z

docs/src/main/asciidoc/rest-json.adoc

+If you want to customize the unqualified, client, and server instances differently, you can use the `@RestClientObjectMapper` and
+`@RestServerObjectMapper` annotations on your `ObjectMapperCustomizer`.  `ObjectMapperCustomizer` instances without any
+qualifier will customize all three `ObjectMapper`s. `ObjectMapperCustomizer` annotated with `@RestClientObjectMapper` will customize
+only the client instance.  `ObjectMapperCustomizer` annotated with `@RestServerObjectMapper` will customize only the server instance.


Hmmmm. With the way it's documented, I would have expected the client and server instances to be created automatically if there are customizers and AFAICS from the code, it's not the case?

I.e. I would have expected the third part of the doc where you have to produce the ObjectMapper yourselves to not be necessary as it's quite brittle and you could definitely get it wrong doing things by yourself.
So basically, I would have produced synthetic beans for these object mappers if the annotations were found on customizers as IIRC, it's not possible to return null if we want to produce a @Singleton.

@geoand does it make sense or am I missing something?

Prior comment from some reviewer asked to remove creation of client and server instances. My quarkus mojo may not be up to the task of creating the beans as part of build.

So we don't want to create the ObjectMappers if they are not needed. My question was: could we create them only if they are required. I think we could do that with synthetic beans but I want to check with @geoand that this makes sense.

Hum. And I'm not really sure this works because IIRC the Client writer is only initialized if the server part is not around: https://github.com/quarkusio/quarkus/blob/main/extensions/resteasy-reactive/rest-client-reactive-jackson/deployment/src/main/java/io/quarkus/rest/client/reactive/jackson/deployment/RestClientReactiveJacksonProcessor.java#L42 .

Also the JacksonBasicMessageBodyReader that you haven't changed and should probably be changed is shared between the two.

So my guess is that this needs more thoughts. But I might be missing something as I'm not that familiar with RESTEasy Reactive.

@gsmet so basically you are asking if we can create the "RestServerObjectMapper" if there are customizers for it?

@geoand so that was my initial question yes. As otherwise it's very cumbersome (especially, since you have to apply the customizers yourself).

But... my other question is: I'm pretty sure the current proposal won't work because in some cases we share the writer between client and server and if I'm not mistaken the reader is always shared.
I'm wondering if we could use @ConstrainedTo(RuntimeType.SERVER) / @ConstrainedTo(RuntimeType.CLIENT) but I'm not sure it's implemented in RESTEasy Reactive.
If we can, we would make the reader/writer for client/server totally distinct and this could work.

As otherwise it's very cumbersome (especially, since you have to apply the customizers yourself).

I think it's acceptable price to pay, at least initially until we see if people actually use this feature.

I'm pretty sure the current proposal won't work because in some cases we share the writer between client and server and if I'm not mistaken the reader is always shared.

What reader and writter are you talking about?

@geoand the ones defined here: https://github.com/quarkusio/quarkus/blob/main/extensions/resteasy-reactive/rest-client-reactive-jackson/deployment/src/main/java/io/quarkus/rest/client/reactive/jackson/deployment/RestClientReactiveJacksonProcessor.java#L42
(and in the server part)

If you want to use specific ObjectMappers for server and client, you will need them to always be specific, right?

Good point, I had totally forgot about that! Indeed we would need to always have both and limit them to client and server

Co-authored-by: Guillaume Smet <[email protected]>

quarkus-bot · 2022-03-21T21:49:51Z

Failing Jobs - Building `4b6d35a`

Status	Name	Step	Failures	Logs	Raw logs
✖	Initial JDK 11 Build	`Build`	Failures	Logs	Raw logs

Failures

⚙️ Initial JDK 11 Build #

- Failing: extensions/resteasy-reactive/quarkus-resteasy-reactive-jackson-common/runtime 
! Skipped: devtools/bom-descriptor-json docs extensions/oidc-client-reactive-filter/deployment and 35 more

📦 extensions/resteasy-reactive/quarkus-resteasy-reactive-jackson-common/runtime

✖ Failed to execute goal net.revelc.code.formatter:formatter-maven-plugin:2.18.0:validate (default) on project quarkus-resteasy-reactive-jackson-common: File '/home/runner/work/quarkus/quarkus/extensions/resteasy-reactive/quarkus-resteasy-reactive-jackson-common/runtime/src/main/java/io/quarkus/resteasy/reactive/jackson/common/RestServerObjectMapper.java' has not been previously formatted. Please format file (for example by invoking `mvn -f extensions/resteasy-reactive/quarkus-resteasy-reactive-jackson-common/runtime net.revelc.code.formatter:formatter-maven-plugin:2.18.0:format`) and commit before running validation!

geoand · 2022-09-23T08:30:46Z

I recently came across another use case where this would be very useful, I think we should reevaluate it, WDYT @gsmet?

quarkus-bot bot added area/documentation area/jackson Issues related to Jackson (JSON library) area/rest area/vertx labels Feb 27, 2022

geoand changed the title ~~23979: Distinct ObjectMappers for RestEasy Reactive / RestEasy Reactive Client~~ Introduce Distinct ObjectMappers for RestEasy Reactive / RestEasy Reactive Client Feb 28, 2022

geoand changed the title ~~Introduce Distinct ObjectMappers for RestEasy Reactive / RestEasy Reactive Client~~ Introduce distinct ObjectMappers for RestEasy Reactive / RestEasy Reactive Client Feb 28, 2022

geoand reviewed Mar 2, 2022

View reviewed changes

...steasy/reactive/jackson/runtime/serialisers/FullyFeaturedServerJacksonMessageBodyWriter.java Outdated Show resolved Hide resolved

geoand reviewed Mar 2, 2022

View reviewed changes

extensions/jackson/runtime/src/main/java/io/quarkus/jackson/runtime/ClientObjectMapper.java Outdated Show resolved Hide resolved