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] manifestEnhancer: Only use valid files for supportedLocales #1080

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

matz3
Copy link
Member

@matz3 matz3 commented Sep 5, 2024

This fixes two problems that could have occurred:

A properties file with an invalid locale was still taken into the list of supported locales, which then caused a runtime exception in the ResourceBundle as it validates the input.

Another problem was that properties files could have a valid name according to BCP47, but the file won't be ever requested with that name.
This is due to the fact that the ResourceBundle does use the legacy Java locale format for the request URL.

In both cases, the properties file is now ignored and no entry for the supportedLocales is created.

Only locales that are valid according to the legacy Java locale format are considered.
However, there is one special case: sr_Latn is also requested by the UI5 runtime, although it contains a BCP47 script, which is not valid according to the legacy Java locale format.

@matz3 matz3 added the bug Something isn't working label Sep 5, 2024
@matz3 matz3 requested a review from a team September 5, 2024 07:37
if (isValidPropertiesFilenameLocale(locale)) {
supportedLocales.push(locale);
} else {
log.verbose(`Skipping invalid locale '${locale}' for bundle '${i18nBundleUrl}'`);
Copy link
Member Author

Choose a reason for hiding this comment

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

Still open: What should happen in this case? Maybe rather an warning or even error log?

Copy link
Member Author

Choose a reason for hiding this comment

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

Decision: Log a warning and explain in commit message that the warning can be suppressed by manually maintaining the "supportedLocales".

Copy link
Member Author

@matz3 matz3 Sep 11, 2024

Choose a reason for hiding this comment

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

To be checked: Built-in handling of private extensions at runtime (1Q, 2Q, 3Q).

Copy link
Member Author

Choose a reason for hiding this comment

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

Warning added via 11ab19f

To be checked: Built-in handling of private extensions at runtime (1Q, 2Q, 3Q).

This just turned out to be a confusion based on a wrong comment / commit message. It referred to variants, but actually it was supposed to be extensions.

Extensions are not considered by the runtime code at all (i.e. can't appear in requests).
Private use is checked for the special SAP supportability locales, but the resulting request does not include an extension or private use section but will rather be e.g. en_US_saptrc (where saptrc is a variant).

@coveralls
Copy link

coveralls commented Sep 5, 2024

Coverage Status

coverage: 94.94% (+0.04%) from 94.905%
when pulling 2745dba on fix-manifestEnhancer-invalid-locale
into 2dd7330 on main.

@matz3 matz3 force-pushed the fix-manifestEnhancer-invalid-locale branch 2 times, most recently from eb9d5d2 to 11ab19f Compare September 11, 2024 15:14
@matz3 matz3 requested review from a team and removed request for a team September 11, 2024 15:24
@matz3 matz3 force-pushed the fix-manifestEnhancer-invalid-locale branch from b2fe975 to 9aea297 Compare September 12, 2024 13:59
This fixes two problems that could have occurred:

A properties file with an invalid locale was still taken into the list
of supported locales, which then caused a runtime exception in the
ResourceBundle as it validates the input.

Another problem was that properties files could have a valid name
according to BCP47, but the file won't be ever requested with that name.
This is due to the fact that the ResourceBundle does not use the
extension / private use sections of the locale when creating the request
URL.

In both cases, the properties file is now ignored and no entry for the
supportedLocales is created.
sapprc is not a valid supportability locale
@matz3 matz3 force-pushed the fix-manifestEnhancer-invalid-locale branch from 2c592a4 to 2745dba Compare October 7, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants