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

Revisit validation documentation to better explain when method validation is invoked #32807

Closed
mlichtblau opened this issue May 13, 2024 · 4 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: documentation A documentation task
Milestone

Comments

@mlichtblau
Copy link

mlichtblau commented May 13, 2024

Affects: spring-web-6.1.6

Hi,
I encountered some unexpected behavior with the new built-in handler method validation.
In my example my handler method has two parameters, one path variable and one request body:

    @PostMapping("/{pathVariable}/test1")
    String test1(@PathVariable("pathVariable") String pathVariable, @RequestBody DemoTransport demoTransport) {
        return "SUCCESS";
    }

The issue occurs when I want to validate the DemoTransport by adding an @Valid annotation. The validation is performed differently depending on whether another constraint annotation is present on any of the paramaters, for example on pathVariable.

I prepared a small demo, here are my two controller methods. Both have the @Valid transportation and one has an additional @NotNull annotation on the pathVariable.

Here are the tests with my expectations. If my requests correctly supplies a non-null pathVariable but an invalid DemoTransport I expect the same HandlerMethodValidationException to be thrown for both controller methods. For the handler method with the @NotNull annotation the handler method is validated and the HandlerMethodValidationException is correctly thrown, because this condition evaluates to true. For the handler method without the @NotNull annotation no hander method validation is performed, only a org.springframework.web.bind.MethodArgumentNotValidException is thrown, because this check does not evaluate to true because DemoTransport is not isIndexOrKeyBasedContainer.

I don't think the presence of an annotation on another parameter should influence whether the validation of the parameter annoted with @Valid is performed as part of the method handler validation or not. I am not sure I understand why, in this case, only an index- or key-based container can be validated? Apparently the parameter is correctly validated as seen by the other controller method.

Is it possible to remove this check and always validate the method when an @Valid is present on one of the parameters?

Consistent behavior is important for us for multiple reasons, for example because we are using ControllerAdvice and want to rely on the HandlerMethodValidationException to be thrown.

Thank you, and please let me know if I can help.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label May 13, 2024
@jhoeller jhoeller added the in: web Issues in web modules (web, webmvc, webflux, websocket) label May 13, 2024
@rstoyanchev
Copy link
Contributor

This was discussed in #31775 (comment) and also under #32082. Essentially, arguments may be validated as an individual value when that's sufficient, or with method validation when method parameters have constraints. The former is long standing behavior that would disrupt a lot of applications that don't need to apply method validation. The two exceptions however are designed to be very similar, and can be handled with almost identical code, or likewise one can be adapted to the other.

@rstoyanchev rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label May 14, 2024
@mlichtblau
Copy link
Author

Thanks for your reply and sorry I missed the previous discussion on the topic.
Now with the added context, the Validation documentation makes also more sense to me. The mistake on my part was that I assumed @Valid is also an @Constraint validation which it is not.

Maybe it was just confusing to me, but I still think the documentation could be improved in regards to wording:

The validation support works on two levels.
First, resolvers for [...] @RequestBody [...] perform validation [...] and raise MethodArgumentNotValidException if necessary.
Second, [...] ,then method validation is applied instead, raising HandlerMethodValidationException if necessary.

The "instead" is quite important and I missed it, because otherwise it sounds like there is a temporal relation between these two levels. What I understood intially: First the first level validation is applied and MethodArgumentNotValidException raised and only if the request body is valid then second level method validation is applied for the other parameters with @Constraint raising HandlerMethodValidationException.

This is why I was confused why my first example test didn't throw the MethodArgumentNotValidException.

Otherwise, the ticket can be closed and I will handle both exceptions as the others did, too.
Thank you!

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 15, 2024
@snicoll snicoll added type: documentation A documentation task and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels May 21, 2024
@snicoll snicoll added this to the 6.1.x milestone May 21, 2024
@snicoll snicoll changed the title Inconsistent behavior of handler method validation of parameters annotated with @Valid Revisit validation documentation to better explain when method validation is invoked May 21, 2024
@snicoll
Copy link
Member

snicoll commented May 21, 2024

Thanks for the feedback. Rossen and I had a quick chat and we're going to look at improving the wording there.

@rstoyanchev rstoyanchev self-assigned this May 22, 2024
@rstoyanchev rstoyanchev modified the milestones: 6.1.x, 6.1.8 May 22, 2024
@rstoyanchev
Copy link
Contributor

I've revised that section of the documentation based on the specific feedback. Hopefully that's better now.

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: documentation A documentation task
Projects
None yet
Development

No branches or pull requests

5 participants