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

Default Locale not set on Spring Webflux [SPR-15705] #20262

Closed
spring-issuemaster opened this issue Jun 26, 2017 · 11 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Jun 26, 2017

David Harrigan opened SPR-15705 and commented

Hi,

spring-projects/spring-boot#9588

Asked to create the ticket here for exploration. The details of the problem are outlined in the github issue (now closed on Spring Boot), but generally, if using spring-boot-starter-webflux with Thymeleaf 3.0 (and Thymleaf-Spring5 (3.0.7-SNAPSHOT)), the default locale is not being set and thus causes Thymeleaf to bomb out with error (as detailed in the github issue).

I was asked to also provide the thymeleaf template used for testing this. Here it is:

<!DOCTYPE HTML>
<html xmlns:th="http://www.thymeleaf.org">
<body>
Hello <span th:text="${name}">Foo</span>!
</body>
</html>

As you can see, pretty simple :-)

The code in ThymeleafReactiveView, has this snippet:

if (getLocale() == null) {
    return Mono.error(new IllegalArgumentException("Property 'locale' is required"));
}

Please let me know if I can be of further assistance.

-=david=-


Affects: 5.0 RC2

Reference URL: spring-projects/spring-boot#9588

Issue Links:

  • #19602 Introduce a LocaleContextResolver abstraction in WebFlux
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 26, 2017

Sébastien Deleuze commented

David Harrigan Could you please share the repro project as proposed on the original Spring Boot issue?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 26, 2017

David Harrigan commented

No problem will be with you in a few moments after I tidy it up :-)

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 26, 2017

David Harrigan commented

Here you go, be gentle :-)

https://github.com/dharrigan/SPR-15705

Please let me know if I can be of further assistance.

-=david=-

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 26, 2017

Sébastien Deleuze commented

Thanks.

After a quick look, I tend to consider providing a null locale to ViewResolver#resolveViewName is a valid behavior since AcceptHeaderLocaleContextResolver could find no locale header and defaultLocale is null by default. If this is confirmed by your repro project I see 2 fixes to do:

  • On Spring side, annotate ViewResolver#resolveViewName locale parameter with @Nullable
  • On Thymeleaf side, null locale shoud be handled without throwing an exception.

Daniel Fernández Could you please have a look to this issue and to the locale support I introduced as part of this commit in order to give me your advice?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 26, 2017

David Harrigan commented

Hi,

Thank you for taking a quick look, but shouldn't defaultLocale resolve to a locale if none is set in

org.springframework.web.server.i18n.AcceptHeaderLocaleContextResolver
/**
 * Configure a fixed default locale to fall back on if the request does not
 * have an "Accept-Language" header (not set by default).
 * @param defaultLocale the default locale to use
 */
public void setDefaultLocale(Locale defaultLocale) {
     this.defaultLocale = defaultLocale;
}

If no locale is set, but the dependency is changed to spring-boot-starter-web, it works (i.e., the AcceptHeaderLocaleContextResolver manages to get a default locale from the system, even if no header is passed into it on the request - whereas, if using webflux, the ContextResolver doesn't even manage to set a default locale).

-=david=-

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 26, 2017

Sébastien Deleuze commented

In MVC case indeed, ServletRequest#getLocale returns the server locale by default while in WebFlux side we let it to null.

Daniel Fernández Rossen Stoyanchev Any take about what should be the behavior between:

  • Annotating ViewResolver#resolveViewName locale parameter with @Nullable and require ViewResolver and View implementations like Thymeleaf ones to support that
  • Using the server locale as the default one when none is provided in the headers

It could be tempting to do like MVC side but I am not 100% confortable with these magic default values like the one we are discussing here especially now that we declare explicitly nullability of parameters, so I prefer to discuss that before making a choice.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 26, 2017

Juergen Hoeller commented

Please note that I recently refined our locale handling in ViewResolutionResultHandler, FreeMarkerView and co for 5.0 RC3: see 0313363

We consistently call LocaleContextHolder.getLocale(exchange.getLocaleContext()) to obtain a locale either from the request or from the framework's centrally managed default locale: potentially set as LocaleContextHolder.setDefaultLocale, falling back to the system locale (Locale.getDefault()). This is then passed onto ViewResolver, and View implementations like FreeMarkerView use the same pattern when they need a non-null Locale, e.g. for dynamic localized template lookup.

With this in place, it seems better to keep enforcing a non-null Locale in ViewResolver. I'm also about to change ScriptTemplateView to not have a shared setLocale setter but rather obtain the locale the same way from each given request.

Servlet MVC has a similar mechanism in RequestContextUtils.getLocale(HttpServletRequest), taking a LocaleResolver into account and falling back to HttpServletRequest.getLocale(). While this is somewhat analogous, I find it cleaner in WebFlux now: The request indicates whether it has any language header associated with it, and we apply the fallback through LocaleContextHolder.getLocale. Processing code can therefore cleanly differentiate between those two scenarios, with no "server locale" getting in the way at the request level.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 26, 2017

Juergen Hoeller commented

We may of course discuss whether ViewResolver should see the default Locale in this case, or should rather indicate that no locale information has been given in a particular request. My take is that ViewResolver is not designed as a request-specific representation: it is rather a lookup SPI for view name plus locale, with no request appearing in its signature at all. Such a lookup could also happen at startup time, or whenever else a view name has been configured and can be pre-resolved. In such a contract, I'd rather have non-null keys passed in. Whereas on ServerWebExchange, the exposed LocaleContext is clearly request-specific and should ideally represent metadata from the actual request only, as it does in our current arrangement.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 8, 2017

Daniel Fernández commented

I'm sorry, somehow I have not been getting email notifications for the mentions in this thread.

I've been able to replicate the error reported by the user with Spring Boot 2.0.0.M2, but it seems to work fine using Spring Boot 2.0.0.BUILD-SNAPSHOT.

I see that in current code ViewResolutionResultHandler#handleResult(exchange,result) calls LocaleContextHolder.getLocale(exchange.getLocaleContext()) (as can be seen here)) and then uses the obtained locale to (indirectly) call ViewResolver#resolveViewName(viewName,locale), so I suppose I'm right in assuming that no null locale can arrive at ViewResolver#resolveViewName(...) with the current code.

Also, I see that the current version of FreeMarkerViewResolver#resolveViewName(viewName,locale), inherited from UrlBasedViewResolver, doesn't really pass on the locale that it receives to the FreeMarkerView instance that it creates, but rather lets the new view object resolve the locale to be applied again by means of calling LocaleContextHolder.getLocale(exchange.getLocaleContext()) once more in FreeMarkerView#renderInternal(...) (as can be seen here).

Thymeleaf's approach to this is different, as it is the ThymeleafReactiveViewResolver who explicitly sets the locale into the new ThymeleafReactiveView object by means of calling ThymeleafReactiveView#setLocale(locale) (unless the view object has been specifically configured to use a different locale and override anything coming from Spring). This can be seen here.

So even if the approach to how the locale gets to the view object is different to that of FreeMarker's artifacts, the result will always be the same as long as the fact that the locale argument in ViewResolver#resolveViewName(viewName,locale) is obtained from LocaleContextHolder.getLocale(exchange.getLocaleContext() as it is now.

So, can I assume this and therefore consider that Thymeleaf will be receiving no null locales at resolveViewName(...)? If so, I believe no changes would be required at the Thymeleaf side…

Juergen, if I understand correctly you talk about ViewResolver and View operation potentially not happening in the same context and only the View being request-bound, but… am I right in supposing that View instances are not meant to be reused between requests (even if they are for the same view name and locale)? and if so, isn't the execution of ViewResolver#resolveViewName(...) also request-bound? Actually, I believe this would match nicely with the fact that the locale being used there as argument actually comes from a LocaleContextHolder.getLocale(exchange.getLocaleContext() call in which the exchange object is involved, so there seems to be some binding to the request…

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 12, 2017

Juergen Hoeller commented

There's another perspective to this as well: Arguably the main reason for ViewResolver featuring a Locale argument is ResourceBundleViewResolver which loads a localized view definition file that it creates View instances from.

Simply speaking, ViewResolver's Locale argument is there for resolution into distinct View instances per language, to the degree of even different View implementation classes per language. This could be sourced from localized view implementation classes, or the resolver could build View instances from the same class but with a pre-set locale property, depending on the implementation strategy of the particular resolver. In all such scenarios, such a resolver has to cache View instances per view name plus locale, as implemented by default in AbstractCachingViewResolver.

However, for most ViewResolver implementations - in particular UrlBasedViewResolver subclasses -, the given resolution-time Locale argument isn't really relevant. Those resolvers build a canonical View instance per view name, cached per view name only (i.e. not per locale), as implemented in UrlBasedViewResolver. The actual View instances may still support localization but here at invocation time, i.e. from a request-derived locale. This is the implementation strategy used by FreeMarkerView and FreeMarkerViewResolver.

In any case, resolveViewName will never receive null for the Locale argument. It's just that a particular implementation may choose to ignore the locale, always returning the same view resource for a given view name.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 17, 2017

Sébastien Deleuze commented

I have validated with the repro project that since this commit only non-null Locale is passed to resolveViewName as explained by Juergen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.