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

Fix adaptation of violations on Set method parameter #33150

Closed

Conversation

snussbaumer
Copy link
Contributor

@snussbaumer snussbaumer commented Jul 4, 2024

Since commit 8552e14, validation of Set values (for example Set<@NotBlank String>) crashes with error : No way to unwrap Iterable without index

With this fix we don't know which value exactly is the problem, but at least we have a working validation

Maybe there is a way to know exactly which value(s) are the culprit, but without an index I don't know how to find it.

Or maybe there is a better way to fix the problem, I trust you'll do a thorough review and find it, I'm not that familiar with the codebase 😄 .

Thanks

This is the error without the fix :

java.lang.IllegalStateException: No way to unwrap Iterable without index
	at org.springframework.util.Assert.state(Assert.java:78)
	at org.springframework.validation.beanvalidation.MethodValidationAdapter.adaptViolations(MethodValidationAdapter.java:358)
	at org.springframework.validation.beanvalidation.MethodValidationAdapter.validateArguments(MethodValidationAdapter.java:247)
	at org.springframework.validation.beanvalidation.MethodValidationAdapterTests.testArgs(MethodValidationAdapterTests.java:237)
	at org.springframework.validation.beanvalidation.MethodValidationAdapterTests.validateValueSetArgument(MethodValidationAdapterTests.java:223)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

Since commit 8552e14, validation of Set values
(for example `Set<@notblank String>`) crashes with error :
No way to unwrap Iterable without index

With this fix we don't know which value exactly is the problem, but at least we
have a working validation
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jul 4, 2024
@snussbaumer
Copy link
Contributor Author

I the fix could then be backported to 6.1.x that would be great

@jhoeller jhoeller added type: regression A bug that is also a regression in: core Issues in core modules (aop, beans, core, context, expression) and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jul 5, 2024
@jhoeller jhoeller added this to the 6.1.11 milestone Jul 5, 2024
@jhoeller jhoeller requested a review from rstoyanchev July 5, 2024 06:33
@rstoyanchev rstoyanchev self-assigned this Jul 8, 2024
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an underlying limitation in that there is no way to access the value in the Set that caused the violation (see jakartaee/validation#194).

Initially, we used to create a ParameterErrors instance for arrays and collections, with the collection/array as the backing object, and the container + container index allowing you to access the value. It sort of worked for Set but there was no index and no way to access the actual value. There was an attempt to improve that in #31530 for Set and other (custom) container types, but it was based on an incomplete understanding of the available metadata, leading to #31746 and the subsequent change d7ce13c is what made it impossible to use a collection that is not indexed.

The subsequent change 8552e14 that you referenced further refined that so that collections/arrays are more correctly represented with ParameterValidationResult as directly validated method parameters (rather than with ParameterErrors for cascaded violations on an Object), so there is no longer a backing object to set, just the argument value. That did not change anything for sets though.

I think we could create ParameterValidationResult with the Set as the value that was validated, rather than the actual element, which we cannot access, but there would be no container and no container index. That would allow the error to be reported as before, but if there are multiple errors on the same Set there would be no way of telling which one is which. This wouldn't be anything new, but would simply restore what was possible in 6.1.2.

rstoyanchev pushed a commit that referenced this pull request Jul 9, 2024
@rstoyanchev rstoyanchev changed the title Fix validation of Set values Fix adaptation of violations on Set method parameter Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: regression A bug that is also a regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants