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: Default I18N provider #17823

Merged
merged 15 commits into from
Oct 18, 2023
Merged

feat: Default I18N provider #17823

merged 15 commits into from
Oct 18, 2023

Conversation

caalador
Copy link
Contributor

Add implementation for a I18N
provider that is loaded if there
are translation properties files
in the defined classpath folder.

If a custom I18N is set default will
never be loaded.

Fixes #5917

Add implementation for a I18N
provider that is loaded if there
are translation properties files
in the defined classpath folder.

If a custom I18N is set default will
never be loaded.

Fixes #5917
@github-actions
Copy link

github-actions bot commented Oct 12, 2023

Test Results

1 013 files  +  3  1 013 suites  +3   1h 2m 35s ⏱️ -16s
6 487 tests +10  6 446 ✔️ +11  41 💤 ±0  0  - 1 
6 743 runs  +  9  6 695 ✔️ +10  48 💤 ±0  0  - 1 

Results for commit ebfa2f3. ± Comparison against base commit 5a62cc9.

♻️ This comment has been updated with latest results.

return key;
}
if (params.length > 0) {
value = MessageFormat.format(value, params);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to move this in a protected method to allow overwriting this to e.g. implement a caching mechanism? MessageFormat can be quite expensive.

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 don't think the default implementation brings in so much functionality that it would make sense to override it instead of just implementing your own I18NProvider as you need to anyway set it as you do at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just wanted to ask if you wanna also implement a PropertyPlaceholderResolver in here, with your comment.. I think I know the answer :D Reference of the resolver used by Spring: https://github.com/spring-projects/spring-framework/blob/main/spring-core/src/main/java/org/springframework/util/PropertyPlaceholderHelper.java

/**
* Default i18n provider that will be initialized if
*/
public class DefaultI18NProvider implements I18NProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

General Question: Should this class be registered as Spring Bean with ConditionalOnMissingBean so that it's also available for DI in Spring as default and people can inject / use it even without always asking the Instantiator for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The springInitiator actually falls back on the defaultInitiator on this if no bean found:

    public I18NProvider getI18NProvider() {
        int beansCount = context.getBeanNamesForType(I18NProvider.class).length;
        if (beansCount == 1) {
            return context.getBean(I18NProvider.class);
        } else {
            if (loggingEnabled.compareAndSet(true, false)) {
                LoggerFactory.getLogger(SpringInstantiator.class.getName())
                        .info("The number of beans implementing '{}' is {}. Cannot use Spring beans for I18N, "
                                + "falling back to the default behavior",
                                I18NProvider.class.getSimpleName(), beansCount);
            }
            return super.getI18NProvider();
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also having Spring annotations is a bit problematic as this is outside of spring in the normal framework.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation! Yeah, I was wondering if it would be worth to add it to the VaadinApplicationConfiguration like a bean:

  @Bean
  @ConditionalOnMissingBean(value = I18NProvider.class)
  public DefaultI18NProvider vaadinI18nProvider() {
    return new DefaultI18NProvider(/* get locales from somewhere*/);
  }

Comment on lines 52 to 53
File bundleFolder = new File(resource.getFile());
if (bundleFolder.exists() && bundleFolder.isDirectory()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a known issue, but this seems not to work when running the application as a Spring Boot fat JAR

Copy link
Collaborator

Choose a reason for hiding this comment

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

And it seems not to work even if I put the translations in a JAR file, although I don't know if this can be considered a valid use case for this feature.

bundleFolder= file:/xxxxxxxxx/vaadin/starters/skeleton-starter-flow/v-i18n.jar!/vaadin-i18n
bundleFolder.exists() = false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good Point. I'll look into it and fix the jar path. It will require using the FileSystem, but should be doable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it knows how to handle the files inside jars also.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Works great with the resource bundle in JAR!

However, is still fails with the Spring Boot fat JAR, both by adding the properties files in the project or in external JAR.

bundleFolder = {File@6570} "file:/xxxx/skeleton-starter-flow-spring/target/spring-skeleton-1.0-SNAPSHOT.jar!/BOOT-INF/classes!/vaadin-i18n"
pathInJar = "/BOOT-INF/classes!/vaadin-i18n"


bundleFolder = {File@8348} "file:/xxxx/skeleton-starter-flow-spring/target/spring-skeleton-1.0-SNAPSHOT.jar!/BOOT-INF/lib/i18n-1.0.jar!/vaadin-i18n"
pathInJar = "/BOOT-INF/lib/i18n-1.0.jar!/vaadin-i18n"

For the first case, removing the ! from pathInJar makes the solution work. The second case is more tricky.
I wonder if it could be better to provide some specific implementations in the Spring add-on, or if we have to find some hack
to add in the current implementation 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to go with a spring based solution in the spring module based on

ResourcePatternResolver allowing to get all resources within a folder in a spring boot application within one line of code - foul and future proof :) relying on it for years to load our default configurations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seeing as this was supposed to be a small enhancement, I would propose to open a new ticket for creating Spring-Boot fat Jar support by extending the default translation with Spring parts in the spring add-on

Copy link
Collaborator

@mcollovati mcollovati Oct 16, 2023

Choose a reason for hiding this comment

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

It looks like we need a ticket as well for the Quarkus add-on.
The class loader used to detect the bundle folder ( I18NUtil.class.getClassLoader()) returns null, whereas it works using Thread.currentThread().getContextClassLoader()
Would it be possible to replace the protected static ClassLoader getClassLoader() method with a non-static version, so it can be overridden to provide the best class loader for each environment?
Or can we have the class loader reference as class member, provided to additional constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use current as that's not a problem. Sending a loader around becomes too complex for testing from instantiator

Copy link
Collaborator

Choose a reason for hiding this comment

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

Works fine in Quarkus now 👍

update filter
add missing new lines
update test with Mockito.CALLS_REAL_METHODS to simplify testing
Update jar handling.
mcollovati
mcollovati previously approved these changes Oct 16, 2023
@sonarcloud
Copy link

sonarcloud bot commented Oct 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@caalador caalador merged commit 3ecb95e into main Oct 18, 2023
26 checks passed
@caalador caalador deleted the issues/5917-default-i18n branch October 18, 2023 05:27
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.3.0.alpha1 and is also targeting the upcoming stable 24.3.0 version.

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.

Provide a default I18NProvider
5 participants