-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Introduces quarkus.locales=all #37106
Conversation
Hello @geoand, So, when one doesn't care about the executable file size, there is an option to include all locales. The thing is that we are overloading the It brings a weird user experience. Take a look at Hibernate Validator test case. It passes just fine as it is now:
As it has its locales specified as:
and then it does:
And it return the message in Croatian as that is the preferred resource bundle If you specify:
The test cease to work and starts returning English response instead of Croatian. That is because the Locale of the quarkus application is suddenly Locale.ROOT, meaning fallback to English.... I think this is rather counterintuitive. Ideas? |
🙈 The PR is closed and the preview is expired. |
This comment has been minimized.
This comment has been minimized.
integration-tests/locales/all/src/test/java/io/quarkus/locales/it/LocalesIT.java
Outdated
Show resolved
Hide resolved
@Karm you will probably need to change quarkus/.github/native-tests.json Line 108 in 97cf6dc
|
Indeed this does seem really weird behavior... I can't say I know what the problem is off the top of my head |
Ouch! I will amend that... |
I don't think it's a bug, it's a feature of Hibernate Validator :-D The extension is trying to offer message bundles based on that configured locale and gets confused if you don't specify one. I will take a closer look into Hibernate Validator, because it seems somewhat weird to do that. |
Works. Feels clumsy though. |
// Locales, Locale ROOT means all locales in this setting. | ||
.locales(localesBuildTimeConfig.locales.contains(Locale.ROOT) ? Set.of(Locale.getAvailableLocales()) | ||
: localesBuildTimeConfig.locales) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, what's the rationale of that? Can't we rely on the config being configured to all
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gsmet It covers situation when user mis/configures something like: quarkus.locales=fr,all,en-GB
as ROOT is what I use in place of "all".
If you find it clumsy, I can redefine that, check config instead for string "all" and treat anything like the aforementioned configuration as an error in LocaleProcessor in core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And so there's no way for the locale to be defined as root by the user/app? Because if you use it as marker, we absolutely need to never be used by a user/app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gsmet https://docs.oracle.com/javase/8/docs/api/java/util/Locale.html#ROOT
That's what I was working with. I.e. user would have to set empty string for the property quarkus.locales to be parsed as ROOT. Setting empty string is not allowed by our value converter:
Failed to build quarkus application: SRCFG00040: The config property quarkus.locales is defined as the empty String ("") which the following Converter considered to be null: io.smallrye.config.Converters$CollectionConverter
O.K., user can do that as I found out when I tried.
If you set:
quarkus.locales=_
It's converted to Locale.ROOT and that is interpreted as "all" , i.e. it results in building a native image with -H:+IncludeAllLocales
included.
...and it is also interpreted as "include all" by the Hibernate validator, so the test still passes. It means these to settings are equivalent:
quarkus.locales=_
and
quarkus.locales=all
Do you think this is wrong?
If you run on a system that has no langpacks, which is a real possibility I encountered, your one and only working Locale is not ROOT though, it is ENGLISH IIRC. Lemme try...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we could live with it. I tried to use LANG=C
and AFAICS, I end up with an English locale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it all falls back to ENGLISH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.e. tests fail due to this, but I already documented it here: https://github.com/quarkusio/quarkus/blob/main/integration-tests/locales/src/test/java/io/quarkus/locales/it/LocalesIT.java#L17
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Disclaimer: I am not familiar with locales and can't really comment on the use of Locale.ROOT
. Please have someone else review this as well.
Hibernate validator interprets Locale.ROOT as array of all Fixes hibernate-validator for quarkus.locales=all
I rebased and squashed, but it has been ready IMHO. |
Failing Jobs - Building c025a1b
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 17 Windows #- Failing: extensions/smallrye-reactive-messaging-amqp/deployment
! Skipped: integration-tests/reactive-messaging-amqp integration-tests/virtual-threads/amqp-virtual-threads 📦 extensions/smallrye-reactive-messaging-amqp/deployment✖
✖
✖
✖
✖
⚙️ JVM Tests - JDK 21 #- Failing: integration-tests/hibernate-orm-tenancy/connection-resolver-legacy-qualifiers
📦 integration-tests/hibernate-orm-tenancy/connection-resolver-legacy-qualifiers✖
✖
✖
✖
✖
⚙️ Native Tests - Cache #- Failing: integration-tests/infinispan-client
📦 integration-tests/infinispan-client✖
⚙️ Native Tests - gRPC #- Failing: integration-tests/grpc-plain-text-gzip
📦 integration-tests/grpc-plain-text-gzip✖
✖
✖
✖
✖
✖
|
The sudden surge in failures is alarming. AMQCannot talk to Artemis
Hibernate ORM, connection
Cannot talk to DB... Infinispan client - HotRod protocol
Cannot start HotRod server? Dunno. gRPC
etc... @gsmet I don't think this is blocking Locales PR. |
@gsmet ? |
@gsmet ? |
Sorry for the delay. |
fixes #36876