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 Web Support for Method Bean Validation #30645

Closed
6 tasks done
rstoyanchev opened this issue Jun 12, 2023 · 10 comments
Closed
6 tasks done

Built-in Web Support for Method Bean Validation #30645

rstoyanchev opened this issue Jun 12, 2023 · 10 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jun 12, 2023

Currently bean validation is applied to @ModelAttribute and @RequestBody arguments if they are also annotated with jakarta.validation.Valid or Spring's @Validated with validation groups. You can put constraint annotations directly on other arguments, but that requires @Validated on the controller class with MethodValidationPostProcessor applying method validation via AOP.

This is not intuitive and creates challenges for handling validation errors, e.g. to handle either ConstraintViolationException or MethodArgumentNotValidException. Also need to introspect method parameters to know if the failure is for a request header, path variable, or other, see #26219. It can lead to double validation, see #24913. It does not support reactive types, see #20781.

We need built-in method validation in web handling for a more integrated experience, default web exception handling of constraint violations to RFC 7807 responses.

This is an umbrella issue for the improvement:

@rstoyanchev
Copy link
Contributor Author

Even if this issue is closed, for completeness I've added #16917 to the list of sub-tasks, and scheduled it for RC1.

@unwx
Copy link

unwx commented Sep 28, 2023

Is it possible to disable this built-in validation?

I looked into the code and noticed that the HandlerMethodValidator is automatically used when jakarta.validation.Validator is present and when @Validated is not used.

In ControllerMethodResolver and in RequestMappingHandlerAdapter:

private final static boolean BEAN_VALIDATION_PRESENT = ClassUtils.isPresent("jakarta.validation.Validator", HandlerMethod.class.getClassLoader());

...

if (BEAN_VALIDATION_PRESENT) {
    List<HandlerMethodArgumentResolver> resolvers = this.argumentResolvers.getResolvers();
    this.methodValidator = HandlerMethodValidator.from(
        this.webBindingInitializer, 
        this.parameterNameDiscoverer,
        methodParamPredicate(resolvers, ModelAttributeMethodProcessor.class),
        methodParamPredicate(resolvers, RequestParamArgumentResolver.class)
    );
}

In HandlerMethod:

/**
 * Whether the method arguments are a candidate for method validation, which
 * is the case when there are parameter {@code jakarta.validation.Constraint}
 * annotations.
 * <p>The presence of {@code jakarta.validation.Valid} by itself does not
 * trigger method validation since such parameters are already validated at
 * the level of argument resolvers.
 * <p><strong>Note:</strong> if the class is annotated with {@link Validated},
 * this method returns false, deferring to method validation via AOP proxy.
 * @since 6.1
 */
public boolean shouldValidateArguments() {
    return this.validateArguments;
}

However, what if a developer has their own custom validation implemented via MethodInterceptor and AbstractAdvisingBeanPostProcessor?
That is, its own implementation of @Validated.
Based on my attempt, this leads to a conflict, with the built-in validation triggering first.

final class ControllerValidationPostProcessor extends AbstractAdvisingBeanPostProcessor {
    public ControllerValidationPostProcessor(ControllerValidationInterceptor interceptor) {
        super.advisor = new DefaultPointcutAdvisor(
                new AnnotationMatchingPointcut(ValidController.class, true),
                interceptor
        );
    }
}

@rstoyanchev
Copy link
Contributor Author

There isn't a way to disable it. You could declare a global Validator that's not a Jakarta EE validator via WebMvcConfigurer#validator. Spring Boot does something similar to Boot's ValidatorAdapter.

@k-seth
Copy link
Contributor

k-seth commented Oct 29, 2023

Hey, @rstoyanchev - I appreciate the work you've put into the validation enhancements. I've had the opportunity to work with it a bit against 6.1.0 RC1. I've encountered two cases (ultimately the same root problem) where MethodValidationAdapter fails with a NotReadablePropertyException.

The problem

Essentially, the issue I found boils down to the limited container support provided in the constructor of MethodValidationAdapter.BeanResultBuilder (425d5a9#diff-b13320a52037a6f742eb8864fbe47b86234f5d263e98369ced7db29e4a7d7065R398).
The constructor only attempts to extract the wrapped type from a List or Map, otherwise it passes the argument through as-is. Unfortunately, if the argument is a Set or an Array, then the container type is passed as the argument. As a result of this, the BeanWrapper that is created will have a target and property accessor for the container, not the wrapped data type.

This leads to any violation on a nested property throwing a NotReadablePropertyException when looked up in AbstractNestablePropertyAccessor. For simple pojos, or nested sets, this will be silently dropped (via a NOOP catch statement) when getting the property type. But, if there is a violation on a nested list or map, the thrown error is fatal as it tries to actually get the property value.

What I got working locally

I had some time and was interested in digging into it a bit more, so I went ahead and played with it locally to see if I could at least offer up some kind of suggestions to address this behaviour.

For arrays, since they appear to produce a containerIndex, adding an additional else-if block for it is trivial:

else if (argument instanceof Object[] array && this.containerIndex != null) {
    this.container = array; // The ParameterErrors documentation currently only states container will be set for List or Map, but I figure this would be fine to set
    argument = array[this.containerIndex];
}

Sets were obviously a bit more awkward, since there is no direct way to access the set element that has a violation. However, it appears the adaptViolations method does already have the element via violation.getLeafBean() (I didn't check all cases, but this is also true for maps and arrays), so I just passed that into the BeanResultBuilder constructor as well.
That produces something like:

public BeanResultBuilder(MethodParameter parameter, @Nullable Object argument, Path.Node node, Object leafBean){
    ...
    else if (argument instanceof Set<?> set){ // Since there is no index or key for the container, just the container type matters
        this.container = set;
        argument = leafBean; // In theory this could be done for all the container types - not sure if it could be safely used for the else case, as I did not test that
    }
}

These tweaks allowed me to get sets and arrays to properly build and produce the desired HandlerMethodValidationException, so I call it a win - I didn't dig much further than this though. I'd be happy to open an issue if it would be more appropriate, I just put the feedback here since #29825 pointed to here. I can probably open a pr if what I provided above seems reasonable.

Example snippets

Below are some snippets that are similar to what I was using in my test cases, just to give a visual representation. If a small reproducer app would be beneficial, I can make that happen.

A simple rest controller:

@Controller
public class TestController {
    // Produces 400 BAD REQUEST with validation error
    @PostMapping("/list")
    void listBody(@RequestBody @Valid List<Recipe> body) { }

    // Throws org.springframework.beans.NotReadablePropertyException: Invalid property 'steps[1]' of bean class [java.util.HashSet] [...]
    @PostMapping("/set")
    void setBody(@RequestBody @Valid Set<Recipe> body) { }
    
    // Throws org.springframework.beans.NotReadablePropertyException: Invalid property 'steps[1]' of bean class [[Lcom.example.TestController$Recipe;] [...]
    @PostMapping("/array")
    void arrayBody(@RequestBody @Valid Recipe[] body) { }
    
    public record Recipe(
        @NotBlank String title,
        @NotEmpty List<@NotBlank String> steps,
        Set<@Size(min = 3, max = 10) String> tags,
        Map<@NotBlank String, @NotNull String> ingredients
    ) { }
}

And this request body:

[
  {
    "title": "A great recipe",
    "steps": [ "Prep ingredients", "  ", "Eat" ],
    "tags": [ "food" ],
    "ingredients": { "chicken": "5 wings" }
  }
]

@rstoyanchev
Copy link
Contributor Author

rstoyanchev commented Oct 30, 2023

Thanks for the feedback, @k-seth.

It does look like Hibernate Validator has a ValueExtractor for arrays and that should fit well alongside the current support for Lists. There is also an IterableValueExtractor, and along with that I see ValueExtractor.ValueReceiver has methods to accept iterableValue vs indexedValue. So a violation on an element in an Iterable works similarly to a List except there is no element index.

It does make sense to add those changes. As you've already experimented, could you open a pull request? Or otherwise, if you don't plan to do that, then let me know or create an issue, and I'll apply the changes.

@k-seth
Copy link
Contributor

k-seth commented Oct 30, 2023

Sure - I'll clean up my changes and do more testing, then I will get a PR up. (I have some additional work to do, the gist being the cascadedViolations map does not play well with sets as-is.)

@jakubszybisty
Copy link

hey @rstoyanchev
I am using Open Api spring generator to generate my model and controllers. Generated controller still comes with class level @validated annotation and invalid path variable annotated with @pattern still results in ConstraintViolationException. Do you know if there is any way to make this work with open api generated code?

@mlichtblau
Copy link

There isn't a way to disable it. You could declare a global Validator that's not a Jakarta EE validator via WebMvcConfigurer#validator. Spring Boot does something similar to Boot's ValidatorAdapter.

Hi, I think it would be very useful to disable built-in validation for specific controllers.

Currently, I have an interface that describes the methods my controllers implement. In my particular case an interface is used by two Spring Boot applications, a main service that provides the actual implementation and a proxy that proxies request to the main service. Both services have controllers that implement this interface. I have constraint annotations on the method parameters of the interface.
I want validation to happen on the main service, not in the proxy, so I would like to disable validation in my proxy controller, but only for this specific controller as other controllers in the proxy can and do benefit from the built-in validation.
Also, I cannot remove the constraint annotation from the interface, because then I cannot put them on the controller method parameters of the main application due to 4.5.5 of the bean validation spec (A subtype cannot have stronger preconditions then its super types).
Having to disable built-in validation only because some controllers cannot use it would be very unfortunate in my opinion.

@rstoyanchev
Copy link
Contributor Author

rstoyanchev commented Feb 16, 2024

@mlichtblau, it's not sustainable to have follow-on requests added onto the original issue.

Briefly, I'm not sure it would be ideal to have annotations specifically to turn off method validation. If your proxy does not need validation at all, make sure it doesn't declare a Jakarta Validator or even have the dependency. The bean validation spec rule seems reasonable for polymorphic use, but controllers are invoked directly by the web framework, and I'm not sure there is a practical difference there.

@mlichtblau
Copy link

Hi @rstoyanchev and thanks for your reply!

it's not sustainable to have follow-on requests added onto the original issue.

Sorry, makes sense. This issue #30645 is specifically mentioned in the upgrade guide for providing feedback, so I was also a bit confused why it was closed:

also the umbrella issue 30645 for all other related tasks and for providing feedback

I opened #32292 for further discussion.

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

No branches or pull requests

5 participants