Use generic return type of method to construct proper Jackson writer #43700

danielbobbert · 2024-10-04T15:03:50Z

Fixes: Quarkus REST fails to write polymorphic type property in JSON #43631

gsmet

Thanks a lot for this PR and all the detective work.

Added a small suggestion and might be a good idea to also avoid the lambda if we can as we try to reduce their usage to the bare minimum in Quarkus runtime code.

gsmet · 2024-10-04T15:41:11Z

...arkus/resteasy/reactive/jackson/runtime/serialisers/BasicServerJacksonMessageBodyWriter.java

+            return this.defaultWriter;
+        } else {
+            // compute and cache a specific writer for the given generic type
+            return genericWriters.computeIfAbsent(genericType, type -> {


For CHM, it's usually recommended to do a get first when getting a hit is the most common pattern as I was told computeIfAbsent locks were a bit less optimized.

lambda removed

get() before computeIfAbsent() added

I am still not 100% happy with the solution, because I feel I am generating too many custom writers (basically one per different method return type), even though that's probably only necessary for methods that return generic collections etc. (basically all those methods where `method.getGenericReturnType() <> method.getReturnType()). For methods with non-generic return types, the default writer works correctly.
I will dig just a little bit deeper to see whether I can differentiate those cases and only generate a custom writer if it is really necessary.

I have checked the details of how and why Jackson works this way and performed various other tests. As commented in the code, Jackson needs that custom writer exactly in those cases, where the target type is a parameterized type.
So I have updated the code once more to compute a specialized writer only in those cases, where the method return type actually is a generic (parameterized) type. In all other cases, the default writer can be used.

Imho, the code is good to go.

BTW: I also compared my solution to what is being done in FullyFeaturedServerJacksonMessageBodyWriter (which computes a customer writer per method/type as well). One difference that I spotted is that FullyFeaturedServerJacksonMessageBodyWriter maps the type by name (perTypeWriter in line 40) instead of using the type itself as the map key. I don't see, why this would be beneficial (on the contrary: computing the type name every time should be more expensive that using hashCode() and equals() of the type itself, right?!), but please correct me if I'm wrong.

gsmet · 2024-10-08T21:30:48Z

@danielbobbert just a heads up that we are at Devoxx this week so we will probably have a closer look next week!

geoand

Thanks!

geoand · 2024-10-14T06:58:04Z

The rest failures seem related

danielbobbert · 2024-10-14T08:18:36Z

Yes. The problem is that the "genericType" argument that is passed to the serializer is not properly unwrapped for CompletableFuture. When a method returns a Uni, the object "o" passed to writeResponse() is an "X" and the "genericType" is "X.class" (and not Uni). But for a return type of "CompletableFuture", the "o" is of type "X" (so the future has been resolved and the actual value is passed to serialization) but the "genericType" is still "CompletableFuture". Because it is generic, my new code selects a special serializer, but of course that fails, because a serializer for "CompletableFuture" cannot serialize an "X".

Now, I could of course add special handling for that case, but I'd rather find out, why the genericType is properly unwrapped for Unis but not for CompletableFutures (and maybe other types such as CompletionStage, Future, etc. as well?!)

danielbobbert · 2024-10-14T08:26:44Z

I just verified that it does work properly for methods returning CompletionStage.
Only Future and its derivatives don't work (which is kind of strange, given that CompletableFuture actually inherits from CompletionStage)

geoand · 2024-10-14T08:31:01Z

Ah, nice find. I'll have to track down where the unwrapping is done as I don't remember of the top of my head.

danielbobbert · 2024-10-14T08:32:10Z

Found it. It's in org.jboss.resteasy.reactive.common.util.types.Types.getEffectiveReturnType()

geoand · 2024-10-14T08:34:02Z

Yup, just found that as well. Feel free to push to the change to that method

danielbobbert · 2024-10-14T08:35:29Z

Line 185 should probably be changed from
rawType == CompletionStage.class
to
CompletionStage.class.isAssignableFrom(rawType)

Do you want me to add that to my PR and give it a try?

geoand · 2024-10-14T08:42:28Z

Do you want me to add that to my PR and give it a try?

Please do

danielbobbert · 2024-10-14T08:43:35Z

Just rebased, amended and pushed the change

danielbobbert · 2024-10-14T13:17:00Z

OK, two problems:
First off, my isAssignableFrom() check was nonsense since it was doing an additional ".getClass()" on the type itself. I fixed that.
Second, there seems to be another problem, when the response entity is wrapped by a filter, e.g. the HalFilter. I am not 100% sure how to properly fix that. I have added another check that the generic return type is class compliant with the type of the actual object (it would then detect that HalEntity.class != X.class and would use the default writer. What do you think about that?

geoand · 2024-10-14T13:19:04Z

Second, there seems to be another problem, when the response entity is wrapped by a filter, e.g. the HalFilter.

Can you elaborate a little more on this?

Thanks

danielbobbert · 2024-10-14T13:29:10Z

Some tests in HalLinksWithJacksonTest were failing. I figured that's because the HalServerResponseFilter wraps the actual entity returned by the method into a HalEntityWrapper (or HalCollectionWrapper).
Now, if my changes in BasicServerJacksonMessageBodyWriter detect that the method was returning a parameterized type (let's call it "X"), they would derive a special writer for "X".
Problem is, that the serializer is then passed a HalEntityWrapper instead of something of type X, and thus fails.
That's why I am now checking that the type of the object being serialized actually matches the genericReturnType of the method, before deriving the specialized writer. If they don't match, I return the default writer as before.

geoand · 2024-10-15T05:53:28Z

But on my fork, all the steps are always skipped and only the final report is being generated (within seconds)

That's odd. IIRC, CI should just run on forks without any necessary intervention on your part.

I can't seem to find the proper way to run the tests myself (and also I can't run them locally on my computer because it keeps failing to resolve "org.gradle:gradle-tooling-api:jar:8.1.1" - even though that is definitely installed in my local .m2 repo)

Try building the entire project with:

mvn -T 1C -DskipDocs -DskipTests -DskipITs -Dinvoker.skip -DskipExtensionValidation -Dskip.gradle.tests -Dskip.gradle.build -Dtruststore.skip clean install -Prelocations

and then running a test like so:

mvn test -f extensions/resteasy-reactive/rest-links/deployment/ -Dtest=HalLinksWithJacksonTest

mariofusco · 2024-10-15T06:39:56Z

...io/quarkus/resteasy/reactive/jackson/runtime/serialisers/ServerJacksonMessageBodyWriter.java

+
+abstract class ServerJacksonMessageBodyWriter extends ServerMessageBodyWriter.AllWriteableMessageBodyWriter {
+
+    ObjectWriter getGenericWriter(Type genericType, ObjectWriter defaultWriter) {


Changing the BodyWriters hierarchy only to add this method seems a bit unnecessary. Actually this method could be a static one and moved to JacksonMapperUtil that already contains a collection of these static utility methods necessary for serialization, thus eliminating this intermediate class in the writers' hierarchy.

OK, I will move the method and also check on those failing tests. Seems like I am still missing a tiny bit...

mariofusco · 2024-10-15T09:36:44Z

...arkus/resteasy/reactive/jackson/runtime/serialisers/BasicServerJacksonMessageBodyWriter.java

+            ObjectWriter writer = genericWriters.get(genericType);
+            if (writer == null) {
+                // no cached writer for that type. Compute it.
+                writer = JacksonMapperUtil.getGenericWriter(genericType, this.defaultWriter);
+                genericWriters.put(genericType, writer);
+            }
+            return writer;


This operation is not atomic and it may happen that multiple writers could be unnecessarily generated for the same type. I believe that this whole block could be replaced with a one-liner, something like:

return genericWriters.computeIfAbsent(genericType, type -> JacksonMapperUtil.getGenericWriter(type, defaultWriter));

danielbobbert · 2024-10-15T15:45:29Z

OK, so I have double checked with resteasy-classic to solve the problem in the same way as it was done there. Moved the method that derives the appropriate root type to JacksonMapperUtil and call that from both Basic- and FullFeaturedServerJacksonResponseWriter.

This should now pass all tests!

Unfortunately, CI still isn't working for me on my fork. But local execution of tests was possible after some fiddling. For some strange reason, tests will not launch correctly every now and then with nonsense ServiceProvider exceptions such as

java.lang.RuntimeException: org.junit.jupiter.api.extension.TestInstantiationException: Failed to create test instance
        at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:736)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: org.junit.jupiter.api.extension.TestInstantiationException: Failed to create test instance
        at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:708)
        ... 1 more
Caused by: java.lang.reflect.InvocationTargetException
        at java.base/java.lang.reflect.Method.invoke(Method.java:568)
        at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:703)
        ... 1 more
Caused by: java.util.ServiceConfigurationError: io.quarkus.runtime.test.TestHttpEndpointProvider: io.quarkus.resteasy.reactive.server.runtime.ResteasyReactiveTestHttpProvider not a subtype
        at java.base/java.util.ServiceLoader.fail(ServiceLoader.java:593)
        at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.hasNextService(ServiceLoader.java:1244)
        at java.base/java.util.ServiceLoader$LazyClassPathLookupIterator.hasNext(ServiceLoader.java:1273)
        at java.base/java.util.ServiceLoader$2.hasNext(ServiceLoader.java:1309)
        at java.base/java.util.ServiceLoader$3.hasNext(ServiceLoader.java:1393)
        at io.quarkus.runtime.test.TestHttpEndpointProvider.load(TestHttpEndpointProvider.java:17)
        at io.quarkus.test.common.http.TestHTTPResourceManager.inject(TestHTTPResourceManager.java:55)

geoand · 2024-10-16T08:20:09Z

We should also keep in mind that this might have a small performance implication. but the change is necessary in any case

quarkus-bot · 2024-10-16T12:00:19Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit 13f9095.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

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

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingWorkingAppPropsTestCase.testContinuousTestingScenario3 - History

io.quarkus.builder.BuildException: Build failure: Build failed due to errors [error]: Build step io.quarkus.apicurio.registry.devservice.DevServicesApicurioRegistryProcessor\#startApicurioRegistryDevService threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image quay.io/apicurio/apicurio-registry-mem:2.4.2.Final at io.quarkus.apicurio.registry.devservice.DevServicesApicurioRegistryProcessor.startApicurioRegistryDevService(DevServicesApicurioRegistryProcessor.java:90) at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732) at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:856) - java.lang.RuntimeException

java.lang.RuntimeException: 
io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.apicurio.registry.devservice.DevServicesApicurioRegistryProcessor#startApicurioRegistryDevService threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image quay.io/apicurio/apicurio-registry-mem:2.4.2.Final
	at io.quarkus.apicurio.registry.devservice.DevServicesApicurioRegistryProcessor.startApicurioRegistryDevService(DevServicesApicurioRegistryProcessor.java:90)
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732)
	at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:856)
	at io.quarkus.builder.BuildContext.run(BuildContext.java:256)
	at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)

