Remarks on the JSR 303 public review

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

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.

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

Other quick remark:

On code like on page 18 you could drop the @author javadoc to reduce the size of the examples and perhaps the size of the spec. Unless this is required?

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

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?

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…

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

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?

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

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?

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

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?

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

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?

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

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?

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

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?

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

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?

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

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]

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

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.

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

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

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

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

Code:

/**
    * Add a new error message. 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.
    * <p/>
    * Aside from the error message, ConstraintViolation objects generated from such a call
    * contains the same contextual information (root bean, path and so on)
    * <p/>
    * This method can be called multiple time. One ConstraintViolation instance per
    * call is created.
    *
    * @param message new unexpanded error message
    */
   void addError(String message);

   /**
    * Add a new error message to a given sub property. This error message 
    * will be interpolated.
    * <p/>
    * If isValid returns false, a ConstraintViolation object will be built
    * per error message including the default one
    * if null apply to the current property or the bean the constraint is applied on,
    * otherwise apply to the <code>property</code> named
    * <p/>
    * TODO exception or swallowed when bean level instance is not present?
    *
    * @param message new unexpanded error message
    * @param property property name the ConstraintViolation is targeting
    *
    * @throws ValidationException when the property is not present on the bean level object
    */
   void addError(String message, String property);

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

FroMage wrote:

Other quick remark:

On code like on page 18 you could drop the @author javadoc to reduce the size of the examples and perhaps the size of the spec. Unless this is required?

done thanks