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

Fix HttpHeaders.getAcceptableLanguages() on empty headers #44259

Merged
merged 2 commits into from
Nov 6, 2024

Conversation

koplas
Copy link
Contributor

@koplas koplas commented Nov 1, 2024

jakarta.ws.rs.core.HttpHeaders.getAcceptableLanguages requires that a wildcard locale is returned, if no acceptable language is specified. The previous behavior returned an empty list.

@koplas koplas changed the title Fix HttpHeaders.getAcceptableLangauges() on empty headers Fix HttpHeaders.getAcceptableLanguages() on empty headers Nov 1, 2024
@geoand geoand requested a review from FroMage November 1, 2024 15:51

This comment has been minimized.

`jakarta.ws.rs.core.HttpHeaders.getAcceptableLanguages` requires
that a wildcard locale is returned, if no acceptable language is
specified. The previous behavior returned an empty list.

Closes quarkusio#44253

This comment has been minimized.

Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

I don't think Locale.ROOT is a wildcard. Its javadoc says its language is set to the empty string, while a wildcard should have its language set to *.

Also, could you add a test for this? It will probably highlight this issue.

And I suspect the test failure may be related, somehow, it should be investigated.

Thanks a lot :)

@koplas
Copy link
Contributor Author

koplas commented Nov 4, 2024

I don't think Locale.ROOT is a wildcard. Its javadoc says its language is set to the empty string, while a wildcard should have its language set to *.

Maybe the docs are a bit confusing, Locale.ROOT is a constant, as is Locale.ENGLISH.
The relevant code is here and here. I did not find another wildcard locale other than Locale.ROOT.

Also, could you add a test for this? It will probably highlight this issue.

This is a good idea; I will try to add a test.

And I suspect the test failure may be related, somehow, it should be investigated.

Some failing tests seem to be I/O related. For example, one error looks like this:

Caused by: io.fabric8.maven.docker.access.hc.http.HttpRequestException: {"message":"Head \"https://registry-1.docker.io/v2/elastic/elasticsearch/manifests/8.15.0\": Get \"https://auth.docker.io/token?account=githubactions&scope=repository%3Aelastic%2Felasticsearch%3Apull&service=registry.docker.io\": net/http: request canceled (Client.Timeout exceeded while awaiting headers)"} (Internal Server Error: 500)

Maybe these tests can refactored to be more robust, but this should be handled in another pull request.

@geoand
Copy link
Contributor

geoand commented Nov 5, 2024

The test failure was transient, nothing to do with this PR

@koplas
Copy link
Contributor Author

koplas commented Nov 5, 2024

I added a test and no longer use Locale.ROOT as a wildcard locale. This behavior does differ from RESTEasy Classic.

@koplas koplas requested a review from FroMage November 5, 2024 13:08
Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

LGTM. I do wonder why this is not tested in the TCK, but 🤷

This comment has been minimized.

Copy link

quarkus-bot bot commented Nov 6, 2024

Status for workflow Quarkus CI

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

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

@geoand geoand merged commit 7ec531d into quarkusio:main Nov 6, 2024
46 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Nov 6, 2024
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.

Missing Accept-Language header results in an empty list with getAcceptableLanguages()
3 participants