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

Recent LocalesBuildTimeConfig.defaultLocale changes break API #44312

Open
zakkak opened this issue Nov 5, 2024 · 7 comments
Open

Recent LocalesBuildTimeConfig.defaultLocale changes break API #44312

zakkak opened this issue Nov 5, 2024 · 7 comments
Labels
area/config kind/bug Something isn't working
Milestone

Comments

@zakkak
Copy link
Contributor

zakkak commented Nov 5, 2024

Describe the bug

The locales adaptation in #43448 in addition to the breaking default behavior change also breaks code depending on LocalesBuildTimeConfig.defaultLocale in two ways:

  1. The type of the field has changed from Locale to Optional<Locale>
  2. Although in Quarkus we default to the system's default locale when defaultLocale is not set (as described in the defaultValueDocumentation), there is no way for 3rd party users to get that default value without doing localesBuildTimeConfig.defaultLocale.orElse(Locale.getDefault()) themselves.

Issue first reported in https://quarkusio.zulipchat.com/#narrow/channel/187030-users/topic/Upgrading.20Quarkus.20from.203.2E16.2E1.20to.20999-SNAPSHOT.20breaks.20Locale

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

c814b75

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

Unfortunately I can't figure a way to implement the behavior described in #43448 (comment) without having defaultLocale be an Optional, or initialized to null or the ROOT locale (which would fix issue 1, but not 2).

The options I see are:

  1. We allow Quarkus native images to be built with the build systems default locale if defaultLocale is not explicit set (we essentially revert to the current as in 3.16 behavior)
  2. Extend the migration guide to let people know they should use localesBuildTimeConfig.defaultLocale.orElse(Locale.getDefault()) when accessing defaultLocale. Or even better don't expose defaultLocale itself and expose a getter instead.

Ideas are very much welcome.

@zakkak zakkak added the kind/bug Something isn't working label Nov 5, 2024
Copy link

quarkus-bot bot commented Nov 5, 2024

You added a link to a Zulip discussion, please make sure the description of the issue is comprehensive and doesn't require accessing Zulip

This message is automatically generated by a bot.

Copy link

quarkus-bot bot commented Nov 5, 2024

/cc @radcortez (config)

@zakkak
Copy link
Contributor Author

zakkak commented Nov 5, 2024

@zakkak zakkak added this to the 3.17 - main milestone Nov 5, 2024
@gsmet
Copy link
Member

gsmet commented Nov 5, 2024

My personal opinion is that we need to have something on top of config to propagate what we want to propagate.

Config shouldn't be considered API from a developer POV: it's an API from the configuration file perspective.

We have the same issue with the HTTP config, I know @radcortez has a PR around and we should probably start to make progress on a pattern we can reuse in different parts of the code.

@geoand
Copy link
Contributor

geoand commented Nov 5, 2024

Config shouldn't be considered API from a developer POV: it's an API from the configuration file perspective.

Right, we've been bumping up against this more and more lately and we should make it clear

@maxandersen
Copy link
Member

So you say if we could go back in time we would have another layer of accessing the resultant config ?

Or that when we see there will be a problem we should have introduced an api - lets call it "LocaleDefaults" which would have been introduced and mark the config api deprecated (or some other way point them to "LocaleDefaults") and then have that in some releases before breaking the before config api?

otherwise all config would have to be considered non public api for extension writers (and users?)

@radcortez
Copy link
Member

It probably is a non-issue for 99% of the configuration, but there are a few cases in which users/extensions shouldn't rely on the config, especially cases where the configuration may not represent the final value, such as a random HTTP port. This requires us to mutate the configuration, which causes a couple of other issues.

Ideally, we should offer APIs for things that we know that the provided configuration value is not the value that we end up using.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/config kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants
@maxandersen @gsmet @zakkak @geoand @radcortez and others