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

feat: add translation file request handler #18680

Merged
merged 59 commits into from
Mar 1, 2024

Conversation

ugur-vaadin
Copy link
Contributor

@ugur-vaadin ugur-vaadin commented Feb 12, 2024

Description

This PR adds a request handler to serve translation properties files. For the request to be handled by this handler, the request type should be i18n. There is one optional parameter to construct the locale: langtag. If a translation properties file that corresponds to the locale, the response contains the translation entries in JSON format. Otherwise, it contains the default file or returns a not found response. The default file is the bundle for the root locale. The system default locale is not considered to be the default, and is not used for fallback. The response header also contains information regarding the retrieved file locale.

Fixes #18641

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.
  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

Copy link

github-actions bot commented Feb 12, 2024

Test Results

1 096 files  + 1  1 096 suites  +1   1h 18m 48s ⏱️ +42s
6 956 tests +17  6 907 ✅ +17  49 💤 ±0  0 ❌ ±0 
7 312 runs  +19  7 251 ✅ +19  61 💤 ±0  0 ❌ ±0 

Results for commit 543417f. ± Comparison against base commit 34aaa33.

♻️ This comment has been updated with latest results.

return ResourceBundle.getBundle(DefaultI18NProvider.BUNDLE_PREFIX,
locale, I18NUtil.getClassLoader());
} catch (MissingResourceException e) {
getLogger().warn("Missing resource bundle for "
Copy link
Contributor

@knoobie knoobie Feb 12, 2024

Choose a reason for hiding this comment

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

Even tho this is a direct copy of

+ " and locale " + locale.getDisplayName(), e);
- I would suggest to lower the severity of the logging here to info or lower.

The other provider is instantiated with the used locales on the server side, making a missing resource bundle defined by a server side locale by the developer more severe than a possible (random) locally supplied by a user via http headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially lowered the severity. However, with the final implementation, we want the handler to return any available default bundle if the requested bundle is not found. So maybe it could be better to have a warning log in this case, since it also means that there is no default translation too. I mentioned that there is no default bundle to fall back on in the log message.

request.getParameter(LANGUAGE_PARAMETER_NAME), "");
String country = Objects.requireNonNullElse(
request.getParameter(COUNTRY_PARAMETER_NAME), "");
return new Locale(language, country);
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there some special consideration for using separate identifiers for language and country instead of the language tag? In the frontend you would usually just want to have a single identifier for a language, such as "en-US". So that means we would have to split up the tag on the frontend and then join it together again on the server. Not a big deal, but might not be necessary. In general I think we're pretty flexible with this and can change this later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR to use a single parameter, langtag. It is parsed on the server side, and can handle all the locales supported by Java Locale.

Locale locale = getLocale(request);
ResourceBundle translationPropertyFile = getTranslationPropertyFile(
locale);
if (translationPropertyFile == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

From the perspective of the client we need to have one set of translations in order to for an app to display something to the user. Returning a not found is problematic:

  • The client would first make a request for the identified language
  • Gets a 404, and then does what?
  • Maybe tries to load a fallback language, which would result in an additional request. That would mean it takes even longer before the app can display something to the user. Also the fallback language configured in the client might not exist either.

I was thinking the server should return a "default" set of translations whenever it doesn't find a set of translations for the specified language. That means there would only ever be one request before the app can display something. In terms of Java resource bundles, that would probably mean we send the contents of the default / root properties file. The response would then also need to contain some information for which language the set of translations are actually for, so the client knows that it received the fallback instead of the thing it requested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR to include the language tag for the retrieved bundle. The handler returns the default file if it is available, and file not found response is only the case when there is no default to return. The language tag is added to the response with the header name Retrieved-locale.

Copy link
Contributor

Choose a reason for hiding this comment

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

So it does return a default bundle now, though it might not be the one you expect. What seems to happen now is:

  • If there is a bundle for the server locale (Locale.getDefault()), it sends that
  • Otherwise it sends the root bundle (translation.properties)

Since Flow's DefaultI18nProvider uses the same method call, I would assume it works the same way, but it would be good to double check. Otherwise we might want to have a discussion what the fallback should be. Should it consider the server locale as fallback, or should it always use the root bundle? If we consider the server locale it could return different results in different environments. On the other hand I don't know what the best practice is for the root bundle. Would it be a safe assumption that the root bundle contains translations for the default language? An alternative could be to let developers explicitly specify a fallback language of which they know that a bundle exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests with mocked system default locale that confirm this behaviour. I will modify the tests based on the decisions on whether to introduce a specified fallback locale and the use of system default locale in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR to have only a single fallback locale. It is currently the root locale and not customizable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to work well, also still handles fallbacks such as from de -> de_DE and vice versa.

@mshabarov mshabarov added the Contribution PRs coming from the community or external to the team label Feb 15, 2024
@vaadin-bot vaadin-bot added +1.0.0 and removed +0.1.0 labels Feb 20, 2024
@ugur-vaadin ugur-vaadin force-pushed the feat-add-translation-file-request-handler branch from e80ddfe to 6637608 Compare February 21, 2024 13:28
@ugur-vaadin ugur-vaadin marked this pull request as ready for review February 21, 2024 13:28
@vaadin-bot vaadin-bot added +0.1.0 and removed +1.0.0 labels Feb 21, 2024
@mshabarov mshabarov self-requested a review February 22, 2024 06:56
sissbruecker
sissbruecker previously approved these changes Feb 26, 2024
Copy link
Contributor

@sissbruecker sissbruecker left a comment

Choose a reason for hiding this comment

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

LGTM, Flow team should have a final look at the changes.

@vaadin-bot vaadin-bot added +0.1.0 and removed +1.0.0 labels Mar 1, 2024
@vaadin-bot vaadin-bot added +1.0.0 and removed +0.1.0 labels Mar 1, 2024
mcollovati
mcollovati previously approved these changes Mar 1, 2024
@mcollovati mcollovati enabled auto-merge (squash) March 1, 2024 14:56
auto-merge was automatically disabled March 1, 2024 15:20

Head branch was modified

@mcollovati mcollovati enabled auto-merge (squash) March 1, 2024 15:32
Copy link

sonarcloud bot commented Mar 1, 2024

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@mcollovati mcollovati merged commit 3fec9e5 into main Mar 1, 2024
25 of 26 checks passed
@mcollovati mcollovati deleted the feat-add-translation-file-request-handler branch March 1, 2024 15:54
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contribution PRs coming from the community or external to the team Released with Vaadin 24.4.0 +1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement an API for getting translation properties file
7 participants