Add internationalization support #133

jpraet · 2024-11-04T08:17:22Z

Fixes #124.

jpraet · 2024-11-04T08:28:14Z

@pvdbosch I have some questions related to this PR:

Current implementation only considers the first value in Accept-Language header, and falls back to English if it's not supported. Is that OK, or should we consider the full list of prioritized languages?
Should we return HTTP 406 Not Acceptable if requested language is not supported? I would say no: it's not because the language is not supported by the problem library, that it couldn't be supported by the API itself.
Should we set Content-Language response header? Again there could be some differences between the languages supported by the problem library, and those supported by the API. But maybe we can set Content-Language response header for problem responses only.
Should we set Vary: Accept-Language response header? Are error responses typically cached?

pvdbosch · 2024-11-04T09:34:55Z

Hi @jpraet , I'd abstract away resolving the locale, including setting response Content-Language or Vary headers.
Internationalization is a separate API-wide concern and shouldn't be the responsibility of a problem library.

Some framework support may be used:

If an API supports an explicit way of specifying the language (e.g. ?lang=...), this usually has precedence.
From MDN :

This header serves as a hint when the server cannot determine the target content language otherwise (for example, use a specific URL that depends on an explicit user decision).

All languages in the list should be considered (with q values), but framework support should best be used to resolve them.

I agree to continue with a default language when no match found:

The server may send back a 406 Not Acceptable error code when unable to serve content in a matching language, but this is rarely implemented. Servers often ignore the Accept-Language header in such cases and send a successful response with the most appropriate resource instead for a better user experience.

jpraet · 2024-11-04T10:09:27Z

For Spring Boot I use RequestContextUtils.getLocale() which should delegate to the configured LocaleResolver.

Maybe the Belgif REST Guide should be adapted, it currently says "In case the server could not honor any of the requested languages, it SHOULD return a 406 Not Acceptable error."
-> change SHOULD to MAY?

pvdbosch · 2024-11-04T14:07:01Z

For Spring, could LocaleContextHolder be used instead?

From RequestContextUtils javadoc:

Consider using LocaleContextHolder.getLocale() which will normally be populated with the same Locale.

It should also work outside a servlet environment, and wouldn't require registering a filter or syncing with a threadlocal variable.

I believe resolving and setting the locale shouldn't be part of the rest-problem-java library so APIs could choose their own logic e.g. based on the authenticated user's language preference. Perhaps in a separate library if the framework doesn't provide an abstraction.
For Jakarta EE, there is Jakarta MVC support but that's probably more for UIs rather than REST APIs and it isn't part of Jakarta EE 10.

pvdbosch · 2024-11-04T14:13:51Z

Maybe the Belgif REST Guide should be adapted, it currently says "In case the server could not honor any of the requested languages, it SHOULD return a 406 Not Acceptable error." -> change SHOULD to MAY?

Agreed, or recommend instead to continue with a supported language. Created belgif/rest-guide#205

jpraet · 2024-11-04T15:08:25Z

For Spring, could LocaleContextHolder be used instead?

I will look into it. Something like a pluggable LocaleResolver then, configurable via /META-INF/services/io.github.belgif.rest.problem.i18n.LocaleResolver? Or did you have something else in mind?

(sonar already complains about it)

pvdbosch · 2024-11-05T13:38:11Z

Something like a pluggable LocaleResolver then, configurable via /META-INF/services/io.github.belgif.rest.problem.i18n.LocaleResolver? Or did you have something else in mind?

Yes, rest-problem-java should only depend on "consulting the current locale", but not on any logic that deduces it from request or user context.

These aspects should probably be best in a separate I18N module for JEE; because it's not really logical to search for such functionality in a rest-problem module. Maybe we should consider having a belgif-rest-java-sdk as root project that bundles modules...

jpraet · 2024-11-05T14:14:19Z

These aspects should probably be best in a separate I18N module for JEE; because it's not really logical to search for such functionality in a rest-problem module. Maybe we should consider having a belgif-rest-java-sdk as root project that bundles modules...

Current implementation has pluggable LocaleResolver, which on Spring Boot delegates to LocaleContextHolder.
On JEE it does currently provide a default implementation that derives it from Accept-Language HTTP header, but can be overridden with a custom LocaleResolver implementation by the implementation. Note that Bean Validation (at least on JBoss / WildFly) also seems to be using the Locale from Accept-Language HTTP header by default.

belgif-rest-java-sdk would be in a separate repository?

belgif-rest-problem/src/main/resources/io/github/belgif/rest/problem/Messages_fr.properties

pvdbosch · 2024-11-05T15:22:41Z

On JEE it does currently provide a default implementation that derives it from Accept-Language HTTP header, but can be overridden with a custom LocaleResolver implementation by the implementation. Note that Bean Validation (at least on JBoss / WildFly) also seems to be using the Locale from Accept-Language HTTP header by default.

Ok, seems like hibernate wrote their own "LocaleResolver" SPI, which integrates with RestEasy.

Too bad that this isn't standardized in JEE, so we could reuse it. If the locale resolver is overridable by JEE applications, it's probably sufficient then not to warrant another module.

belgif-rest-java-sdk would be in a separate repository?

I was thinking about a repo to group all possible belgif rest java modules ( incl rest-problem-java, i18n, possible future validation lib, ...). But if internationalization can be done without an additional module, probably not needed.

jflabatBCSS

I didn't review the NL and DE translations :-)

Peter talks about a separate module of -problem , are you planning to more everything afterwards

belgif-rest-problem/src/main/resources/io/github/belgif/rest/problem/Messages_fr.properties

...java-ee/src/main/java/io/github/belgif/rest/problem/jaxrs/i18n/I18NAcceptLanguageFilter.java

...-ee/src/test/java/io/github/belgif/rest/problem/jaxrs/i18n/I18NAcceptLanguageFilterTest.java

...lem-spring/src/test/java/io/github/belgif/rest/problem/spring/i18n/I18NConfiguratorTest.java

jpraet · 2024-11-28T15:52:58Z

Peter talks about a separate module of -problem, are you planning to move everything afterwards

Not planned at this point.

…124-i18n

belgif-rest-problem/src/main/resources/io/github/belgif/rest/problem/Messages_fr.properties

jpraet · 2024-12-11T10:56:14Z

There's a failure on quarkus:

 Error:  Failed to execute goal io.quarkus.platform:quarkus-maven-plugin:3.17.3:build (default) on project belgif-rest-problem-quarkus-it: Failed to build quarkus application: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
Error: [ERROR] 	[error]: Build step io.quarkus.arc.deployment.ArcProcessor#validate threw an exception: jakarta.enterprise.inject.spi.DeploymentException: jakarta.enterprise.inject.UnsatisfiedResolutionException: Unsatisfied dependency for type jakarta.servlet.ServletContext and qualifiers [@Default]
Error:  	- injection target: io.github.belgif.rest.problem.jaxrs.i18n.I18NAcceptLanguageFilter#servletContext
Error:  	- declared on CLASS bean [types=[jakarta.ws.rs.container.ContainerRequestFilter, io.github.belgif.rest.problem.jaxrs.i18n.I18NAcceptLanguageFilter, jakarta.ws.rs.container.ContainerResponseFilter, java.lang.Object], qualifiers=[@Default, @Any], target=io.github.belgif.rest.problem.jaxrs.i18n.I18NAcceptLanguageFilter]

clone1612 · 2024-12-12T12:38:11Z

src/main/asciidoc/index.adoc

+[[i18n]]
+== Internationalization (I18N)
+
+The detail messages of Belgif problem and issue types can be localized to the language requested in the https://www.belgif.be/specification/rest/api-guide/#i18n-negotiation[Accept-Language HTTP request header].


We have a LocaleResolver interface that determines the language to use, clients could in theory provide a custom implementation that has nothing to do with the header (unlikely though). Shouldn't we describe or at least mention the locale resolver logic as we provide default implementations for Spring and Java EE?

Added some documentation on LocaleResolver.

belgif-rest-problem/src/main/java/io/github/belgif/rest/problem/i18n/I18N.java

...java-ee/src/main/java/io/github/belgif/rest/problem/jaxrs/i18n/I18NAcceptLanguageFilter.java

clone1612 · 2024-12-12T13:33:26Z

...est-problem-it-common/src/main/java/io/github/belgif/rest/problem/AbstractRestProblemIT.java

+    @Test
+    void i18n() {
+        getSpec().when()
+                .header("Accept-Language", "nl-BE")


Could be interesting to add an additional test that verifies behavior with multiple values that use weighting

Test added. Note that we just take the one with the highest weight though, and if that one is not supported, it will fallback to English. So with Accept-Language = "fr-BE;q=0.5, es;q=0.7" the resulting language will be English, not French.

clone1612 · 2024-12-12T13:50:16Z

...est-problem-it-common/src/main/java/io/github/belgif/rest/problem/AbstractRestProblemIT.java

@@ -338,4 +339,39 @@ void constraintViolationBodyInheritance() {
                .body("issues[1].detail", equalTo("must not be blank"));
    }



We never test localization can be disabled, would it be easy to add a test for that?

I guess we could expose an endpoint that calls I18N.setEnabled(false).

...-ee/src/test/java/io/github/belgif/rest/problem/jaxrs/i18n/I18NAcceptLanguageFilterTest.java

clone1612 · 2024-12-12T14:10:02Z

belgif-rest-problem/src/main/java/io/github/belgif/rest/problem/i18n/I18N.java

+     * Checks whether I18N should be enabled, based on system property / environment variable.
+     */
+    static void init() {
+        if (System.getProperty(I18N_FLAG) != null) {


Isn't this code Java EE specific? For Spring we always let Spring look for the property in all its locations through @ConfigurationProperties and set its state here with I18NConfigurator.

The only scenario where this method is relevant is for a Java EE application where it wasn't provided as a servlet context parameter.

Moved to java ee I18NConfigurator.

clone1612 · 2024-12-12T14:17:17Z

belgif-rest-problem/src/main/java/io/github/belgif/rest/problem/api/InputValidationIssues.java

@@ -67,7 +69,7 @@ public static InputValidationIssue schemaViolation(InEnum in, String name, Objec

    public static InputValidationIssue unknownInput(InEnum in, String name, Object value) {
        return new InputValidationIssue(ISSUE_TYPE_UNKNOWN_INPUT, "Unknown input")
-                .detail(String.format("Input %s is unknown", name))
+                .detail(I18N.getLocalizedString("unknownInput.detail", name))


Would it be worth it to add a localizedDetail method, similar to I18N.getLocalizedDetail(...)? Would make the code less verbose + remove duplication of ".detail" part of the bundle key.

clone1612 · 2024-12-12T14:23:38Z

belgif-rest-problem/src/main/resources/io/github/belgif/rest/problem/Messages_nl.properties

Is there a risk we ever forget to keep the files in sync? Shouldn't happen with a good review though, but was thinking about adding a test that verifies all messages are defined in all languages.

If we ever forget an entry it would result in a MissingResourceException and cause the request to fail, but risk seems limited.

Added a check for missing keys.

(ServletContext injection for JAX-RS provider does not work on Quarkus)

…est-problem-java-ee

sonarqubecloud · 2024-12-17T10:46:21Z

Quality Gate passed

Issues
0 New issues
8 Accepted issues

Measures
0 Security Hotspots
94.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Add internationalization support

b606b1a

jpraet added 6 commits November 5, 2024 10:03

Merge branch 'main' into feature/#124-i18n

4b3a1c1

Add french and german translations (by CBSS translation service)

d4d0770

Pluggable LocaleResolver

f585c5c

Remove obsolete dependency

73997eb

Locale constructor is deprecated in JDK21

a683b8d

(sonar already complains about it)

Sonar

075d8f5

jpraet marked this pull request as ready for review November 5, 2024 14:04

jpraet requested a review from a team November 5, 2024 14:14

jpraet commented Nov 5, 2024

View reviewed changes

belgif-rest-problem/src/main/resources/io/github/belgif/rest/problem/Messages_fr.properties Show resolved Hide resolved

jpraet added 3 commits November 6, 2024 14:20

Merge branch 'main' into feature/#124-i18n

ec89168

Merge branch 'main' into feature/#124-i18n

4508118

Merge branch 'main' into feature/#124-i18n

07b96a1

jflabatBCSS requested changes Nov 28, 2024

View reviewed changes

jflabatBCSS and others added 6 commits November 29, 2024 07:56

Correct french translations

84fcb31

Merge remote-tracking branch 'origin/feature/#124-i18n' into feature/#…

1a091fa

…124-i18n

handle review

170d04a

Fix french translations after review of @usku01

66412bd

Merge branch 'main' into feature/#124-i18n

a8103be

Minor message tweaks

d0d46b4

jpraet commented Dec 9, 2024

View reviewed changes

belgif-rest-problem/src/main/resources/io/github/belgif/rest/problem/Messages_fr.properties Outdated Show resolved Hide resolved

jpraet requested a review from jflabatBCSS December 9, 2024 15:14

jpraet and others added 3 commits December 9, 2024 17:53

Merge branch 'main' into feature/#124-i18n

28ab016

Fix translation of zeroOrExactlyOneOfExpected.detail

6028ec9

Merge branch 'main' into feature/#124-i18n

58c298f

jpraet requested a review from clone1612 December 11, 2024 10:20

clone1612 requested changes Dec 12, 2024

View reviewed changes

jpraet added 12 commits December 14, 2024 11:53

Add check from missing keys in I18N bundle

0bee5cc

Use Locale.ENGLISH

238ceb7

Extract I18NConfigurator from I18NAcceptLanguageFilter

2a483e1

(ServletContext injection for JAX-RS provider does not work on Quarkus)

Disable I18N tests on Quarkus (not working)

f2a0a56

Add i18nDisabled test

7d53e5c

Move system property / environment property configuration to belgif-r…

5cd75d6

…est-problem-java-ee

Add test for weighted Accept-Language header

3cd8f4c

Document LocaleResolver

c77f154

Support I18N on Quarkus

7587a79

Add integration tests for unsupported language

cf9795a

Add integration test for disabled I18N

8367304

Add InputValidationIssue.localizedDetail()

a7e75e4

jpraet requested a review from clone1612 December 16, 2024 15:10

Merge branch 'main' into feature/#124-i18n

16543b8

jflabatBCSS approved these changes Dec 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add internationalization support #133

Add internationalization support #133

jpraet commented Nov 4, 2024

jpraet commented Nov 4, 2024

pvdbosch commented Nov 4, 2024

jpraet commented Nov 4, 2024

pvdbosch commented Nov 4, 2024

pvdbosch commented Nov 4, 2024

jpraet commented Nov 4, 2024

pvdbosch commented Nov 5, 2024

jpraet commented Nov 5, 2024

pvdbosch commented Nov 5, 2024

jflabatBCSS left a comment

jpraet commented Nov 28, 2024

jpraet commented Dec 11, 2024

clone1612 Dec 12, 2024

jpraet Dec 16, 2024

clone1612 Dec 12, 2024 •

edited

Loading

jpraet Dec 16, 2024

clone1612 Dec 12, 2024

jpraet Dec 16, 2024

clone1612 Dec 12, 2024

jpraet Dec 16, 2024

clone1612 Dec 12, 2024 •

edited

Loading

clone1612 Dec 12, 2024

jpraet Dec 16, 2024

sonarqubecloud bot commented Dec 17, 2024

		@@ -338,4 +339,39 @@ void constraintViolationBodyInheritance() {
		.body("issues[1].detail", equalTo("must not be blank"));
		}

Add internationalization support #133

Are you sure you want to change the base?

Add internationalization support #133

Conversation

jpraet commented Nov 4, 2024

jpraet commented Nov 4, 2024

pvdbosch commented Nov 4, 2024

jpraet commented Nov 4, 2024

pvdbosch commented Nov 4, 2024

pvdbosch commented Nov 4, 2024

jpraet commented Nov 4, 2024

pvdbosch commented Nov 5, 2024

jpraet commented Nov 5, 2024

pvdbosch commented Nov 5, 2024

jflabatBCSS left a comment

Choose a reason for hiding this comment

jpraet commented Nov 28, 2024

jpraet commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clone1612 Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clone1612 Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 17, 2024

Quality Gate passed

clone1612 Dec 12, 2024 •

edited

Loading

clone1612 Dec 12, 2024 •

edited

Loading