gsmet · 2024-10-16T12:05:15Z

I think it's worth pushing it to 3.16.0, I added the backport label.

geoand · 2024-10-16T12:10:25Z

Good idea!

gsmet · 2024-10-16T12:11:12Z

@danielbobbert thanks for all your work on this!

quarkus-bot bot added the area/rest label Oct 4, 2024

gsmet reviewed Oct 4, 2024

View reviewed changes

danielbobbert force-pushed the main branch 5 times, most recently from ecf71fc to ea589e4 Compare October 8, 2024 18:41

geoand approved these changes Oct 14, 2024

View reviewed changes

geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 14, 2024

This comment has been minimized.

Sign in to view

danielbobbert force-pushed the main branch from ea589e4 to f3edaa1 Compare October 14, 2024 07:37

danielbobbert force-pushed the main branch from f3edaa1 to dfe45ed Compare October 14, 2024 08:41

danielbobbert force-pushed the main branch from dfe45ed to a164e24 Compare October 14, 2024 08:42

This comment has been minimized.

Sign in to view

quarkus-bot bot added the triage/flaky-test label Oct 14, 2024

danielbobbert force-pushed the main branch 2 times, most recently from 0fe61ce to 1fccf49 Compare October 14, 2024 13:16

This comment has been minimized.

