Skip to content

Custom converters cannot be used when creating endpoint-related beans due to eager initialization triggered by ServletEndpointRegistrar #20714

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

Closed
WalkingGhost1975 opened this issue Mar 28, 2020 · 5 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@WalkingGhost1975
Copy link

WalkingGhost1975 commented Mar 28, 2020

This is related to the issues #6222 and #12148

It affects Spring Boot version 2.2.x

The behavior of configuration and value binding is still not completly consistent. The use of a custom Converter registered as bean or inside a conversionService bean only works in the following cases:

  • The configuration value will be bound to a configuration bean annotated with @ConfigurationProperties
  • The configuration value will be injected to bean via @Value annotation if the bean is created after complete refresh of the ApplicationContext.

It does not work, if the configuration value if injected into a bean with @Value if the bean is created during "refresh phase" of the ApplicationContext. This applies for example to all beans created during the initialization of the Tomcat server in a ServletWebServerApplicationContext

I provided a test project which contains multiple test cases showcasing the inconsistent behavior:
Test scenario 1: HealthIndicator bean created during refresh with configuration object => SUCCESS
Test scenario 2: HealthIndicator bean created during refresh with constructor injection via @Value => FAIL
Test scenario 3: @Service bean created after refresh with constructor injection via @Value => SUCCESS

spring-boot-converter-bug.zip

Please also note that the failure of scenario 2 can only be detected in an integration test if a "real" webEnvironment, e.g. RANDOM_PORT, is used. In a mocked web environment, i.e. without actually starting a Tomcat, the problem will not occur.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 28, 2020
@wilkinsona wilkinsona self-assigned this Apr 1, 2020
@wilkinsona
Copy link
Member

Thank you for the sample. I have reproduced the problem.

The problem is caused by some eager initialization that's triggered by ServletWebServerApplicationContext retrieving all ServletContextInitializer beans from the context during startup. ServletEndpointRegistrar is one such bean. Its creation triggers endpoint discovery which ultimately leads to creation of all of the health indicators that are used by the health endpoint. Creation of your ConstructorHealthIndicator fails because this is all happening before your custom converter has been registered.

@wilkinsona wilkinsona changed the title Custom ConversionService not used for constructor injection for beans in WebApplicationContext Custom converters cannot be used when creating endpoint-related beans due to eager initialization triggered by ServletEndpointRegistrar Apr 1, 2020
@wilkinsona wilkinsona added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 1, 2020
@wilkinsona wilkinsona added this to the 2.2.x milestone Apr 1, 2020
@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Apr 1, 2020
@wilkinsona
Copy link
Member

wilkinsona commented Apr 1, 2020

The following will reproduce the problem when added to spring-boot-actuator-autoconfigure:

@Test
void endpointThatRequiresCustomConversionForItsConstruction() {
    new WebApplicationContextRunner(AnnotationConfigServletWebServerApplicationContext::new)
            .withConfiguration(AutoConfigurations.of(ServletWebServerFactoryAutoConfiguration.class,
                    EndpointAutoConfiguration.class, WebEndpointAutoConfiguration.class,
                    ServletEndpointManagementContextConfiguration.class, DispatcherServletAutoConfiguration.class))
            .withUserConfiguration(CustomConversionEndpoint.class, CustomConversionConfiguration.class)
            .run((context) -> {
                assertThat(context).hasSingleBean(CustomConversionEndpoint.class);
            });
}

@Component
@Endpoint(id = "customconversion")
static class CustomConversionEndpoint {

    CustomConversionEndpoint(@Value("custom.type") CustomType customType) {

    }

}

@Configuration(proxyBeanMethods = false)
static class CustomConversionConfiguration {

    @Bean
    public ConversionService conversionService() {
        FormattingConversionService conversionService = new DefaultFormattingConversionService();
        conversionService.addConverter(new StringToCustomTypeConverter());
        return conversionService;
    }

}

static class CustomType {

}

static class StringToCustomTypeConverter implements Converter<String, CustomType> {

    @Override
    public CustomType convert(String source) {
        return new CustomType();
    }

}

The test passes if the creation of WebApplicationContextRunner is changed to use the default context type (AnnotationConfigServletWebApplicationContext). This changes the startup ordering due to its use of a mocked servlet context.

@WalkingGhost1975
Copy link
Author

@wilkinsona Thanks for your analysis. Yes, the ServletEndpointRegistrar potentially triggers the eager initialization of quite a big tree of beans in this early phase.

We have currently a workaround by manually setting the custom ConversionService to the ConfigurableBeanFactory in the factory method:

        DefaultFormattingConversionService conversionService = new DefaultFormattingConversionService();
        conversionService.addConverter(myCustomConverter);
        beanFactory.setConversionService(conversionService); 

This is working for us but it's is a bit "hacky" and we would be happy to remove this workaround. :-)

@wilkinsona
Copy link
Member

The scope of the problem can be narrowed considerably by changing ServletEndpointDiscoverer so that it only triggers initialization of beans annotated with @ServletEndpoint rather than @Endpoint. The test above passes with the following change:

diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/annotation/EndpointDiscoverer.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/annotation/EndpointDiscoverer.java
index bbdf544f66..b4756df202 100644
--- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/annotation/EndpointDiscoverer.java
+++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/annotation/EndpointDiscoverer.java
@@ -16,6 +16,7 @@
 
 package org.springframework.boot.actuate.endpoint.annotation;
 
+import java.lang.annotation.Annotation;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
@@ -127,7 +128,7 @@ public abstract class EndpointDiscoverer<E extends ExposableEndpoint<O>, O exten
        private Collection<EndpointBean> createEndpointBeans() {
                Map<EndpointId, EndpointBean> byId = new LinkedHashMap<>();
                String[] beanNames = BeanFactoryUtils.beanNamesForAnnotationIncludingAncestors(this.applicationContext,
-                               Endpoint.class);
+                               getEndpointAnnotation());
                for (String beanName : beanNames) {
                        if (!ScopedProxyUtils.isScopedTarget(beanName)) {
                                EndpointBean endpointBean = createEndpointBean(beanName);
@@ -261,6 +262,10 @@ public abstract class EndpointDiscoverer<E extends ExposableEndpoint<O>, O exten
                return true;
        }
 
+       protected Class<? extends Annotation> getEndpointAnnotation() {
+               return Endpoint.class;
+       }
+
        private boolean isEndpointFiltered(EndpointBean endpointBean) {
                for (EndpointFilter<E> filter : this.filters) {
                        if (!isFilterMatch(filter, endpointBean)) {
diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/annotation/ServletEndpointDiscoverer.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/annotation/ServletEndpointDiscoverer.java
index 5c7dc7bd67..cb1508f57d 100644
--- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/annotation/ServletEndpointDiscoverer.java
+++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/annotation/ServletEndpointDiscoverer.java
@@ -16,6 +16,7 @@
 
 package org.springframework.boot.actuate.endpoint.web.annotation;
 
+import java.lang.annotation.Annotation;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
@@ -80,4 +81,9 @@ public class ServletEndpointDiscoverer extends EndpointDiscoverer<ExposableServl
                throw new IllegalStateException("ServletEndpoints must not declare operations");
        }
 
+       @Override
+       protected Class<? extends Annotation> getEndpointAnnotation() {
+               return ServletEndpoint.class;
+       }
+
 }

@philwebb philwebb self-assigned this Apr 8, 2020
@philwebb
Copy link
Member

This is a nasty one. I've tried approach here where we defer creating the endpoint beans as late as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants