Closed Sam-Kruglov closed 1 year ago
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}
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.
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.
And one more: org.springframework.beans.TypeMismatchException
doesn't have propertyName
initialized even though it's subtype knows it, just doesn't call initPropertyName()
.
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")
+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 :)
@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.
@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 :-)
Is there any update on this issue, or any plans that relate to this issue?
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
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.
Thanks for all feedback @Sam-Kruglov and everyone else who commented. I've created #29825 as the main issue for validation improvements targeting 6.1 M1. Please, have a look at the details in the description of that issue, and provide feedback as needed.
I'll leave this issue open, and schedule it for 6.1 M1 too, as it has a number of extra pointers such as the ones for TypeMismatchException
and MissingServletRequestParameterException
, and for Jackson's MismatchedInputException
. We'll likely create separate issues for those, but I'll use this as a checklist to ensure we've addressed the feedback from all comments.
A large part of this issue is superseded by the support for built-in web method validation in #30645 where ConstraintViolationException
is extended by MethodValidationException
and exposes MessageSource
resolvable error codes for each violation, including Errors
for an @Valid
argument with cascaded violations. Initial support for M1 with #29825 is available. There are further changes planned under the umbrella issue #30645 to add built-in response handling.
6b50b7b addresses https://github.com/spring-projects/spring-framework/issues/26219#issuecomment-741729335
1e3161b addresses https://github.com/spring-projects/spring-framework/issues/26219#issuecomment-748503180
For Jackson's MismatchedInputException
from https://github.com/spring-projects/spring-framework/issues/26219#issuecomment-741618630, that would require something similar to adapting ConstraintViolation
s to MessageSource
resolvable error codes, but for Jackson errors. It's more part of message conversation than validation, and would need to be considered separately.
@ascopes thanks for the ideas to group errors by method parameter (headers, path variables, etc). That's easier now with #29825 where MethodValidationException
exposes ParameterValidationResult
s, and each has a MethodParameter
that in turn provides access to annotations. This is method validation support from spring-context, but the goal is to improve further at the web level with #30644 for M2 so you can subscribe there for progress and feedback.
As for https://github.com/spring-projects/spring-framework/issues/26219#issuecomment-1195260724, I see your point there although for error handling purposes you still need to differentiate which is request header, path variable, etc. In addition, the method validation support we are now adding should be able to address these needs for error handling.
Quick further update: #30644 and #30813 are now complete. In particular, for headers, cookie values, etc, see the visitResults
method in the new HandlerMethodValidationException
.
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:
If we send the following requests, we will get these results:
Now, handling these 2 is not very nice although we want the same result in the end:
So, I see 3 inconveniences: