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.

Sign up for GitHub

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

Jump to bottom

Explore FreeForm validation v2 #989

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Explore FreeForm validation v2 #989

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f61a93d
[property-holder-preparation] Remove impossible case from ValidatorIm…
marko-bekhta Jun 29, 2018
80a4bb8
[property-holder-preparation] Delegate retrieving bean metadata to Ca…
marko-bekhta Jun 29, 2018
9418a35
[property-holder] Introduce property holder property and property holder
marko-bekhta Jul 8, 2018
415abf7
[property-holder-preparation] Move metadata manager to its own package
marko-bekhta Jul 9, 2018
4a523a8
[property-holder-preparation] Extract bean metadata provider code to its
marko-bekhta Jul 9, 2018
37d4545
[property-holder-preparation] Extract BeanMetaDataBuilder to upper level
marko-bekhta Jul 12, 2018
00a7af7
[property-holder-preparation] Make ConstraintMetaDataManager return m…
marko-bekhta Jul 12, 2018
f75e407
[property-holder] Create implementations of cascading metadata for pr…
marko-bekhta Jul 15, 2018
ac7835f
[property-holder] Make property holder configuration use location bui…
marko-bekhta Jul 18, 2018
92b0696
[property-holder] Introduce dummy metadata provider for property holders
marko-bekhta Jul 19, 2018
4e021be
[property-holder] Introduce method for validating property holders an…
marko-bekhta Jul 19, 2018
9f1b1da
[property-holder] Rework how constraints are validated for default group
marko-bekhta Jul 22, 2018
1807098
[property-holder] Rewrite how cascading constraints are created
marko-bekhta Jul 22, 2018
8076e67
[property-holder] start working on programmatic definition
marko-bekhta Aug 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 50 additions & 32 deletions engine/src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,42 +453,60 @@ private <U> void validateConstraintsForDefaultGroup(BaseBeanValidationContext<?>
final BeanMetaData<U> beanMetaData = valueContext.getCurrentBeanMetaData();
final Map<Class<?>, Class<?>> validatedInterfaces = new HashMap<>();

// evaluating the constraints of a bean per class in hierarchy, this is necessary to detect potential default group re-definitions
for ( Class<? super U> clazz : beanMetaData.getClassHierarchy() ) {
BeanMetaData<? super U> hostingBeanMetaData = constraintMetaDataManager.getBeanMetaData( clazz );
boolean defaultGroupSequenceIsRedefined = hostingBeanMetaData.isDefaultGroupSequenceRedefined();

// if the current class redefined the default group sequence, this sequence has to be applied to all the class hierarchy.
if ( defaultGroupSequenceIsRedefined ) {
Iterator<Sequence> defaultGroupSequence = hostingBeanMetaData.getDefaultValidationSequence( valueContext.getCurrentBean() );
Set<MetaConstraint<?>> metaConstraints = hostingBeanMetaData.getMetaConstraints();

while ( defaultGroupSequence.hasNext() ) {
for ( GroupWithInheritance groupOfGroups : defaultGroupSequence.next() ) {
boolean validationSuccessful = true;

for ( Group defaultSequenceMember : groupOfGroups ) {
validationSuccessful = validateConstraintsForSingleDefaultGroupElement( validationContext, valueContext, validatedInterfaces, clazz,
metaConstraints, defaultSequenceMember );
}
if ( !validationSuccessful ) {
break;
}
}
boolean defaultGroupSequenceIsRedefined = beanMetaData.isDefaultGroupSequenceRedefined();
if ( defaultGroupSequenceIsRedefined ) {
validateConstraintsForRedefinedGroupSequence( validationContext, valueContext, validatedInterfaces, beanMetaData.getBeanClass(), beanMetaData );
validationContext.markCurrentBeanAsProcessed( valueContext );
}
else {
validateConstraintsForSingleDefaultGroupElement( validationContext, valueContext, validatedInterfaces, beanMetaData.getBeanClass(), beanMetaData.getDirectMetaConstraints(), Group.DEFAULT_GROUP );
List<Class<? super U>> classHierarchy = beanMetaData.getClassHierarchy();
Copy link
Member Author

Choose a reason for hiding this comment

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

I plan to make BeanMetaData an abstract class with regular and property holder impls. Then in case of property holder there will be nothing else but the property holder class in this list.

Also note the change itself. Before this we were accessing a map to get the metadata that we already have. With this we remove that one additional map access.

Copy link
Member

Choose a reason for hiding this comment

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

Could you explain the rationale of this change?

It's hard to see the difference, considering all the method is rewritten in the diff.

Once you got this cleared, I think I'll merge it separately (and probably run a JMH benchmark to check everything is OK).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah ... Let me try to explain how I got here. The main "problem" that I was trying to "solve" was to reduce the number of places where constraintMetaDataManager.getBeanMetaData( clazz ); is called. In case of property holders we cannot use such call And that's why for example in case of cascadebles I've moved such call into metadata class. In this particular place except the call for metadata itself we also access the metadata for the "most specific" class twice.

We already have the metadata from the context here - BeanMetaData<U> beanMetaData = valueContext.getCurrentBeanMetaData(). But then in the loop for ( Class<? super U> clazz : beanMetaData.getClassHierarchy() ) we would get the beans class as a first element and try to get that same metadata again - BeanMetaData<? super U> hostingBeanMetaData = constraintMetaDataManager.getBeanMetaData( clazz );

Hence to skip this additional metadata retrieval I've broken the method into two parts - first one will work with the existing "current" metadata valueContext.getCurrentBeanMetaData() and in the second part we iterate through the bean class hierarchy skipping the first element which is the bean class.

I also remember running JMH tests for these changes and the results were almost identical.

In the end this change gives us:

  • one additional call to metadata manager removed ( as metadata for that class is already cached it's basically removing one Map access call). Which doesn't give us much in terms of perfomance
  • as we don't call the metadata manager to get metadata at this point, the code is working for the property holders. Because we build the correct metadata for them in corresponding value contexts instead of building it here in the ValidtorImpl

Now thinking about wrapping the String mappingName or beans class into wrapper ... we might end up not needing this at all. As we would then, most likely, have the getClassHierarchy() method return the wrappers. Or even maybe return the metadata right away, instead of simply returning classes.


validationContext.markCurrentBeanAsProcessed( valueContext );
// we start from the second element as first one (the most specific class) was already validated ^

// evaluating the constraints of a bean per class in hierarchy, this is necessary to detect potential default group re-definitions
for ( int i = 1; i < classHierarchy.size(); i++ ) {
Class<? super U> clazz = classHierarchy.get( i );

BeanMetaData<? super U> hostingBeanMetaData = constraintMetaDataManager.getBeanMetaData( clazz );
Copy link
Member Author

Choose a reason for hiding this comment

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

As for this part constraintMetaDataManager.getBeanMetaData( clazz ); I don't think that property holders should have anything in their "hierarchy", but if we decide that we want to have something like that we would either need to move out this entire method into the metadata impls (which doesn't feels right) or have two different ValidartorImpls where we override this specific method to use either property holders or simple beans.

Copy link
Member

Choose a reason for hiding this comment

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

I agree we can skip hierarchy for property holders.

Maybe there will be a use case at some point but let's wait to see how it is used.

defaultGroupSequenceIsRedefined = hostingBeanMetaData.isDefaultGroupSequenceRedefined();

// if the current class redefined the default group sequence, this sequence has to be applied to all the class hierarchy.
if ( defaultGroupSequenceIsRedefined ) {
validateConstraintsForRedefinedGroupSequence( validationContext, valueContext, validatedInterfaces, clazz, hostingBeanMetaData );
}
// fast path in case the default group sequence hasn't been redefined
else {
Set<MetaConstraint<?>> metaConstraints = hostingBeanMetaData.getDirectMetaConstraints();
validateConstraintsForSingleDefaultGroupElement( validationContext, valueContext, validatedInterfaces, clazz, metaConstraints, Group.DEFAULT_GROUP );
}

validationContext.markCurrentBeanAsProcessed( valueContext );

// all constraints in the hierarchy has been validated, stop validation.
if ( defaultGroupSequenceIsRedefined ) {
break;
}
}
// fast path in case the default group sequence hasn't been redefined
else {
Set<MetaConstraint<?>> metaConstraints = hostingBeanMetaData.getDirectMetaConstraints();
validateConstraintsForSingleDefaultGroupElement( validationContext, valueContext, validatedInterfaces, clazz, metaConstraints,
Group.DEFAULT_GROUP );
}
}
}

validationContext.markCurrentBeanAsProcessed( valueContext );
private <U> void validateConstraintsForRedefinedGroupSequence(BaseBeanValidationContext<?> validationContext, ValueContext<U, Object> valueContext, Map<Class<?>, Class<?>> validatedInterfaces, Class<? super U> clazz, BeanMetaData<? super U> hostingBeanMetaData) {
Iterator<Sequence> defaultGroupSequence = hostingBeanMetaData.getDefaultValidationSequence( valueContext.getCurrentBean() );
Set<MetaConstraint<?>> metaConstraints = hostingBeanMetaData.getMetaConstraints();

// all constraints in the hierarchy has been validated, stop validation.
if ( defaultGroupSequenceIsRedefined ) {
break;
while ( defaultGroupSequence.hasNext() ) {
for ( GroupWithInheritance groupOfGroups : defaultGroupSequence.next() ) {
boolean validationSuccessful = true;

for ( Group defaultSequenceMember : groupOfGroups ) {
validationSuccessful = validateConstraintsForSingleDefaultGroupElement( validationContext, valueContext, validatedInterfaces, clazz,
metaConstraints, defaultSequenceMember );
}
if ( !validationSuccessful ) {
break;
}
}
}
}
Expand Down