Sign in to view

mariofusco reviewed Oct 15, 2024

View reviewed changes

danielbobbert force-pushed the main branch 3 times, most recently from e8ce996 to 5cda62d Compare October 15, 2024 09:32

mariofusco reviewed Oct 15, 2024

View reviewed changes

danielbobbert force-pushed the main branch 2 times, most recently from 64b0966 to c7a3de7 Compare October 15, 2024 15:39

Use generic return type of method to construct proper Jackson writer

13f9095

danielbobbert force-pushed the main branch from c7a3de7 to 13f9095 Compare October 15, 2024 20:53

geoand removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 16, 2024

mariofusco approved these changes Oct 16, 2024

View reviewed changes

geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 16, 2024

geoand merged commit b8415d8 into quarkusio:main Oct 16, 2024
45 checks passed

quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Oct 16, 2024

quarkus-bot bot added this to the 3.17 - main milestone Oct 16, 2024

gsmet added triage/backport-3.15 triage/backport-3.16 and removed triage/backport-3.15 labels Oct 16, 2024

gsmet modified the milestones: 3.17 - main, 3.16.0 Oct 21, 2024

gsmet removed the triage/backport-3.16 label Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use generic return type of method to construct proper Jackson writer #43700

Use generic return type of method to construct proper Jackson writer #43700

