quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.64k stars 2.64k forks source link

RESTEasy + Jackson + PATCH application/merge-patch+json gives error 500 #16592

Open csotiriou opened 3 years ago

csotiriou commented 3 years ago

Describe the bug

RESTEasy gives error 500 on PATCH requests when combined with the quarkus-resteasy-jackson module, and the content type is application/merge-patch+json

Expected behavior

The PATCH request should proceed normally

Actual behavior

An error of 500 is being returned, with a message: javax.ws.rs.ProcessingException: GET method returns the patch/merge json object target not found

To Reproduce

Environment (please complete the following information):

Happens on all Windows and Mac and Linux, version is irrelevant.

Output of java -version

openjdk version "11.0.10" 2021-01-19 OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.10+9) OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.10+9, mixed mode)

Quarkus version or git rev

Quarkus 1.13.2. Happens with 1.11.x, haven't tested with older versions, probably this exists for a long time.

Additional context

It seems that it has to to with the PatchMethodFilter that is being included with the resteasy-jackson2-provider jar. The line that bugs it is this one:

resourceInovker = methodRegistry.getResourceInvoker(request);

I am not aware of the utility of this filter - but if I add resteasy.patchfilter.disabled=true in the application.properties it seems to temporarily fix the problem, as it disables this filter. However, I am not aware of what else it might break because of this workaround.

geoand commented 3 years ago

@ronsigal care to take a look at this one?

ronsigal commented 3 years ago

Hi @geoand, yes, I'll can take a look, assuming the problem occurs in Linux. I don't have a functioning Windows machine or any Mac machine.

geoand commented 3 years ago

Thanks!

I assume the problem is not Windows specific, I'd be surprised if it is 🙂

csotiriou commented 3 years ago

Just to be 100% certain, I just tested it on Linux and I can confirm that the issue also exists there.

ronsigal commented 3 years ago

Ok, thanks.

csotiriou commented 3 years ago

I should add that the temporary workaround of putting resteasy.patchfilter.disabled=true seems to be working for 1.13.x but not 1.11.x.

csotiriou commented 3 years ago

Issue persists in Quarkus 2.1.1.Final

GoncaloPT commented 2 years ago

Issue persists on Quarkus 2.6.1.Final with java 17

gsmet commented 2 years ago

@ronsigal any chance someone from the RESTEasy team could have a look at it?

erik-wramner commented 2 years ago

I think this is a feature rather than a bug. Check JSON Patch and JSON Merge Patch. The way I understand it the merge patch works by calling GET to find the current object; it then patches it with the data from the request and passes the already merged object to the method.

In other words the real solution here would be to declare a method annotated with GET that provides the original object to the framework. Alternatively the feature can be disabled or the PATCH method can be declared to use another content type. Regardless I for one wouldn't want this "fixed" as the feature is quite good.

I would suggest closing or updating the documentation.

csotiriou commented 2 years ago

Thanks for the clarification, @erik-wramner .

I would like to argue that in the microservices world a microservice may have just a tiny portion of the whole business logic to take care of, so it may have to implement just the path method. Moreover, using another content-type is not always possible in the cases where one follows a very strict specification which leaves no space for deviations.

In those cases, I suppose that using resteasy.patchfilter.disabled=true is the way to go?

erik-wramner commented 2 years ago

I'm not an expert, but I think so. I can also add that this seems to have been removed by now. I don't get this with the latest Quarkus release and PatchMethodFilter seems to be gone. Perhaps they felt that the method did not offer enough control.

sdaschner commented 1 year ago

I ran into the same issue and can confirm, resteasy.patchfilter.disabled=true helps if one doesn't want to go with the implicit RESTEasy way (invoking GET, etc.).

Documenting this would certainly be helpful, I was quite surprised about the impact of changing the @Consumes media type...