2016-11-21 17:08 GMT+01:00 Emmanuel Bernard <emmanuel(a)hibernate.org>:
On 7 Nov 2016, at 23:42, Hendrik Ebbers <hendrik.ebbers(a)me.com> wrote:
And this is not the only example. We already have some classes in JavaSE
that will end in such problems:
The basic Property class in JavaFX can be used like this:
private Property<String> myStringProperty;
Based on the current approach we should add a constraint annotation like
this:
private Property<@NotNull String> myStringProperty;
But next to the property class JavaFX contains the class StringProperty
that is a specific implementation of „Property<String>„. If we want to use
this class we can not add a annotation to a generic type since we don’t
have such a generic type. Here we need to do it this way:
@NotNull
private StringProperty myStringProperty2;
And again we have 2 different approaches:
private Property<@NotNull String> myStringProperty;
@NotNull
private StringProperty myStringProperty2;
Based on this I do not think that it’s a good idea to use the constraint
annotation for generic types to validate the content of a wrapper.
General comment on you entire email.
I think you are throwing the baby out with the bath water. Granted, more
complicated cases don’t scale as nicely as we would hope but let’s get back
tot he basics. 95%+ of this feature usage will be done for collections and
maps. Based on this, I’d be a bit unhappy at giving up parameterized type
use location for landing that annotations. This is sooooo natural.
Our job is to make the common case simple and ideally make it scale in a
not too ugly way and not close doors to the most complex use cases. And if
one thing has to break, we can drop some of the less common cases.
+100.
More specifically to this question, the proposal favored the use of
parameterized type use but offers a backup plan for when the
parameterized
type use location is not present (like specialized subclasses of Java FX).
Java FX is a clear cut as the wrapper is purely technical and all of the
constraints do apply to the wrapped value.
But everywhere we have a parameterized type use, we should have the app
developer make use of them instead of using the @ConstraintAppliesTo
workaround.
To be true I do not have the ultimative solution for this problem.
Currently I trying to add some metadata to the annotations to define it’s
goal. Here is an example:
@NotEmpty
private List<String> names1;
@NotEmpty(applyTo = ConstraintsApplyTo.INTERNALS)
private List<String> names2;
@NotEmpty
@NotEmpty(applyTo = ConstraintsApplyTo.INTERNALS)
private List<String> names3;
Here the „applyTo“ value was added to the @NotEmpty annotation (like bean
validation already does with the groups). By using this value a developer
can specify if he wants to validate the value of the field or its
internals. By doing so „names1“ is valid if the list contains at leas 1
element, „names2“ is valid if all elements in the list have at least a
length of 1 (an empty list is allowed) and in „names3“ the list must
contain at least one element and all elements must have a length > 0.
A similar approach is already defined in the proposal and I understand why
this is not the perfect solution since you need to define it whenever you
do not want to valide a plain value:
@NotNull(applyTo = ConstraintsApplyTo.INTERNALS)
private StringProperty myStringProperty2;
Extracting this information to an additional (optional) annotation will be
a problem since this can end in such an expression:
@NotEmpty
@ConstraintsApplyTo(CONTAINED_VALUES)
@NotEmpty
private List<String> names3;
By doing so you can not define the same constraints type for the container
and for the containing value. In the example above the same is working.
We did explore per annotation location overriding. This is a question in
the initial proposal. The reason it did not make the first cut is as you
have seen:
- you need a lot of information on side that it not trivial to digest
- each existing constraint annotation will have to be changed to add these
new attributes
- on that front, the spec has declared that attribute names starting
with “valid” are reserved for the spec, so your proposal would have to have
attributes prefixed with “valid” not to break existing code
- at the end of the day, the feature is really for code using type use, so
we are twisting it for edge cases
But I am very open to the idea of doing per annotation location overriding
if people think that they are an important feature.
Next to the „applyTo“ value all the constraint annotations needs
additional information to really validate all the given examples. In
addition a plugin must be present that defines how we can get the internal
value(s) of a class (in this case „StringProperty“).
Here are some code snippets that use additional information in the
annotations to define the validation:
@NotEmpty
@NotEmpty(applyTo = ConstraintsApplyTo.INTERNALS)
@NotEmpty(applyTo = ConstraintsApplyTo.INTERNALS, applyToInternalDepth = 2)
@NotEmpty(applyTo = ConstraintsApplyTo.INTERNALS, applyToInternalDepth = 3)
private List<List<List<StringProperty>>> spooky;
@NotEmpty(applyTo = ConstraintsApplyTo.INTERNALS, valueContainerClass =
RealValue.class)
private RealValue<String> myValue;
The „applyToInternalDepth“ value is used to define how often the validator
should get the internal value (depth). In the given example
„@NotEmpty(applyTo = ConstraintsApplyTo.INTERNALS, applyToInternalDepth =
3)“ would validate the content of the „StringProperty“. In addition we need
some information for the container if the container implements several
interfaces. This could be defined in the annotation, too. In the example
this is defined by the „valueContainerClass“ value.
Hopefully we can converge on my idea not to have to
add valueContainerClass (see the other emails). But you do need a way to
identify the correct type parameter targeted. This is equivalent to the
@ExtractedValue which today has typeParameterHost and
either typeParameterIndex or typeParameterName to uniquely identify them.
On the depth approach, your example would be more easily rewritten as
@NotEmpty List<@NotEmpty List<@NotEmpty List<@NotEmpty
StringProperty>>> spooky;
which clarifies everything. But let’s assume we have a type
ListOfListOfListOfStringProperty type. do we really want to support
@NotEmpty
@NotEmpty(applyTo = ConstraintsApplyTo.INTERNALS)
@NotEmpty(applyTo = ConstraintsApplyTo.INTERNALS, applyToInternalDepth = 2)
@NotEmpty(applyTo = ConstraintsApplyTo.INTERNALS, applyToInternalDepth = 3)
private ListOfListOfListOfStringProperty spooky;
or even
@NotEmpty
// support nested levels by positioning ConstraintsApplyTo at the right
level
private List<@NotEmpty List<@NotEmpty @NotEmpty(applyTo =
ConstraintsApplyTo.CONTAINED_VALUES) ListOfStringProperty>> spooky;
Or do we throw the towel and say that nested levels are not supported for
non parameterized type use locations? What I am realizing now is that this
option is available essentially to support containers that do not offer
parameterized type. And in a previous email, I am questioning that use case.
I'm also not convinced of this. In a way it seems to me like breaking
encapsulation.
When dealing with a type such as ListOfStringListsProperty, you don't know
it's inner works from a variable declaration. Hence I'm doubting that you
should be allowed to apply constraints to them from the outside. It's
getting more apparent when thinking of more real-world domain-specific
types:
public class CustomerContacts {
List<List<String>> contacts; // one single inner list contains the
contacts valid at a given time
}
I don't think it makes sense to allow application of constraints "from the
outside" when dealing with a property of CustomerContacts. It makes
assumptions on the implementation of that type which is a dangerous thing
to do. What if the implementation of CustomerContacts changes to be
map-based? Existing constraint configurations given at the usage side would
have to be adapted then.
Instead the constraints should be declared within CustomerContacts itself.
It's different for generic types. When dealing with a type like
List<List<String>>, its "inner workings" are apparent to the user,
i.e.
it's reasonable to allow them to apply constraints to the elements of the
inner list for instance.
Based on this a constraint annotation needs this additional fields:
ConstraintsApplyTo applyTo() default ConstraintsApplyTo.VALUE; // VALUE
or INTERNALS
int applyToInternalDepth() default 1;
Class<?> valueContainerClass() default Object.class;
As I said I don’t have a good solution :( But I think that based on the
given issues simply adding annotations to the generic types isn’t the best
idea.
I think it’s quite important to have support for all the wrapper classes.
Validation collections and (JavaFX) properties will be a big benefit.
_______________________________________________
beanvalidation-dev mailing list
beanvalidation-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/beanvalidation-dev