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

Avoid exceptions when evaluating validation hints #26787

Conversation

dreis2211
Copy link
Contributor

Hi,

we noticed in one of our loadtests that a substantial amount of "hidden" NoSuchMethodExceptions is being thrown when using @javax.validation.Valid on controller methods.

@PostMapping("test")
public void test(@RequestBody @Valid @NotNull TestRequest request) {
     // Some code
}

This is caused by calling AnnotationUtils.getValue() on this annotation, which has no such property. This PR bypasses the annotation utility call. I also took the freedom to refactor this logic into a common place in ValidationUtils to avoid copy-paste throughout the different modules.

An isolated JMH benchmark shows the following results when dealing with @Valid:

Benchmark                                         Mode  Cnt     Score     Error   Units
MyBenchmark.testNew                               avgt   10     6,253 ±   0,312   ns/op
MyBenchmark.testOld                               avgt   10  1761,995 ±  92,262   ns/op

As a workaround you can use org.springframework.validation.annotation.Validated of course, but I would prefer not to have such a drawback when using javax.validation.Valid.

Let me know what you think.

Cheers,
Christoph

Prior to this commit, evaluating validation hints for @javax.validation.Valid
caused exceptions being raised when getting the value of this annotation, which
does not exist. Bypassing AnnotationUtils.getValue() in those cases can improve
performance by avoiding the cost incurred by raising exceptions.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 11, 2021
* @param ann the annotation (potentially a validation annotation)
* @return the validation hints to apply (possibly an empty array),
* or {@code null} if this annotation does not trigger any validation
* @since 5.3.6
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be 5.3.7 now - let me know if I should add that.

@jhoeller jhoeller added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Apr 21, 2021
@jhoeller jhoeller added this to the 5.3.7 milestone Apr 21, 2021
@jhoeller
Copy link
Contributor

jhoeller commented Apr 21, 2021

Hey @dreis2211 it's indeed worth handling the common javax.annotation.Valid case explicitly there, and also worth extracting the evaluation logic. However, it'll have to be a new org.springframework.validation.annotation.ValidationAnnotationUtils class in order to avoid a package cycle, since it refers to the Validated annotation in the subpackage there. If you could refactor it that way with a since marker for 5.3.7, we'd be happy to roll it in...

@dreis2211
Copy link
Contributor Author

dreis2211 commented Apr 21, 2021

@jhoeller Thanks for the review. I just committed the requested changes.

@snicoll snicoll self-assigned this Apr 22, 2021
@snicoll
Copy link
Member

snicoll commented Apr 22, 2021

@dreis2211 I can see ModelAttributeMethodProcessorTests is failing. Can you please have a look?

@dreis2211
Copy link
Contributor Author

@snicoll Should be fixed - I was a bit overly ambitious and wanted to optimize the custom @Valid annotations case as well, which broke when the annotation was defined as an inner class.

snicoll pushed a commit that referenced this pull request Apr 22, 2021
Prior to this commit, evaluating validation hints for
@javax.validation.Valid caused exceptions being raised when getting the
value of this annotation, which does not exist. Bypassing
AnnotationUtils.getValue() in those cases can improve performance by
avoiding the cost incurred by raising exceptions.

See gh-26787
@snicoll snicoll closed this in e4c0ff5 Apr 22, 2021
@snicoll
Copy link
Member

snicoll commented Apr 22, 2021

Thanks for the follow-up Christoph.

Zoran0104 pushed a commit to Zoran0104/spring-framework that referenced this pull request Aug 20, 2021
Prior to this commit, evaluating validation hints for
@javax.validation.Valid caused exceptions being raised when getting the
value of this annotation, which does not exist. Bypassing
AnnotationUtils.getValue() in those cases can improve performance by
avoiding the cost incurred by raising exceptions.

See spring-projectsgh-26787
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this pull request Mar 26, 2022
Prior to this commit, evaluating validation hints for
@javax.validation.Valid caused exceptions being raised when getting the
value of this annotation, which does not exist. Bypassing
AnnotationUtils.getValue() in those cases can improve performance by
avoiding the cost incurred by raising exceptions.

See spring-projectsgh-26787
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants