2:19 a.m.
No feedback? :(
Wonder if I missed something or if you think that my thoughts go in the right direction /
show real problems with the discussed approach.
Cheers,
Hendrik
Am 07.11.2016 um 23:42 schrieb Hendrik Ebbers
<hendrik.ebbers(a)me.com>:
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(a)kaltepoth.de
<mailto:christian@kaltepoth.de>>:
>
> Hey Gunnar,
>
> 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.
>
> 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
> Blog: http://blog.kaltepoth.de/ <http://blog.kaltepoth.de/>
> Twitter: http://twitter.com/chkal <http://twitter.com/chkal>
> GitHub: https://github.com/chkal <https://github.com/chkal>
>
> _______________________________________________
> beanvalidation-dev mailing list
> beanvalidation-dev(a)lists.jboss.org <mailto:beanvalidation-dev@lists.jboss.org>
> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
_______________________________________________
beanvalidation-dev mailing list
beanvalidation-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/beanvalidation-dev