Skip to content

StackOverflowException on FactoryBean with @Validated and @ConfigurationProperties #13922

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.

Sign up for GitHub

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

Jump to bottom
Closed
thorntonrp opened this issue Jul 26, 2018 · 11 comments
Closed

StackOverflowException on FactoryBean with @Validated and @ConfigurationProperties #13922

thorntonrp opened this issue Jul 26, 2018 · 11 comments
Labels
for: external-project For an external project and not something we can fix status: invalid An issue that we don't feel is valid

Comments

@thorntonrp
Copy link
Contributor

thorntonrp commented Jul 26, 2018 •
edited
Loading

In a Spring Boot 2.0.3 demo application with spring-boot-starter-validation, a StackOverflowException is thrown on startup when there is a bean class that extends FactoryBean<T> and if the bean class is annotated with @ConfigurationProperties and @Validated.

package com.example.demo;

import javax.validation.constraints.NotBlank;

import org.springframework.beans.factory.FactoryBean;
import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.context.annotation.Bean;
import org.springframework.validation.annotation.Validated;

@SpringBootApplication
public class DemoApplication {
    public static void main(String[] args) {
        SpringApplication.run(DemoApplication.class, args);
    }
    @Bean
    //@Validated
    //@ConfigurationProperties("demo")
    public DemoBeanFactory demoBean() {
        return new DemoBeanFactory();
    }
}

@Validated
@ConfigurationProperties("demo")
class DemoBeanFactory implements FactoryBean<DemoBean> {
    private String name;
    @NotBlank
    public String getName() {
        return name;
    }
    public void setName(String name) {
        this.name = name;
    }
    public DemoBean getObject() {
        return new DemoBean();
    }
    public Class<?> getObjectType() {
        return DemoBean.class;
    }
}

class DemoBean {}

application.properties

demo.name=foo
@thorntonrp
Copy link
Contributor Author

Workaround: If the @Validated annotation is moved to an @Bean method, there is no exception.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 26, 2018
@snicoll
Copy link
Member

snicoll commented Jul 27, 2018

This applies to 1.5.x as well and seems to be looping on getObjectType

@snicoll snicoll added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 27, 2018
@snicoll snicoll added this to the 1.5.x milestone Jul 27, 2018
@nosan
Copy link
Contributor

nosan commented Aug 21, 2018 •
edited
Loading

@snicoll
I've investigated this issue, this issue happens because of @Lazy which was added by @wilkinsona due to gh-9416.
I try to explain step by step:

  1. @Lazy creates a proxy on Validator bean (ContextAnnotationAutowireCandidateResolver)
  2. MethodValidationPostProcessor with a Lazy Validator creates a proxy on FactoryBean with MethodValidationInterceptor.
  3. MethodValidationInterceptor tries to retrieve ExecutableValidator execVal = this.validator.forExecutables();
  4. DefaultListableBeanFactory tries to get doGetBeanNamesForType(ResolvableType type='javax.validation.Validator', boolean includeNonSingletons='true', boolean allowEagerInit='true')
