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

Javadoc for ModelAttributeMethodProcessor.validateIfApplicable is misleading [SPR-12655] #17256

Closed
spring-issuemaster opened this issue Jan 22, 2015 · 5 comments

Comments

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

commented Jan 22, 2015

Lukas Hinsch opened SPR-12655 and commented

Update:
The javadoc for ModelAttributeMethodProcessor.validateIfApplicable claims that validation is triggered by checking for annotation of type javax.validation.Valid, whereas the actual implementation looks for annotation type names starting with "Valid" to allow for additional annotations (see comment below).
To avoid confusion the javadoc should be adjusted to reflect this behavior (such as "The default implementation checks for annotations starting with "Valid" to cover {@code javax.validation.Valid}, {@code
org.springframework.validation.annotation.Validated} and others.")

Was originally:
When checking for annotation javax.validation.Valid the code only checks if the simple class name starts with "Valid".
I was playing around with controller aspects in combination with custom annotations that by accident also started with "Valid" and it took me a while to understand why bean validation was triggered by my custom annotation.

So I think in: ModelAttributeMethodProcessor:162
the code should be changed from

if (validatedAnn != null || ann.annotationType().getSimpleName().startsWith("Valid"))

to something like

if (validatedAnn != null || ann.annotationType().getName().equals("javax.validation.Valid"))

(https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/web/method/annotation/ModelAttributeMethodProcessor.java#L162)


Affects: 3.2.13, 4.1.4

Referenced from: commits 9a7871f, 7585be8, d77af71, 186fef6, 25644db, 7191050

Backported to: 3.2.14

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 22, 2015

Juergen Hoeller commented

This was originally intentional, allowing for custom validation annotations - and also covering Spring's @Validated annotation that way.

Admittedly, this is indeed a little bit naive, and custom validation-triggering annotations may not be too common. However, there's certainly some code out there relying on it in the meantime, so I'm afraid it's hard to change this.

Juergen

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 22, 2015

Lukas Hinsch commented

Thanks for taking the time to clarify. I did not even consider the fact that the pattern matching might be intentional. Might have to do with the javadoc for the method being a little misleading ("The default implementation checks for {@code javax.validation.Valid}") and I think that is something that could be improved. I'll update the title and description accordingly.

Lukas

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 10, 2015

Juergen Hoeller commented

I've revised the javadoc accordingly in all affected places, and also introduced protected validate and isBindingErrorFatal methods in the applicable argument resolvers while being at it.

Juergen

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 19, 2015

Juergen Hoeller commented

I've eventually renamed isBindingErrorFatal back to isBindExceptionRequired (the original name of the private method), since there already is a same-named protected method in ModelAttributeMethodProcessor that we'd like to be consistent with.

Juergen

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 19, 2015

Juergen Hoeller commented

Partially backported to 3.2.14, i.e. the javadoc fixes but not turning any private methods to protected.

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.