Some feedback reading the spec, and having used OVal (which I'm surprised to not see mentioned that much).
Sebastian (OVal fame) is part of the EG and joined rather recently but he is contributing actively. Part of the spec has been heavily influenced by his contribution.
1) All parameters and return types should document whether they accept or return null. If null is accepted its meaning should be described. The JSR-310 code uses:
@param foo the foo stuff, not null
@param bar the bar stuff, null means ....
@return the new instance, never null
noted the point.
2) 2.1 - I find the naming of Constraint confusing. The constraint should be the annotation itself, such as @Size. The class which performs the check should be named Check (as in OVal) or Validator (but this clashes). Thus rename Constaint to Check, and rename ConstraintValidator to Constraint.
Agreed we renamed things in the latest version. check out http://in.relation.to/10679.lace
3) @NotNull is confusing as it is not the same as JSR305/308. It may well be desirable to annotate a field with both annotations, resulting in one being fully qualified, which is very messy.
Could you consider prefixing all constraints? This would aid clarity and readability in general:
private String foo;
Possible prefixes are 'Check', 'Validate' or 'Val'. For example, @CheckValid and @CheckUpperCase are a lot more expressive than @Valid or @UpperCase.
Well I have hard time understanding why the java namespace mechanism is a bad thing. The problem with the prefix, say "Check", is that it equally can apply to a 305 constraint. So it does not really solve any of the confusion problem. I opened a discussion with Roberto (Sun EE) and Bill P on the subject of 305 / 303, @NotNull / @Nonnull is really the main clashing name. The rest seems fine.
4) 2.4 - The current Constraint interface has a major flaw in that it does not pass in enough data. Oval passes in four parameters, including the object being validated:
public boolean isValid(Object validatedObject, Object valueToValidate, OValContext context, Validator validator)
In my work use of OVal we use the validatedObject parameter to perform cross-field validation:
private LocalDate startDate;
private LocalDate endDate;
This is very neat and would be a major loss. In addition, JSR303 doesn't provide access to the Validator, which is the bootstrap object.
It does, you need to use a class level constraint which accept the java bean instance. you can then access sub properties. There were various problems in allowing cross property validation from a property level constraint, and class-level validation does solves that elegantly (IMO).
5) 2.4 - ConstraintContext should not be an interface. Using an interface here means that the key validation API that will be widely implemented cannot be extended in a version 2 of JSR303. I recommend using a class (possibly abstract) instead.
How is that not be extendable in future versions? This interface is implemented by a Bean VAlidation provider, not by a client.
6) 3.4 - Default should be named DefaultGroup. Default is rather meaningless by itself.
well it's in the group package :) We discussed the point, there were no clear agreement. We ended up choosing the smallest name.
7) 3.4.2 - This seems to indicate that constraints are not evaluated in order. This is not acceptable. By default annotations on a field need to be evaluated in the order that they are declared. This is because webpages may only be able to display one error at a time, and thus will just select the first returned error for each field. This needs to be predictable. The group sequencing is way too complex for this simple requirement.
Web page don't accept more than one error per field? JSF might not, but fore sure that's not a hard limitation.
The problem with ordering is that the order in which annotations are retrieved for an element are not guaranteed to be predictable by the JVM / compiler, AFAIK. If it's not the case (and annotations are retrieved in predictable order), then I am ok with adding this ordering constraint (ie for a given element (field, method, class), constraints are validated in the order they are retrieved by the reflection API.
let me know.
8) I found the sections on group sequences difficult to follow. I don't know why I'd need them or how I'd use them. They obviously make the spec a lot more complex, so I'd ask if you really need them? (ie. just let the user call the Default group followed by the Detailed group. Don't over-complicate)
People vehemently wanted a way to order constraints, hence the introduction of group sequence.
You approach might be a solution but it forces every client to remember the ordering, leading to duplication.
9) 4 - Validator should not be an interface. As an interface, you won't be able to extend it in a version 2 of the spec. Similarly, ConstraintViolation.
Same remark. Look at JPA, EntityManager has new methods in JPA 2, this will be backward compatible.
10) Much of the info from ConstraintViolation should be made available to implementors of constraint validators (root bean, leaf bean, property path, groups, descriptor...)
Can you describe a use case for each?
11) The bootstrap process has builders, factories, factories of factories, and more. This seems remarkably complicated (I don't have time to study it). OVal has one class - Validator - which is thread-safe. Shouldn't this spec simply provide a simple bean-style API, and allow other specs to integrate it how they want? Maybe I'm missing something...
I renamed a lot of the bootstrap elements in the latest version, it should be more readable http://in.relation.to/10679.lace
The complexity (comparing with Hibernate Validator legacy and OVal) is due to:
- support for multiple implementations of the spec
- using the fluent API design simplify the API usage (but complexify its design and implementation)
A default user will do
ValidatorFactory vf = validation.buildDefaultValidatorFactory();
Validator v = vf.getValidator();
Again, I'd note that since the key participants are interfaces, you're not able to extend in a second version of the spec.
12) 5.5 - OVal allows us to get the constraint validation implementation. We use this to get hold of the maximum length of a field which can go in HTML. The metadata API seems quite tricky to use for simple cases like this (eg. find the MaxLength annotation on this property and get the configured values)
I don't follow you, can you detail the use case for me?
Overall, the spec is well written, and I'm reasonably happy with it. However, I'm not yet convinced that I would receommend migration from OVal, especially wrt item #4 above.
Generally speaking the spec supports extensibility. So Oval implementing Bean Validation can offer these extra functionalities.
I don't think they are needed though (at least in a spec).
We will see :)
Thanks for your feedback.