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

Use java.util.Locale to parse the languages from the Accept-Language header #36228

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

nderwin
Copy link
Contributor

@nderwin nderwin commented Oct 1, 2023

@quarkus-bot

This comment was marked as resolved.

@nderwin nderwin changed the title Use java.util.Locale to parse the languages from the Accept-Language header. Use java.util.Locale to parse the languages from the Accept-Language header Oct 1, 2023
@geoand
Copy link
Contributor

geoand commented Oct 2, 2023

Thanks for this!

Would you like to add some new tests that verify the behavior?

@nderwin
Copy link
Contributor Author

nderwin commented Oct 2, 2023

@geoand Sure, where would they go, in the integration-tests module? Hrm, I don't see an existing resteasy-reactive module there (not counting the Kotlin one).

@geoand
Copy link
Contributor

geoand commented Oct 2, 2023

I would add one here

@geoand
Copy link
Contributor

geoand commented Oct 4, 2023

I have launched CI, let's see what it says

@nderwin
Copy link
Contributor Author

nderwin commented Oct 4, 2023

🤞

@quarkus-bot

This comment has been minimized.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

This looks good, thanks.

I will let @geoand approve and merge when he's back from Devoxx.

@geoand
Copy link
Contributor

geoand commented Oct 5, 2023

Thanks. I want to have a closer looks because either the Javadoc of the method is outdated, or something more subtle is going on.

@nderwin
Copy link
Contributor Author

nderwin commented Oct 6, 2023

@geoand Also keep an eye on resteasy/resteasy#3808 - I made the same change for the non-reactive RESTEasy, and it tripped on the Jakarta REST TCK. If it's rejected there, then we should probably reject this too, for consistency.

@geoand
Copy link
Contributor

geoand commented Oct 9, 2023

Thanks for the heads up @nderwin.

Let's convert this into a draft as well.

@geoand geoand marked this pull request as draft October 9, 2023 08:11
@nderwin
Copy link
Contributor Author

nderwin commented Mar 18, 2024

My PR for RESTEasy was replaced with these two that were merged:

resteasy/resteasy#4054
resteasy/resteasy#4055

@gsmet
Copy link
Member

gsmet commented Mar 18, 2024

Thanks for the follow-up. How did the TCK issue get fixed?

@nderwin
Copy link
Contributor Author

nderwin commented Mar 18, 2024

I'm not sure. I wasn't really clear on what the TCK issue was, unfortunately.

…header

* added some tests with various language tag scenarios
* Fixes quarkusio#36227

Signed-off-by:Nathan Erwin <[email protected]>
@nderwin nderwin marked this pull request as ready for review October 1, 2024 22:12
@nderwin nderwin requested a review from gsmet October 1, 2024 22:14
@nderwin
Copy link
Contributor Author

nderwin commented Oct 1, 2024

I think it's been long enough that we can turn this back into a real PR.

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link

quarkus-bot bot commented Oct 2, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 633a16c.

✅ 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.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/infinispan-cache/deployment

io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls - History

  • expected: "thread1" but was: "thread2" - org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError: 

expected: "thread1"
 but was: "thread2"
	at io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls(InfinispanCacheTest.java:283)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:513)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:427)

@geoand
Copy link
Contributor

geoand commented Oct 2, 2024

As this is doing the same as RESTEasy is now doing, it makes perfect sense to merge it.

Thanks a lot for the contribution!

@geoand geoand merged commit 1365271 into quarkusio:main Oct 2, 2024
46 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Oct 2, 2024
@nderwin nderwin deleted the bugfix/36227 branch October 2, 2024 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accept-Language header not parsed correctly for language subtags, variants, extension and private subtags
3 participants