spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
55.48k stars 37.71k forks source link

Spring MVC's locale resolver can no longer be customized in parent context #25290

Closed wilkinsona closed 3 years ago

wilkinsona commented 3 years ago

Affects: 5.3 snapshots

The fix for #25209 appears to have removed or at least limited the ability to customize the locale resolver that is used by the DispatcherServlet.

The introduction of a default LocaleResolver bean means that Spring Boot's auto-configuration of a LocaleResolver backs off as it is presumed that the bean that's now present is one provided by the user. This means that the spring.mvc.locale and spring.mvc.locale-resolver properties are no longer honoured.

This change will also break any Spring Boot user who has defined their own localeResolver bean as it would attempt to override the bean defined by WebMvcConfigurationSupport. This will fail as bean definition overriding is disabled by default in Boot.

sdeleuze commented 3 years ago

@wilkinsona @bclozel I would like to discuss with you in order to see if there is way to keep achieving the goal of #25209 which is important for our GraalVM story and an interesting improvement on the JVM as well since it avoids to load DispatcherServlet.properties by default and improve consistency of how Spring MVC beans are managed.

On Spring Framework side, I should indeed probably remove this newly added LocaleResolver bean in order to let Boot or users customize it more easily.

The problem is that with current Boot autoconfiguration, DispatcherServlet#getDefaultStrategies is invoked, DispatcherServlet.properties is parsed and a reflection call invisible to GraalVM native compiler happens for this single bean since no LocaleResolver bean is defined by default.

Current configuration in WebMvcAutoConfiguration.WebMvcAutoConfigurationAdapter#localeResolver only configures a LocaleResolver bean when the user configure it because of the double @ConditionalOnMissingBean + @ConditionalOnProperty(prefix = "spring.mvc", name = "locale") conditions.

I would need Boot to configure a default AcceptHeaderLocaleResolver bean by default as well. Is it possible?

wilkinsona commented 3 years ago

Yeah, I think we could do that. We document that, by default, we'll use the Accept-Language header to resolve the locale. This relies on DispatcherServlet using AcceptHeaderLocaleResolver by default. It would be safer if Boot configured that default instead as we then wouldn't have to worry about an (unlikely) change to the DispatcherServlet default.

sdeleuze commented 3 years ago

As pointed out by @jhoeller, in WebFluxConfigurationSupport we create a LocaleContextResolver and provide a protected method to customize it.

protected LocaleContextResolver createLocaleContextResolver() {
    return new AcceptHeaderLocaleContextResolver();
}

@Bean
public LocaleContextResolver localeContextResolver() {
    return createLocaleContextResolver();
}

Why this out-of-the-box definition of a locale resolver is a problem with MVC but not with WebFlux? Does Boot auto-configure locale resolution with WebFlux? If yes, then how? Could we use similar arrangement in order to allow Boot to configure the LocaleReolver bean used?

I am asking that because it would be more consistent and would avoid Spring Framework to introduce a dedicated config subclass like shown below for Spring MVC without Spring Boot purpose:

@Configuration(proxyBeanMethods = false)
public class DefaultWebMvcConfiguration extends DelegatingWebMvcConfiguration {
    @Bean
    public LocaleResolver localeResolver() {
        return new AcceptHeaderLocaleResolver();
    }
}

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
@Documented
@Import(DefaultWebMvcConfiguration.class)
public @interface EnableWebMvc {
}
wilkinsona commented 3 years ago

We don't have any support for customising WebFlux's LocaleContextResolver at the moment. I don't think there's any specific reason for that and we could add some, but it wouldn't be as flexible as what is possible with Spring MVC in Framework 5.2.

If MVC switched to the WebFlux approach, users would no longer be able to provide their own LocaleResolver bean. It would require overriding the MVC-configured bean and bean overriding is disabled by default in Boot. This change in behaviour would be a regression from what was possible with Boot 2.3.

sdeleuze commented 3 years ago

As discussed with @wilkinsona today, it seems possible to turn those newly added beans into Spring Boot compliant ones.

In a nutshell, in Boot it is possible override WebMvcConfigurationSupport beans with @ConditionalOnMissingBean and similar Boot annotations, something like:

