-->
These old forums are deprecated now and set to read-only. We are waiting for you on our new forums!
More modern, Discourse-based and with GitHub/Google/Twitter authentication built-in.

All times are UTC - 5 hours [ DST ]



Forum locked This topic is locked, you cannot edit posts or make further replies.  [ 11 posts ] 
Author Message
 Post subject: Feedback for specification draft
PostPosted: Sat Jan 24, 2009 1:27 pm 
Hibernate Team
Hibernate Team

Joined: Sat Jan 24, 2009 12:46 pm
Posts: 388
Hi,

compliments for the great draft, it looks really promising and I think, beans validation will be a great success. As I read through the spec, the following questions/ideas came to my mind:

- The question was raised before, but I didn't find an answer: Couldn't the Constraint interface be parametrized with the field type validatable by a concrete implementation as follows:

Code:
public interface Constraint<A extends Annotation, T> {

   void initialize(A constraintAnnotation);

   boolean isValid(T object, ConstraintContext constraintContext);
}


That way, implementations could be created in a type-safe manner. In case a constraint annotation is allowed at multiple types, this would require a separate Constraint implementation for each type, but I don't think, this is a big issue. Actually, it would lead to cleaner code without "if-instanceof-...-else-if-instanceof-..." cascades.

To specify, which validator should be used for a certain field type supported by a given constraint annotation, an interface ValidatorDescriptor could be introduced:

Code:
public interface ValidatorDescriptor<A extends Annotation, T, V extends Constraint<A, T>> {

   Class<T> getType();

   Class<V> getValidatorClass();

}


Following this approach, the ConstraintValidator meta annotation would refer to an array of such validator descriptors:

Code:
public @interface ConstraintValidator {

   public abstract Class<? extends ValidatorDescriptor<?, ?, ?>>[] value();
}

Beside a safer implementation of validators, this approach would enable another use case: compile-time checking for misplaced constraint annotations by implementing an annotation processor, which could not only check for proper usage of the standard annotations, but also - by evaluating the validator descriptors - of any custom constraint annotations.

- The interface name "Constraint" appears irritating to me. I would regard Min, Max, Size etc. as "constraints" and classes that check for such constraints as "validators". Throughout the spec, that nomenclature is also used several times (so the JavaDoc of ConstraintFactory says: "This class manages the creation of constraint validators."). And in fact, the package, in which the constraint annotations live, is named javax.validation.constraints.

Therefore I suggest to rename the Constraint interface to "Validator" (the current Validator interface might be named "BeansValidator" or similar) or "ConstraintValidator" (the current ConstraintValidator interface could e.g. be named "ValidatedBy" then).

- The class comments of the Past and Future annotations say that the JVM's current time will be used for validity checking. This could be an issue because "now" is not in every case defined as the JVM's current time.

In my code for instance, rather than calling new Date() or similar, I prefer the usage of a simple time service: Date myDate = DateService.getDate(). This service might return new Date() normally, but for instance yesterday's date in case a failed batch run shall be re-executed. Using a date set by some IoC container as "now", might be another scenario.

Either way, validation of a Date field might fail, if the constraint validator for Past and Future strictly relies on JVM time. So I think, a way to let that validator know, what "now" means, is required. I'm not sure though, how to do that. One option might be to allow the usage of an expression language (unified EL?) to specify the comparison value:

Code:
...
private Date now;
   
@Future(currentDate="${now}")
private Date futureDate;
...


Being able to use an EL to specify comparison values would also be useful for other constraints such as Min, Max or Size. That way, some scenarios, for which now a custom class level constraint is required, could be realized leveraging the standard annotations (e.g. one number field required to be larger than another one). Alternatively, to provide a simple solution just for the Past/Future problem, some setter for the current time at the Validator API might be considered.

- Could the Size constraint also be allowed for attributes of type Map? The class comment of Size mentions String, Collection and Array as allowed types, leaving Map out, as it doesn't inherit from Collection.

- At the Min/Max constraints the comment says, that you are not sure wether to allow them at byte variables or not. What would be the rationale for excluding byte-typed fields?

Thanks for any answers in advance. Keep up the good work and with kind regards,

Gunnar

_________________
Visit my blog at http://musingsofaprogrammingaddict.blogspot.com/


Top
 Profile  
 
 Post subject: Re: Feedback for specification draft
PostPosted: Sun Jan 25, 2009 3:03 am 
Newbie

Joined: Sun Mar 30, 2008 12:19 am
Posts: 15
Gunnar wrote:
- The interface name "Constraint" appears irritating to me. I would regard Min, Max, Size etc. as "constraints" and classes that check for such constraints as "validators". Throughout the spec, that nomenclature is also used several times (so the JavaDoc of ConstraintFactory says: "This class manages the creation of constraint validators."). And in fact, the package, in which the constraint annotations live, is named javax.validation.constraints.

Therefore I suggest to rename the Constraint interface to "Validator" (the current Validator interface might be named "BeansValidator" or similar) or "ConstraintValidator" (the current ConstraintValidator interface could e.g. be named "ValidatedBy" then).


I also would like to voice my support of calling the annotations 'constraint annotations' and the actual classes that process these 'constraint validators, implementing a ConstraintValidator interface. ValidatedBy also seems like a good annotation name to mark constraint annotations.

Quote:
- The class comments of the Past and Future annotations say that the JVM's current time will be used for validity checking. This could be an issue because "now" is not in every case defined as the JVM's current time.

In my code for instance, rather than calling new Date() or similar, I prefer the usage of a simple time service: Date myDate = DateService.getDate(). This service might return new Date() normally, but for instance yesterday's date in case a failed batch run shall be re-executed. Using a date set by some IoC container as "now", might be another scenario.


Past and Future are good examples of constraints that rely on external state for validation, problem is that external state isn't really established in any context for the validators. The ConstraintContext interface passed into the Constraint.isValid method really is more of an ErrorHandler-ish interface. Today it looks like context would have to be established via external globals (such as your DateService class above).


Top
 Profile  
 
 Post subject: Re: Feedback for specification draft
PostPosted: Tue Jan 27, 2009 4:44 pm 
Hibernate Team
Hibernate Team

Joined: Sun Sep 14, 2003 3:54 am
Posts: 7256
Location: Paris, France
Hi Gunnar, thanks for your feedback.

Quote:
- The interface name "Constraint" appears irritating to me. I would regard Min, Max, Size etc. as "constraints" and classes that check for such constraints as "validators". Throughout the spec, that nomenclature is also used several times (so the JavaDoc of ConstraintFactory says: "This class manages the creation of constraint validators."). And in fact, the package, in which the constraint annotations live, is named javax.validation.constraints.

Therefore I suggest to rename the Constraint interface to "Validator" (the current Validator interface might be named "BeansValidator" or similar) or "ConstraintValidator" (the current ConstraintValidator interface could e.g. be named "ValidatedBy" then).


So I have good news for you.
Check that out
http://lists.jboss.org/pipermail/hibern ... 03581.html
and let me know if you like the new names.
I changed the constraint related classes and the bootstrap API is also a lot more readable (I think)

_________________
Emmanuel


Top
 Profile  
 
 Post subject: Re: Feedback for specification draft
PostPosted: Tue Jan 27, 2009 4:54 pm 
Hibernate Team
Hibernate Team

Joined: Sun Sep 14, 2003 3:54 am
Posts: 7256
Location: Paris, France
Gunnar wrote:
- The question was raised before, but I didn't find an answer: Couldn't the Constraint interface be parametrized with the field type validatable by a concrete implementation as follows:

Code:
public interface Constraint<A extends Annotation, T> {

   void initialize(A constraintAnnotation);

   boolean isValid(T object, ConstraintContext constraintContext);
}


That way, implementations could be created in a type-safe manner. In case a constraint annotation is allowed at multiple types, this would require a separate Constraint implementation for each type, but I don't think, this is a big issue. Actually, it would lead to cleaner code without "if-instanceof-...-else-if-instanceof-..." cascades.

To specify, which validator should be used for a certain field type supported by a given constraint annotation, an interface ValidatorDescriptor could be introduced:

Code:
public interface ValidatorDescriptor<A extends Annotation, T, V extends Constraint<A, T>> {

   Class<T> getType();

   Class<V> getValidatorClass();

}


Following this approach, the ConstraintValidator meta annotation would refer to an array of such validator descriptors:

Code:
public @interface ConstraintValidator {

   public abstract Class<? extends ValidatorDescriptor<?, ?, ?>>[] value();
}

Beside a safer implementation of validators, this approach would enable another use case: compile-time checking for misplaced constraint annotations by implementing an annotation processor, which could not only check for proper usage of the standard annotations, but also - by evaluating the validator descriptors - of any custom constraint annotations.


We came roughly to the same conclusion but I think we have found a slightly better approach. Also your proposal does not address all corner cases.
Let me know what you think of
http://lists.jboss.org/pipermail/hibern ... 03567.html

_________________
Emmanuel


Top
 Profile  
 
 Post subject: Re: Feedback for specification draft
PostPosted: Tue Jan 27, 2009 11:55 pm 
Hibernate Team
Hibernate Team

Joined: Sun Sep 14, 2003 3:54 am
Posts: 7256
Location: Paris, France
Gunnar wrote:
- The class comments of the Past and Future annotations say that the JVM's current time will be used for validity checking. This could be an issue because "now" is not in every case defined as the JVM's current time.

In my code for instance, rather than calling new Date() or similar, I prefer the usage of a simple time service: Date myDate = DateService.getDate(). This service might return new Date() normally, but for instance yesterday's date in case a failed batch run shall be re-executed. Using a date set by some IoC container as "now", might be another scenario.

Either way, validation of a Date field might fail, if the constraint validator for Past and Future strictly relies on JVM time. So I think, a way to let that validator know, what "now" means, is required. I'm not sure though, how to do that. One option might be to allow the usage of an expression language (unified EL?) to specify the comparison value:

Code:
...
private Date now;
   
@Future(currentDate="${now}")
private Date futureDate;
...


Being able to use an EL to specify comparison values would also be useful for other constraints such as Min, Max or Size. That way, some scenarios, for which now a custom class level constraint is required, could be realized leveraging the standard annotations (e.g. one number field required to be larger than another one). Alternatively, to provide a simple solution just for the Past/Future problem, some setter for the current time at the Validator API might be considered.



Frankly I am not a big fan of the EL approach. There could be two solutions to solve this problem:
- define in the spec a way to override a given constraint validator implementation for a given constraint (or if we lack time, a BV provider could offer such a feature).
- use ConstraintValidatorFactory (new name for ConstraintFactory) to inject the DateService.

Would that be satisfying?

Quote:
- Could the Size constraint also be allowed for attributes of type Map? The class comment of Size mentions String, Collection and Array as allowed types, leaving Map out, as it doesn't inherit from Collection.


Added, this was an oversight.

Quote:
- At the Min/Max constraints the comment says, that you are not sure wether to allow them at byte variables or not. What would be the rationale for excluding byte-typed fields?


Somehow I thought of byte as a different beast than numbers but it seems the Java class hierarchy disagree. I will add it.

_________________
Emmanuel


Top
 Profile  
 
 Post subject:
PostPosted: Sun Feb 01, 2009 10:43 am 
Hibernate Team
Hibernate Team

Joined: Sat Jan 24, 2009 12:46 pm
Posts: 388
emmanuel wrote:
So I have good news for you.
Check that out
http://lists.jboss.org/pipermail/hibern ... 03581.html
and let me know if you like the new names.
I changed the constraint related classes and the bootstrap API is also a lot more readable (I think)

...
emmanuel wrote:
Added, this was an oversight.

...
emmanuel wrote:
Somehow I thought of byte as a different beast than numbers but it seems the Java class hierarchy disagree. I will add it.


That's all great news. I like the new names. That way they are way more understandable. The bootstrap API is also easier to use now. Kudos for the quick implementation of the changes in the RI, too. Actually I noticed the renaming before you posted here, when I updated my SVN checkout of the RI, and my sample project didn't compile any more :-)

emmanuel wrote:
Frankly I am not a big fan of the EL approach. There could be two solutions to solve this problem:
- define in the spec a way to override a given constraint validator implementation for a given constraint (or if we lack time, a BV provider could offer such a feature).
- use ConstraintValidatorFactory (new name for ConstraintFactory) to inject the DateService.

Would that be satisfying?


I think the first suggestion is a possible approach. Within my own validator for Past/Future I could do whatever is neccessary to get the "current" date. I don't understand the second approach fully, though. How would a standard validator implementation know how to use my DateService? If such a service were part of the API it would work, leaving it to the user to provide an implementation based on her requirements.

_________________
Visit my blog at http://musingsofaprogrammingaddict.blogspot.com/


Top
 Profile  
 
 Post subject: Re: Feedback for specification draft
PostPosted: Sun Feb 01, 2009 2:58 pm 
Hibernate Team
Hibernate Team

Joined: Sat Jan 24, 2009 12:46 pm
Posts: 388
emmanuel wrote:
We came roughly to the same conclusion but I think we have found a slightly better approach. Also your proposal does not address all corner cases.
Let me know what you think of
http://lists.jboss.org/pipermail/hibern ... 03567.html


Great that you considered that. At the time of my writing, I believed, that the concrete type parameters of a ConstraintValidator implementation weren't available at runtime, because of which I introduced the indirection of the descriptor class. But as one actually can access the concrete types, this is not necessary, and you solution is preferable.

I checked out the proposed resolution algorithm, too. It seems understandable and logically. One question got into my mind though. If I am not mistaken, the resolution solely depends on the declared type of a field/getter. Did you think about taking into account the actual type of the element to be validated on runtime to determine the proper validator? This would show some resemblance to the late binding of method invocations, though I am not sure, whether that would be advantageous or not.

One more question: For a compound constraint, that only comprises some basic constraints, but has no validation logic itself, one still has to declare a constraint validator (which would always return true in isValid()) in the @Constraint annotation.

As solution one might either set an empty array as default value for validatedBy() in @Constraint (not specifying at least one validator would only be allowed for compound constraint annotations then). Or one could introduce a NullConstraintValidator in the API, that can be used in validatedBy() in such cases.

What do you think?

Regards, Gunnar

_________________
Visit my blog at http://musingsofaprogrammingaddict.blogspot.com/


Top
 Profile  
 
 Post subject:
PostPosted: Mon Feb 02, 2009 3:17 am 
Hibernate Team
Hibernate Team

Joined: Sun Sep 14, 2003 3:54 am
Posts: 7256
Location: Paris, France
Gunnar wrote:
emmanuel wrote:
emmanuel wrote:
Frankly I am not a big fan of the EL approach. There could be two solutions to solve this problem:
- define in the spec a way to override a given constraint validator implementation for a given constraint (or if we lack time, a BV provider could offer such a feature).
- use ConstraintValidatorFactory (new name for ConstraintFactory) to inject the DateService.

Would that be satisfying?


I think the first suggestion is a possible approach. Within my own validator for Past/Future I could do whatever is neccessary to get the "current" date.


I added that in the spec Friday :) It fits very well with the new type-safe validators

_________________
Emmanuel


Top
 Profile  
 
 Post subject: Re: Feedback for specification draft
PostPosted: Mon Feb 02, 2009 3:25 am 
Hibernate Team
Hibernate Team

Joined: Sun Sep 14, 2003 3:54 am
Posts: 7256
Location: Paris, France
Gunnar wrote:
If I am not mistaken, the resolution solely depends on the declared type of a field/getter. Did you think about taking into account the actual type of the element to be validated on runtime to determine the proper validator? This would show some resemblance to the late binding of method invocations, though I am not sure, whether that would be advantageous or not.


That's how I initially wrote the algorithm but some expert group member had compelling arguments to use compile time types rather than runtime types:
- what about the null value
- the behavior is more predictable and can be fully checked at compile time (via a annotation processor)
- this is what you declare really, constraints in a way are an extension of the Java type system. Following the java type system rules is better (method overloading in Java is using the compile time type).

Quote:
One more question: For a compound constraint, that only comprises some basic constraints, but has no validation logic itself, one still has to declare a constraint validator (which would always return true in isValid()) in the @Constraint annotation.

As solution one might either set an empty array as default value for validatedBy() in @Constraint (not specifying at least one validator would only be allowed for compound constraint annotations then). Or one could introduce a NullConstraintValidator in the API, that can be used in validatedBy() in such cases.


-1 on a NullConstraintValidator in the API that's for sure :)

We could support {} as you describe but my question is what if the user makes an error and don't list the validator. This thing will be valid all the time wo him noticing. I wonder which approach is best:
- save some typing
- vs report probable errors to users

_________________
Emmanuel


Top
 Profile  
 
 Post subject: Re: Feedback for specification draft
PostPosted: Mon Feb 02, 2009 4:10 pm 
Hibernate Team
Hibernate Team

Joined: Sat Jan 24, 2009 12:46 pm
Posts: 388
emmanuel wrote:
We could support {} as you describe but my question is what if the user makes an error and don't list the validator. This thing will be valid all the time wo him noticing. I wonder which approach is best:
- save some typing
- vs report probable errors to users


Yes, I thought about the issue with someone forgetting to specify the ConstraintValidator, if {} was the default value, too. Actually one can explicitely specify validatedBy={} in the annotation, if no validator is needed. Maybe it could clearly be mentioned in the spec, that this is valid for "pure" compound constraints.

Annother option might be to introduce a dedicated annotation @CompoundConstraint, that has no validatedBy() attribute, but only has the purpose to mark a pure compound constraint annotation type.

Regards,

Gunnar

_________________
Visit my blog at http://musingsofaprogrammingaddict.blogspot.com/


Top
 Profile  
 
 Post subject:
PostPosted: Fri Feb 13, 2009 9:05 am 
Hibernate Team
Hibernate Team

Joined: Sun Sep 14, 2003 3:54 am
Posts: 7256
Location: Paris, France
I took the point. Not sure it is worth the effort compared to the other tasks left.

_________________
Emmanuel


Top
 Profile  
 
Display posts from previous:  Sort by  
Forum locked This topic is locked, you cannot edit posts or make further replies.  [ 11 posts ] 

All times are UTC - 5 hours [ DST ]


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum

Search for:
© Copyright 2014, Red Hat Inc. All rights reserved. JBoss and Hibernate are registered trademarks and servicemarks of Red Hat, Inc.