Improve the experience of handling validation errors in controller methods

[Sam-Kruglov]

Affects: 5.3.1
Relates to #23107

Basically, I have the exact same feeling as the author did but he never clarified his point.

You can validate a DTO using @Valid annotation (whether it is built from a bunch of query parameters or from the body) but you can't validate literals like that. So, to validate a request parameter or a path parameter, one has to apply @Validated annotation on the controller class level to enable the AOP proxy for method validation, then it will pick up any JSR 380 annotations.

So, here is a little example. Given the following definition:

class UserDto { @Email String email; }

@RestController
@RequestMapping("users")
@Validated
class UserController {
  
  @PostMapping
  void createUser(@Valid @RequestBody UserDto userDto) {...}
  
  @GetMapping("{email}")
  UserDto getUser(@Email @PathVariable String email) {...}

If we send the following requests, we will get these results:

--------->
POST users
{
  "email": "invalid-email"
}

<---------
throws MethodArgumentNotValidException

--------->
GET users/invalid-email
<---------
throws ConstraintViolationException

Now, handling these 2 is not very nice although we want the same result in the end:

class ValidationErrorResponse { List<Violation> violations; }
class Violation { String fieldName, message; }

  @ExceptionHandler
  ResponseEntity<ValidationErrorResponse> onConstraintValidationException(ConstraintViolationException e) {
    ValidationErrorResponse error = new ValidationErrorResponse();
    for (ConstraintViolation violation : e.getConstraintViolations()) {
      error.getViolations().add(
        new Violation(violation.getPropertyPath().toString(), violation.getMessage()));
    }
    return ResponseEntity.badRequest().body(error);
  }

  @ExceptionHandler
  ResponseEntity<ValidationErrorResponse> onMethodArgumentNotValidException(MethodArgumentNotValidException e) {
    ValidationErrorResponse error = new ValidationErrorResponse();
    for (FieldError fieldError : e.getBindingResult().getFieldErrors()) {
      error.getViolations().add(
        new Violation(fieldError.getField(), fieldError.getDefaultMessage()));
    }
    return ResponseEntity.badRequest().body(error);
  }

So, I see 3 inconveniences:

• I have to always specify @Valid annotation whenever I have a @RequestBody. Can't we just assume I want to validate all bodies? I feel like I might forget it in one place and it won't be validated. Or maybe we could add @Valid onto the DTO class itself and look there for it? Or maybe we could just scan all fields and see if any field has the @constraint annotation on it?
• I have to always specify @validated annotation on the controller class level to inspect the literals. Why doesn't it inspect them by default since it already searches for @Valid/@validated on the body?
• Error handling is cumbersome

[Sam-Kruglov]

Also worth mentioning. ResponseEntityExceptionHandler has handlers for both BindException and for MethodArgumentNotValidException which also extends BindException since the related issue was fixed. The handlers are identical, so we can remove the MethodArgumentNotValidException from there.

Actually, it would be great (although not sure if possible) if MethodArgumentNotValidException, BindException, and ConstraintViolationException would have something in common so I would only handle them all with one method transforming the result into an array of {propertyName, message}

[Sam-Kruglov]

I found one more thing, that during handling of ConstraintViolationException I actually must take a look into the class it originated from (e.constraintViolations[*].rootBeanClass) to see if that is a Controller or not. If it is, then I should inspect the parameter annotations to enrich the response saying that was a path variable or request parameter, etc. If it is not a Controller, then it would no longer qualify for 4XX response, it would be 5XX instead.

So, the point of this issue to simply highlight that this needs to be somehow redesigned because these things are very tightly coupled although I don't have a direct implementation suggestion in mind.

[Sam-Kruglov]

And one more: org.springframework.http.converter.HttpMessageNotReadableException may contain deserialization errors, which may also be client errors. In case of Jackson, I need to catch HttpMessageNotReadableException and check if it has a cause that is assignable from com.fasterxml.jackson.databind.exc.MismatchedInputException, then I will construct the JSON path like this mismatchedInputException.getPath().stream().map(JsonMappingException.Reference::getFieldName).collect(joining(".")).
So, for example, if I receive a wrong string for a DTO that has an enum there, I will be able to respond with 400 saying that field "x.y.z" has the wrong entry. While by default HttpMessageNotReadableException will respond with 400 without a body.

[Sam-Kruglov]

And one more: org.springframework.beans.TypeMismatchException doesn't have propertyName initialized even though it's subtype knows it, just doesn't call initPropertyName().

[Sam-Kruglov]

Also MissingServletRequestParameterException does not have a proper access to the parameter type (getParameterType is a String) unlike MissingPathVariableException which does. I need it to detect if the "missing variable" is actually a conversion error that came from org.springframework.data.repository.support.DomainClassConverter.

For example, if I search for users/777 and there's no user in the database with this id, I want to respond with 404 and my custom payload with an error code. In the controller I have @PathVariable("id") User user to automatically convert the Long to the User domain entity type. But if it's not found, I receive a MissingServletRequestParameterException (because DomainClassConverter returns null), so I have to check if ex.getParameter().getParameterType().getPackageName().startsWith("my.package.with.entities")

[ascopes]

+1 for this, purely because it will provide a way to ensure that the Error code API can be used consistently for IoC JSR 380 validation failures.

Worth noting that the ConstraintViolationException appears to default to being an Internal Server Error as of v2.4.0.

Excuse this if this appears completely out of place for me to suggest this, as my intention is definitely not to cause insult! Currently, with the state of the framework sometimes triggering ConstraintViolation, and sometimes MethodArgumentNotValidException, I cannot make use of the error codes that are provided. In this sense, it kind of makes using the binding error response shape pointless, as one cannot maintain a consistent response shape.

For this reason, I currently have to work around these issues and roll my own set of validation, attempting to unify these cases into something that when put down on a swagger spec, appears to be consistent. This would be a massive help towards being able to write consistent and reusable components and validation rules to use across a platform of microservices that use Spring MVC and Spring WebFlux :)

---

As a slightly off-topic on-topic suggestion...

As a slightly off-topic on-topic suggestion, it would be nice to eventually have a way to traverse the PropertyPath of the internal constraint violation that was triggered by the JSR-380 implementation, but in such a way that we can reflectively inspect each element that we are attempting to look at (i.e. get a Class or Parameter directly for each nested part of the validation.

The reason for suggesting this, is that we could then essentially have logic that identifies all the annotations on an annotated element that fails validation (currently, we can only view the annotation that caused the constraint violation). By allowing this, we would have the ability to produce responses such as the following:

{
  "status": 400,
  "error": "Bad Request",
  "message": "One or more validation failures occurred in the request",
  "invalidParameters": {
    "headers": [{
        // How can I consistently get this header name in this format for my response messages?
        // the constraint violation format makes this overly difficult. If I could reflect to see if there 
        // was a @RequestHeader annotation, this would be much easier.
        "Correlation-ID": "must be specified"
     }],
     "body": [{
       // we currently have to do all the reflection manually that has to discover the json naming
       // conventions in use, taking into acount @JsonUnwrapped, @JsonProperty. If we have
       // to provide XML, we need the ability to consider JAXB too, etc. This means if our API
       // has to use kebab-case naming, we can still provide sensible meaningful and potentially
       // machine-readable responses to the consumer if we need to, without mixing up conventions
       // that may reduce the meaningfulness of the validation violation location.
       "location": "$.user.roles[2].name",
       "reason": "must not be blank"
     }] 
  }
}

Making reference to one of the JSON path guidelines that can be found online.

Whether this would come under the scope of being implementation detail on behalf of JSR-303 and JSR-370 or not, I would not like to say, but the reason I bring this up is that it may be overly beneficial to take a larger look at how this API is presented to those who may be using Spring Web MVC or WebFlux to produce integration-level microservices rather than full-stack applications that also serve their front end. While I appreciate that Spring has opinionated defaults, currently it can be very difficult to migrate to Spring as your primary framework for web development when you have a set of business defined guidelines respecting the shape of response messages you provide, as it can result in writing a lot of boilerplate code to work around the current limitations of how the design is presented.

I guess as a TLDR, I am wondering whether it is worth considering how the validation API is presented overall in terms of usage for Web validation, as currently it can cause a lot of work if you are unable to follow the exact designs that Spring has to provide. Ideally we would like to be able to use Spring alongside our existing standards for how to provide responses.

If this is completely out of place to suggest this, then that is fine, and I apologise for placing this wall of text here, but my hope is that if not, there may be other suggestions on how to work around the issues in the original message among others with providing specific representations of errors in a way that allows migration from other frameworks without having to rewrite all consumers of a service to work with the new validation model.

[Sam-Kruglov]

@ascopes thanks for your input!

I kind of agree that ConstraintViolationException should be a server error. The reason is that you can annotate any @Component with @Validated and validate method parameters. These can come from a 3rd party API for example, so it's you who should decide for sure if it's the client's fault or not. Or you can just throw this exception yourself because it's not owned by Spring. Or Hibernate could throw it before saving stuff to the database. However, I think if it's originated from a @Controller class, it should definitely be a client error like I stated here.

About ConstraintViolationException vs MethodArgumentNotValidException I explained here. Basically constraint error originates from @Validated. I agree that it should be somehow refactored.

About property path, not sure what you mean. You can access constraintViolationException.constraintViolations[*].propertyPath to build the location in your example. As for the access to all annotations of each property along the way, I don't see how it would be useful: if I have 10 different @Pattern annotations on a field and I only failed one, I don't want to see all 10 errors in the response but I do want to see all 10 in the API documentation but that's different. So, I think just having access to the name of the property is fine. Take a look at my test cases here, they use a nested field like that and the response contains the proper path. Exceptions handled are MethodArgumentNotValidException (JSR 380), and HttpMessageNotReadableException (wrong data type in JSON).

About correlation id, not sure again why would you want it inside the error response body, distributed tracing is usually done in headers, not in the body. You would still have the trace id in the response header even if that was a successful response.

I'm not sure if I missed something or not, let me know.

[ascopes]

@Sam-Kruglov

What you have suggested sounds like a good plan; I am happy with that.

The main thing about the ability to access other annotations is not necessarily to get other validation info, as much as it is an ability to introspect annotations for say, JAXB or Jackson or whatever is in use. This would provide the ability to give accurate XPath or JSONPath references to the erroneous element that failed validation.

While we can use the property path to some extent, this kind of begins to break down when using annotations such as @JsonUnwrapped, or @XmlAttribute. I do agree most of the time that this is just as meaningful to a consumer, but when serving larger entities in responses, the ability to add extra clarification can also be useful. E.g. producing content-type aware messages

<!-- Based off of RFC-7807's example format, modified for this example -->
<problem>
  <status>400</status>
  <title>Bad Request</title>
  <detail>Validation failures occurred</detail>
  <invalidParams>
    <invalidParam>
      <in>headers</in>
      <name>X-RateLimitUnit</name>
      <detail>Must be either seconds or milliseconds</detail>
    </invalidParam>
    <invalidParam>
      <in>body</in>
      <location>//user/authorities[@type='role'][5]</location>
      <type>role</type>
      <detail>Role names must not contain spaces</detail>
    </invalidParam>
  </invalidParams>
</problem>

which would for a corresponding JSON request for the same schema, perhaps look like this:

{
  "status": 400,
  "title": "Bad Request",
  "detail": "Validation failures occurred",
  "invalidParams": [
    {
      "in": "headers",
      "name": "X-RateLimitUnit",
      "detail": "Must be either \"seconds\" or \"milliseconds\""
    },
    {
      "in": "body",
      "location": "$.authorities.roles[5]",
      "type": "role",
      "detail": "Role names must not contain spaces"
    }
  ]
}

Being able to easily perform this kind of thing would also then have the side effect of encouraging new users to use spring boot if they were previously using, say, some custom in-house validation with a JAX-RS servlet, as they will be able to still interoperate with existing services and conventions across their systems without having to perform a large rewrite of multiple services that rely on specific validation formatting.

Additionally, it allows easier code-reuse when handling things like headers that store their representation in a second annotation.

For example

Mono<ResponseEntity<Void>> createUser(
    @Valid @NotBlank @RequestHeader("Correlation-ID") String correlationId,
    @Valid @RequestBody NewUser user
) {
    ...
}

...where you would want to show "Correlation-ID" as the erroneous item name if the @NotBlank invalidated the method call, rather than the parameter name itself which is "correlationId".

Of course, you can just validate most of these manually by creating the boilerplate, but that does feel like it kind of breaks the spirit of how spring is meant to be used, and can be a bit awkward for larger projects or groups of projects that want to be able to enforce consistency by extracting validation formatting to a common component, for example :-)

[ascopes]

Is there any update on this issue, or any plans that relate to this issue?

[vvalchev]

I came upon this issue, because we noticed the inconsistencies in the Errors API. So in order to generate the same error response we came up with this quick'n'dirty solution:

@Component
public class CustomErrorAttributes extends DefaultErrorAttributes {

  @Override
  public Map<String, Object> getErrorAttributes(WebRequest webRequest, ErrorAttributeOptions options) {
    Map<String, Object> errorAttributes = super.getErrorAttributes(webRequest, options);
    Throwable error = getError(webRequest);
    if (error instanceof ConstraintViolationException) {
      Set<ConstraintViolation<?>> violations = ((ConstraintViolationException) error).getConstraintViolations();
      errorAttributes.put("errors", violations.stream()
          .map(CustomErrorAttributes::of)
          .collect(Collectors.toList())
      );
    }

    return errorAttributes;
  }

  private static FieldError of(ConstraintViolation<?> cv) {
    return new FieldError(
        cv.getRootBeanClass().getCanonicalName(), // object name
        cv.getPropertyPath().toString(), // FIXME: currently is 'method.param'
        cv.getInvalidValue(), // rejected value
        false,
        new String[]{cv.getConstraintDescriptor().getAnnotation().annotationType().getSimpleName()},
        cv.getExecutableParameters(),
        cv.getMessage()
    );
  }

}

It is far from perfect. It can be improved, if we reuse the code in SpringValidatorAdapter, which also converts from ConstraintViolationException to errors.

Further details on the improved solution can be found in the following discussion: https://stackoverflow.com/questions/14893176/convert-jsr-303-validation-errors-to-springs-bindingresult

[ascopes]

The thing I wish we could do with this is be able to generate a consistent header name, cookie name, path variable, JSON path or xpath from the property path without adding massive amounts of reflective code. This would be really helpful for giving error messages to the user that reflect the payload shape rather than the spring internal representation which may differ with some Jackson annotations like unwrapping.

{
  "status": 400,
  "title": "Bad Request",
  "detail": "Validation failed",
  "invalidParams": [
    {
      "location": "headers",
      "name": "X-Request-ID",
      "reason": "Must be a non-empty string"
    },
    {
      "location": "body",
      "name": "$.user.name",
      "reason": "Must be specified"
    }
  ]
}

While I can write tonnes of code to try and cover the cases, it would be a great set of features to have internally. Error messages that use standard notations to refer to the request format would also prevent leaking internal implementation detail.

The issue I currently see with the Spring 5.x.x branch is that since there is no consistent way to generate tidy validation without dealing with lots of edge cases, everyone seems to end up implementing their own solutions to this, so there is never a standard format that is widely accepted. Most enterprise companies won't use the current binding error responses on their public facing systems because the error messages can leak internal system details like the platform and libraries in use, or the error messages don't tarry up with the request format if snake_case, kebab-case, or custom named attributes are used.

Businesses then consider that to be a security risk in some cases. (Security through obscurity isn't really an excuse in my opinion but opinions on the subject tend to differ wildly between people).

One thing that does come to mind to make this easier is how JAXRS allows @BeanParam to define beans containing common request attributes. That would not only allow reduction of parameters crammed with lots of annotations, but would also allow using a single validator to cover the headers, path, and body rather than needing lots of separate components to do this.

It would also allow body validation failures to be reported alongside header validation errors.

@Validated
public record NewUserRequest(
  @NotBlank @RequestHeader("X-Request-ID") String requestId,
  @NotNull @Valid @RequestBody NewUser body
) { }

...

@PostMapping("/users")
public void newUser(@Valid @RequestBean NewUserRequest request) {
    ...
}

From previous experience, BeanParam in JAXRS made consistent validation much simpler to handle.

This would also potentially make using bean validation with functional routing easier too, as you could still produce an annotated payload and validate it. It isn't uncommon to want to move a lot of headers, path variables, the body, and other components around as one bean between components either.