public static class EnableWebMvcConfiguration extends DelegatingWebMvcConfiguration implements ResourceLoaderAware {

        @Bean
        @ConditionalOnMissingBean
        @ConditionalOnProperty(prefix = "spring.mvc", name = "locale", matchIfMissing = true)
        @Override
        public LocaleResolver localeResolver() {
            if (this.mvcProperties.getLocaleResolver() == WebMvcProperties.LocaleResolver.FIXED) {
                return new FixedLocaleResolver(this.mvcProperties.getLocale());
            }
            AcceptHeaderLocaleResolver localeResolver = new AcceptHeaderLocaleResolver();
            localeResolver.setDefaultLocale(this.mvcProperties.getLocale());
            return localeResolver;
        }

        @Bean
        @ConditionalOnMissingBean
        @Override
        public ThemeResolver themeResolver() {
            return super.themeResolver();
        }

        @Bean
        @ConditionalOnMissingBean
        @Override
        public FlashMapManager flashMapManager() {
            return super.flashMapManager();
        }

        @Bean
        @ConditionalOnMissingBean
        @Override
        public RequestToViewNameTranslator viewNameTranslator() {
            return super.viewNameTranslator();
        }
}

The original LocaleResolver bean declaration in Spring Boot should be removed since replaced by the one above.

3 possibilities or LocaleResolver:

This also allows ThemeResolver, FlashMapManager and RequestToViewNameTranslator easy override by the user via declaring a bean with the name name.

As a consequence I close this issue, please reopen in case of blocking point.

schuch commented 3 years ago

I just tried to migrate a "classic" Spring MVC Webapp to 5.3.0

It has a custom LocaleResolver defined in applicationContext.xml

<bean id="localeResolver" class="...CustomLocaleResolver" />

This config is no longer honored. Feels like a breaking change to me.

sdeleuze commented 3 years ago

I agree this is an issue, we are discussing the best solution to fix it. I will keep you informed.

sbrannen commented 3 years ago

@schuch, are you using <mvc:annotation-driven /> in your XML config file?

sbrannen commented 3 years ago

@schuch, would you mind creating a new issue to report this possible regression, since this issue has already been closed?

schuch commented 3 years ago

@schuch, would you mind creating a new issue to report this possible regression, since this issue has already been closed?

I assumed you would be open to reopen, due to:

As a consequence I close this issue, please reopen in case of blocking point. (https://github.com/spring-projects/spring-framework/issues/25290#issuecomment-647781356)

But yes, i can raise a new issue.

schuch commented 3 years ago

@schuch, are you using <mvc:annotation-driven /> in your XML config file?

yes, this is part of the XML configuration

sdeleuze commented 3 years ago

Indeed we can probably reopen it since it is not targeting a specific release yet.

nikomiranda commented 3 years ago

Same problem with Java Config.


    @Bean
    public LocaleResolver localeResolver() {
        Locale fixedLocale = new Locale("ja", "JP");
        FixedLocaleResolver localeResolver = new FixedLocaleResolver();
        localeResolver.setDefaultLocale(fixedLocale);

        return localeResolver;
    }         

Is ignored and accept-header is used.

sdeleuze commented 3 years ago

@schuch I have created a sample web XML project using spring-framework-issues scripts and I can't reproduce the issue. What I did is adding <bean id="localeResolver" class="org.springframework.web.servlet.i18n.FixedLocaleResolver" /> to servlet-context.xml. With logging configured with log4j.category.org.springframework.web.servlet.DispatcherServlet=DEBUG I can see the bean customized because without the bean declaration I see:

DEBUG [org.springframework.web.servlet.DispatcherServlet] - <Detected AcceptHeaderLocaleResolver>

And with it:

DEBUG [org.springframework.web.servlet.DispatcherServlet] - <Detected FixedLocaleResolver>

Could you please provide a repro project?

@nikomiranda On Javaconfig side for non Boot applications there is an expected change of behavior that I will shortly document in the upgrade notes and will also explain that more in detail in the reference documentation.

We have done those changes in order to improve GraalVM compatibility (avoid reflection usage in the default code path) and also to unify the web configuration around @Bean configuration in the default infrastructure Spring provides (instead of the default DispatcherServlet.properties).

With 5.3, a LocaleResolver bean in an arbitrary @Configuation will not be used by Spring MVC anymore.

@Configuration
public class FooConfig {
    @Bean
    public LocaleResolver localeResolver() {
        return new FixedLocaleResolver(new Locale("fr", "FR"));
    }
}

But it will be taken in account if you add it as part of the Spring MVC configuration via WebMvcConfigurationSupport class:

@EnableWebMvc
@ComponentScan(basePackages="org.springframework.issues")
@Configuration
public class WebConfig extends WebMvcConfigurationSupport {
    @Bean
    @Override
    public LocaleResolver localeResolver() {
        return new FixedLocaleResolver(new Locale("fr", "FR"));
    }
}

Or Spring MVC WebMvcConfigurer:

@EnableWebMvc
@ComponentScan(basePackages="org.springframework.issues")
@Configuration
public class WebConfig implements WebMvcConfigurer {
    @Bean
    public LocaleResolver localeResolver() {
        return new FixedLocaleResolver(new Locale("fr", "FR"));
    }
}

You have of course also the option to provide your own Spring MVC configuration.

For Boot application the behavior will be unchanged thanks to the @ConditionalOnMissingBean override.

nikomiranda commented 3 years ago

@sdeleuze Yes. That worked. It need to be documented on the upgrade notes!

Thank you

sdeleuze commented 3 years ago

It is now documented in the upgrade notes.

I am waiting @schuch repro to determine if something should be done for the XML side.

schuch commented 3 years ago

I will try to reproduce this on greenfield and provide a small repo this week.

PiotrTraczynski commented 3 years ago

Solution at: https://github.com/spring-projects/spring-framework/wiki/Upgrading-to-Spring-Framework-5.x#spring-mvc do not work. there is still override error. How to customize LocalResolver?

sdeleuze commented 3 years ago

@PiotrTraczynski Could you please provide a repro as well since it works on my basic sample?

PiotrTraczynski commented 3 years ago

@PiotrTraczynski Could you please provide a repro as well since it works on my basic sample?

spring53issueLocalRevoler.zip

There is my project. I deleted most code. But still do not work. I think this is problem dependencies.

sdeleuze commented 3 years ago

@PiotrTraczynski You are using Spring Boot 2.3.5.RELEASE with Spring Framework 5.3.0 which is not supported and which does not contain the related update that allows Spring Boot to take in account the customized LocaleResolver. Please use Spring Boot 2.4.0-RC1 instead.

@schuch Do you think you could provide me a repro today in order to allow me to work on a fix (if needed) before Spring Framework 5.3.1 to be released November 10, 2020?

schuch commented 3 years ago

@sdeleuze i pushed a demo repo to https://github.com/schuch/spring-locale-resolver

I think the issue only occurs, if the custom locale resolver is loaded in a parent spring context, which is the case in the project where i was hit by this change.

The HomeControllerTest fails when switching to 5.3.0.

I hope this helps.

jhoeller commented 3 years ago

I've pushed a change for detection of those beans across the context hierarchy in MvcNamespaceUtils (as used for the MVC XML configuration). @sdeleuze, as discussed, please double-check whether this actually fixes our target scenarios here.

sdeleuze commented 3 years ago

It seems the 2 repro projects still show the issue after this change, so I reopen this issue for another look on this.

sdeleuze commented 3 years ago

My mistake, after testing again it works as expect.

@schuch Feel free to test on your side with the latest 5.3.1-SNAPSHOT.

schuch commented 3 years ago

@schuch Feel free to test on your side with the latest 5.3.1-SNAPSHOT.

Tested successfully with 5.3.1-SNAPSHOT 🥳

bjornharvold commented 3 years ago

Hi Spring Team. Just upgraded to Spring Boot 2.4.0. This is still an issue. Attaching sample project. demo.zip

sdeleuze commented 3 years ago

@bjornharvold I think what happens is that when using @EnableWebMvc Spring Boot web config backs off, so @ConditionalOnMissingBean on LocalResolver bean which is Boot specific is not taken anymore in account and you are just using Spring MVC LocaleResolver bean which requires spring.main.allow-bean-definition-overriding=true as rightfully explained in the error message. I think this is the expected behavior.

For typical use case I would advise to not use @EnableWebMvc with Boot, and if you really need it,setting spring.main.allow-bean-definition-overriding=true is the way to go.

bjornharvold commented 3 years ago

Thank you for clarifying @sdeleuze.

Y00sh00 commented 3 years ago

Ran into the same issue as bjornharvold however not with @EnableWebMvc but I think from WebMvcConfigurer in order to use i18n / internalization would this indeed be such a use-case @bjornharvold where overriding would be the best alternative or am I missing something. Can't seem to find an alternative that doesn't involve using the addInterceptor method from that class.

    @Override
    public void addInterceptors(InterceptorRegistry registry) {
        registry.addInterceptor(localeChangeInterceptor());
        registry.addInterceptor(new SquareInterceptor(loggingUtils()));
    }
PiotrTraczynski commented 3 years ago

spring53issueLocalRevoler.zip

I updated spring boot to 2.4.0. And I have the same issue with localResolver.

PiotrTraczynski commented 3 years ago

@bjornharvold I think what happens is that when using @EnableWebMvc Spring Boot web config backs off, so @ConditionalOnMissingBean on LocalResolver bean which is Boot specific is not taken anymore in account and you are just using Spring MVC LocaleResolver bean which requires spring.main.allow-bean-definition-overriding=true as rightfully explained in the error message. I think this is the expected behavior.

For typical use case I would advise to not use @EnableWebMvc with Boot, and if you really need it,setting spring.main.allow-bean-definition-overriding=true is the way to go.

I know, but if I on this option. I will disabled mechanizm which control duplicates of bean. So It open potencial debugging issue.

sdeleuze commented 3 years ago

@PiotrTraczynski Your repro has @EnableWebMvc so it seems to be exactly the use case I already answered to. You can avoid to set spring.main.allow-bean-definition-overriding=true by not using @EnableWebMvc which is IMO a good practice in modern Boot apps.

sdeleuze commented 3 years ago

@PiotrTraczynski @schuch Complementary note: if you really need for your use case to customize Spring MVC web configuration instead of Spring Boot one, there is a third solution suggested by @rstoyanchev which is probably better than @EnableWebMvc + spring.main.allow-bean-definition-overriding=true : extend WebMvcConfigurationSupport and override localeResolver():

@Configuration
public class MyWebMvcConfig extends WebMvcConfigurationSupport {

    @Bean
    @Override
    public SessionLocaleResolver localeResolver() {
        SessionLocaleResolver slr = new SessionLocaleResolver();
        slr.setDefaultLocale(StringUtils.parseLocale("pl"));
        return slr;
    }
}

That will give you all the flexibility to override the default beans in a Spring MVC fashion without having to set spring.main.allow-bean-definition-overriding=true on Spring Boot side. Ccing @snicoll in case Boot time want to provide some guidance about that in Spring Boot 2.4 release notes.

PiotrTraczynski commented 3 years ago

Thank you very much:)

pon., 16 lis 2020, 16:05 użytkownik Sébastien Deleuze < notifications@github.com> napisał:

@PiotrTraczynski https://github.com/PiotrTraczynski @schuch https://github.com/schuch Complementary note: if you really need for your use case to customize Spring MVC web configuration instead of Spring Boot one, there is a third solution suggested by @rstoyanchev https://github.com/rstoyanchev which is probably better than @EnableWebMvc + spring.main.allow-bean-definition-overriding=true : extend WebMvcConfigurationSupport and override localeResolver():

@Configuration public class MyWebMvcConfig extends WebMvcConfigurationSupport {

@Bean @Override public SessionLocaleResolver localeResolver() { SessionLocaleResolver slr = new SessionLocaleResolver(); slr.setDefaultLocale(StringUtils.parseLocale("pl")); return slr; } }

That will give you all the flexibility to override the default beans in a Spring MVC fashion without have to set spring.main.allow-bean-definition-overriding=true on Spring Boot side. Ccing @snicoll https://github.com/snicoll in case Boot time want to provide some guidance about that in Spring Boot 2.4 release notes.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/spring-projects/spring-framework/issues/25290#issuecomment-728119519, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOUPXSBLO6RZZ2QBVOYNO2TSQE5R5ANCNFSM4ODA53ZA .

fdlk commented 3 years ago

I am upgrading to 5.3.

The suggestions given here and in the upgrade docs are useful but incomplete.

What worked for me was to replace @EnableWebMvc with a WebMvcConfig that extends DelegatingWebMvcConfiguration instead of WebMvcConfigurationSupport:

@Configuration
public class WebMvcConfig extends DelegatingWebMvcConfiguration {
  @Bean
  @Override
  public LocaleResolver localeResolver() { ... }
}
prdebski commented 3 years ago

Another solution is to name your bean differently than "localeResolver" and annotate it with @Primary.

@Bean
@Primary
public LocaleResolver customLocaleResolver() { ... }
sdeleuze commented 3 years ago

Thanks for your feedback, I have updated the upgrade notes with @fdlk proposal which seems the most relevant one.

catfishlty commented 2 years ago

Here're some tips when using custom starter. In my projects, if a LocaleResolver is setup in a custom starter, it won't work in the application.

So we need to make sure LocaleResolver is setup in application configuration but not starter configuration . For example, there're 2 repos named test-demo & test-demo-starter, which means test-demo-starter is used by test-demo. So make sure that LocaleResolver is setup in application config not starter config. For example, there're 2 repos named test-demo & test-demo-starter, which means test-demo-starter is used by test-demo.

WebConfig in test-demo-starter

@Configuration
@ConditionalOnWebApplication
@ConditionalOnMissingBean(WebMvcConfig.class)
public class WebMvcConfig extends DelegatingWebMvcConfiguration {
   ...
}

public class CommonLocaleResolver implements LocaleResolver {
   ...
}

CustomWebMvcConfig in test-demo

@Configuration
@ConditionalOnWebApplication
public class CustomWebMvcConfig extends WebMvcConfig {
    @Bean
    public LocaleResolver localeResolver() {
        return new CommonLocaleResolver();
    }

   ...
}

I hope it can help.

max91 commented 2 years ago

Hello. Faced the following problem. Overridden LocaleResolver by deriving a new class. When starting the application from the idea, everything works fine, but if you pack the application in a jar package, then when you start app the bin conflicts with what is in the class DelegatingWebMvcConfiguration. Please tell me how to solve the problem. Spring version 5.3.18

@Configuration
public class CustomWebMvcConfiguration extends DelegatingWebMvcConfiguration {

    public static final String COOKIE_NAME = "lang";
    public static final Locale DEFAULT_LOCALE = new Locale("ru");
    public static final TimeZone DEFAULT_TIMEZONE = TimeZone.getTimeZone(ZoneOffset.UTC);

    @Bean
    @NotNull
    @Override
    public LocaleResolver localeResolver() {
        CookieLocaleResolver resolver = new CookieLocaleResolver();
        resolver.setCookieName(COOKIE_NAME);
        resolver.setDefaultLocale(DEFAULT_LOCALE);
        resolver.setDefaultTimeZone(DEFAULT_TIMEZONE);
        return resolver;
    }

    @Bean
    public LocaleChangeInterceptor localeChangeInterceptor() {
        LocaleChangeInterceptor interceptor = new LocaleChangeInterceptor();
        interceptor.setParamName(COOKIE_NAME);
        return interceptor;
    }

}
bonkerman commented 8 months ago

Faced this issue too while trying to create a bean inside a configuration imported from library using @AutoConfiguration. As the LocaleResolver bean was being created in WebMvcAutoConfiguration which has @AutoConfigureOrder(Ordered.HIGHEST_PRECEDENCE + 10), the configuration I was providing should have had higher precedence, so adding @AutoConfigureOrder(Ordered.HIGHEST_PRECEDENCE + 9) to my configuration class solved the issue.