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

Turn MethodArgumentNotValidException into subclass of BindException #23107

Closed
rafafael03 opened this issue Jun 10, 2019 · 6 comments
Closed

Turn MethodArgumentNotValidException into subclass of BindException #23107

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

Comments

@rafafael03
Copy link

When creating a REST controller with Spring Boot it's possible to use Bean Validation to validate the methods arguments annotating the class with @Validated.

Then Bean Validation annotations can be used directly on the argument or, if the argument is a complex class with properties that must be validated (such as the request body), using @Valid.

The inconsistency I found was while handling violation on those validations.
While using the annotations from javax.validation.constraints (the validations from the Bean Validation API) the violations are handled by the @Validated handler (MethodValidationInterceptor) and throws ConstraintViolationException.
When using @Valid the violation is handled by ModelAttributeMethodProcessor and throws MethodArgumentNotValidException.

See the problem?

The @Validated, a Spring annotation that says it is a "Variant of JSR-303's Valid" and "Designed for convenient use with Spring's JSR-303 support but not JSR-303 specific" (see it here), throws ConstraintViolationException, an exception from the Bean Validation API (JSR-303).
And the opposite also is true, the @Valid (from Bean Validation API) throws MethodArgumentNotValidException (Spring exception).

I think it would be more concise if their behavior changed between them.
Does it makes sense?

@philwebb
Copy link
Member

I've transferred this issue to the spring-framework team since the code in question is part of spring-webmvc.

@philwebb philwebb transferred this issue from spring-projects/spring-boot Jun 10, 2019
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jun 10, 2019
@ah1508
Copy link

ah1508 commented Nov 15, 2019

There is also the BindException, thrown by spring mvc if the invalid object has been built from the request parameters.

exemple : GET /books?title=&author.lastname= calls a method which declares a @Valid Book parameter (query by example),where title property is @NotBlank.

This forces to handle BindException and MethodNotValidArgumentException, but the code is the same. For example :

@ExceptionHandler(MethodArgumentNotValidException.class)
@ResponseStatus(HttpStatus.BAD_REQUEST)
public Map<String, String> onMethodArgumentNotValidException(MethodArgumentNotValidException e) {
    return e.getBindingResult().getFieldErrors().stream().collect(Collectors.toMap(FieldError::getField, FieldError::getDefaultMessage));
}

@ExceptionHandler(BindException.class)
@ResponseStatus(HttpStatus.BAD_REQUEST)
public Map<String, String> onBindException(BindException e)  {
    return e.getBindingResult().getFieldErrors().stream().collect(Collectors.toMap(FieldError::getField, FieldError::getDefaultMessage));
}

a @ValidationFailureHandler could help to unify these common behaviors.

@sbrannen sbrannen added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement labels Nov 15, 2019
@sbrannen
Copy link
Member

@ah1508,

This forces to handle BindException and MethodNotValidArgumentException, but the code is the same. For example :

I was going to say you can handle both exceptions at the same time as follows...

@ExceptionHandler({MethodArgumentNotValidException.class, BindException.class})
@ResponseStatus(HttpStatus.BAD_REQUEST)
public Map<String, String> onValidationFailureException(Exception ex) {
    return ((CommonBindingResultInterface) ex).getBindingResult().getFieldErrors().stream().collect(Collectors.toMap(FieldError::getField, FieldError::getDefaultMessage));
}

... but then I realized that MethodArgumentNotValidException and BindException do not share a common super type that declares the getBindingResult() method.

So one option to make this easier would be to introduce a common interface (like the CommonBindingResultInterface in the above example) that declares BindingResult getBindingResult(), and then MethodArgumentNotValidException and BindException could both implement that interface.

@rstoyanchev
Copy link
Contributor

@rafafael03 I've read your description several times and I can't be sure I understand what you mean and there are claims that aren't true.

First you can use either @Valid or @Validated interchangeably. They result in the same handling and do not raise different exceptions. The main difference for Spring MVC applications is that @Validated allows you to specify validation groups (hints). Also ModelAttributeMethodProcessor does not and throw MethodArgumentNotValidException but BindException.

What @ah1508 has written is correct. Validation through either of these annotations is supported for @ModelAttribute and for @RequestBody arguments. The former raises BindException but only if you don't handle it in the controller method by declaring a BindingResult argument. The latter raises MethodArgumentNotValidException.

Is this the same as your point?

As for finding a way to consolidate, perhaps MethodArgumentNotValidException could be made to extend BindException. They both have the same internal state more or less.

@rstoyanchev rstoyanchev added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 15, 2019
@ah1508
Copy link

ah1508 commented Nov 17, 2019

Indeed, if MethodArgumentNotValidException extends BindException, only one @ExceptionHandler would be needed.

@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 17, 2019
@sbrannen sbrannen added the status: pending-design-work Needs design work before any code can be developed label Nov 20, 2019
@sbrannen sbrannen added this to the 5.3 M1 milestone Nov 20, 2019
@sbrannen
Copy link
Member

Tentatively slated for 5.3 for pending design work and assessment of viability regarding the proposal to have MethodArgumentNotValidException extend BindException.

@rstoyanchev rstoyanchev removed the status: feedback-provided Feedback has been provided label Nov 20, 2019
@jhoeller jhoeller modified the milestones: 5.3 M1, 5.3 M2 Jun 17, 2020
@jhoeller jhoeller self-assigned this Jun 17, 2020
@jhoeller jhoeller changed the title Change exceptions for bean validation on Controllers Turn MethodArgumentNotValidException into subclass of BindException Jul 27, 2020
@jhoeller jhoeller removed the status: pending-design-work Needs design work before any code can be developed label Jul 27, 2020
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

7 participants