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

Loosen restriction on casing of the Accept header #20026

Merged
merged 1 commit into from
Sep 20, 2021
Merged

Loosen restriction on casing of the Accept header #20026

merged 1 commit into from
Sep 20, 2021

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Sep 9, 2021

Fixes: #20021

@geoand
Copy link
Contributor Author

geoand commented Sep 9, 2021

The TCK passes, but I am on the fence about this one. @FroMage WDYT?

crinjes
crinjes reviewed Sep 9, 2021
View reviewed changes
.../runtime/src/main/java/org/jboss/resteasy/reactive/server/handlers/FixedProducesHandler.java
@@ -40,8 +41,15 @@ public void handle(ResteasyReactiveRequestContext requestContext) throws Excepti
requestContext.setResponseContentType(mediaType);
requestContext.setEntityWriter(writer);
} else {
throw new WebApplicationException(
Response.notAcceptable(Variant.mediaTypes(mediaType.getMediaType()).build()).build());
// some clients might be sending the header with incorrect casing...
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically not incorrect since it's supposed to be case-insensitive ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point :)

@geoand geoand requested a review from FroMage September 9, 2021 13:00
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 9, 2021

Failing Jobs - Building b3d5028

Status Name Step Failures Logs Raw logs
✖ MicroProfile TCKs Tests Verify Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ MicroProfile TCKs Tests #

- Failing: tcks/microprofile-fault-tolerance

📦 tcks/microprofile-fault-tolerance

✖ org.eclipse.microprofile.fault.tolerance.tck.TimeoutUninterruptableTest.testTimeoutAsyncBulkhead line 190 - More details - Source on GitHub

java.lang.AssertionError: Unexpected exception thrown from Future
	at org.testng.Assert.fail(Assert.java:85)
	at org.eclipse.microprofile.fault.tolerance.tck.util.Exceptions.expect(Exceptions.java:98)

@geoand
Copy link
Contributor Author

geoand commented Sep 9, 2021

The RESTEasy Reactive TCK tests passed - the failure is in an unrelated module

@geoand
Copy link
Contributor Author

geoand commented Sep 14, 2021

@FroMage can I get a review of this please?

@geoand geoand requested a review from gastaldi September 20, 2021 06:14
gastaldi
gastaldi approved these changes Sep 20, 2021
View reviewed changes
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay, but I guess this might happen with other header parameters as well, maybe move this check to a Headers class or something like it (in a different PR if you like)?

@geoand
Copy link
Contributor Author

geoand commented Sep 20, 2021

Yeah, it could perhaps happen. But let's fix this case for the time being 😎

@geoand geoand merged commit 879c866 into quarkusio:main Sep 20, 2021
@quarkus-bot quarkus-bot bot added this to the 2.3 - main milestone Sep 20, 2021
@geoand geoand deleted the #20021 branch September 20, 2021 11:57
@gastaldi
Copy link
Contributor

For the posterity, probably a more object oriented solution would be getRequestHeader above to return a Header object than can be compared against such values

@gsmet gsmet modified the milestones: 2.3.0.CR1, 2.2.4.Final Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crinjes crinjes crinjes left review comments

@gastaldi gastaldi gastaldi approved these changes

@FroMage FroMage Awaiting requested review from FroMage

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.2.4.Final
Development

Successfully merging this pull request may close these issues.

Resteasy-Reactive Accept Matching should be case insensitive
4 participants
@geoand @gastaldi @crinjes @gsmet