spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
74.5k stars 40.53k forks source link

DefaultErrorAttributes doesn't populate errors for MethodValidationResult #42013

Open sunruh opened 3 weeks ago

sunruh commented 3 weeks ago

GH-39865 added support for MethodValidationResult to DefaultErrorAttributes but it filters the list errors published in the result and used for counting to instances of ObjectError.

With a method signature (in a RestController) of e.g. public void method(@PathVariable @Pattern(regexp = "^a.*") String parameter), this results in

This is due to the fact, that the MethodValidationResult generated by MethodValidationAdapter will contain instances of ParameterValidationResult that have resolvable errors of type DefaultMessageSourceResolvable (and not ObjectError).

Spring Boot 3.3.1 Spring Framework 6.1.10

wilkinsona commented 3 weeks ago

This was (somewhat) intentional as we wanted to be confident that the error was suitable for serializing to JSON. Filtering things so that only ObjectError instances are considered achieved this and aligned with what we already do for BindingResult. Perhaps we can relax things a little. To help us to investigate, please provide a minimal sample that reproduces the behaviour that you have described.

sunruh commented 3 weeks ago

As an ObjectError extends DefaultMessageSourceResolvable, relaxing it should be ok.

Actually the example in https://github.com/spring-projects/spring-framework/issues/32396#issuecomment-1985602958 would cause it.

wilkinsona commented 2 weeks ago

Thanks for the additional details but I'd really like to see a complete, minimal sample that reproduces this. Can you please provide one?

sunruh commented 1 week ago

spring-boot-issue-42013.zip

Here's a minimal sample. I based it on Spring Boot 3.3.3.

The tests do not use MockMvc on purpose as otherwise the error controller would never be called.

philwebb commented 1 week ago

I'm wondering if we should create our own wrapper object for serialization and try to support the MessageSourceResolvable. Looking at the code, there's a lot of overlap with the reactive and servlet version of DefaultErrorAttributes and I wonder if now might also be the time to try and pull the common code up to the org.springframework.boot.web.error package.

philwebb commented 1 week ago

We discussed this today and would like to add a wrapper object. This will be a slight breaking change and the Javadoc of DefaultErrorAttributes will need to be updated. We'll need to provide a way for the user to get the original ObjectError if they wish to.