Improve method validation support for containers with constraints on container elements

[mcso]

Affects: 6.1.1

---

With the method validation improvement in 6.1, a few inconsistencies compared to bean validation. I could imagine there are usecases I am not aware of being the reason, but it seems like the two types could be a lot more aligned.

1. List naming

@GetMapping
public String get(@RequestParam(required = false) @Valid List<@NotBlank String> list) {
    // ... 
}

Validation of the above snippet will result in the object name when handling the HandlerMethodValidationException to be "stringList". Had the parameter been a plain String, you can get the name of the parameter as expected. With bean validation, it also manages to tell the correct field path, name, and index.

In addition, the error for the invalid list when using HandlerMethodValidationException.getAllErrors() doesn't contain information about which index had the validation failure. You can reconstruct this information by using HandlerMethodValidationException.getAllValidationResults() instead, as the object there does contain index information.

As a sidenote. When looking at the codes field when handling the error, it seems to be very different for a String parameter and a list.
String parameter:

List parameter:

Controller and parameter name is not present there either, generally making it look like lists are not handled in a similar way that the String parameter is.

2. Error handling

@Override
protected ResponseEntity<Object> handleMethodArgumentNotValid(MethodArgumentNotValidException ex, HttpHeaders headers, HttpStatusCode status, WebRequest request) {
    List<Map<String, String>> errors = ex
            .getAllErrors()
            .stream()
            .map(objectError -> {
                Map<String, String> error = new HashMap<>();
                error.put("field", ((FieldError) objectError).getField());
                error.put("error", objectError.getDefaultMessage());

                return error;
            }).toList();

    ex
            .getBody()
            .setProperty("errors", errors);

    return super.handleMethodArgumentNotValid(ex, headers, status, request);
}

@Override
protected ResponseEntity<Object> handleHandlerMethodValidationException(HandlerMethodValidationException ex, HttpHeaders headers, HttpStatusCode status, WebRequest request) {
    List<Map<String, String>> errors = ex
            .getAllErrors()
            .stream()
            .map(messageSourceResolvable -> {
                Map<String, String> error = new HashMap<>();
                String parameterValue;
                if(messageSourceResolvable instanceof ObjectError objectError) {
                    parameterValue = objectError.getObjectName();
                } else {
                    parameterValue = ((MessageSourceResolvable) messageSourceResolvable.getArguments()[0]).getDefaultMessage();
                }

                error.put("parameter", parameterValue);
                error.put("error", messageSourceResolvable.getDefaultMessage());

                return error;
            }).toList();

    ex
            .getBody()
            .setProperty("errors", errors);

    return super.handleHandlerMethodValidationException(ex, headers, status, request);
}

