-
Notifications
You must be signed in to change notification settings - Fork 38k
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
NotReadablePropertyException due to mismatch between ConstraintViolation
property path and BindingResult
target in MethodValidationAdapter
#31746
Comments
Thanks for the follow-up report @k-seth. The code snippets seem to match the tests in |
Here are a few test cases to demonstrate each of the problems: k-seth@c0035aa#diff-84f5eb0790cab0b59468f24f88055fe660b4342c385a6774e033779dc9dadfd0R256 I provided a comment to demonstrate roughly where and why each test case fails. I also included (though did not enable) the node-based container detection I mentioned as a potential option for problem 3. |
Seems like #31835 is a related issue - I'll try taking another look at this to see if I can offer some further input. |
Indeed, #31835 is a mismatch between a full I'm having a closer look myself at what we changed for #31530, and what we could do to fix handling of violations on nested objects. |
ConstraintViolation
property path and BindingResult
target in MethodValidationAdapter
I've had a closer look. Indeed, for the value unwrapping feature of bean validation supported through a If bean validation exposed the unwrapped value for a I will restore something close to the previous behavior we had, and add extra tests. |
The goal for #31530 was to support bean validation on Set and other method parameters that are containers of value(s) for which there is a registered Jakarta Validation ValueExtractor. Unfortunately, bean validation does not expose the unwrapped value for a Path.Node, which we need for a method parameter in order to create a BindingResult for the specific bean within the container, and the leafBean that we tried to use is really the node at the very bottom of the property path (i.e. not what we need). This change removes the use of beanLeaf, restores the logic as it was before, adds support for arrays, and a new test class for scenarios with cascaded violations. See gh-31746
ConstraintViolation
property path and BindingResult
target in MethodValidationAdapter
ConstraintViolation
property path and BindingResult
target in MethodValidationAdapter
Hm, thanks for looking into it Rossen. Sadly, I don't have much else to add. It is disappointing that there is no obvious way to offer parity to using Lastly, your commit 0a94dce referenced my attempt to expand collection support, and so it expanded the check to include |
Thanks for the review. I will make those adjustments. For the longer term, I created jakartaee/validation#194 to see if an improvement would be considered. |
After the updates to MethodValidationAdapter in commit d7ce13 related to method validation on element containers, we also need to adjust the checks in HandlerMethod when method validation applies. See gh-31746
The title is pretty vague, sorry - couldn't think of something better. There is plenty of detail here to make up for that though.
Spring 6.1.x brought method bean validation (#30645) - as part of the feedback to this enhancement, I provided a PR #31530 to attempt to refine this further.
In hindsight, the solution I provided also has its problems. Essentially, all the issues stem from the use of the leaf bean - whose behaviour is, unfortunately, not as I had understood at the time. As I have come to release, the change I submitted had some unintended side effects that only became clear as I fully started migrating to Spring Boot 3.2.
1. Cascaded validation can fail
In cases where a validation error occurs on a nested bean,
SpringValidatorAdopt#getRejectedValue
may attempt to get the raw value for the property. As a result, one will see an error likeJSR-303 validated property 'professor.name' does not have a corresponding accessor for Spring data binding
. This is because the code attempts to walk the full property path, starting from the provided bean. Unfortunately, the leaf bean points to the nested bean with the cascaded validation, not the initial bean where validation started. Thus, the property path is invalid from this context (Go figure, despite giving the exact bean in question, it is no good). The JSR error is not thrown in all cases, as paths which include collections with non-indexed/keyed access are fine because thegetRejectedValue
method explicitly skips walking paths with[]
in them - instead, it uses the value on the violation directly - because there is no way to access the element in question.As an example, consider the following code:
The failure can be caused by having a validation error on the professor parameter - say the name, as my error above uses.
After looking at the information available in
MethodValidationAdaptor
, I'm not sure how to get that root bean for all reasonable data types - specifically types likeSet
where there is no direct way to access the element. The leaf bean was supposed to solve that issue. It is unfortunate that theConstraintViolation
doesn't expose this either, given that it had to have known the bean to produce the path nodes in the first place. At this point, I'm not sure where else to try drawing this information from. I wonder if it is worth looking at the downstream constraint violating processing to work this behaviour in there instead if nothing better comes up.The next two are more minor in my opinion. I found them accidentally while debugging rather than because they strictly posed a problem.
2. Container element result argument incorrect when validating non-field object error
Since the leaf bean appears to represent the last bean accessed, the opposite problem to 1 can happen. Rather than the leaf bean being too precise, it isn't precise enough, though here it doesn't cause an error. Essentially, in the case of a situation like
the
leafBean
will represent the service/controller/etc. that has the parameter since there is no other bean to set the leaf to. As a result, all violations will be under the sameParameterErrors
, rather than having aParameterErrors
for each element with violations. Because of this, it should only impact types which rely on the leaf bean to provide the unique side of theBeanResultKey
. Even then, it only means that the documented behaviour is broken. All the violations are there, so no information is lost - the structure is just a bit off.All container types are impacted in some way, however, as the argument set on the parameter error will be incorrect, regardless of whether the errors are split by element or not.
I haven't had a chance to dig too deep into this one, as a solution for problem 1 may address this too. However, I did consider a few things:
CONTAINER_ELEMENT
, and then simply pass the invalid value as the beanString
, is it fine to be lumping these under the bean validation umbrella? Even if not, can an invalid string and say a null passed for a custom Bean type be reasonably distinguished?3. ParameterErrors may incorrectly set the container parameter
In the case of cascaded validation the condition here
can be evaluated as true, even if there is no container. Once again, the doing of my friend the leaf bean.
Using the same example code from problem 1, if one of the student elements has a validation error when calling
registerCourse
, the leaf bean will be for thePerson
while the arg is theCourse
. TheCourse
will then be set as a container even though it isn't one. (Note this example only works because of theSet
type, which sidesteps problem 1.)In this case, there could be a solution that relies on neither the leaf bean nor the arg class.
Path.Node
has a few subtypes (BeanNode
,ContainerElementNode
andPropertyNode
) which expose a method to check if a node is in a container. I had some success with something like:I've added some additional tests on my forked repo to exercise this and ensure the container is properly set, so I could provide that pretty easily. That said, I am making some assumption that since the others don't provide this
getContainerClass
, it is reasonable to expect them not to be container elements.If it is okay, I'd like your perspective on this @rstoyanchev, since we discussed the original change I provided. I'd for sure like to continue contributing - though I clearly need to spend more time to get more context around any changes to avoid a mishap like this again.
The text was updated successfully, but these errors were encountered: