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

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

Closed
donalmurtagh opened this issue Sep 19, 2024 · 12 comments · Fixed by #107
Closed
Milestone

Comments

@donalmurtagh
Copy link

donalmurtagh commented Sep 19, 2024

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
Copy link
Author

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
Copy link
Owner

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

@donalmurtagh
Copy link
Author

donalmurtagh commented Sep 20, 2024

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
Copy link
Author

donalmurtagh commented Sep 20, 2024

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
Copy link
Owner

I created a PR based on the code you posted here. See #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
Copy link
Author

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
Copy link
Owner

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.

wimdeblauwe added a commit that referenced this issue Sep 25, 2024
feat: Support HandlerMethodValidationException
@donalmurtagh
Copy link
Author

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
Copy link
Owner

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

@wimdeblauwe
Copy link
Owner

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
Copy link
Author

donalmurtagh commented Sep 25, 2024

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants