Remarks on the JSR 303 public review

emmanuel · **Posted:** Mon Jan 12, 2009 4:54 pm

FroMage wrote:

page 22 I don't understand the rationale for having to put A.class as part of the default group sequence for A. It says _must have_ so why not make it implied rather than required?

because we don't know where it the sequence it should be added:
- begining
- middle
- end?

FroMage wrote:

For me it is easier to think that constraints without groups are in group Default, and that @GroupSequence on the constrained class defines the default group sequence for validating that class, which implicitely includes the Default group first (A.class right now in this example). Perhaps it's just me, but I find it less confusing, never mind the fact that groups cannot have circular dependencies: the Default group validation sequence for class A includes the Default group validation for class A's property and type validation. I don't see that as circular…

That's a possibility but it's a bit confusing and requires to bend the rules. I noted it as a possibility.

emmanuel · **Posted:** Mon Jan 12, 2009 4:59 pm

FroMage wrote:

Having thought about it some more I really think conditional validation could be expressed as such:

Code:

public class Foo {

 public interface Admin{}

 boolean isAdmin;

 @StrongPassword(groups = Admin.class)
 @Password
 String password;

 @GroupSequenceProvider
 public List<Class<?>> getValidationGroups(){
   if(isAdmin)
     return Arrays.asList(Default.class, Admin.class);
   return Arrays.asList(Default.class);
 }

This means automatic validation by Hibernate when saving entities would validate the "password" property using the @StrongPassword constraint only when the "isAdmin" property is true. I think this would solve most (if not all) of my problems and looks type-safe enough.

@GroupSequenceProvider if found on a class method would be called to provide the default group sequence for the given validated instance. If I really understood the rationale for it, according to the spec it would not return Default.class but Foo.class. This would be mutually exclusive with a type annotation @GroupSequence since it serves a similar purpose. I would name @GroupSequenceProvider differently so as not to make it confusing for properties annotated with @GroupSequence where it defines the sequence for the current property, and not the Default sequence for the whole instance.

What do you think?

I like the idea. It would make sense to move it completly out of the class?

Code:

@GroupSequenceProvider(FooSequenceProvider.class)
public class Foo {
}

To be honest it might not be part of the spec but it would totally make sense to add it in the RI.
http://opensource.atlassian.com/project ... se/BVAL-85

emmanuel · **Posted:** Mon Jan 12, 2009 5:19 pm

FroMage wrote:

Page 42 it says that for Class-level constraints nothing is appended to the "propertyPath", yet the ConstraintContext.addError(String message, String property) takes a "property" parameter to specify the faulty property. How are we supposed to get the faulty property out of a ConstraintViolation set wit ConstraintContext.addError(message, property) then?

Not sure I follow you. If you want to redefine the message of a class level constraint, use addError(String)
If you want to apply it on a property of the class, use addError(String, String).

I reworded the Javadoc a little bit

Code:

/**
    * Add a new error message to a given sub property <code>property</code>. 
    * This error message will be interpolated.
    * <p/>
    * If isValid returns false, a ConstraintViolation object will be built
    * per error message including the default one unless #disableDefaultErrorMEssage()
    * has been called.
    * If the constraint being validated is not a class-level constraint, 
    * a ValidationException is raised.
    * 
    * <p/>
    *
    * @param message new unexpanded error message
    * @param property property name the ConstraintViolation is targeting
    *
    * @throws ValidationException if the constraint is not set on a class-level
    */
   void addError(String message, String property);

[/quote]

emmanuel · **Posted:** Mon Jan 12, 2009 5:22 pm

FroMage wrote:

Page 68 wouldn't it make it easier to use if BeanDescriptor had a getTypeConstraints() or getBeanConstraints() or getClassConstraints() (however it is named) which returns the class-level constraints? It seems to be missing and since there is a hasConstraints() method there, it seems fitting to include them there.

I suspect they would be included from the ElementDescriptor.getConstraintDescriptors() but still, why make this harder than for property constraints?

ElementDescriptor.getConstraintDescriptor() does return the element level constraints. This is true for BeanDescriptor, PropertyDescriptor, Parameterdescriptor etc. not sure why you think it's different between proeprty and bean.

BTW I am planning to move hasConstraints() up to ElementDescriptor

emmanuel · **Posted:** Mon Jan 12, 2009 5:26 pm

FroMage wrote:

Would it make any sort of sense if constraints could provide something as specific as a javascript implementation?

This could then be used by frameworks to do first checks on the browser side.

I suspect something like that would be too intrusive:

Code:

@NotNull
@ConstraintValidator(NotEmptyConstraint.class)
@Target({ METHOD, FIELD })
@Retention(RUNTIME)
public @interface NotEmpty{
 String jsChecker() default "function(string){return string && string.length > 0;}"; // or whatever it is in correct JS
}

But perhaps there could be a way to map other language checks (I'm thinking of DB constraints too here) to constraints?

Maybe this is the job of the framework, but when you define your own constraints the framework is out of the game (for JS and for DB constraints) so if the constraint implementor could have a way to get in the game and provide those, it could be useful for the frameworks.

What do you think?

I think that's the job of the integrating framework, JSF for example.
We thought about an integration possibility where org.hibernate.constraints.Email.js would be picked up when JSF needs a JS function implementing the validation.
Unfortunately this will not be standardized in this JSF release.

I like composing constraints as a way to expose the encessary metadata and let each framework define how to express out of java constraints.

emmanuel · **Posted:** Mon Jan 12, 2009 5:30 pm

FroMage wrote:

We use @AssertFalse on methods a lot to make higher-level checks for a given property. Yet when we return false from the method, we can set the message correctly, but the associated property is gotten from the name of the checking method when really this method was just checking another property:

Code:

class Foo {

 String bar;

 @AssertFalse
 public isBarValid(){
  return bar != null && bar.equals("yeah");
 }
}

This would report that the invalid property is "barValid" when really for us it is "bar". Again I know we can set the message correctly, but could there be a way to set the applicable property too?

well I would rather have a @ValidBar constraint set at the class level with the logic you describe in an actual Constraint implementation. from there you can define the property the constraint violation is applied to ConstraintContext.addError(String, String)

emmanuel · **Posted:** Mon Jan 12, 2009 5:32 pm

FroMage wrote:

Is there a way to perform all validation without stopping at the first constraint violation?

We use Hibernate Validation to check REST provided JAXB entities, and we would like to report not only the first error, but all errors. Is there a way to do that with Validator other than iterating manually over all validators using the introspection API?

Yes that that's the default :) Actually today there is no way to stop at the first violation :)

emmanuel · **Posted:** Mon Jan 12, 2009 5:33 pm

FroMage wrote:

Page 88:

Quote:

In case of a constraint violation report detected and generated by the database
(not null, etc), the Java persistence provider catches this report and translates
it into a BV error report. From the perspective of the application, constraint
errors are viewed through a unified layer. BV must provide some API to create a
constraint violation error (constraintDescriptor.createConstraintViolation(...)).

Can we get unicity constraints mapped to BV errors too? Please?

Unfortunately looks like this one is not going to make it :( Maybe we can do that in Hibernate

emmanuel · **Posted:** Mon Jan 12, 2009 5:41 pm

FroMage wrote:

Page 88:

Quote:

Java Persistence delegates the validation of entities to BV on the following entity events:
- pre persist (an entity must be valid to be inserted in the DB)
- pre update (an entity change must be valid before being propagated to the database)
- pre remove (eg. a Customer still owning some Payments cannot be removed)
- question has been asked about post load. What would be the value add?

The value added would be for application upgrades when the DB data is from a previous application with a compatible schema but different BV constraints, or when developing using SQL imported test data that might become invalid when new constraints are applied. We've had these cases before and an error early on (when loading them) would be nice.

[/quote]

But then what would you do with them? Validating on change is one thing but validating on load is a whole different story.

emmanuel · **Posted:** Mon Jan 12, 2009 5:42 pm

emmanuel wrote:

FroMage wrote:

Page 88:

Quote:

Java Persistence delegates the validation of entities to BV on the following entity events:
- pre persist (an entity must be valid to be inserted in the DB)
- pre update (an entity change must be valid before being propagated to the database)
- pre remove (eg. a Customer still owning some Payments cannot be removed)
- question has been asked about post load. What would be the value add?

The value added would be for application upgrades when the DB data is from a previous application with a compatible schema but different BV constraints, or when developing using SQL imported test data that might become invalid when new constraints are applied. We've had these cases before and an error early on (when loading them) would be nice.

But then what would you do with them? Validating on change is one thing but validating on load is a whole different story.[/quote]

remember that JPA raises an exception. So you would be totally stuck.

emmanuel · **Posted:** Mon Jan 12, 2009 5:49 pm

FroMage wrote:

On Page 91 about the integration with JSF2 I don't see anything that makes me think that the beans holding the edited properties would be validated.

I feel strongly that validating single properties one at a time is not enough in most cases, as it bypasses class-level validation and higher-level property validation (such as @AssertXXX methods). This forces us to not be DRY and implement JSF validators.

The problems stems from JSF checking and applying property values one at a time when BV needs all properties to have been set before it can call class-level validation (and also validate the other non-edited properties).

Isn't there any way to have the ModelValidator be a bit smarter and if all edited properties have been validated, then validate their owner bean instances according to regular BV rules? If that then fails, you can revert the edited properties and return a failure to JSF.

I feel strongly that this is a key issue.

That's correct. While we all agreed that this was an important concern, we could not agree on a natural way to validate whole beans rather than simple properties and decided to keep it for later. I hope natual JSF / Bean Validation provider innovation will fill this gap and that we will be able to standardize that in the next rev.

emmanuel · **Posted:** Mon Jan 12, 2009 5:50 pm

FroMage wrote:

Let's not forget: un grand bravo for this specification, it looks very serious and well done.

Thanks. And thank you for all the feedback. We disagreed at time but the spec got better from all these exchanges.

FroMage · **Joined:** Tue Apr 01, 2008 11:10 am **Posts:** 69

emmanuel wrote:

FroMage wrote:

Just a quick remark about ConstraintContext.addError(): on page 13 it says the message is not expanded, while on page 15 the example uses a localised message:

Code:

constraintContext.addError( "{constraint.length.min}" );

Is that expanded or not expanded?

Perhaps in the example you could add several messages too, just to prove it is possible.

I did improve the JavaDoc to make that clearer

Much clearer to me, thanks.

FroMage · **Joined:** Tue Apr 01, 2008 11:10 am **Posts:** 69

emmanuel wrote:

FroMage wrote:

page 22 I don't understand the rationale for having to put A.class as part of the default group sequence for A. It says _must have_ so why not make it implied rather than required?

because we don't know where it the sequence it should be added:
- begining
- middle
- end?

Aha! OK I did not get that at all from reading the specs. That seems like a fairly good reason. Perhaps it should be noted that this is possible in the spec? Maybe it's just me not reading it properly ;)

FroMage · **Joined:** Tue Apr 01, 2008 11:10 am **Posts:** 69

emmanuel wrote:

I like the idea. It would make sense to move it completly out of the class?

Code:

@GroupSequenceProvider(FooSequenceProvider.class)
public class Foo {
}

To be honest it might not be part of the spec but it would totally make sense to add it in the RI.
http://opensource.atlassian.com/project ... se/BVAL-85

OK, I can totally live with it being in the RI since that's probably what I'll be using ;)

As for putting it in another class I like the possibility of being able to do it, but in several cases I like my validation to be self-contained withing the validated class, so if that could _also_ be possible it'd be great. I don't see any reason to limit the location of this provider to be only external to the validated bean, but maybe you thought of one reason I've overlooked?