Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@Consumes ignored for sub-resource #28460

Closed
QuentinCazelles opened this issue Oct 9, 2022 · 5 comments · Fixed by #28565
Closed

@Consumes ignored for sub-resource #28460

QuentinCazelles opened this issue Oct 9, 2022 · 5 comments · Fixed by #28565
Labels
area/resteasy-reactive kind/bug Something isn't working
Milestone

Comments

@QuentinCazelles
Copy link

Describe the bug

I have a main resource and a nested sub-resource which I want to be able PATCH (both of them) with "application/merge-json+patch" request content.

Works for the main resource but @consumes seems ignored in sub-resource.

Maybe a bug or maybe something I don't quite understand ...

Expected behavior

Using my minimal-reproducer project :

curl -i -X PATCH -H "Content-Type: application/merge-json+patch" localhost:8080/main/1/sub/1 -d '{"test": true}'
Should return an HTTP 200.

curl -i -X PATCH -H "Content-Type: application/application/json" localhost:8080 -d '{"test": true}'
Should return an HTTP 415.

Actual behavior

Using my minimal-reproducer project :

curl -i -X PATCH -H "Content-Type: application/merge-json+patch" localhost:8080/main/1/sub/1 -d '{"test": true}'
Returns an HTTP 415.

curl -i -X PATCH -H "Content-Type: application/application/json" localhost:8080/main/1/sub/1 -d '{"test": true}'
Returns an HTTP 200.

How to Reproduce?

minimal-reproducer.zip

  1. Run the project with ./mvnw compile quarkus:dev
  2. Make a request with :
    curl -i -X PATCH -H "Content-Type: application/merge-json+patch" localhost:8080/main/1/sub/1 -d '{"test": true}'
    and
    curl -i -X PATCH -H "Content-Type: application/application/json" localhost:8080/main/1/sub/1 -d '{"test": true}'

Output of uname -a or ver

Linux myuser 5.15.0-48-generic #54~20.04.1-Ubuntu SMP Thu Sep 1 16:17:26 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Output of java -version

OpenJDK Runtime Environment (build 17+35-2724) OpenJDK 64-Bit Server VM (build 17+35-2724, mixed mode, sharing)

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.13.1.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63) Maven home: /home/myuser/.m2/wrapper/dists/apache-maven-3.8.6-bin/67568434/apache-maven-3.8.6 Java version: 17, vendor: Oracle Corporation, runtime: /usr/lib/jvm/jdk-17 Default locale: en_US, platform encoding: UTF-8 OS name: "linux", version: "5.15.0-48-generic", arch: "amd64", family: "unix"

Additional information

I think @consumes is ignored because header "application/json" works even if I define @consumes(MediaType.TEXT_PLAIN) on sub-resource PATCH method.

PATCH method for Main Resource is working fine.

I am using quarkus-resteasy-reactive and quarkus-resteasy-reactive-jsonb.
Maybe this is a JAX-RS/Resteasy issue, I don't know.

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 10, 2022

/cc @FroMage, @stuartwdouglas

@geoand
Copy link
Contributor

geoand commented Oct 11, 2022

cc @Sgitario

@Sgitario
Copy link
Contributor

I took a look into this issue and the reproducer is wrong as the annotation "@consumes" of the resource has the value "application/merge-patch+json" (type is "merge-patch" and subtype "json") and the CURL statements described in the issue description uses "application/merge-json+patch" (type is "merge-json" and subtype "patch").

Trying to reproduce the issue using the same endpoint in the root resource and a sub-resource, both behaved the same. This means that we used the wrong content type "application/merge-json+patch", both tests failed and when using the correct content type, both worked.

About when using the media type "text/xml", this indeed worked even though the content type was "application/merge-json+patch". This is because the server is taking the media type from the content-type header and not checking whether is indeed acceptable/compatible with the @Consumes annotation in the method resource (see

). However, I'm not sure whether this is intentional as described in the specs or it's a bug. Any ideas @geoand ?

@geoand
Copy link
Contributor

geoand commented Oct 13, 2022

It's likely intended. You can try changing it and running the TCK, you'll likely see a host of failures :)

Sgitario added a commit to Sgitario/quarkus that referenced this issue Oct 13, 2022
At the moment, there are two places where resteasy reactive is verifying the media types: (1) at the handler `ClassRoutingHandler` only for the main resources (it's not invoked for sub-resources; and (2) at the handler `RequestDeserializeHandler` for the rest. 
The issue is that the verification at `RequestDeserializeHandler` takes the one from the user which might not be compatible with the one set by the user using the annotation `@Consumes`. 
Note that I tried to make this handler to be invoked also for sub resources but it caused more troubles and it would not fix the root cause of the issue that is the logic in the handler `RequestDeserializeHandler` is wrong. 
Fix quarkusio#28460
@Sgitario
Copy link
Contributor

It's likely intended. You can try changing it and running the TCK, you'll likely see a host of failures :)

I changed the mentioned logic and the TCK test suite worked fine (at least, locally). See my changes at #28565. However, I'm not sure if this is the right fix (see the description).

Sgitario added a commit to Sgitario/quarkus that referenced this issue Oct 14, 2022
At the moment, there are two places where resteasy reactive is verifying the media types: (1) at the handler `ClassRoutingHandler` only for the main resources (it's not invoked for sub-resources; and (2) at the handler `RequestDeserializeHandler` for the rest. 
The issue is that the verification at `RequestDeserializeHandler` takes the one from the user which might not be compatible with the one set by the user using the annotation `@Consumes`. 
Note that I tried to make this handler to be invoked also for sub resources but it caused more troubles and it would not fix the root cause of the issue that is the logic in the handler `RequestDeserializeHandler` is wrong. 
Fix quarkusio#28460
@quarkus-bot quarkus-bot bot added this to the 2.14 - main milestone Oct 15, 2022
igorregis pushed a commit to igorregis/quarkus that referenced this issue Oct 16, 2022
At the moment, there are two places where resteasy reactive is verifying the media types: (1) at the handler `ClassRoutingHandler` only for the main resources (it's not invoked for sub-resources; and (2) at the handler `RequestDeserializeHandler` for the rest. 
The issue is that the verification at `RequestDeserializeHandler` takes the one from the user which might not be compatible with the one set by the user using the annotation `@Consumes`. 
Note that I tried to make this handler to be invoked also for sub resources but it caused more troubles and it would not fix the root cause of the issue that is the logic in the handler `RequestDeserializeHandler` is wrong. 
Fix quarkusio#28460
igorregis pushed a commit to igorregis/quarkus that referenced this issue Oct 16, 2022
At the moment, there are two places where resteasy reactive is verifying the media types: (1) at the handler `ClassRoutingHandler` only for the main resources (it's not invoked for sub-resources; and (2) at the handler `RequestDeserializeHandler` for the rest. 
The issue is that the verification at `RequestDeserializeHandler` takes the one from the user which might not be compatible with the one set by the user using the annotation `@Consumes`. 
Note that I tried to make this handler to be invoked also for sub resources but it caused more troubles and it would not fix the root cause of the issue that is the logic in the handler `RequestDeserializeHandler` is wrong. 
Fix quarkusio#28460
igorregis pushed a commit to igorregis/quarkus that referenced this issue Oct 16, 2022
At the moment, there are two places where resteasy reactive is verifying the media types: (1) at the handler `ClassRoutingHandler` only for the main resources (it's not invoked for sub-resources; and (2) at the handler `RequestDeserializeHandler` for the rest. 
The issue is that the verification at `RequestDeserializeHandler` takes the one from the user which might not be compatible with the one set by the user using the annotation `@Consumes`. 
Note that I tried to make this handler to be invoked also for sub resources but it caused more troubles and it would not fix the root cause of the issue that is the logic in the handler `RequestDeserializeHandler` is wrong. 
Fix quarkusio#28460
igorregis pushed a commit to igorregis/quarkus that referenced this issue Oct 17, 2022
At the moment, there are two places where resteasy reactive is verifying the media types: (1) at the handler `ClassRoutingHandler` only for the main resources (it's not invoked for sub-resources; and (2) at the handler `RequestDeserializeHandler` for the rest. 
The issue is that the verification at `RequestDeserializeHandler` takes the one from the user which might not be compatible with the one set by the user using the annotation `@Consumes`. 
Note that I tried to make this handler to be invoked also for sub resources but it caused more troubles and it would not fix the root cause of the issue that is the logic in the handler `RequestDeserializeHandler` is wrong. 
Fix quarkusio#28460
Liuigi pushed a commit to Liuigi/quarkus that referenced this issue Oct 17, 2022
At the moment, there are two places where resteasy reactive is verifying the media types: (1) at the handler `ClassRoutingHandler` only for the main resources (it's not invoked for sub-resources; and (2) at the handler `RequestDeserializeHandler` for the rest. 
The issue is that the verification at `RequestDeserializeHandler` takes the one from the user which might not be compatible with the one set by the user using the annotation `@Consumes`. 
Note that I tried to make this handler to be invoked also for sub resources but it caused more troubles and it would not fix the root cause of the issue that is the logic in the handler `RequestDeserializeHandler` is wrong. 
Fix quarkusio#28460
@gsmet gsmet modified the milestones: 2.14 - main, 2.13.3.Final Oct 17, 2022
gsmet pushed a commit to gsmet/quarkus that referenced this issue Oct 17, 2022
At the moment, there are two places where resteasy reactive is verifying the media types: (1) at the handler `ClassRoutingHandler` only for the main resources (it's not invoked for sub-resources; and (2) at the handler `RequestDeserializeHandler` for the rest.
The issue is that the verification at `RequestDeserializeHandler` takes the one from the user which might not be compatible with the one set by the user using the annotation `@Consumes`.
Note that I tried to make this handler to be invoked also for sub resources but it caused more troubles and it would not fix the root cause of the issue that is the logic in the handler `RequestDeserializeHandler` is wrong.
Fix quarkusio#28460

(cherry picked from commit bfbfea6)
tmihalac pushed a commit to tmihalac/quarkus that referenced this issue Oct 27, 2022
At the moment, there are two places where resteasy reactive is verifying the media types: (1) at the handler `ClassRoutingHandler` only for the main resources (it's not invoked for sub-resources; and (2) at the handler `RequestDeserializeHandler` for the rest. 
The issue is that the verification at `RequestDeserializeHandler` takes the one from the user which might not be compatible with the one set by the user using the annotation `@Consumes`. 
Note that I tried to make this handler to be invoked also for sub resources but it caused more troubles and it would not fix the root cause of the issue that is the logic in the handler `RequestDeserializeHandler` is wrong. 
Fix quarkusio#28460
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/resteasy-reactive kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants