HV-1529 Add dynamic payload to HibernateConstraintValidator #890

mkurz · 2017-11-24T21:46:18Z

@marko-bekhta and @gsmet probably are the persons who should take a look at this 😉
All the information can be found here: https://hibernate.atlassian.net/browse/HV-1529

Basically we can now pass a dynamic payload to a (Hibernate)ConstraintValidator via:

someValidator
    .unwrap(ValidatorImpl.class)
    .withDynamicConstraintValidatorInitializationPayload(payloadObject)
    .validate(objectToValidate)

And retrieve it via getDynamicPayload:

public class MyFutureValidator implements HibernateConstraintValidator<MyFuture, Instant> {

	private SomeType dynamicPayload;

	@Override
	public void initialize(ConstraintDescriptor<MyFuture> constraintDescriptor,
			HibernateConstraintValidatorInitializationContext initializationContext) {
		this.dynamicPayload = initializationContext.getDynamicPayload(SomeType.class);
	}

	@Override
	public boolean isValid(Instant instant, ConstraintValidatorContext constraintContext) {
		// Here I can use the dynamic payload now
		dynamicPayload.getSomeData();
		//...
	}
}

mkurz · 2017-11-24T22:29:38Z

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

+
+	public ValidatorImpl withDynamicConstraintValidatorInitializationPayload(Object dynamicPayload) {
+		this.constraintValidatorInitializationContext = new HibernateConstraintValidatorInitializationContextImpl(
+			this.scriptEvaluatorFactory,


Instead of saving a reference to the ScriptEvaluatorFactory here inside the ValidatorImpl class we could also add a getScriptEvaluatorFactory() to the (Hibernate)ConstraintValidatorInitializationContext(Impl) - if it would be ok to expose that ScriptEvaluatorFactory from that context.
We could then replace this line here with this.constraintValidatorInitializationContext.getScriptEvaluatorFactory() instead. I leave it up to you which approach to use.

We sure don't want to expose it (see the clear() method for instance).

What you could do instead is creating another class that wraps the original HibernateConstraintValidatorInitializationContextImpl and adds the payload. The original HibernateConstraintValidatorInitializationContextImpl would simply return null for the payload.

-> but this comment won't be of any use to you if we move the payload to ValidatorFactory/HibernateValidatorContext level.

gsmet

@mkurz Interesting idea. Happy to see that our vision covers more use cases than anticipated.

I started to add a few minor comments inline but then discovered the big concurrency issue.

Most of the comments are still valid even if you adopt the new approach I presented (not sure we should call the payload "dynamic" then as it won't be very dynamic).

gsmet · 2017-11-27T10:16:39Z

...ernate/validator/constraintvalidation/HibernateConstraintValidatorInitializationContext.java

+
+	/**
+	 * @param type The type of payload to retrieve
+	 * @return an instance of the specified type set by the user via


Please make the long version the description of the method and keep a short one for the @return.

gsmet · 2017-11-27T10:17:04Z

...ernate/validator/constraintvalidation/HibernateConstraintValidatorInitializationContext.java

+	 * @param type The type of payload to retrieve
+	 * @return an instance of the specified type set by the user via
+	 * {@link ValidatorImpl#withDynamicConstraintValidatorInitializationPayload(Object)} or {@code null} if no constraint payload
+	 * if the given type has been set.


s/if the given type/of the given type/

gsmet · 2017-11-27T10:17:24Z

...ernate/validator/constraintvalidation/HibernateConstraintValidatorInitializationContext.java

+	/**
+	 * @param type The type of payload to retrieve
+	 * @return an instance of the specified type set by the user via
+	 * {@link ValidatorImpl#withDynamicConstraintValidatorInitializationPayload(Object)} or {@code null} if no constraint payload


s/constraint payload/constraint validator payload/

gsmet · 2017-11-27T10:18:34Z

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

+
+	public ValidatorImpl withDynamicConstraintValidatorInitializationPayload(Object dynamicPayload) {
+		this.constraintValidatorInitializationContext = new HibernateConstraintValidatorInitializationContextImpl(
+			this.scriptEvaluatorFactory,


We sure don't want to expose it (see the clear() method for instance).

What you could do instead is creating another class that wraps the original HibernateConstraintValidatorInitializationContextImpl and adds the payload. The original HibernateConstraintValidatorInitializationContextImpl would simply return null for the payload.

-> but this comment won't be of any use to you if we move the payload to ValidatorFactory/HibernateValidatorContext level.

gsmet · 2017-11-27T10:22:25Z

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

+		this.constraintValidatorInitializationContext = new HibernateConstraintValidatorInitializationContextImpl( validatorScopedContext.getScriptEvaluatorFactory(), validatorScopedContext.getClockProvider(), validatorScopedContext.getTemporalValidationTolerance(), null );
+	}
+
+	public ValidatorImpl withDynamicConstraintValidatorInitializationPayload(Object dynamicPayload) {


I think I would stick to a more simple name. Something like withConstraintValidatorDynamicPayload(). The fact that it is related to the initialization can be removed I think. We don't really care (mostly because it can still be used at runtime by adding an attribute to the constraint validator).

gsmet · 2017-11-27T10:25:13Z

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

 	/**
 	 * The constraint initialization context is stored at this level to prevent creating a new instance each time we
 	 * initialize a new constraint validator as, for now, it only contains Validator scoped objects.
 	 */
-	private final HibernateConstraintValidatorInitializationContext constraintValidatorInitializationContext;
+	private HibernateConstraintValidatorInitializationContext constraintValidatorInitializationContext;


A wise man said to me one day: "Every time you remove a final, God kills a kitten." (yes, he's so wise he uses backticks when he speaks).

You're paving the way to concurrency issues. Keep in mind that the ValidatorImpl is shared across thread.

gsmet · 2017-11-27T10:34:25Z

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

+	}
+
+	public ValidatorImpl withDynamicConstraintValidatorInitializationPayload(Object dynamicPayload) {
+		this.constraintValidatorInitializationContext = new HibernateConstraintValidatorInitializationContextImpl(


Concurrency issue. You're not overriding the context for your validate() call here but also for all the other random threads running. And the other threads will override it too.

I think we won't be able to make your approach work.

My opinion is that you'd be better served by having the payload at the ValidatorFactory level AND the HibernateValidatorContext level.

The drawback is that before your calls to validate() you would have to instantiate the Validator first by unwrapping the context and setting the payload. The Validator creation is quite lightweight so I don't think it will be an issue in practice.

Could you try this approach and see how it goes with your use case?

gunnarmorling · 2017-11-27T10:52:39Z

Hi @mkurz, a couple of comments/questions:

What's the use case this is trying to address? In the issue you mention a country/language, but that's where the message interpolator and its locale handling comes in. The code example shows a temporal constraint, but no actual usage of the proposed feature.
j.v.Validator is not meant to be recreated for every call to validate(), which I think your proposal implies. Instead it seems a context parameter would have to be added to the validate() method.
ValidatorImpl is an internal class, i.e. it shouldn't be exposed to clients. So if we wanted to provide some additional functionality, we'd have to do so via a new interface which one then can unwrap to.

Note there are two existing ways for passing in contextual information:

Use a class-level constraint, allowing you to obtain e.g. a language from the bean hosting the constrained property; admittedly that may be complex, but then again it'd help to better understand the usecase this is about.
Use dependency injection into constraint validators via a custom ConstraintValidatorFactory, which e.g. returns CDI or Spring enabled validator instances. They could have request-scoped contextual information injected, e.g. the locale of the current request's users.

gsmet · 2017-11-27T11:02:35Z

@gunnarmorling

What's the use case this is trying to address?

Matthias gave an example in the JIRA issue: "think of zip codes which differ for each country".

j.v.Validator is not meant to be recreated for every call to validate(), which I think your proposal implies. Instead it seems a context parameter would have to be added to the validate() method.

That's not what the PR does (and it indeed introduces concurrency issues I mentioned in my review). Frankly, taking a look at the ValidatorImpl constructor, I don't think it would be that bad to instantiate a Validator for each call. And we could imagine a CHM of Validator per country code if required.

Do you think it would be better to add a parameter to the various validate...() calls? I don't like that very much.

ValidatorImpl is an internal class, i.e. it shouldn't be exposed to clients. So if we wanted to provide some additional functionality, we'd have to do so via a new interface which one then can unwrap to.

Yup, I didn't mention it because we can't introduce the feature here anyway (concurrency issues).

gsmet · 2017-11-27T11:07:43Z

Btw, I think it makes sense to consider it Validator scoped.

If the same user makes several validate() calls at once, I would expect them to be run with the same payload.

gunnarmorling · 2017-11-27T11:20:47Z

"think of zip codes which differ for each country".

Ok, so the requirement is then:

public class Address {
    @ValidZipCode // acts differently based on the country
    String zipCode;

    Country country;

?

As said, I think the easiest would be to use a class-level constraint or context injection into the validator then.

Do you think it would be better to add a parameter to the various validate...() calls? I don't like that very much.

Actually, the problem with either approach is that the user typically isn't in the business of obtaining a validator (in the context of one request/transaction etc.) nor of calling validate().

Instead this will be done by some framework for them. I.e. usually passing in such context will not be available to them, whereas the other two approaches allow to get hold of contextual information also in such managed environment.

In a way it's related to BVAL-237, too, i.e. the idea of making the root or leaf bean (or at least their properties in a detyped manner) available to constraint validators.

gunnarmorling · 2017-11-27T11:26:53Z

Btw. there already is a way for passing Validator-scoped metadata: ValidatorFactory#usingContext(). HibernateValidatorContext could be expanded to allow for the propagation of arbitrary data, but as said I'm not convinced that this helps the discussed use case much.

gsmet · 2017-11-27T11:45:22Z

Btw. there already is a way for passing Validator-scoped metadata: ValidatorFactory#usingContext(). HibernateValidatorContext could be expanded to allow for the propagation of arbitrary data, but as said I'm not convinced that this helps the discussed use case much.

Yeah, that's exactly what I proposed in my review :).

gsmet · 2017-11-28T12:52:57Z

@gunnarmorling I thought about it a bit more and I don't think having the ability to pass some context to constraint validators is useless.

It might be handy to manage differences between 2 environments or 2 different apps using the same jars.

Having them at VF and context level would be enough.

@mkurz could you give us more details about your use case? As you might have noticed, @gunnarmorling is a bit skeptic about the usefulness of such addition so having more details on why it's not practical for you to use class level constraints and some data in the model would help.

Thanks!

mkurz · 2017-12-01T18:54:11Z

@gsmet and @gunnarmorling thank you both very much for your comprehensive replies. I really appreciate that.

So what is my use case?
In my free time I work as an open source contributor for the Play Framework. Right now Play stores various information about a request in a play.mvc.Http.Context object. That would be e.g. the locale of the current request (or the one the user has set explicit via a cookie) and various other things, e.g. there is an args map in which a framework user can put various other stuff inside that context.
Right now that http-context is stored in a ThreadLocal and therefore we can access the current http-context from basically everywhere, including the isValid method of a ConstraintValidator.

However in near future that ThreadLocal will be removed an that http-context has to be passed to every method were it is needed. That means we somehow have to pass that http-context (or just parts of it, e.g. the locale) inside the isValid method because right now we use e.g. the locale of the current request for validation (like the zip code example @gunnarmorling gave). And therefore that has to be done for each request - therefore it has to be "dynamic".

That is how hibernate-validator is set up in Play righ now
We bind the components we need via dependency injection. We set up a Validator Factory that provides us a new validator each time we need one. (I know now these validators could be cached, but that is not the point here. I will probably add caching soon. Because I didn't know caching them is allowed is why I stumpled into that concurrency issues in my pull request....) The ConstraintValidatorFactory simply returns us a new instance of the desired constraint.
So now a Validator gets injected into the FormFactory here and eventually we call the validate(...) method that javax.validation.Validator (we don't unwrap it right now) provides here.

Actually at first I was thinking the same like what @gunnarmorling proposed - to add a parameter to all the various (unwrapped) validate...() calls. However I also don't like that approach, that is why I came up with the with...Dynamic...Payload() method.

About the suggestions @gunnarmorling mentioned

Using a class-level constraint
I am aware of that feature, however I don't think this is what we are looking for here. The only way I can think of to get that working is that a user would have to pollute his/her form classes with "special" properties into which the framework sets e.g. the locale. (A form class in Play is simply a POJO that represents a HTML form on the server side and that uses bean validation/hibernate constraints annotations for validation). However that would mean the framework would have to provide the user some kind of contract of how he/she has to set up that "special" properties (which values will later set by the framework) e.g. the user woud have to sub-class his/her forms of a specific super-class that the framework provides and where the locale can be set on... I really don't like that. For users, nothing should change and everything should just work out of the box as simple as possible (without a framework user having taking care of this)
So I think with the design Play is following this approach isn't really working.
Request-scoped DI
Play wont work with request-scoped dependencies (right now). See [WIP] Provide a way to inject request-scoped dependencies in Guice playframework/playframework#6798 - it was closed this year and it looks like that wont happen. So that is also not an option for us.

Status quo of this pull request
(First of all I had to rebase against the latest master because of the changes introduces for HV-1531).
I updated the pull request as suggested so the dynamic payload is now at ValidatorFactory and HibernateValidatorContext level. As far as I can tell in Play we now would have to inject the ValidatorFactory instead of a Validator itself into the FormFactory and then just before calling validate(...) unwrap the context and set the http-context (or parts of it): validatorFactory.usingContext().dynamicPayload(ctx.getLocale()).getValidator() like shown in the docs.
I think that should work for us.

However I just set up another branch which - like my original idea - adds a withConstraintValidatorDynamicPayload (on top of that pull request here) which now returns a new validator instance - which is basically a copy of the origin one - but with the given payload set. For that I just added a private ValidatorImpl() constructor and a copy constructor for ValidatorScopedContext. I think that way concurrency isn't a problem anymore: mkurz@f382761
This way setting a dynamic payload is really dynamic. You can just "copy" your validators and set the payload you need. That would also work when caching the validators. I think this is what we are really looking for here for the Play Framework.

What do you think about all of this? Am I missing something?
I really would like to find a solution for this and get it into one of the next HV releases. Frankly I think that stuff makes sense, not just for Play.

mkurz · 2017-12-01T18:56:49Z

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

@@ -183,7 +183,8 @@ public ValidatorFactoryImpl(ConfigurationState configurationState) {
 				getTemporalValidationTolerance( configurationState, properties ),
 				getScriptEvaluatorFactory( configurationState, properties, externalClassLoader ),
 				getFailFast( hibernateSpecificConfig, properties ),
-				getTraversableResolverResultCacheEnabled( hibernateSpecificConfig, properties )
+				getTraversableResolverResultCacheEnabled( hibernateSpecificConfig, properties ),
+				null


I am not sure about just passing null here. Suggestions?

I think what's you're missing here is a way to define it globally at the ValidatorFactory level. That's why you're forced to pass null. But the API building the VF programmatically should allow to pass a global payload.

gsmet · 2017-12-04T18:08:47Z

This way setting a dynamic payload is really dynamic. You can just "copy" your validators and set the payload you need. That would also work when caching the validators. I think this is what we are really looking for here for the Play Framework.

I don't understand what is the value of that. I think it's better to pass the ValidatorFactory and create new validators from it using a context. Which benefits do you see in your approach?

You could either have a Validator cache per locale, or at least try to reuse the same Validator for the whole request.

mkurz · 2017-12-07T11:45:08Z

I don't understand what is the value of that. I think it's better to pass the ValidatorFactory and create new validators from it using a context. Which benefits do you see in your approach?

In case we don't pass a locale as dynamic payload but e.g. the whole play request context which could contain different data on each request we can't cache that validator like we could when just passing the locale as payload. So when no caching is possible, "copying" a validator and passing the payload is cheaper than using ValidatorFactory.getValidator() each time? Only of course if we could cache a "base" Validator which would be copied each time. What do you think? Or am I wrong?

gsmet · 2017-12-13T09:51:48Z

@mkurz just to let you know that I'm thinking about this. Will get back to you shortly.

gsmet

So, after thinking a bit more about it, I think what was bugging me is that I'm not comfortable with creating Validators outside of the ValidatorFactory. Thus I would really like us to stick to the approach you have here.

I think we can reduce the overhead of creating a brand new Validator a bit by making the ValidationOrderGenerator global to the ValidatorFactory (this should be made in a separate commit).

We would still end up with a higher cost (basically, creating the cache key + the CHM lookup) than with your Validator cloning idea but I think it's safer for the future.

I also added a couple of inline comments to improve the naming of the API.

I'd like to include it in our next minor if you can come up with an updated patch.

gsmet · 2017-12-12T14:13:16Z

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

@@ -183,7 +183,8 @@ public ValidatorFactoryImpl(ConfigurationState configurationState) {
 				getTemporalValidationTolerance( configurationState, properties ),
 				getScriptEvaluatorFactory( configurationState, properties, externalClassLoader ),
 				getFailFast( hibernateSpecificConfig, properties ),
-				getTraversableResolverResultCacheEnabled( hibernateSpecificConfig, properties )
+				getTraversableResolverResultCacheEnabled( hibernateSpecificConfig, properties ),
+				null


I think what's you're missing here is a way to define it globally at the ValidatorFactory level. That's why you're forced to pass null. But the API building the VF programmatically should allow to pass a global payload.

gsmet · 2017-12-12T14:18:03Z

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

@@ -137,6 +137,12 @@ public HibernateValidatorContext temporalValidationTolerance(Duration temporalVa
 		return this;
 	}

+	@Override
+	public HibernateValidatorContext dynamicPayload(Object dynamicPayload) {


Let's call it withConstraintValidatorPayload().

We use withDynamicPayload() in other HV APIs so I think it makes sense to stick with this with convention for the payloads.

And I would like it to be clear that it's for the constraint validators. We might want to introduce other payloads in the future.

Obviously, let's be consistent and update all the other places with this name.

gsmet · 2017-12-20T15:07:50Z

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

+	 *
+	 * @return {@code this} following the chaining method pattern
+	 *
+	 * @since 6.0.5


Let's target 6.0.8 now that 6.0.7 has been released.

mkurz · 2018-01-09T11:16:47Z

@gsmet thanks for your feedback! I will update the pull request asap - I am very busy these days.

gsmet · 2018-01-09T11:19:34Z

@mkurz ah ah, funny thing: i had on my TODO list for today to check if you were still interested in pursuing this. Let me know if you need any help or if you have questions!

mkurz · 2018-02-05T23:18:20Z

@gsmet I finally found time to update this pull request. Please check the commits I added (the last one probably is the important one).
And about commit f0418e0 - is that what you meant by "...I think we can reduce the overhead of creating a brand new Validator a bit by making the ValidationOrderGenerator global to the ValidatorFactory..."?

~~Also CI does not pass. Do you know what's going on? Can you help out and checkout my branch and see if you get it working? Thanks!~~

mkurz · 2018-02-05T23:23:59Z

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

+				return hibernateSpecificConfig.getConstraintValidatorPayload();
+			}
+		}
+		return null;


@gsmet question: Here I am simply returning null and not trying to get a result out of the properties (like in getTemporalValidationTolerance(...) above. Is that OK/enough here? As I understand looking into the properties is the way to handle the xml config, however does it make sense to define a payload in the xml at all? If so it would probably just be of type string (even tough the payload could be "object") Do you know what I mean? (Its late... 😉)

Yeah, no need to implement it in the properties. It does not make much sense.

I simplified the parameters of the method as we don't need the properties at all.

marko-bekhta · 2018-02-06T08:54:03Z

@mkurz Hi, about that build failure I think it's because of this error in javadoc:

[ERROR] /var/lib/jenkins/workspace/hibernate-validator-PR/distribution/../engine/src/main/java/org/hibernate/validator/HibernateValidatorConfiguration.java:305: error: @param name not found

there's a mismatch between the param name in javadoc dynamicPayload and the name of the actual param constraintValidatorPayload

mkurz · 2018-02-06T12:14:48Z

@marko-bekhta Thanks! I fixed the problem (squashed the fix into the last commit were it belongs)

mkurz · 2018-02-13T12:06:02Z

@gsmet Updated! Removed final and the tab. Please check the testPayloadViolation test.
~~BTW: Seems like travis doesn't start...~~

mkurz · 2018-02-13T12:09:34Z

Alright, it's green now....

gsmet · 2018-02-13T12:37:50Z

@mkurz thanks, I'll take it from here (I would like to add something to the documentation about this feature but I still need to figure out where).

I'll try to get this done this week so that we can merge it before the end of the week.

mkurz · 2018-02-13T12:40:13Z

@gsmet Great! Thank you very much for your help 👍 Looking forward to the next release 😛

gunnarmorling · 2018-02-22T07:38:36Z

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

+	 * @since 6.0.8
+	 */
+	@Incubating
+	HibernateValidatorConfiguration constraintValidatorPayload(Object constraintValidatorPayload);


Ok, let's do it then. I think the confusion stems from the very generic nature of this thing (just passing in and getting out any object). For the originally discussed use case (locale), it doesn't make really sense on the VF, but yes, perhaps for others.

On your environment example, can you detail that a bit, i.e. when would validation be different per environment?

gunnarmorling · 2018-02-22T08:19:30Z

...ernate/validator/constraintvalidation/HibernateConstraintValidatorInitializationContext.java

+	 * @return an instance of the specified type or null if no payload of this type has been set
+	 * @since 6.0.8
+	 */
+	<C> C getPayload(Class<C> type);


I was about to say that it should be named getConstraintValidatorPayload() for the sake of consistency with the other new API methods. But actually I'm wondering whether this all should be named "payload" at all. That term already is used in the Bean Validation API (payload() on constraints, and also HibernateConstraintViolation#getDynamicPayload().

These existing payloads are exposed on the resulting constraint violation. So my question is should we a) use another name here (if it's about some contextual information meant for the constraint validator information) or b) expose that new payload information on resulting constraint violations as we do for the other payloads (which are scoped to a single constraint and its violations, though).

I feel finding another name might be the best thing to avoid confusion with the existing payload concepts (of course a given validator implementation can still decide to forward this new contextual information as a dynamic constraint payload in emitted violations).

Any thoughts?

@gunnarmorling well I asked on HipChat and you told me to name it whatever I feel it to be the most appropriate so that's what I did :). Considering we are in the validator context, I think the context is clear enough, that's why I went for the short form. I couldn't think of another payload we would want it here. I don't have a strong opinion though as I hesitated between the consistency and the impression of redundancy. So if you prefer the full name, let's name it that way, I'm +0 on this.

As for the payload term, I think it's the appropriate generic term for this sort of things so I don't think it's an issue to use it there and I don't think it collides with the other payloads as it's a very generic term.

I asked on HipChat and you told me to name it whatever I feel it to be the most appropriate so that's what I did :)

Well, of course I was expecting you to do a wise choice ;)

My concern is that using the same name may lead people to expect that it will automatically be exposed via getDynamicPayload(), which won't be the case unless a specific validator implementation decides to do so. The problem is though I can't think of another good name either, apart from yet another "context" flavor.

I think payload is a more appropriate term than context.

Let's rename getPayload() to getConstraintValidatorPayload() and be extra clear in the documentation I still have to write :).

One other thing to note is that it's only exposed in the initialization context passed to the validator. It's not exposed in the isValid() context. That might help with the confusion as only people willing to use the advanced initialize() method will notice it.

Ok, getConstraintValidatorPayload() it is. I see it's all marked as incubating, so we got some liberty to adjust down the road, if needed. Thanks!

Alright, @gsmet will you do the renaming when you write the docs?

Yes, will do.

gsmet · 2018-02-26T14:35:11Z

@gunnarmorling @mkurz I added a commit with some documentation. Feedback welcome.

Once the dust is settled on the documentation, we can (finally) merge this feature.

Thanks.

mkurz · 2018-02-26T16:08:18Z

@gsmet Had a quick look at the documentation you added, LGTM.

gunnarmorling · 2018-02-26T19:29:02Z

documentation/src/main/asciidoc/ch06.asciidoc

+----
+====
+
+Once you have set the constraint validator payload, it is time to use it in your constrait validators as shown in the example below:


s/constrait/constraint/

gunnarmorling · 2018-02-26T19:30:08Z

documentation/src/main/asciidoc/ch06.asciidoc

+----
+====
+
+`HibernateConstraintValidatorInitializationContext#getConstraintValidatorPayload()` has a type parameter


Question on that one, what happens if I have registered two payloads of type A and B, B extends A and I request A? Is an order of precedence defined?

If I understand your question right you can't have two payloads, because the last one wins.

gunnarmorling · 2018-02-26T19:31:07Z

documentation/src/main/asciidoc/ch06.asciidoc

+the constraint violation raised.
+
+The whole purpose of this constraint validator payload is to be used to condition the behavior of your constraint validators.
+It is not included in the constraint violations.


, unless a specific ConstraintValidator implementation passes on the payload to emitted constraint violations by calling xyz.

I leave that up to @gsmet if he wants to mention that.

gunnarmorling · 2018-02-26T19:34:18Z

...ator/referenceguide/chapter06/constraintvalidatorpayload/ConstraintValidatorPayloadTest.java

+				.unwrap( HibernateValidatorFactory.class );
+
+		Validator validator = hibernateValidatorFactory.usingContext()
+				.constraintValidatorPayload( "US" )


Now that I'm seeing the API used, I'm wondering whether it should be constraintValidatorPayload( String.class, "US" ). I can see two advantages:

It makes it clearer that there can be multiple payloads (for different types)

One can register a given implementation type under the key of a (public) interface

Btw. what happens if I register multiple payloads of the same type? Last one wins, or is an exception raised?

Hum, no, it seems the implementation actually doesn't allow what I had in mind. Wondering whether that's not a bit too limited?

We had the exact same discussion about the other payloads recently and you answered me the user could use a Map or a bean of some sort.

I don't think it's limited and it's consistent with the other payloads in the API.

...and you answered me the user could use a Map or a bean of some sort.

I don't think it's limited and it's consistent with the other payloads in the API.

I fully agree.

gunnarmorling · 2018-02-27T09:04:45Z

Rebased and applied. Also added a commit for some JavaDoc adjustments. Thanks, everyone!

mkurz · 2018-02-27T09:07:31Z

🎉 Woohoo! Thanks everyone for your feedback and the time you invested! You do a real great job 👍 😄

gsmet · 2018-02-27T10:42:16Z

@mkurz thanks for your contribution.

gsmet · 2018-03-08T15:05:42Z

@mkurz just a heads up that we released 6.0.8.Final today with the constraint validator payload feature. Time to use in the Play framework :).

mkurz · 2018-03-08T21:49:46Z

@gsmet Thanks for letting me know! Hopefully next week I will find time to set up a pull request for Play 👍

mkurz · 2018-03-16T22:07:20Z

@gsmet I tried to make use the hibernate constraint validator payload feature within the Play Framework this week, however I ran into an issue with the caching behaviour of HibernateConstraintValidator instances. I am pretty sure it's broken.
See https://hibernate.atlassian.net/browse/HV-1589 and for a fix #939.

mkurz commented Nov 24, 2017

View reviewed changes

gsmet reviewed Nov 27, 2017

View reviewed changes

gsmet added the Changes suggested label Nov 28, 2017

gsmet self-assigned this Nov 28, 2017

mkurz force-pushed the dynamicPayload branch from a8cce2c to aaf7b8a Compare December 1, 2017 16:25

mkurz commented Dec 1, 2017

View reviewed changes

gsmet reviewed Dec 21, 2017

View reviewed changes

mkurz force-pushed the dynamicPayload branch from 5517893 to 3cfa602 Compare February 5, 2018 22:18

mkurz commented Feb 5, 2018

View reviewed changes

mkurz force-pushed the dynamicPayload branch from 3cfa602 to 6b62f77 Compare February 6, 2018 12:01

gsmet added Ready for review and removed Changes suggested labels Feb 6, 2018

gsmet force-pushed the dynamicPayload branch 2 times, most recently from b11891d to 12dcfb8 Compare February 8, 2018 16:38

gsmet added Needs documentation and removed Needs work labels Feb 13, 2018

gunnarmorling reviewed Feb 22, 2018

View reviewed changes

mkurz and others added 7 commits February 26, 2018 12:38

HV-1529 Allow to pass a payload to constraint validators

10b8aeb

HV-1529 Various fixes, mostly cosmetic

ac2046a

HV-1567 Make ValidationOrderGenerator global to the ValidatorFactory

44238e3

HV-1529 Add tests for constraint validator payload

c7957ab

HV-1529 Rename getPayload() to getConstraintValidatorPayload()

e82181e

HV-1529 Some minor adjustments to the tests

1bdeac8

HV-1529 Constraint validator payload documentation

274dcea

gsmet force-pushed the dynamicPayload branch from 6f4e8d7 to 274dcea Compare February 26, 2018 14:33

gsmet added Ready for review and removed Needs documentation labels Feb 26, 2018

gunnarmorling reviewed Feb 26, 2018

View reviewed changes

HV-1529 Some adjustments to the documentation

fe2ee4e

gunnarmorling closed this Feb 27, 2018

mkurz deleted the dynamicPayload branch February 27, 2018 09:13

mkurz mentioned this pull request Mar 27, 2018

Pass a payload with useful information into Java validators playframework/playframework#8316

Merged

HV-1529 Add dynamic payload to HibernateConstraintValidator #890

HV-1529 Add dynamic payload to HibernateConstraintValidator #890

Conversation

mkurz commented Nov 24, 2017

Choose a reason for hiding this comment

gsmet Nov 27, 2017 • edited

Choose a reason for hiding this comment

gsmet left a 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 Nov 27, 2017 • edited

Choose a reason for hiding this comment

gsmet Nov 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet Nov 27, 2017 • edited

Choose a reason for hiding this comment

gunnarmorling commented Nov 27, 2017

gsmet commented Nov 27, 2017

gsmet commented Nov 27, 2017

gunnarmorling commented Nov 27, 2017

gunnarmorling commented Nov 27, 2017

gsmet commented Nov 27, 2017

gsmet commented Nov 28, 2017

mkurz commented Dec 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet commented Dec 4, 2017

mkurz commented Dec 7, 2017

gsmet commented Dec 13, 2017

gsmet left a comment • edited

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

mkurz commented Jan 9, 2018

gsmet commented Jan 9, 2018

mkurz commented Feb 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marko-bekhta commented Feb 6, 2018

mkurz commented Feb 6, 2018

mkurz commented Feb 13, 2018 • edited

mkurz commented Feb 13, 2018

gsmet commented Feb 13, 2018

mkurz commented Feb 13, 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

gsmet commented Feb 26, 2018

mkurz commented Feb 26, 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

gunnarmorling commented Feb 27, 2018

mkurz commented Feb 27, 2018

gsmet commented Feb 27, 2018

gsmet commented Mar 8, 2018

mkurz commented Mar 8, 2018

mkurz commented Mar 16, 2018

gsmet Nov 27, 2017 •

edited

gsmet Nov 27, 2017 •

edited

gsmet Nov 27, 2017 •

edited

gsmet Nov 27, 2017 •

edited

mkurz commented Dec 1, 2017 •

edited

gsmet left a comment •

edited

mkurz commented Feb 5, 2018 •

edited

mkurz commented Feb 13, 2018 •

edited