BVAL-508 Adding latest proposal for value extraction as an appendix #116
Conversation
|
|
||
| == Value extraction for cascaded validation and type-level constraints (BVAL-508) | ||
|
|
||
| This appendix describes the current work-in-progress around the retrieval of values to be validated during cascaded validation and evaluation of type-level constraints (e.g. ). It is based on the original http://beanvalidation.org/proposals/BVAL-508/[proposals for BVAL-508]. |
| Value extraction is needed when constraints are applied to the element(s) stored within a container type. There are two categories: | ||
|
|
||
| * Cascaded validation of collections, maps and arrays as triggered via `@Valid`: `@Valid List<Order> orders`; in this case all the values stored in the list must be extracted so each can be validated | ||
| * Validation of type-level constraints as enabled by Java 8: `Optional<@Email String> email` or `Property<@Min(1) Integer> value`; in this case, e.g. the `Integer` wrapped by the property must be extracted so the `@Min` constraint can be applied |
There was a problem hiding this comment.
e.g. not necessary here? It seems to be a leftover of a previous formulation.
| [NOTE] | ||
| .Motivation for callback-style API | ||
| ==== | ||
| Instead of returning the extracted values from the method call, implementations of `ValueExtractor` pass the extracted values to the given receiver callback. This helps to avoid object allocations and allows to handle the case of a single extracted value (`Optional<T`) and multiple extracted values (`List<T>`) in a uniform fashion. |
|
|
||
| * arrays of objects and all primitive data types | ||
| * `java.util.Iterable` (when `@Valid` is given on the iterable element itself, the element and all its entries will be validated; this is to grant backwards compatability with BV 1.1; when `@Valid` is given on the type parameter of an iterable element, all the entries will be validated). A node of type `TYPE_PARAMETER` and with the name "<collection element>" will be added to the path when validating a type parameter constraint. | ||
| * `java.util.Map` (when `@Valid` is given on the map element itself, the element and all its values will be validated; this is to grant backwards compatability with BV 1.1; when `@Valid` is given on the key type parameter, only the keys will be validated; when `@Valid` is given on the value type parameter, only the value will be validated. A node of type `TYPE_PARAMETER` and with the name "<map value>" will be added to the path when validating a type parameter constraint on the map value. A node of type `TYPE_PARAMETER` and with the name "<map key>" will be added to the path when validating a type parameter constraint on the map key. |
There was a problem hiding this comment.
s/only the value/only the valueS/
|
|
||
| * Invoke the new method `Configuration#addValueExtractor(ValueExtractor<?>)` (to apply it on the validation factory level) | ||
| * Invoke the new method `ValidatorContext#addValueExtractor(ValueExtractor<?>)` (to apply it for a single `Validator`) | ||
| * Specify the FQN of one or several extractors in `META-INF/validation.xml`: |
|
|
||
| When detecting a type-level constraint or cascade, the applicable extractor is determined as follows: | ||
|
|
||
| * Chose the most specific extractor matching the type parameter in question; an extractor A is more specific than another extractor B if A extracts a subtype of the type extracted by B. |
| When detecting a type-level constraint or cascade, the applicable extractor is determined as follows: | ||
|
|
||
| * Chose the most specific extractor matching the type parameter in question; an extractor A is more specific than another extractor B if A extracts a subtype of the type extracted by B. | ||
| * If there are several extractors which are equally specific, a `UnexpectedTypeException` is raised. TODO: apply rules similar to "ConstraintValidator resolution algorithm". |
There was a problem hiding this comment.
a UnexpectedTypeException -> an UnexpectedTypeException
| @Valid List<Order> orders; | ||
| ---- | ||
|
|
||
| This would also validate any constraints on a custom list type (e.g. `MyList#getId()`). TODO: we never clarified that in 1.1. Should it be made explicite? |
| Map<@RegExp(...) String, @RetailOrder Order> orders; | ||
| ---- | ||
|
|
||
| This would validate the map's key's against `@RegExp`, the map's values against `@RetailOrder` and apply cascaded validation of the map (values). |
There was a problem hiding this comment.
s/key's/keys/
having (values) in parenthesis is weird.
|
|
||
| This would validate the map's key's against `@RegExp`, the map's values against `@RetailOrder` and apply cascaded validation of the map (values). | ||
|
|
||
| * When selecting extractors, type parameters must be throroughly traced in the hierarchy. Consider this case where the order of the type parameters of `Map` is swapped in a sub-type: |
There was a problem hiding this comment.
you used to write type parameters with an hyphen. I think we should be consistent. Personally, I vote for removing the hyphen. See the glossary here: http://beanvalidation.org/glossary/
| ---- | ||
| interface NumericMap<T extends Number> extends Map<T, T> {} | ||
| ---- | ||
| private NumericMap<@Min(1) Integer> integerMap; |
There was a problem hiding this comment.
Missing [source,java] formatting.
| ---- | ||
| private NumericMap<@Min(1) Integer> integerMap; | ||
|
|
||
| The `@Min` constraint is to be applied to the map' keys and values as the annotated type parameter maps to `K` and `V` of `Map`. |
|
|
||
| TODO | ||
|
|
||
| ==== Meta-data retrieval |
There was a problem hiding this comment.
Please use metadata in one word.
|
@gunnarmorling I made a quick proofreading pass. HTH. |
| * Should the presence of type-level constraints alone trigger cascaded validation? It doesn't seem right, but it may be useful when e.g. considering the case of `Tuple` above. | ||
| * Should we allow extractors to be defined for specific paramterizations of generic types, e.g. |
There was a problem hiding this comment.
s/paramterizations/parameterizations/
| ---- | ||
|
|
||
| I can't see a compelling use case for this (when would extractor behavior differ between different parameterizations of the same generic type) and am leaing towards only supporting the wildcard parameterization (`implements ValueExtractor<List<@ExtractedValue ?>>`). |
|
|
||
| [[appendix-value-extraction]] | ||
|
|
||
| == Value extraction for cascaded validation and type-level constraints (BVAL-508) |
There was a problem hiding this comment.
Note that the title means nothing for 99% of people.
What was wrong with
Validating elements contained in a container (like collections)
There was a problem hiding this comment.
I find these terms more precise as they are using the lingo from the spec.
There was a problem hiding this comment.
Replaced "type-level" with "type argument" now.
|
|
||
| === Motivation | ||
|
|
||
| Value extraction is needed when constraints are applied to the element(s) stored within a container type. There are two categories: |
There was a problem hiding this comment.
For info, it's better to follow the one sentence per line for better diff and all.
There was a problem hiding this comment.
I'll update it before pushing.
|
|
||
| Value extraction is needed when constraints are applied to the element(s) stored within a container type. There are two categories: | ||
|
|
||
| * Cascaded validation of collections, maps and arrays as triggered via `@Valid`: `@Valid List<Order> orders`; in this case all the values stored in the list must be extracted so each can be validated |
There was a problem hiding this comment.
Also why isn't Optional<@Valid Foo> a cascaded example? In which case Optional and many more should be on the list.
There was a problem hiding this comment.
| Value extraction is needed when constraints are applied to the element(s) stored within a container type. There are two categories: | ||
|
|
||
| * Cascaded validation of collections, maps and arrays as triggered via `@Valid`: `@Valid List<Order> orders`; in this case all the values stored in the list must be extracted so each can be validated | ||
| * Validation of type-level constraints as enabled by Java 8: `Optional<@Email String> email` or `Property<@Min(1) Integer> value`; in this case, e.g. the wrapped `String` and `Integer` values must be extracted so the `@Email` and `@Min` constraints can be applied |
There was a problem hiding this comment.
I'm not sure what you mean by type-level constraint. Can you either use a term defined in http://beanvalidation.org/glossary/ or add a definition of type-level to it.
There was a problem hiding this comment.
I mean constraints on a type argument. I'll replace it everywhere.
| * Cascaded validation of collections, maps and arrays as triggered via `@Valid`: `@Valid List<Order> orders`; in this case all the values stored in the list must be extracted so each can be validated | ||
| * Validation of type-level constraints as enabled by Java 8: `Optional<@Email String> email` or `Property<@Min(1) Integer> value`; in this case, e.g. the wrapped `String` and `Integer` values must be extracted so the `@Email` and `@Min` constraints can be applied | ||
|
|
||
| Both cases can also overlap: `List<@RetailOrder @Valid> retailOrders`. |
There was a problem hiding this comment.
You're missing the type in your list definition
List<@RetailOrder @Valid Order> retailOrders
| === Open questions | ||
|
|
||
| * `ConstraintsApplyTo` only allows one behavior per annotated element. Should it be per constraint? E.g. for `@NotNull @Email StringProperty email` it may be desirable to apply `@NotNull` to the wrapper but `@Email` to the wrapped value. That's not possible currently. | ||
| * Should type-level constraints be validated when the container is null? |
There was a problem hiding this comment.
how would that work? No container, no value to validate against, right?
There was a problem hiding this comment.
Yes, that looks right.
| * `ConstraintsApplyTo` only allows one behavior per annotated element. Should it be per constraint? E.g. for `@NotNull @Email StringProperty email` it may be desirable to apply `@NotNull` to the wrapper but `@Email` to the wrapped value. That's not possible currently. | ||
| * Should type-level constraints be validated when the container is null? | ||
| * What to return from `PropertyDescriptor#getElementClass()` if there is a field of type `Foo` but a getter of type `Optional<Foo>`. So far, BV assumed the types of field and getter to be the same and exposed a single property descriptor (which btw. also may fall apart as of BV 1.1 when the field is of a sub-type of the getter's type). What to return here? | ||
| * Should the presence of type-level constraints alone trigger cascaded validation? It doesn't seem right, but it may be useful when e.g. considering the case of `Tuple` above. |
There was a problem hiding this comment.
Why do you think it's useful in the Tuple example?
There was a problem hiding this comment.
You could have further constraints within the Typle class itself. I can see how someone could expect their validation to be implicitly triggered when validating constraints given on the type arguments. Now you have to remember to put @Valid on the usage, too. I think it's better that way but wanted to hear what others think.
| public class ListOfStringExtractor implements ValueExtractor<List<@ExtractedValue String>> { ... } | ||
| ---- | ||
|
|
||
| I can't see a compelling use case for this (when would extractor behavior differ between different parameterizations of the same generic type) and am leaning towards only supporting the wildcard parameterization (`implements ValueExtractor<List<@ExtractedValue ?>>`). |
There was a problem hiding this comment.
I'd say +1 for only supporting the wildcard for now. It looks like the algorithm could be refined if we find a usage later.
| .Implementation note | ||
| ==== | ||
| As extractor retrieval for type parameter constraints is done using the static type of constrained elements, the retrieval can be done once at initialization time and then be cached. This is not possible for retrieval of extractors for cascaded validation. | ||
| ==== |
There was a problem hiding this comment.
Add a rule for the following case. When two extractors with the same container type and generic type.
- probably raise an exception
Do we allow the extraction of non generic type?
class WeightedList extends List<String> {
Integer weight();
}
class WeightExtractor implements ValueExtractor<WeightList<?>> {
void extractValues(T originalValue, ValueReceiver receiver) {
receiver.value(originalValue.weight(), "weight");
}
}
If yes, then we might want several of them on the same type.
There was a problem hiding this comment.
Add a rule for the following case. When two extractors with the same container type and generic type.
Should be covered by "If there are several extractors which are equally specific"?
There was a problem hiding this comment.
Do we allow the extraction of non generic type?
Your example isn't fully clear to me. The extractor definition is invalid as WeightList has no type parameter. Taking a step back, what's the behavior you'd want to see for WeightList?
There was a problem hiding this comment.
I think you answered the question by no. You do not want the mechanism to extract values that are not related to a type parameter.
I'm only slightly uneased by it as it might block us in a future where we want to use the extractor for more things that type parameter validation and @Valid. I don't have concrete examples of such future however.
There was a problem hiding this comment.
Note you can declare class WeightExtractor implements ValueExtractor<@ExtractedValue WeightList> { ... }.
There was a problem hiding this comment.
@gunnarmorling the spec does not explain what that does though, right?
There was a problem hiding this comment.
We say
In some cases, an extractor applies to the entire extracted type itself (e.g. for arrays). In this case the @ExtractedValue annotation is to be given on the extracted type itself
There was a problem hiding this comment.
ok so WeightExtractor in this case would extract weight and no longer map keys and map values.
|
|
||
| If a non-null value is passed for `nodeName`, a path node of type `TYPE_PARAMETER` will be appended to the property path. That's desirable for collection types for instance. If null is passed, no node will be appended, resulting in the same path as if the constraint had been given on the element itself instead of a type parameter. That's desirable for pure "wrapper types" such as `Optional`. | ||
|
|
||
| In some cases, an extractor applies to the entire extracted type itself (e.g. for arrays). In this case the `@ExtractedValue` annotation is to be given on the extracted type itself: |
There was a problem hiding this comment.
We should capture somewhere (here or in the wiki) why @ExtractedValue lost the ability to refer to a supertype generic type. The main reason being that you can make an extractor for the supertype.
Note that it precludes:
- a subtype to use better access methods for a faster container navigation (i.e. a better extractor implementation)
- JavaFX
IntegerPropertyextractor to enforce@ConstraintsApplyTo(WRAPPED_VALUE)while JavaFXProperty<?>extractor to enforce@ConstraintsApplyTo(ANNOTATED_ELEMENT)
There was a problem hiding this comment.
Can you do it? I've never seen the motivation behind it.
* Renaming node kind TYPE_PARAMETER to TYPE_ARGUMENT to be consistent with our usage of terms * Expanding examples * Not auto-apply element-level constraints for Optional
|
@emmanuelbernard Thanks for reviewing. I've just pushed an update which should either address your remarks or capture them at the end of the document. Most notably, I've expended the sections on default extractor behavior and extractor resolution. I think with that it's good to go for the appendix and we can refine from there and answer the open questions. Can you take another look and let me know of your thoughts? |
|
Rebased and merged. Thanks a ton for doing the review dance, @emmanuelbernard! |
@emmanuelbernard That's the latest proposal around extractors, based on our recent discussions and insights from the PoC in the RI. It takes an opinion on some of the questions from the original proposal. Some remaining questions are listed at the end. Overall I think we are getting there.