wimdeblauwe / error-handling-spring-boot-starter

Spring Boot starter for configurable REST API error handling
https://wimdeblauwe.github.io/error-handling-spring-boot-starter/
Apache License 2.0
430 stars 52 forks source link

Include validation errors in response when a HandlerMethodValidationException is thrown #102

Closed donalmurtagh closed 1 month ago

donalmurtagh commented 1 month ago

I have a @RestController with the following endpoint

@PutMapping
public void updateEvent(
    @Valid @RequestPart EventRequest eventRequest,
    @RequestPart(required = false) MultipartFile file) {
    // body omitted
}

EventRequest is a standard DTO with constraints such as

@NotNull(message = "{event.endDate.empty}")
private LocalDateTime endDate;

If eventRequest violates any of these constraints a MethodArgumentNotValidException is thrown, which gets converted to a list of validation errors in the response body.

If I add a validator to the file request part (e.g. to restrict the allowed file types)

@PutMapping
public void updateEvent(
    @Valid @RequestPart EventRequest eventRequest,
    @Valid @ValidFileType @RequestPart(required = false) MultipartFile file) {
    // body omitted
}

Now if validation of eventRequest fails, a different exception HandlerMethodValidationException is thrown, but the response does not include the validation errors, e.g.

{
    "code": "HANDLER_METHOD_VALIDATION",
    "message": "400 BAD_REQUEST \"Validation failure\""
}

HandlerMethodValidationException has a public getAllErrors() method, so it should be possible to include the errors in the response when this exception is thrown.

Incidentally, I tried using a @RequestParam instead of @RequestPart for the file request part, but the outcome is the same in both cases.

donalmurtagh commented 1 month ago

I had a quick look at the source code and there doesn't seem to be a specific handler for HandlerMethodValidationException.

In order to include the validation errors in the response when this exception is thrown, one would have to implement a handler similar to ConstraintViolationApiExceptionHandler.

wimdeblauwe commented 1 month ago

This seems like it would be a good addition to this library. How does @ValidFileType look like exactly?

donalmurtagh commented 1 month ago

ValidFileType is a regular custom constraint annotation

@Documented
@Constraint(validatedBy = MultiPartFileValidator.class)
@Target(ElementType.PARAMETER)
@Retention(RetentionPolicy.RUNTIME)
public @interface ValidFileType {

    // Default list of allowed file types
    String[] value() default {
        MediaType.TEXT_PLAIN_VALUE,
        MediaType.APPLICATION_PDF_VALUE
    };

    String message() default "";

    Class<?>[] groups() default {};

    Class<? extends Payload>[] payload() default {};
}

For the sake of completeness, here's the validator it calls

class MultiPartFileValidator implements ConstraintValidator<ValidFileType, MultipartFile> {

    private List<String> allowed;

    @Override
    public void initialize(ValidFileType constraintAnnotation) {
        allowed = List.of(constraintAnnotation.value());
    }

    @Override
    public boolean isValid(MultipartFile file, ConstraintValidatorContext context) {
        return file == null || allowed.contains(file.getContentType());
    }
}

The specific details of this validator aren't particularly relevant. The important point is that when there's more than one validated request part, the exception that's thrown on validation failure is a HandlerMethodValidationException rather than a MethodArgumentNotValidException

donalmurtagh commented 1 month ago

I've added the following custom handler for this exception and it seems to work reasonably well for my use case

@Component
public class MethodValidationExceptionHandler extends AbstractApiExceptionHandler {

    public MethodValidationExceptionHandler(HttpStatusMapper httpStatusMapper,
                                            ErrorCodeMapper errorCodeMapper,
                                            ErrorMessageMapper errorMessageMapper) {

        super(httpStatusMapper, errorCodeMapper, errorMessageMapper);
    }

    @Override
    public boolean canHandle(Throwable exception) {
        return exception instanceof HandlerMethodValidationException;
    }

    @Override
    public ApiErrorResponse handle(Throwable ex) {
        var response = new ApiErrorResponse(HttpStatus.BAD_REQUEST, getErrorCode(ex), getErrorMessage(ex));
        var validationException = (HandlerMethodValidationException) ex;
        var errors = validationException.getAllErrors();

        errors.stream()
            .filter(error -> StringUtils.isNotBlank(error.getDefaultMessage()))
            .forEach(error -> {
                if (error instanceof FieldError fieldError) {
                    var apiFieldError = new ApiFieldError(
                        fieldError.getCode(),
                        fieldError.getField(),
                        fieldError.getDefaultMessage(),
                        fieldError.getRejectedValue(),
                        null);
                    response.addFieldError(apiFieldError);

                } else {
                    var lastCode = Optional.ofNullable(error.getCodes())
                        .filter(codes -> codes.length > 0)
                        .map(codes -> codes[codes.length - 1])
                        .orElse(null);
                    var apiGlobalErrorMessage = new ApiGlobalError(lastCode, error.getDefaultMessage());
                    response.addGlobalError(apiGlobalErrorMessage);
                }
            });

        return response;
    }
}
wimdeblauwe commented 1 month ago

I created a PR based on the code you posted here. See https://github.com/wimdeblauwe/error-handling-spring-boot-starter/pull/107. I did not include the filter on the non-blank default error message, because I think there is still value in having it and being able to set the message from the properties.

donalmurtagh commented 1 month ago

Thanks for your response. The PR looks good to me.

Am I right in saying that if the starter and an application both include a handler for the same exception type, then the handler in the application will take precedence over the handler in the starter? I'm assuming this is what @ConditionalOnMissingBean does.

Hopefully I can use your handler instead of mine, but I'll try it out once you've released a version that includes it.

wimdeblauwe commented 1 month ago

The @ConditionalOnMissingBean means that you can create your own HandlerMethodValidationExceptionHandler bean and it will take precedence over the one that is automatically created. So you would need to subclass that actual class to make it work. Another class that just mentions the same exception type in the canHandle method would not replace the autoconfigured one.

donalmurtagh commented 1 month ago

The @ConditionalOnMissingBean means that you can create your own HandlerMethodValidationExceptionHandler bean and it will take precedence over the one that is automatically created. So you would need to subclass that actual class to make it work. Another class that just mentions the same exception type in the canHandle method would not replace the autoconfigured one.

If one wants to replace a handler in the starter, then defining the replacement handler as a subclass of the class being replaced seems quite odd.

It would be preferable if it were possible to replace a handler by implementing ApiExceptionHandler or extending AbstractApiExceptionHandler and giving the bean the same name as the handler being replaced.

wimdeblauwe commented 1 month ago

You can test it now: https://github.com/wimdeblauwe/error-handling-spring-boot-starter/releases/tag/4.5.0

wimdeblauwe commented 1 month ago

If one wants to replace a handler in the starter, then defining the replacement handler as a subclass of the class being replaced seems quite odd.

True, but that is how the Spring mechanism works.

donalmurtagh commented 1 month ago

True, but that is how the Spring mechanism works.

In my experience if I want to replace a bean defined by Spring Boot, Spring Security, etc. then I usually just have to have implement a particular interface and/or give the bean a particular name. I don't ever recall having to extend the class of the bean I'm replacing.

donalmurtagh commented 1 month ago

You can test it now: https://github.com/wimdeblauwe/error-handling-spring-boot-starter/releases/tag/4.5.0

It works like a charm. Thanks very much for your help.