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

marko-bekhta · 2018-12-03T14:03:18Z

This PR is to address

IIRC, Gunnar had a good idea about requiring a wrapping here. It goes with the idea of wrapping the mapping name or the class.

from the #989

The wrapping class is called HibernateConstrainedType. Prior to actually wrapping the class as a "thing" that defines validation rules a few more changes were applied. I tried to make all of them as small as I could.

In this PR you would see the similar change for cascading metadata and how it is retrieved. And to answer the question from the other PR - I was thinking about the case when we would have something like

class A {
    @ValidPropertyHolder("someMapping")
    private PropHolder holder;
}

where the properties of a regular bean are property holders and I would like us not to "special case" them, but rather treat them as a regular properties marked for cascading.

If the changes in general are OK, we can also try to reduce the amount of created HibernateConstrainedTypes especially the ones in constraint location classes. Didn't want to go for it right away as it looks like it might require some more change, and the PR is getting big already 😃

I'm also still not 100% convinced that we should keep the metadata in ValueContext...

Any feedback is welcomed, especially on the naming of classes 😄

gunnarmorling

Hey @marko-bekhta, this one caught my curiosity as I saw my name :)

Two small technical comments inline, but one more general question on the overall approach: Is HibernateConstrainedType meant to represent non-bean based types, down the road, too? If so, how would getActualType() behave in that case?

I'm very far away by now from HV and probably shouldn't bother you with my comments anyways, but then I was just curious how the free-form work goes :) Thanks a lot!

gunnarmorling · 2018-12-03T14:17:27Z

engine/src/main/java/org/hibernate/validator/engine/HibernateConstrainedType.java

+	/**
+	 * @return a class of an object that will be validated.
+	 */
+	Class<T> getActuallClass();


getActualClass()

gunnarmorling · 2018-12-03T14:18:23Z

engine/src/main/java/org/hibernate/validator/engine/HibernateConstrainedType.java

+	 */
+	Class<T> getActuallClass();
+
+	List<HibernateConstrainedType<? super T>> getHierarchy(Filter... filters);


Filter is a type from an internal package, whereas this interface isn't. That's a bit inconsistent (and there's nightly build in place which should spot this), I reckon this type should go under internal, too?

hmmm, I missed this part (about Filter being an internal class).

I reckon this type should go under internal, too?

that's a good question... And the answer I suppose would be driven by the answer to the next question. How do we want the users to pass the mapping-name for free form validation? I was thinking that we would actually ask them to pass this HibernateConstrainedType as one of the parameters. Something like

<T> Set<ConstraintViolation<T>> validate(T object, HibernateConstrainedType<T> mapping, Class<?>... groups);

then we would need this class to be in the public API. But if we decide that we would just ask users for a string

<T> Set<ConstraintViolation<T>> validate(T object, String mapping, Class<?>... groups);

And later inside the method just wrap it with our own impl ... we could then move it to internal package.

HibernateConstrainedType being public interface gives us interesting opportunities. For example using this hierarchy method from it we could, in theory, build a constraints hierarchy for free form validation.

marko-bekhta · 2018-12-03T14:41:04Z

Hi @gunnarmorling, thanks for the questions :)

Yes, the idea is to use this same type for the property holders (or free form validation) as well. And in that case I was thinking that we would return the class of an object that really holds the data - map, json etc.
Now that you've asked it - I looked at the final version of the code and I think we can move this method to JavaBeanConstrainedType which is an impl of this interface. Right now it is used to log some messages (we can just update the methods in LOG to use this new interface instead and add nice toString), and inside javabean specific code like in annotation based provider (and in this case we don't want to pass anything else but java beans to it). It was useful initially as there were a lot of places that would fail to compile 😃. So I'll probably apply the change that I've just described above and then we can continue looking at this PR.

gunnarmorling · 2018-12-03T14:42:46Z

I looked at the final version of the code and I think we can move this method to JavaBeanConstrainedType which is an impl of this interface.

Yes, that'd seem reasonable.

marko-bekhta · 2019-03-21T16:30:20Z

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

@@ -761,14 +761,14 @@ private void validateCascadedContainerElementsInContext(Object value, BaseBeanVa
 		}
 	}

-	private BeanValueContext<?, Object> buildNewLocalExecutionContext(ValueContext<?, ?> valueContext, Object value) {
+	private BeanValueContext<?, Object> buildNewLocalExecutionContext(CascadingMetaData cascadingMetaData, ValueContext<?, ?> valueContext, Object value) {
 		BeanValueContext<?, Object> newValueContext;
 		Contracts.assertNotNull( value, "value cannot be null" );
 		BeanMetaData<?> beanMetaData = beanMetaDataManager.getBeanMetaData( value.getClass() );


remove the line

…gMetaData 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.

…s class `HibernateConstrainedType` wraps a class and is used as a key for metadata manager. It can also wrap strings or any other objects/data for metadata of property holders and other kinds of objects.

marko-bekhta added Needs more opinions 6.1 Exploration labels Dec 3, 2018

marko-bekhta requested a review from gsmet December 3, 2018 14:03

gunnarmorling reviewed Dec 3, 2018

View reviewed changes

marko-bekhta force-pushed the value-context branch 3 times, most recently from 6e46eed to f643595 Compare December 21, 2018 15:39

marko-bekhta force-pushed the value-context branch from f643595 to 7332ba0 Compare March 12, 2019 11:15

marko-bekhta commented Mar 21, 2019

View reviewed changes

marko-bekhta added 3 commits April 4, 2019 13:13

[value-context-changes] Return metadata hierarchy instead of a class one

5eb1fb4

[value-context-changes] Introduce HibernateConstrainedType, that wrap…

abd5bf5

…s class `HibernateConstrainedType` wraps a class and is used as a key for metadata manager. It can also wrap strings or any other objects/data for metadata of property holders and other kinds of objects.

marko-bekhta force-pushed the value-context branch from 7332ba0 to abd5bf5 Compare April 5, 2019 11:46

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

marko-bekhta commented Dec 3, 2018

gunnarmorling left a comment

gunnarmorling Dec 3, 2018

gunnarmorling Dec 3, 2018

marko-bekhta Dec 3, 2018

marko-bekhta commented Dec 3, 2018

gunnarmorling commented Dec 3, 2018

marko-bekhta Mar 21, 2019

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

Are you sure you want to change the base?

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

Conversation

marko-bekhta commented Dec 3, 2018

gunnarmorling left a comment

Choose a reason for hiding this comment

gunnarmorling Dec 3, 2018

Choose a reason for hiding this comment

gunnarmorling Dec 3, 2018

Choose a reason for hiding this comment

marko-bekhta Dec 3, 2018

Choose a reason for hiding this comment

marko-bekhta commented Dec 3, 2018

gunnarmorling commented Dec 3, 2018

marko-bekhta Mar 21, 2019

Choose a reason for hiding this comment