-
Notifications
You must be signed in to change notification settings - Fork 579
HV-1591 Log warning when @Valid
is used improperly
#1635
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
base: main
Are you sure you want to change the base?
Conversation
The rootLonger didn't seemt to be configured correctly
Thanks for your pull request! This pull request appears to follow the contribution rules. › This message was automatically generated. |
@Valid
is used improperly@Valid
is used improperly
The specification doesn't define what should happen when `@Valid` is used on the container and on the container type. We've decided to log a warning.
It's possible to see the log via this test: b762667 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @DavideD, thanks for taking a look at this one!
HV has three ways to define the constraints: annotations, XML and programmatic API, so if we'd go with this, we'd need to do it in a place where we combine all the sources....
But ... I realised that I've misled you by pointing to this JIRA while actually thinking of something slightly different 😖 🙈
The problem is that people keep using the legacy approach for validating container elements, i.e. write this:
@Valid List<MyPojo> list; // expecting HV to check that the list elements are `@Valid`.
and we want them to finally switch to the more appropriate way of:
List<@Valid MyPojo> list;
particularly because we'd eventually want to get rid of that legacy approach (see jakartaee/validation#266)
and eventually something like
@Valid // (1)
MyContainer<@Valid /*(2)*/ MyElement> container;
would actually be doing what it's intended to: (1)
will apply cascading validation to the container itself, while (2)
will apply it to the container elements.
Hence, we'd want to log a warning for @Valid List<MyPojo> list
instead.
All the "cascading magic" happens in here:
Lines 210 to 263 in d0ecda5
// We are now in the case where @Valid is defined on the whole object e.g. @Valid SomeType property;. | |
// | |
// In this case, we try to detect if SomeType is a container i.e. if it has a valid value extractor. | |
// | |
// If SomeType is a container, we will enable cascading validation for this container, if and only if | |
// we have only one compatible value extractor. | |
// | |
// In the special case of a Map, only MapValueExtractor is considered compatible in this case as per | |
// the Bean Validation spec. | |
// | |
// If we find several compatible value extractors, we throw an exception. | |
// | |
// The value extractor returned here is just used to add the proper cascading metadata to the type | |
// argument of the container. Proper value extractor resolution is executed at runtime. | |
Set<ValueExtractorDescriptor> containerDetectionValueExtractorCandidates = valueExtractorManager.getResolver() | |
.getValueExtractorCandidatesForContainerDetectionOfGlobalCascadedValidation( enclosingType ); | |
if ( !containerDetectionValueExtractorCandidates.isEmpty() ) { | |
if ( containerDetectionValueExtractorCandidates.size() > 1 ) { | |
throw LOG.getUnableToGetMostSpecificValueExtractorDueToSeveralMaximallySpecificValueExtractorsDeclaredException( | |
ReflectionHelper.getClassFromType( enclosingType ), | |
ValueExtractorHelper.toValueExtractorClasses( containerDetectionValueExtractorCandidates ) | |
); | |
} | |
return ContainerCascadingMetaData.of( | |
valueExtractorManager, | |
new CascadingMetaDataBuilder( | |
enclosingType, | |
typeParameter, | |
cascading, | |
addCascadingMetaDataBasedOnContainerDetection( enclosingType, containerElementTypesCascadingMetaData, groupConversions, | |
containerDetectionValueExtractorCandidates.iterator().next() ), | |
groupConversions | |
), | |
context | |
); | |
} | |
// If there are no possible VEs that can be applied right away to a declared type we should check if | |
// there are any VEs that can be potentially applied to our type at runtime. This should cover cases | |
// like @Valid Object object; or @Valid ContainerWithoutRegisteredVE container; where at runtime we can have | |
// object = new ArrayList<>(); or container = new ContainerWithRegisteredVE(); (with ContainerWithRegisteredVE | |
// extends ContainerWithoutRegisteredVE) | |
// so we are looking for VEs such that ValueExtractorDescriptor#getContainerType() is assignable to the declared | |
// type under inspection. | |
Set<ValueExtractorDescriptor> potentialValueExtractorCandidates = valueExtractorManager.getResolver() | |
.getPotentialValueExtractorCandidatesForCascadedValidation( enclosingType ); | |
// if such VEs were found we return an instance of PotentiallyContainerCascadingMetaData that will store those potential VEs | |
// and they will be used at runtime to check if any of those could be applied to a runtime type and if PotentiallyContainerCascadingMetaData | |
// should be promoted to ContainerCascadingMetaData or not. | |
if ( !potentialValueExtractorCandidates.isEmpty() ) { | |
return PotentiallyContainerCascadingMetaData.of( this, potentialValueExtractorCandidates, context ); | |
} |
and then a bit more here at "runtime":
Lines 89 to 102 in d0ecda5
return new ContainerCascadingMetaData( | |
valueClass, | |
Collections.singletonList( | |
new ContainerCascadingMetaData( | |
compliantValueExtractor.getContainerType(), | |
compliantValueExtractor.getExtractedTypeParameter(), | |
compliantValueExtractor.getContainerType(), | |
compliantValueExtractor.getExtractedTypeParameter(), | |
groupConversionHelper.isEmpty() ? GroupConversionHelper.EMPTY : groupConversionHelper | |
) | |
), | |
groupConversionHelper, | |
Collections.singleton( compliantValueExtractor ) | |
); |
These would be the primary places to add the new warning.
HV also has this annotation processor that checks some constraint definition rules at compile time ( e.g.
Line 24 in 3f8dc0a
public class MixDirectAndListAnnotationCheck extends AbstractConstraintCheck { |
@Valid
at built-in containers and report warnings. But you can put the annotation processor into a different ticket/PR if you'd like.
Some other possible "test cases":
@Valid List<MyBean> list;
@Valid Object = List<MyBean> objectButActuallyList;
@Valid Map<String, MyBean> map;
@Valid List<@Valid MyBean> list; // this will also get a warning in that cascading metadata builder.
btw, you could probably try logging the context in that new warning as done here:
Line 195 in 3f8dc0a
validateGroupConversions( context ); |
Not sure if there's a way to test that's actually logging.
There's this ListAppender
in test utils that can be used to collect logged messages. It's not too widely used 😃 but here's a test with it:
Line 35 in 3f8dc0a
public class ParameterMessageInterpolatorTest { |
|
Thanks @marko-bekhta, I will update the pr |
Fix https://hibernate.atlassian.net/browse/HV-1591
I've updated the log4j configuration because changing it wasn't having any effect.
Not sure if there's a way to test that's actually logging.
I can add a test that shows the log (if you need it), but it's not really a test.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on licensing, please check here.