...
	for (String beanName('Proxied Factory Bean') : this.beanDefinitionNames) {

boolean matchFound = (allowEagerInit || !isFactoryBean || (dbd != null && !mbd
  			.isLazyInit()) || containsSingleton(
  			beanName)) && (includeNonSingletons || (dbd != null ? mbd
  			.isSingleton() : isSingleton(beanName)))
  			&&  /** StackOverflow **/isTypeMatch(beanName, type);
...
}

  public boolean isTypeMatch(String name, ResolvableType typeToMatch) throws NoSuchBeanDefinitionException {
  	...
  	Object beanInstance = getSingleton(beanName, false);
  	if (beanInstance != null && beanInstance.getClass() != NullBean.class) {
  		if (beanInstance instanceof FactoryBean) {
  			if (!BeanFactoryUtils.isFactoryDereference(name)) {
  				/**StackOverflow**/Class<?> type = getTypeForFactoryBean((FactoryBean<?>) beanInstance);
  				...
  			
}

so when BeanFactory tries to get FactoryBean.getObjectType() it calls Proxied by MethodValidationPostProcessor 'FactoryBean' which requires a Validator bean, hence we will be moved to point 3 once again...
That is why Stack Overflow Error has occurred.
As a workaround IgnoreFactoryBeanMethodValidationPostProcessor could be used in the ValidationAutoConfiguration instead of plain MethodValidationPostProcessor :

class IgnoreFactoryBeanMethodValidationPostProcessor
		extends MethodValidationPostProcessor {

	@Override
	protected Advice createMethodValidationAdvice(Validator validator) {
		Advice delegate = super.createMethodValidationAdvice(validator);
		Assert.isInstanceOf(MethodInterceptor.class, delegate);
		return new IgnoreFactoryBeanMethodInterceptor(((MethodInterceptor) delegate));
	}


	private static final class IgnoreFactoryBeanMethodInterceptor
			implements MethodInterceptor {

		private final MethodInterceptor delegate;

		IgnoreFactoryBeanMethodInterceptor(
				MethodInterceptor delegate) {
			this.delegate = delegate;
		}

		@Override
		public Object invoke(MethodInvocation methodInvocation) throws Throwable {
			if (isFactoryBean(methodInvocation.getThis())) {
				return methodInvocation.proceed();
			}
			return this.delegate.invoke(methodInvocation);
		}

		private boolean isFactoryBean(Object target) {
			return target instanceof FactoryBean<?>;
		}
	}
}

P.S. But I'm not sure about this. I can create PR if it is necessary.

@snicoll
Copy link
Member

snicoll commented Aug 27, 2018

Thanks for the analysis @nosan but I don't think we can ignore FactoryBean. We'll have to revisit this one and see if we can move the lazy part somewhere else.

@wilkinsona
Copy link
Member

Here is a possible fix. I'm not sure if it's good, bad, ugly, or some combination of the three.

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Oct 1, 2018
@snicoll
Copy link
Member

snicoll commented Oct 1, 2018

I like it, I think. I wonder if we could't generalize this one level up so that any users of the Validator gets a lazy version.

@wilkinsona
Copy link
Member

I'm a bit wary of moving it one level up in case someone is casting the Validator to a specific implementation. I thought it probably better to keep the change as focussed as possible.

@wilkinsona wilkinsona modified the milestones: 1.5.x, 2.0.x Oct 10, 2018
@wilkinsona wilkinsona removed the for: team-attention An issue we'd like other members of the team to review label Oct 10, 2018
@philwebb philwebb modified the milestones: 2.0.x, 2.0.6 Oct 10, 2018
@wilkinsona
Copy link
Member

Turns out the fix is ugly as it doesn't work in 2.0.x (perhaps I was mistaken and it also doesn't work in 1.5). The stack overflow still occurs and looks like this:

java.lang.StackOverflowError: null
	at ch.qos.logback.classic.spi.LoggingEvent.<init>(LoggingEvent.java:119) ~[logback-classic-1.2.3.jar:na]
	at ch.qos.logback.classic.Logger.buildLoggingEventAndAppend(Logger.java:419) ~[logback-classic-1.2.3.jar:na]
	at ch.qos.logback.classic.Logger.filterAndLog_0_Or3Plus(Logger.java:383) ~[logback-classic-1.2.3.jar:na]
	at ch.qos.logback.classic.Logger.log(Logger.java:765) ~[logback-classic-1.2.3.jar:na]
	at org.apache.logging.slf4j.SLF4JLogger.logMessage(SLF4JLogger.java:232) ~[log4j-to-slf4j-2.10.0.jar:2.10.0]
	at org.apache.logging.log4j.spi.AbstractLogger.tryLogMessage(AbstractLogger.java:2163) ~[log4j-api-2.10.0.jar:2.10.0]
	at org.apache.logging.log4j.spi.AbstractLogger.logMessageTrackRecursion(AbstractLogger.java:2118) ~[log4j-api-2.10.0.jar:2.10.0]
	at org.apache.logging.log4j.spi.AbstractLogger.logMessageSafely(AbstractLogger.java:2101) ~[log4j-api-2.10.0.jar:2.10.0]
	at org.apache.logging.log4j.spi.AbstractLogger.logMessage(AbstractLogger.java:1995) ~[log4j-api-2.10.0.jar:2.10.0]
	at org.apache.logging.log4j.spi.AbstractLogger.logIfEnabled(AbstractLogger.java:1967) ~[log4j-api-2.10.0.jar:2.10.0]
	at org.apache.commons.logging.LogFactory$Log4jLog.log(LogFactory.java:302) ~[spring-jcl-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.apache.commons.logging.LogFactory$Log4jLog.warn(LogFactory.java:264) ~[spring-jcl-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.FactoryBeanRegistrySupport.getTypeForFactoryBean(FactoryBeanRegistrySupport.java:69) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.AbstractBeanFactory.isTypeMatch(AbstractBeanFactory.java:494) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.doGetBeanNamesForType(DefaultListableBeanFactory.java:426) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.getBeanNamesForType(DefaultListableBeanFactory.java:397) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.BeanFactoryUtils.beanNamesForTypeIncludingAncestors(BeanFactoryUtils.java:214) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.findAutowireCandidates(DefaultListableBeanFactory.java:1273) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.doResolveDependency(DefaultListableBeanFactory.java:1098) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory$DependencyObjectProvider.getObject(DefaultListableBeanFactory.java:1665) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.boot.autoconfigure.validation.ValidationAutoConfiguration$LazyValidator.getValidator(ValidationAutoConfiguration.java:122) ~[classes/:na]
	at org.springframework.boot.autoconfigure.validation.ValidationAutoConfiguration$LazyValidator.forExecutables(ValidationAutoConfiguration.java:117) ~[classes/:na]
	at org.springframework.validation.beanvalidation.MethodValidationInterceptor.invoke(MethodValidationInterceptor.java:92) ~[spring-context-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:185) ~[spring-aop-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.aop.framework.CglibAopProxy$DynamicAdvisedInterceptor.intercept(CglibAopProxy.java:688) ~[spring-aop-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at com.example.demo.DemoBeanFactory$$EnhancerBySpringCGLIB$$7e01b577.getObjectType(<generated>) ~[classes/:na]
	at org.springframework.beans.factory.support.FactoryBeanRegistrySupport.getTypeForFactoryBean(FactoryBeanRegistrySupport.java:64) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.AbstractBeanFactory.isTypeMatch(AbstractBeanFactory.java:494) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.doGetBeanNamesForType(DefaultListableBeanFactory.java:426) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.getBeanNamesForType(DefaultListableBeanFactory.java:397) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.BeanFactoryUtils.beanNamesForTypeIncludingAncestors(BeanFactoryUtils.java:214) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.findAutowireCandidates(DefaultListableBeanFactory.java:1273) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.doResolveDependency(DefaultListableBeanFactory.java:1098) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory$DependencyObjectProvider.getObject(DefaultListableBeanFactory.java:1665) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.boot.autoconfigure.validation.ValidationAutoConfiguration$LazyValidator.getValidator(ValidationAutoConfiguration.java:122) ~[classes/:na]
	at org.springframework.boot.autoconfigure.validation.ValidationAutoConfiguration$LazyValidator.forExecutables(ValidationAutoConfiguration.java:117) ~[classes/:na]
	at org.springframework.validation.beanvalidation.MethodValidationInterceptor.invoke(MethodValidationInterceptor.java:92) ~[spring-context-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:185) ~[spring-aop-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.aop.framework.CglibAopProxy$DynamicAdvisedInterceptor.intercept(CglibAopProxy.java:688) ~[spring-aop-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at com.example.demo.DemoBeanFactory$$EnhancerBySpringCGLIB$$7e01b577.getObjectType(<generated>) ~[classes/:na]
	at org.springframework.beans.factory.support.FactoryBeanRegistrySupport.getTypeForFactoryBean(FactoryBeanRegistrySupport.java:64) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.AbstractBeanFactory.isTypeMatch(AbstractBeanFactory.java:494) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.doGetBeanNamesForType(DefaultListableBeanFactory.java:426) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.getBeanNamesForType(DefaultListableBeanFactory.java:397) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.BeanFactoryUtils.beanNamesForTypeIncludingAncestors(BeanFactoryUtils.java:214) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.findAutowireCandidates(DefaultListableBeanFactory.java:1273) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.doResolveDependency(DefaultListableBeanFactory.java:1098) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory$DependencyObjectProvider.getObject(DefaultListableBeanFactory.java:1665) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.boot.autoconfigure.validation.ValidationAutoConfiguration$LazyValidator.getValidator(ValidationAutoConfiguration.java:122) ~[classes/:na]
	at org.springframework.boot.autoconfigure.validation.ValidationAutoConfiguration$LazyValidator.forExecutables(ValidationAutoConfiguration.java:117) ~[classes/:na]
	at org.springframework.validation.beanvalidation.MethodValidationInterceptor.invoke(MethodValidationInterceptor.java:92) ~[spring-context-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:185) ~[spring-aop-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.aop.framework.CglibAopProxy$DynamicAdvisedInterceptor.intercept(CglibAopProxy.java:688) ~[spring-aop-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at com.example.demo.DemoBeanFactory$$EnhancerBySpringCGLIB$$7e01b577.getObjectType(<generated>) ~[classes/:na]
	at org.springframework.beans.factory.support.FactoryBeanRegistrySupport.getTypeForFactoryBean(FactoryBeanRegistrySupport.java:64) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.AbstractBeanFactory.isTypeMatch(AbstractBeanFactory.java:494) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.doGetBeanNamesForType(DefaultListableBeanFactory.java:426) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.getBeanNamesForType(DefaultListableBeanFactory.java:397) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.BeanFactoryUtils.beanNamesForTypeIncludingAncestors(BeanFactoryUtils.java:214) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.findAutowireCandidates(DefaultListableBeanFactory.java:1273) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.doResolveDependency(DefaultListableBeanFactory.java:1098) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	at org.springframework.beans.factory.support.DefaultListableBeanFactory$DependencyObjectProvider.getObject(DefaultListableBeanFactory.java:1665) ~[spring-beans-5.0.10.BUILD-SNAPSHOT.jar:5.0.10.BUILD-SNAPSHOT]
	…

@wilkinsona wilkinsona modified the milestones: 2.0.6, 2.0.x Oct 12, 2018
@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Oct 12, 2018
@wilkinsona
Copy link
Member

wilkinsona commented Oct 12, 2018 •
edited
Loading

Workaround: If the @Validated annotation is moved to an @Bean method, there is no exception.

This works because the validation is solely performed by the binder. By contrast, when @Validated is declared at the type level on DemoBeanFactory, it's then eligible for validation via MethodValidationPostProcessor which results in a proxy being created. All methods are then intercepted and validation is performed. Interestingly, this is at odds with the javadoc of MethodValidationPostProcessor which reads as follows:

Applicable methods have JSR-303 constraint annotations on their parameters and/or on their return value (in the latter case specified at the method level, typically as inline annotation).

getObjectType() on the factory bean has no such annotations. Arguably, this means that validation is not applicable and no attempt should be made to retrieve the validator when it's called.

@wilkinsona
Copy link
Member

wilkinsona commented Oct 12, 2018 •
edited
Loading

I've opened SPR-17374 to see if anything can be done in Framework to prevent the unnecessary use of the Validator when getObjectType() is being called.

@wilkinsona wilkinsona added the status: on-hold We can't start working on this issue yet label Oct 12, 2018
@philwebb philwebb added status: blocked An issue that's blocked on an external project change and removed for: team-attention An issue we'd like other members of the team to review labels Oct 12, 2018
@wilkinsona wilkinsona removed the status: on-hold We can't start working on this issue yet label Oct 12, 2018
@wilkinsona wilkinsona modified the milestones: 2.0.x, 2.x Oct 12, 2018
Closed
@philwebb philwebb removed the status: blocked An issue that's blocked on an external project change label Jan 14, 2019
@philwebb philwebb modified the milestones: 2.x, 2.1.x Jan 14, 2019
@wilkinsona
Copy link
Member

wilkinsona commented Feb 14, 2019 •
edited
Loading

Thanks to the changes made in Framework in spring-projects/spring-framework#21907 and spring-projects/spring-framework#21919 the problem has been resolved as of Boot 2.0.8 and 2.1.0.

@wilkinsona wilkinsona removed this from the 2.1.x milestone Feb 14, 2019
@wilkinsona wilkinsona added status: invalid An issue that we don't feel is valid for: external-project For an external project and not something we can fix and removed type: bug A general bug labels Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
for: external-project For an external project and not something we can fix status: invalid An issue that we don't feel is valid
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@snicoll @philwebb @wilkinsona @thorntonrp @nosan @spring-projects-issues