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

Built-in method validation validates @RequestBody parameter twice #31711

Closed
giger85 opened this issue Nov 29, 2023 · 7 comments
Closed

Built-in method validation validates @RequestBody parameter twice #31711

giger85 opened this issue Nov 29, 2023 · 7 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Milestone

Comments

@giger85
Copy link

giger85 commented Nov 29, 2023

Version

  • spring framework: 6.1.1 (spring boot 3.2.0)

Situation

I tested below spring MVC rest controller and model. (use kotlin)

@RestController
@RequestMapping(value = ["/api"])
class FooController {
    @PostExchange("/foo")
    fun createFoo(
        @RequestBody @Valid myData: MyData,
        @Min(1) @RequestParam("hint") hint: Int
    ): Map<String, Any?> {
        return mapOf("myData" to myData)
    }
}

data class MyData(
    val name: String,
    @get:Min(1)
    val age: Int
)

And call api and debug.

> curl -X POST \
-H 'Content-Type: application/json' \
'http://localhost:8080/api/foo?hint=2' \
-d '{"name": "foo-1", "age": 3}'

Then, the validation of age field occurs twice.
Is this behavior valid?

Related code

In invokeForRequest method, executed getMethodArgumentValues method first and then methodValidator.applyArgumentValidation method.

If requestBody is validate object and built-in method validation is activated, then request body object is validated twice.

Object[] args = getMethodArgumentValues(request, mavContainer, providedArgs);

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 29, 2023
@giger85 giger85 changed the title Using built-in method validation might be validate @RequestBody parameter twice Using built-in method validation might validate @RequestBody parameter twice Nov 29, 2023
@snicoll
Copy link
Member

snicoll commented Nov 29, 2023

Then, the validation of age field occurs twice.

How did you come to this conclusion?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Nov 29, 2023
@Crain-32
Copy link

Crain-32 commented Nov 29, 2023

I can somewhat confirm this same behavior, but for a different reason. (Spring Boot 3.1.3, but this is Spring Framework level behavior)

The following Controller does reproduce the issue I'm describing. It appears the DataBinder does an initial check before it converts the request to a body, and throws a MethodArgumentInvalidException if it fails. Then it also calls the Jakarta Validator. If it fails in this stage we get a ConstraintViolationException.

@Validated
@RestController
@RequestMapping("/rest/test")
public class TestController {
    public record MyData(String foo, @NotNull String bar) {}

    @PostMapping("/list")
    public String sayHello(@Valid @RequestBody @NotEmpty List<@Valid MyData> myList) {
         return "Hello";
    }
    
    @PostMapping
    public String sayWorld(@Valid @RequestBody @NotNull MyData myData) {
          return "World";
    }
}

Sending in the following to /rest/test/list will result in a ConstraintViolationException

[
   { "bar": null, "foo" : "buzz" }
]

Where sending this to the /rest/test will result in a MethodArgumentInvalidException

{"bar": null, "foo": "buzz"}

I'm not 100% confident in the degree this Exception difference and the Double validation are related, but it does feel like they lay on a similar issue.

@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 Nov 29, 2023
@giger85
Copy link
Author

giger85 commented Nov 30, 2023

@snicoll
Using Intellij (IDEA), I specified break point to AbstractMinValidator as below
break-point-2

And, attached partial stack traces

  • first validation at getMethodArgumentValues() method
at org.hibernate.validator.internal.constraintvalidators.bv.number.bound.AbstractMinValidator.isValid(AbstractMinValidator.java:34)
at org.hibernate.validator.internal.engine.constraintvalidation.ConstraintTree.validateSingleConstraint(ConstraintTree.java:180)
at org.hibernate.validator.internal.engine.constraintvalidation.SimpleConstraintTree.validateConstraints(SimpleConstraintTree.java:66)
at org.hibernate.validator.internal.engine.constraintvalidation.ConstraintTree.validateConstraints(ConstraintTree.java:75)
at org.hibernate.validator.internal.metadata.core.MetaConstraint.doValidateConstraint(MetaConstraint.java:130)
at org.hibernate.validator.internal.metadata.core.MetaConstraint.validateConstraint(MetaConstraint.java:123)
at org.hibernate.validator.internal.engine.ValidatorImpl.validateMetaConstraint(ValidatorImpl.java:555)
at org.hibernate.validator.internal.engine.ValidatorImpl.validateConstraintsForSingleDefaultGroupElement(ValidatorImpl.java:518)
at org.hibernate.validator.internal.engine.ValidatorImpl.validateConstraintsForDefaultGroup(ValidatorImpl.java:488)
at org.hibernate.validator.internal.engine.ValidatorImpl.validateConstraintsForCurrentGroup(ValidatorImpl.java:450)
at org.hibernate.validator.internal.engine.ValidatorImpl.validateInContext(ValidatorImpl.java:400)
at org.hibernate.validator.internal.engine.ValidatorImpl.validate(ValidatorImpl.java:172)
at org.springframework.validation.beanvalidation.SpringValidatorAdapter.validate(SpringValidatorAdapter.java:105)
at org.springframework.boot.autoconfigure.validation.ValidatorAdapter.validate(ValidatorAdapter.java:67)
at org.springframework.validation.DataBinder.validate(DataBinder.java:1247)
at org.springframework.web.servlet.mvc.method.annotation.AbstractMessageConverterMethodArgumentResolver.validateIfApplicable(AbstractMessageConverterMethodArgumentResolver.java:252)
at org.springframework.web.servlet.mvc.method.annotation.RequestResponseBodyMethodProcessor.resolveArgument(RequestResponseBodyMethodProcessor.java:141)
at org.springframework.web.method.support.HandlerMethodArgumentResolverComposite.resolveArgument(HandlerMethodArgumentResolverComposite.java:122)
at org.springframework.web.method.support.InvocableHandlerMethod.getMethodArgumentValues(InvocableHandlerMethod.java:218)
at org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest(InvocableHandlerMethod.java:171)
...
  • second validation at methodValidator.applyArgumentValidation() method
at org.hibernate.validator.internal.constraintvalidators.bv.number.bound.AbstractMinValidator.isValid(AbstractMinValidator.java:34)
at org.hibernate.validator.internal.engine.constraintvalidation.ConstraintTree.validateSingleConstraint(ConstraintTree.java:180)
at org.hibernate.validator.internal.engine.constraintvalidation.SimpleConstraintTree.validateConstraints(SimpleConstraintTree.java:66)
at org.hibernate.validator.internal.engine.constraintvalidation.ConstraintTree.validateConstraints(ConstraintTree.java:75)
at org.hibernate.validator.internal.metadata.core.MetaConstraint.doValidateConstraint(MetaConstraint.java:130)
at org.hibernate.validator.internal.metadata.core.MetaConstraint.validateConstraint(MetaConstraint.java:123)
at org.hibernate.validator.internal.engine.ValidatorImpl.validateMetaConstraint(ValidatorImpl.java:555)
at org.hibernate.validator.internal.engine.ValidatorImpl.validateConstraintsForSingleDefaultGroupElement(ValidatorImpl.java:518)
at org.hibernate.validator.internal.engine.ValidatorImpl.validateConstraintsForDefaultGroup(ValidatorImpl.java:488)
at org.hibernate.validator.internal.engine.ValidatorImpl.validateConstraintsForCurrentGroup(ValidatorImpl.java:450)
at org.hibernate.validator.internal.engine.ValidatorImpl.validateInContext(ValidatorImpl.java:400)
at org.hibernate.validator.internal.engine.ValidatorImpl.validateCascadedAnnotatedObjectForCurrentGroup(ValidatorImpl.java:629)
at org.hibernate.validator.internal.engine.ValidatorImpl.validateCascadedConstraints(ValidatorImpl.java:590)
at org.hibernate.validator.internal.engine.ValidatorImpl.validateParametersInContext(ValidatorImpl.java:880)
at org.hibernate.validator.internal.engine.ValidatorImpl.validateParameters(ValidatorImpl.java:283)
at org.hibernate.validator.internal.engine.ValidatorImpl.validateParameters(ValidatorImpl.java:235)
at org.springframework.validation.beanvalidation.MethodValidationAdapter.invokeValidatorForArguments(MethodValidationAdapter.java:260)
at org.springframework.validation.beanvalidation.MethodValidationAdapter.validateArguments(MethodValidationAdapter.java:240)
at org.springframework.web.method.annotation.HandlerMethodValidator.validateArguments(HandlerMethodValidator.java:115)
at org.springframework.web.method.annotation.HandlerMethodValidator.applyArgumentValidation(HandlerMethodValidator.java:83)
at org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest(InvocableHandlerMethod.java:178)

@rstoyanchev
Copy link
Contributor

@giger85 this is not expected, but I will need to debug to find out more. Do you have a small sample?

@rstoyanchev
Copy link
Contributor

@Crain-32 your case is a little different. First you have a class-level @Validated which applies method validation via AOP. That's exactly what we tried to solve in 6.1 with the Spring Web built-in method validation, removing the need to use AOP for this case. Please, check the reference docs. Beyond that there may be an issue with the example signatures that you shared. For example where constraint annotations are placed directly on the method parameter for @RequestBody. At the moment, we expect that to be mostly @Valid and that may be causing an issue, I'll need to debug to find out more.

@rstoyanchev rstoyanchev self-assigned this Dec 14, 2023
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Dec 18, 2023
@rstoyanchev rstoyanchev added this to the 6.1.3 milestone Dec 18, 2023
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Dec 18, 2023

I was able to confirm the issue. It's related to the Spring Boot VaildatorAdapter that wraps the jakarta.validation.Validator and precludes us from recognizing it as such. The change for #31082 unwraps before checking so we correctly determine if method validation applies. However, there is an one more check in DefaulDataBinderFactory that should exclude bean validation at the DataBinder level, for @RequestBody and @ModelAttribute arguments, if method validation applies.

@Crain-32 for your case you only need to remove @Validated from the class level in order to turn off validation through an AOP proxy, and rely on the Spring web, built-in method validation (new in 6.1) instead.

@rstoyanchev rstoyanchev changed the title Using built-in method validation might validate @RequestBody parameter twice Built-in method validation validates @RequestBody parameter twice Dec 18, 2023
rstoyanchev added a commit that referenced this issue Dec 18, 2023
@giger85
Copy link
Author

giger85 commented Dec 19, 2023

@rstoyanchev
Thanks for your support.

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: bug A general bug
Projects
None yet
Development

No branches or pull requests

5 participants