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

MethodValidationPostProcessor validates FactoryBean methods for which validation is not applicable [SPR-17374] #21907

Closed
spring-issuemaster opened this issue Oct 12, 2018 · 8 comments
Assignees
Milestone

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Oct 12, 2018

Andy Wilkinson opened SPR-17374 and commented

Please see the referenced Spring Boot issue for some background. I've also attached a sample project that reproduces the StackOverflowError that's caused by an attempt to validate a factory bean's getObjectType() method while trying to retrieve the Validator from the context due to the validator being @Lazy.

As described in the Boot issue, my investigation of the problem has led me to wonder if MethodValidationPostProcessor is doing more than it should. In this particular case, the FactoryBean is annotated with @Validated but its getObjectType() method is not annotated with any validation annotations. As such, I would not expect a call to that method to attempt to use the Validator which, due to its laziness, then triggers an attempt to retrieve the Validator from the context which causes the factory bean's getObjectType() method to be called again.

 


Affects: 4.3.19, 5.0.9, 5.1 GA

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

Attachments:

Issue Links:

  • #21919 MethodValidationPostProcessor still validates FactoryBean methods on CGLIB proxies
  • #21964 MethodValidationPostProcessor does not support JDK proxy from FactoryBean

Backported to: 5.0.10, 4.3.20

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 12, 2018

Juergen Hoeller commented

Our MethodValidationInterceptor currently uses the Validator itself to determine constraint annotations; we don't check for those ourselves. Adding explicit constraint annotation detection on our end just for that reason seems odd. Can we do anything else here, possibly skip the particular methods declared in the FactoryBean interface since they're never meant to have validation applied to them?

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 13, 2018

Andy Wilkinson commented

I had a hunch that would be the case. It would seem odd to check for validation annotations as there'd then be some duplication of that effort in the cases where the validator is then called.

I think that skipping the methods declared on the FactoryBean interface would fix the underlying problem. In fact, skipping getObjectType() alone would be sufficient I believe.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 14, 2018

Juergen Hoeller commented

MethodValidationInterceptor explicitly skips a validation attempt for FactoryBean metadata methods now (just applies it to getObject since that may be intended and actually useful). This turns out to be an easy enough patch, so I'll backport it to 5.0.10 as well as 4.3.20.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 15, 2018

Andy Wilkinson commented

Unfortunately, the change hasn't fixed the problem as the isFactoryBeanMetadataMethod check does not appear to work correctly. When a class implements FactoryBean, the declaring class of a method from the FactoryBean interface is the implementing class, not FactoryBean or SmartFactoryBean.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 15, 2018

Juergen Hoeller commented

I was able to reproduce a kind of stack overflow in a unit test (MethodValidationTests.testLazyValidatorForMethodValidation), and that specific check did fix that problem at least. The container is calling a FactoryBean implementation through the FactoryBean interface, so the method handle used for dispatching those calls declares FactoryBean.class as its declaring class (I've just re-verified that in the debugger).

Are there any extra calls to getObjectType() in Boot, possibly reflectively via getMethod("getObjectType") on the implementation class? Any such calls should change to FactoryBean.class.getMethod("getObjectType"), just in case...

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 15, 2018

Andy Wilkinson commented

Boot need not be involved at all. You can reproduce the failure with Framework 5.0.10.RELEASE using the sample I originally provided and adding <spring.version>5.0.10.RELEASE</spring.version> to the <properties> in its pom.xml.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 15, 2018

Juergen Hoeller commented

Ouch, I thought my unit test did mirror your sample... but there's one key difference: You're using proxyTargetClass, therefore enforcing a CGLIB proxy, whereas the unit test goes with the default interface-based proxy. I'm sorry for missing that; I guess we have to deal with the CGLIB case in a follow-up issue.

Unfortunately the CGLIB case incurs a significantly more expensive runtime check since we have to individually check for getObjectType(), isSingleton(), isPrototype() and isEagerInit() by method name plus parameter signature, or by checking for their presence on the FactoryBean / SmartFactoryBean interface through an extra getMethod call. We have to check for all those methods since in my tests, every one of them could cause a stack overflow; it's a shame to have to enumerate them and hard-code the interface structure that way. Sigh.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Oct 15, 2018

Juergen Hoeller commented

I've created #21919 as a follow-up for the CGLIB case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.