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.

Sign up for GitHub

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

Jump to bottom

Resteasy Reactive not working with unspecified accept header and multiple produces #18997

Closed
antonwiens opened this issue Jul 26, 2021 · 13 comments · Fixed by #19000
Closed

Resteasy Reactive not working with unspecified accept header and multiple produces #18997

antonwiens opened this issue Jul 26, 2021 · 13 comments · Fixed by #19000
Labels
area/rest kind/bug Something isn't working
Milestone
2.1.1.Final

Comments

@antonwiens
Copy link

antonwiens commented Jul 26, 2021 •
edited
Loading

Describe the bug

I have discovered a problem with resteasy-reactive and produces media type matching.

If i have more than one media type (e.g. text/plain and application/json) and no specific mediatype is selected in the accept header, then the response contains content-type: application/octet-stream.

Also i noticed that the qs parameter is ignored in resteasy-reactive, which is unfortunatly very important in our projekt.

This is in quarkus 2.0.3.Final.

I added an repoducer:

reproducer.zip

Just execute the test.
@antonwiens antonwiens added the kind/bug Something isn't working label Jul 26, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 26, 2021

/cc @FroMage, @geoand, @stuartwdouglas

@geoand
Copy link
Contributor

geoand commented Jul 26, 2021

I see the same issue with RESTEasy Classic, so I am inclined to think it's just how the JAX-RS spec handles these cases.

WDYT @FroMage ?

@antonwiens
Copy link
Author

But this used to work for me in resteasy classic in quarkus 1.10, so i dont think its normal behavior.
Also the qs parameter used to work.

@geoand
Copy link
Contributor

geoand commented Jul 26, 2021

I just switched your reproducer to resteasy classic and the tests still failed

@antonwiens
Copy link
Author

antonwiens commented Jul 26, 2021 •
edited
Loading

Sorry, i had an error in the test assertion, which also failed by mistake in resteasy-classic but not due to the same problem:

        assert(body.equals("{\"string\": \"string\"}"))

should be:

        assert(body.equals("{\"string\":\"string\"}"))

EDIT: Sorry i accidently swapped the quoted code

@antonwiens
Copy link
Author

Here is the the test with resteasy-classic with fixed assertion that is not failing:
resteasy-classic-not-failing.zip

Here with resteasy-reactive failing with fixed assertion:
reproducer-fixed-assertion.zip

@geoand
Copy link
Contributor

geoand commented Jul 26, 2021

The workaround is to use RestAssured.given().accept("application/json").get(...) instead of RestAssured.given().get(...)

@geoand
Copy link
Contributor

geoand commented Jul 26, 2021

It does seem like RESTEasy Reactive is not using the same default as RESTEasy Classic though... I'll have a look

@antonwiens
Copy link
Author

It does seem like RESTEasy Reactive is not using the same default as RESTEasy Classic though... I'll have a look

Thanks for looking into it.

geoand added a commit to geoand/quarkus that referenced this issue Jul 26, 2021
@geoand
Make media type negotiation more spec compliant
5d4f4bd 
This is meant to better implement point 5 of part 3.8
of the spec that mentions:

```
For each member of P, p:
– If a is compatible with p, add S(a, p) to M , where the function S returns the most specific
media type of the pair with the q-value of a and server-side qs-value of p.
```

Now, what I have done here might not be the complete solution, I would indeed
like to proceed with caution on this (and this only fixed the case reported)
as the media type handling is fraught with a lot of corner cases...

Fixes: quarkusio#18997
@geoand geoand mentioned this issue Jul 26, 2021
Merged
@geoand
Copy link
Contributor

geoand commented Jul 26, 2021

#19000 should fix the issue

@antonwiens
Copy link
Author

As i can see the quality factor (qs parameter) is not yet considered in this fix?

@geoand
Copy link
Contributor

geoand commented Jul 26, 2021

It is because the supported media types are sorted by qs

@antonwiens
Copy link
Author

It is because the supported media types are sorted by qs

Thanks for clarifing this!

@geoand geoand changed the title Resteasy Reactive not working with unspecific accept header and multiple produces Resteasy Reactive not working with unspecified accept header and multiple produces Jul 27, 2021
FroMage added a commit that referenced this issue Jul 27, 2021
@FroMage
Merge pull request #19000 from geoand/#18997
35bf51b 
Make media type negotiation in RESTEasy Reactive more spec compliant
@quarkus-bot quarkus-bot bot added this to the 2.2 - main milestone Jul 27, 2021
@gsmet gsmet modified the milestones: 2.2 - main, 2.1.1.Final Aug 3, 2021
gsmet pushed a commit to gsmet/quarkus that referenced this issue Aug 3, 2021
@geoand @gsmet
Make media type negotiation more spec compliant
5b0624d 
This is meant to better implement point 5 of part 3.8
of the spec that mentions:

```
For each member of P, p:
– If a is compatible with p, add S(a, p) to M , where the function S returns the most specific
media type of the pair with the q-value of a and server-side qs-value of p.
```

Now, what I have done here might not be the complete solution, I would indeed
like to proceed with caution on this (and this only fixed the case reported)
as the media type handling is fraught with a lot of corner cases...

Fixes: quarkusio#18997
(cherry picked from commit 5d4f4bd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/rest kind/bug Something isn't working
Projects
None yet
Milestone
2.1.1.Final
Development

Successfully merging a pull request may close this issue.

Make media type negotiation in RESTEasy Reactive more spec compliant geoand/quarkus
3 participants
@gsmet @geoand @antonwiens