Hi all,
I want to give some feedback to this proposal. 

Let’s start with a small example like this:

@NotEmpty
private List<String> values;

Here I assume that we have a @NotEmpty constraint that can be used of several types (like collections or Strings). In the example above the annotation defines some metadata for the field. By doing so you will read it as „The list should not be empty - The List should contain at least one element“.
I think this is the same syntax as it was done before and everyone will understand it.

Here is the next example:

private List<@NotEmpty String> desc;

By defining it this way we have a list that can contain 0…n elements but each element must be a string with a length > 0. Since the @NotEmpty annotation now marks the generic type of the list it is related to the content of the list.

The next example contains a mix:

@NotEmpty
private List<@NotEmpty String> desc;

Here 2 @NotEmpty annotations are used. In the end the field will be valid if the list contains at least one String and all Strings of the list have a length > 0.

Based on this we could create a field that looks like this:

private List<List<@NotEmpty List<StringProperty>>> spooky;

Even here we can say that the field must contain a list with 0…n elements where each element contains a list that must not be empty.

Until now I only worked with lists and here the approach is quite nice. Sadly it won’t work always. By adding an constraints annotation to the generic type of the list we know that the annotation mentions the content of the list. But this is only working because of one point: The generic type of a collection defines the type of the collection content. This works fine for collections (and for example JavaFX properties) but in other cases this will end in problems.

Let’s say we have the following 2 interfaces:

public interface CachedValue<V> {
    V getCachedValue();
}

public interface RealValue<V> {
    V getRealValue();
}

Based on this interfaces we can easily create a new class that implements both interfaces:

public class CachableValue<V> implements CachedValue<V>, RealValue<V> {

    private V cachedValue;

    @Override
    public V getCachedValue() {
        return cachedValue;
    }

    @Override
    public V getRealValue() {
        V realValue = receiveValueFromServer();
        cachedValue = realValue;
        return realValue;
    }

    private V receiveValueFromServer() {
        return ServerConnector.getCurrentValue(); //Some fake code
    }
}

Let’s try to add a constraint annotation to validate the content of such a CachableValue:

private CachableValue<@NotEmpty String> myValue;

Based on this definition you have absolutely no idea if the @NotEmpty annotation is defined for the real value, the cached value or both values. From my point of view this is a big problem. Until now its was always easy to see how the validation of a model should work based on the validation information (annotations) in the model. With this new approach you have no idea what will happen.

The most simple solution would be to add the support only to some special container / wrapper classes like collections, JavaFX properties, etc. I think this is a bad idea since new default might come to future versions of JavaSE and JavaEE (or maybe Spring) and that won’t be supported. Based on this I think that it will be a must to support all wrapper types. 

In addition I think that simply adding a constraints annotation to the generic type is wrong. All examples that we have mentioned are just perfect examples in that the generic type defines the content. Let’s say we have a class like this:

public abstract class ConvertableInteger<V> {

private int value;

public void setValue(int value) { this.value = value; }

public int getValue() { return value; }

public abstract V getConvertedValue();
}

If you want to validate an instance of this class you normally want to validate the integer and not the converted value:

private ConvertableInteger<@Min(2_000_000) Date> myValue;

Such an expression looks like if you want to validate a Date object. In this case the better solution would be something like this:

@Min(2_000_000)
private ConvertableInteger<Date> myValue;

And now we end in 2 different ways how we annotation value wrappers:

private CachableValue<@NotEmpty String> myCachedValue;

@Min(2_000_000)
private ConvertableInteger<Date> myIntegerValue;

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. 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. 

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.

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. 


I don’t think that support for Optional is that important since having a field of type Optional looks like an anti-pattern to me. Normally Optional should be used as a method return type and not as a type for a field. Supporting it in the bean validation might end in strange model classes. 

Example:

The following model looks good to me:

public class Model {

@NotNull
private String name; 

public String getName() { return name; }

public void setName(String name) { this.name = name; }

public Optional<String> name() { return Optional.ofNullable(name);}

}

On the other hand this looks like an anti-pattern to me:

public class Model {

@NotNull
private Optional<String> name; 

public Optional<String> getName() { return name; }

public void setName(String name) { this.name = Optional.ofNullable(name); }

}

I now that some of the code samples will not compile (for example several annotations of same type for element) but I think the code is more easy to read by doing it that way ;)

Cheers,

Hendrik


Am 30.09.2016 um 14:35 schrieb Christian Kaltepoth <christian@kaltepoth.de>:

Hey Gunnar,

  1. I was wondering whether the distinction between SingleContainerValueExtractor and ManyContainerValuesExtractor is really necessary. I'm not sure if the unnecessary instantiation of an iterator in the single value case is really a problem in practice. I think extractor implementations will provide some special implementation of the Iterator/Iterable contract in any case. Not sure how others feel about that.
Good question. I reckon we'll know more after experimenting with this in the RI. 

I agree. The proposal mentions the possibility of having different functional rules and better performance for the single value case as reasons for the distinction. I just think that the latter one won't actually be a real problem.

  1. Section 2.1 states that BV will support Iterable and Map by default. What about Optional? We will support the JDK8 date/time API, so why don't we plan to support Optional?
Optional will be part of it by default. It should be stated somewhere, I hope :) 

The document explicitly states:

   Bean Validation will have out of the box support for containers Iterable and Map.

I was expecting Optional to be listed here too. That's why I mentioned it. :-)


Christian



--
Christian Kaltepoth

_______________________________________________
beanvalidation-dev mailing list
beanvalidation-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/beanvalidation-dev