As a continuation of the inconsistency above, when handling the exception afterwards, extracting the validation failure information is inconsistent.
For bean validation, it seems like the errors are always type FieldError (really SpringValidationAdapter#ViolationFieldError, but that is private), making it consistent across the different field types in your beans. MethodArgumentNotValidException.getAllErrors() is returning List<ObjectError>.
Method validation is less consistent. HandlerMethodValidationException.getAllErrors() return List<? extends MessageSourceResolvable>, and the specific type of MessageSourceResolvable differs depending if it is a String or a list parameter. For a list, it is ObjectError (SpringValidationAdapter#ViolationObjectError), and for String it is DefaultMessageSourceResolvable, making you have to consider the specific type if you want consistent error information. If you want the field name:
String: ((MessageSourceResolvable) messageSourceResolvable.getArguments()[0]).getDefaultMessage()
List: ((ObjectError) messageSourceResolvable).getObjectName()
This can also be seen in the sidenote from point 1, the object name is not in the same place in either of the examples.

Demo project: demo.zip

[rstoyanchev]

Thanks for the feedback and demo.

In Spring Validation, an error is represented with MessageSourceResolvable where error codes are built from an
object name, field name, and field type. An Errors instance is created for a specific object and it contains a combination of object and field errors each of which is MessageSourceResolvable.

The goal of the method validation improvement is to adapt all violations to this. Method parameters that have nested ConstraintViolations such as @ModelAttribute or @RequestBody map to Errors easily. There is an object and a field for each violation. Method parameters directly annotated with @Constraints such as @RequestParam, @PathVariable however can only be mapped to a basic MessageSourceResolvable and as there is no object name or field name, error codes are built from {controllerName}#{methodName} and the parameter name and type instead.

This explains the difference you see in the error codes for some method parameters vs others. That said, when it comes to lists, arrays, and maps, we currently handle those as beans with nested violations, but as you have correctly pointed out they can also be lists with direct constraints on elements, leading to the inconsistency in how @NotBlank String a vs List<@NotBlank String> b is handled.

I'll make changes to ensure list, array, and map handling is independent of whether a method parameter is a bean with nested violations or simple type elements. This means container, containerIndex, and containerKey will move from ParameterErrors up to ParameterValidationResult. You'll still to access such information from there. In other words, I don't think it belongs in the error code since those usually don't vary by index.

[rstoyanchev]

@mcso, container element support now applies for any ParameterValidationResult and independent to whether the element is an object with nested violations, or a simple type. That means String and List<String> parameters should now be treated consistently.

As mentioned before, error codes still don't reflect a container index. This is by design since error messages are for elements irrespective of their position within a list. However, you can get the container index from ParameterValidationResult if needed. To make the test in the sample pass I changed RestExceptionHandler as follows:

@Override
protected ResponseEntity<Object> handleHandlerMethodValidationException(
        HandlerMethodValidationException ex, HttpHeaders headers, HttpStatusCode status, WebRequest request) {

    ex.getBody().setProperty("errors", ex.getAllValidationResults().stream()
            .flatMap(result -> result.getResolvableErrors().stream()
                    .map(error -> {
                        String param = (error instanceof ObjectError objectError ?
                                objectError.getObjectName() :
                                ((MessageSourceResolvable) error.getArguments()[0]).getDefaultMessage());

                        param = (result.getContainerIndex() != null ?
                                param + "[" + result.getContainerIndex() + "]" : param);

                        return Map.of("parameter", param, "error", error.getDefaultMessage());
                    }))
            .toList());

    return super.handleHandlerMethodValidationException(ex, headers, status, request);
}

If you could you check with 6.1.3-SNAPSHOT and let me know what you think that would be much appreciated.

[mcso]

Hope you had pleasant holidays, and a happy New Year.

I admit I used the codes as an indicator/proof for this ticket that things were handled differently, not really something I have used in my code. It was only when trying to figure out differences between the two exceptions I noticed these were different as well.

This change allows me to get rid of a really ugly hack with reflection, in order to achieve the same result. So this is very much appreciated, and my primary objective is solved.
It also seems appropriate when it is a bugfix version number getting bumped, and not minor. Any larger changes would probably break code for many others.

It could be on the endless TODO list to look at, if MethodArgumentNotValidException and HandlerMethodValidationException could be merged together, or just simply handling them to be more aligned. I assume most working with these, would not know about your explanation earlier. They seem so closely related, yet has to be handled differently.
Probably not a discussion to have on a closed ticket, just an idea for future improvements.

[rstoyanchev]

Happy New Year, and thanks for the additional feedback!

MethodArgumentNotValidException is from validation of a single argument such as @ModelAttribute, @RequestBody, or @RequestPart. It's an exception that implements BindingResult (and Errors). HandlerMethodValidationException is raised when method validation is required for constraints directly on method parameters. It contains ParameterValidationResults that the above argument types would be ParameterErrors (i.e. also Errors). Essentially the same information, but as a result of validating a single argument vs applying method validation.

We continue to raise MethodArgumentNotValidException for now for backwards compatibility in cases where method validation is not needed, but I can see that one may wish to opt into dealing with just HandlerMethodValidationException. It's actually easy to adapt one to the other.