danielbobbert commented Oct 4, 2024 •

edited by geoand

Loading

gsmet left a comment

gsmet Oct 4, 2024

danielbobbert Oct 4, 2024

danielbobbert Oct 4, 2024

danielbobbert Oct 7, 2024

gsmet commented Oct 8, 2024

geoand left a comment

This comment has been minimized.

geoand commented Oct 14, 2024

danielbobbert commented Oct 14, 2024 •

edited

Loading

danielbobbert commented Oct 14, 2024

geoand commented Oct 14, 2024

danielbobbert commented Oct 14, 2024

geoand commented Oct 14, 2024

danielbobbert commented Oct 14, 2024

geoand commented Oct 14, 2024

danielbobbert commented Oct 14, 2024

This comment has been minimized.

danielbobbert commented Oct 14, 2024

geoand commented Oct 14, 2024

danielbobbert commented Oct 14, 2024

This comment has been minimized.

geoand commented Oct 15, 2024 •

edited

Loading

mariofusco Oct 15, 2024

danielbobbert Oct 15, 2024

mariofusco Oct 15, 2024

danielbobbert commented Oct 15, 2024 •

edited

Loading

geoand commented Oct 16, 2024 •

edited

Loading

quarkus-bot bot commented Oct 16, 2024

gsmet commented Oct 16, 2024

geoand commented Oct 16, 2024

gsmet commented Oct 16, 2024


		abstract class ServerJacksonMessageBodyWriter extends ServerMessageBodyWriter.AllWriteableMessageBodyWriter {

		ObjectWriter getGenericWriter(Type genericType, ObjectWriter defaultWriter) {

Use generic return type of method to construct proper Jackson writer #43700

Use generic return type of method to construct proper Jackson writer #43700

Conversation

danielbobbert commented Oct 4, 2024 • edited by geoand Loading

gsmet left a comment

Choose a reason for hiding this comment

gsmet Oct 4, 2024

Choose a reason for hiding this comment

danielbobbert Oct 4, 2024

Choose a reason for hiding this comment

danielbobbert Oct 4, 2024

Choose a reason for hiding this comment

danielbobbert Oct 7, 2024

Choose a reason for hiding this comment

gsmet commented Oct 8, 2024

geoand left a comment

Choose a reason for hiding this comment

This comment has been minimized.

geoand commented Oct 14, 2024

danielbobbert commented Oct 14, 2024 • edited Loading

danielbobbert commented Oct 14, 2024

geoand commented Oct 14, 2024

danielbobbert commented Oct 14, 2024

geoand commented Oct 14, 2024

danielbobbert commented Oct 14, 2024

geoand commented Oct 14, 2024

danielbobbert commented Oct 14, 2024

This comment has been minimized.

danielbobbert commented Oct 14, 2024

geoand commented Oct 14, 2024

danielbobbert commented Oct 14, 2024

This comment has been minimized.

geoand commented Oct 15, 2024 • edited Loading

mariofusco Oct 15, 2024

Choose a reason for hiding this comment

danielbobbert Oct 15, 2024

Choose a reason for hiding this comment

mariofusco Oct 15, 2024

Choose a reason for hiding this comment

danielbobbert commented Oct 15, 2024 • edited Loading

geoand commented Oct 16, 2024 • edited Loading

quarkus-bot bot commented Oct 16, 2024

Status for workflow Quarkus CI

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

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

gsmet commented Oct 16, 2024

geoand commented Oct 16, 2024

gsmet commented Oct 16, 2024

danielbobbert commented Oct 4, 2024 •

edited by geoand

Loading

danielbobbert commented Oct 14, 2024 •

edited

Loading

geoand commented Oct 15, 2024 •

edited

Loading

danielbobbert commented Oct 15, 2024 •

edited

Loading

geoand commented Oct 16, 2024 •

edited

Loading

Status for workflow `Quarkus CI`