On 7 Nov 2016, at 23:42, Hendrik Ebbers <hendrik.ebbers@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.

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.

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@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/beanvalidation-dev