Explore FreeForm validation v2 #989

marko-bekhta · 2018-10-22T15:45:12Z

Link to previous discussion on the same topic - #949

Note: This is still a work in progress.

…pl#buildNewLocalExecutionContext()

…scadingMetaData As in case of property holders we would need to produce metadata not based on a class of a current object at runtime but based on a string representing the constraint mapping, we need to delegate this to cascading metadata as that's the place where this information will be stored.

accessor creator with simple a implementation for Map

and rename it As metadata manager will serve as provider of bean metadata as well as property holder metadata, it'll be better to keep it in specific package

own class

class and create corresponding analog for property holders

…etadata for property holders

…operty holders

…lder

…d add a simple test

marko-bekhta · 2018-10-22T15:53:22Z

engine/src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java

@@ -739,23 +739,14 @@ private void validateCascadedContainerElementsInContext(Object value, BaseBeanVa

 	private ValueContext<?, Object> buildNewLocalExecutionContext(ValueContext<?, ?> valueContext, Object value) {
 		ValueContext<?, Object> newValueContext;
-		if ( value != null ) {


based on the usage of this method null values could not be passed in here, as they go through a different logical branch. Reducing the number of different variations of ValueContext creations helps to fit in the property holders in the future.

Right, I merged this one.

marko-bekhta · 2018-10-22T15:58:55Z

.../java/org/hibernate/validator/internal/properties/propertyholder/PropertyHolderProperty.java

+/**
+ * @author Marko Bekhta
+ */
+public class PropertyHolderProperty implements Property {


This property holder property can be used for any kind property holder. It should work just fine with Maps JsonObjects and anything else. All the accessing logic is provided by a property accessor defined by PropertyAccessorCreator interface. PropertyAccessorCreatorProvider gives a creator based on a property holder type. This allows to store the user entered metadata once and then create "real" validation metadata based on a property holder type.

marko-bekhta · 2018-10-22T16:05:33Z

...main/java/org/hibernate/validator/internal/metadata/aggregated/CascadingMetaDataBuilder.java

@@ -47,6 +50,8 @@
 	private static final CascadingMetaDataBuilder NON_CASCADING =
 			new CascadingMetaDataBuilder( null, null, null, null, false, Collections.emptyMap(), Collections.emptyMap() );

+	private final String mapping;


For now property holders and regular beans are using the same cascading metadata builder. When I've tried to break it into smaller reusable pieces, I've broke it so badly 😄 that I decided to revert the changes for now and just use the same builder. But this still is needed to be done.

OK, let's try to have a todo list somewhere so that we don't forget about it.

marko-bekhta · 2018-10-22T16:11:05Z

engine/src/main/java/org/hibernate/validator/internal/metadata/core/MetaConstraintBuilder.java

+public class MetaConstraintBuilder<A extends Annotation> {
+
+	private final ConstraintDescriptorImpl<A> constraintDescriptor;
+	private final ConstraintLocation.Builder locationBuilder;


The part about location builder in this commit can be ignored. It will be removed in later commits. And I think about removing this builder as well. I think that storing the ConstraintDefs will be enough and this logic will be moved elsewhere.

marko-bekhta · 2018-10-22T16:28:43Z

engine/src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java

+		}
+		else {
+			validateConstraintsForSingleDefaultGroupElement( validationContext, valueContext, validatedInterfaces, beanMetaData.getBeanClass(), beanMetaData.getDirectMetaConstraints(), Group.DEFAULT_GROUP );
+			List<Class<? super U>> classHierarchy = beanMetaData.getClassHierarchy();


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.

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).

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.

marko-bekhta · 2018-10-22T16:32:03Z

engine/src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java

+			for ( int i = 1; i < classHierarchy.size(); i++ ) {
+				Class<? super U> clazz = classHierarchy.get( i );
+
+				BeanMetaData<? super U> hostingBeanMetaData = constraintMetaDataManager.getBeanMetaData( clazz );


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.

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.

marko-bekhta · 2018-10-22T16:40:34Z

From the notes that I've found on this I have:

need to make BeanMetaData abstract. There's a comment about it above. There will be two impls one for regular bean and another for property holder.
Store simple constraint defs instead of Set<MetaConstraintBuilder<?>> constraints in programmatic API
figure out how users will provide "type argument" constraints.

gsmet

Impressive work.

I added quite a lot of comments but most of them are minor.

Maybe we should discuss this container element issue live before applying these changes?

I already merged two of the changes. I think we could merge at least one more thing: the cascading changes which are applied to the existing stuff. That would get them out of the way. It's probably better that it's you extracting these as it's more complicated than the ones I extracted.

BTW, quick update: I think the review is best seen commit per commit instead of comment per comment. Otherwise, you will lose the logic of my comments.

Additional update: for the minor issues, we should probably use the "Resolve conversation" feature to keep track of things.

gsmet · 2018-10-25T10:17:43Z

engine/src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java

 		ValueContext<?, Object> newValueContext;
 		Contracts.assertNotNull( value, "value cannot be null" );
 		newValueContext = ValueContext.getLocalExecutionContext(
 				validatorScopedContext.getParameterNameProvider(),
 				value,
-				beanMetaDataManager.getBeanMetaData( value.getClass() ),
+				cascadingMetaData.getBeanMetaDataForCascadable( beanMetaDataManager, value ),


Do you expect to have the metadata for the free form case in the beanMetaDataManager?

If not, I suspect we should push the beanMetaDataManager to the constructor (and we would have 2 implementations of CascadingMetaData in the end).

Not sure what you had in mind for that?

gsmet · 2018-10-25T10:40:40Z

engine/src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java

@@ -739,23 +739,14 @@ private void validateCascadedContainerElementsInContext(Object value, BaseBeanVa

 	private ValueContext<?, Object> buildNewLocalExecutionContext(ValueContext<?, ?> valueContext, Object value) {
 		ValueContext<?, Object> newValueContext;
-		if ( value != null ) {


Right, I merged this one.

gsmet · 2018-10-25T12:34:33Z

.../hibernate/validator/internal/properties/propertyholder/PropertyAccessorCreatorProvider.java

@@ -0,0 +1,113 @@
+/*


Commit message: simple a implementation for Map. I think the a is misplaced.

gsmet · 2018-10-25T12:36:05Z

.../hibernate/validator/internal/properties/propertyholder/PropertyAccessorCreatorProvider.java

+		) );
+		for ( PropertyAccessorCreator propertyAccessorCreator : propertyAccessorCreators ) {
+			configuredPropertyCreators.add( propertyAccessorCreator );
+		}


We should make the set Immutable if we expect it to be.

gsmet · 2018-10-25T12:37:07Z

.../hibernate/validator/internal/properties/propertyholder/PropertyAccessorCreatorProvider.java

+			throw LOG.getUnableToFindPropertyCreatorException( propertyHolderType );
+		}
+		else if ( creators.size() > 1 ) {
+			throw LOG.getUnableToFinUniquedPropertyCreatorException( propertyHolderType );


ToFinUniquedProperty -> ToFindUniquedProperty

gsmet · 2018-10-25T18:07:21Z

...ava/org/hibernate/validator/cfg/propertyholder/ContainerElementConstraintMappingContext.java

+
+/**
+ * Constraint mapping creational context representing a type argument of a property, parameter or method return value
+ * with a generic (return) type. Allows to place constraints on that type argument, mark it as cascadable and to


This is far more restricted in the case of a property holder (e.g. no methods for instance)

gsmet · 2018-10-25T18:08:36Z

engine/src/main/java/org/hibernate/validator/cfg/propertyholder/Cascadable.java

+ * @author Gunnar Morling
+ * @author Kevin Pollet &lt;[email protected]&gt; (C) 2011 SERLI
+ */
+public interface Cascadable<C extends Cascadable<C>> {


This is very generic. Maybe CascadableProperty would be better?

gsmet · 2018-10-25T18:09:04Z

engine/src/main/java/org/hibernate/validator/cfg/propertyholder/Constrainable.java

+ * @author Gunnar Morling
+ * @author Kevin Pollet &lt;[email protected]&gt; (C) 2011 SERLI
+ */
+public interface Constrainable<C extends Constrainable<C>> {


Same here about it being generic. Maybe ConstrainableProperty would be better?

gsmet · 2018-10-25T18:11:10Z

engine/src/main/java/org/hibernate/validator/internal/metadata/location/ConstraintLocation.java

-			return new PropertyHolderPropertyConstraintLocationBuilder();
-		}
-	}
-


Maybe it should be removed in the commit where it has been added? Or was it useful at the time?

gsmet · 2018-10-25T18:11:32Z

...c/main/java/org/hibernate/validator/internal/metadata/manager/ConstraintMetaDataManager.java

@@ -14,7 +14,7 @@
 import org.hibernate.validator.internal.metadata.aggregated.BeanMetaData;
 import org.hibernate.validator.internal.metadata.core.ConstraintHelper;
 import org.hibernate.validator.internal.metadata.provider.MetaDataProvider;
-import org.hibernate.validator.internal.metadata.provider.PropertyHolderMetaDataProvider;
+import org.hibernate.validator.internal.metadata.provider.proeprtyholder.PropertyHolderMetaDataProvider;


There's a typo in the package name: it should be propertyholder.

marko-bekhta added 14 commits October 22, 2018 17:05

[property-holder-preparation] Remove impossible case from ValidatorIm…

f61a93d

…pl#buildNewLocalExecutionContext()

[property-holder] Introduce property holder property and property holder

9418a35

accessor creator with simple a implementation for Map

[property-holder-preparation] Move metadata manager to its own package

415abf7

and rename it As metadata manager will serve as provider of bean metadata as well as property holder metadata, it'll be better to keep it in specific package

[property-holder-preparation] Extract bean metadata provider code to its

4a523a8

own class

[property-holder-preparation] Extract BeanMetaDataBuilder to upper level

37d4545

class and create corresponding analog for property holders

[property-holder-preparation] Make ConstraintMetaDataManager return m…

00a7af7

…etadata for property holders

[property-holder] Create implementations of cascading metadata for pr…

f75e407

…operty holders

[property-holder] Make property holder configuration use location bui…

ac7835f

…lder

[property-holder] Introduce dummy metadata provider for property holders

92b0696

[property-holder] Introduce method for validating property holders an…

4e021be

…d add a simple test

[property-holder] Rework how constraints are validated for default group

9f1b1da

[property-holder] Rewrite how cascading constraints are created

1807098

[property-holder] start working on programmatic definition

8076e67

marko-bekhta commented Oct 22, 2018

View reviewed changes

gsmet added 6.1 Exploration labels Oct 25, 2018

gsmet reviewed Oct 25, 2018

View reviewed changes

gsmet mentioned this pull request Nov 7, 2018

Explore JSON validation #949

Closed

marko-bekhta mentioned this pull request Dec 3, 2018

Wrapping of a class that defines the constraint validation rules #994

Open

Base automatically changed from master to main March 19, 2021 08:47

marko-bekhta added the On ice while we think about it label Jun 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explore FreeForm validation v2 #989

Explore FreeForm validation v2 #989

marko-bekhta commented Oct 22, 2018

marko-bekhta Oct 22, 2018

gsmet Oct 25, 2018

marko-bekhta Oct 22, 2018

marko-bekhta Oct 22, 2018

gsmet Oct 25, 2018

marko-bekhta Oct 22, 2018

marko-bekhta Oct 22, 2018

gsmet Oct 25, 2018

marko-bekhta Nov 1, 2018

marko-bekhta Oct 22, 2018

gsmet Oct 25, 2018

marko-bekhta commented Oct 22, 2018

gsmet left a comment •

edited

Loading

gsmet Oct 25, 2018

gsmet Oct 25, 2018

gsmet Oct 25, 2018

gsmet Oct 25, 2018

gsmet Oct 25, 2018

gsmet Oct 25, 2018

gsmet Oct 25, 2018

gsmet Oct 25, 2018

gsmet Oct 25, 2018

gsmet Oct 25, 2018

Explore FreeForm validation v2 #989

Are you sure you want to change the base?

Explore FreeForm validation v2 #989

Conversation

marko-bekhta commented Oct 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marko-bekhta commented Oct 22, 2018

gsmet left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet left a comment •

edited

Loading