[bv-dev] Spec question on ContainerElementTypeDescriptor
Gunnar Morling
gunnar at hibernate.org
Wed Jun 21 03:52:38 EDT 2017
> My thinking was to make it possible for a user to configure container
> element constraints for various levels of the inheritance hierarchy
> for a given e.g. method return type. So continuing the lookup example,
> perhaps we want to constrain the key of a LookupTable only for
> LookupImpl and not for every implementation of ILookup.
Ah, got you.
I don't think it's something we should support really, as it'd abandon
the symmetry of capabilities of annotations and XML. For such use
case, LookupImpl#getLookup() should declare a return type of Map<...,
...> itself, so container element constraints can be applied via
<beanClass class="LookupImpl">...</beanClass>.
2017-06-20 22:36 GMT+02:00 Matt Benson <mbenson at apache.org>:
> On Tue, Jun 20, 2017 at 8:04 AM, Gunnar Morling <gunnar at hibernate.org> wrote:
>> 2017-06-19 17:50 GMT+02:00 Matt Benson <mbenson at apache.org>:
>>> On Mon, Jun 19, 2017 at 8:04 AM, Gunnar Morling <gunnar at hibernate.org> wrote:
>>>> Hi,
>>>>
>>>> Putting constraints to type parameters of generic types or methods as
>>>> well as putting constraints to type arguments in extends/implements
>>>> clauses isn't supported as of BV 2.0. I thought that's mentioned
>>>> somewhere in the spec, but couldn't find it during a quick check. If
>>>> it's indeed missing, I'll add it. We can look into this for 2.1 of
>>>> course.
>>>
>>> I'm pretty sure wildcards are explicitly mentioned as not being
>>> supported, and bounds are mentioned there. I don't remember type vars
>>> being included in that. You say "as of BV 2.0." Can I take that to
>>> mean that you think it's at least possible that this might be
>>> something the spec should support in the future? Is this basically a
>>> case of limiting feature creep so that we can finish this iteration
>>> ASAP?
>>
>> Yes, it's definitely something we can consider to support in a future
>> revision. But for BV 2.0 it's too late to add it, as I'll have to
>> submit the Proposed Final Draft tomorrow or say latest Thursday. While
>> we can do some more adjustments until Final Approval Ballot, this may
>> only be corrections and clarifications of already added stuff at this
>> point unfortunately.
>>
>> For sure we should explore this on the implementation level, so we
>> have some experiences already when starting to work on the next spec
>> revision.
>>
>>
>>>
>>>>
>>>>> I also suspect that it will make sense to introduce a <container>
>>>>> element to the XML mapping schema with an optional type, to
>>>>> (optionally?) house the <container-element-type> elements.
>>>>
>>>
>>> If we've established that the ContainerElementType needs the container
>>> type to be certain of providing enough information to locate the
>>> relevant value extractor, then consider the XML mapping. Say we have:
>>>
>>> interface ILookup {
>>> Map<String, String> getLookup();
>>> }
>>>
>>> class LookupTable extends Hashtable<String, String> {
>>> }
>>>
>>> class LookupImpl implements ILookup {
>>> private LookupTable lookupTable;
>>>
>>> LookupTable getLookup() {
>>> return lookupTable;
>>> }
>>> }
>>>
>>> And we want to constrain the type parameters of the Map in XML:
>>>
>>> <bean class="ILookup">
>>> <getter name="getLookup">
>>> <!-- TBD -->
>>> </getter>
>>> </bean>
>>>
>>> We could do:
>>>
>>> <container-element-type container-type="java.util.Map" type-argument-index="0">
>>> <constraint annotation="NotNull" />
>>> <constraint annotation="NotBlank" />
>>> </container-element-type>
>>> <container-element-type container-type="java.util.Map" type-argument-index="1">
>>> <constraint annotation="NotBlank" />
>>> </container-element-type>
>>
>> I don't think there's a need to specify the container type in this
>> case, as this can be obtained from the configured field, method etc.
>> So for configuring ILookup#getLookup(), it'd look like this with the
>> currently proposed schema:
>>
>> <bean class="ILookup">
>> <getter name="lookup">
>> <container-element-type type-argument-index="0">
>> <constraint annotation="NotNull" />
>> <constraint annotation="NotBlank" />
>> </container-element-type>
>> <container-element-type type-argument-index="1">
>> <constraint annotation="NotBlank" />
>> </container-element-type>
>> </getter>
>> </bean>
>>
>> I.e. this is about Map<String, String> as that's the type of
>> ILookup#getLookup().
>>
>>>
>>> ... and for simplicity's sake, since the typical container likely has
>>> a single element type, we probably should continue to support this.
>>> But should we also allow, e.g.:
>>>
>>> <container type="java.util.Map">
>>> <container-element-type type-argument-index="0">
>>> <constraint annotation="NotNull" />
>>> <constraint annotation="NotBlank" />
>>> </container-element-type>
>>> <container-element-type type-argument-index="1">
>>> <constraint annotation="NotBlank" />
>>> </container-element-type>
>>> </container-type>
>>
>> I think it's answered by my previous comment?
>>
>
> My thinking was to make it possible for a user to configure container
> element constraints for various levels of the inheritance hierarchy
> for a given e.g. method return type. So continuing the lookup example,
> perhaps we want to constrain the key of a LookupTable only for
> LookupImpl and not for every implementation of ILookup. Given the time
> issue though we can leave this alone for now.
>
>>>
>>> WRT the optionality of the type argument index, it's clear that for
>>> containers with a single element type, that could be omitted. Is it
>>> further intended that where the list of container-element-type
>>> elements has a length matching the number of type variables declared
>>> on the container type, these should be understood to specify a 1:1
>>> ordered correspondence? If so, is there a place (and hopefully a more
>>> intelligible way ;) ) to clarify that in the spec?
>>
>> That behaviour is not intended. I don't like the idea of relying on
>> ordering, as it'd require you to specify (empty) container element
>> type nodes for all type arguments, also if only one is to be
>> constrained.
>>
>> That's what we currently say in the spec:
>>
>> "The type-argument-index is used to specify the index of the
>> configured type argument. The type-argument-index can be omitted, if
>> the container element type has exactly one type argument. [...] If an
>> invalid container type element configuration is detected, a
>> ValidationException is raised. This includes the following
>> configuration errors:
>>
>> * The type of the surrounding element (field, getter etc.) has no
>> type arguments.
>> * The type of the surrounding element has no type argument with
>> the index given via type-argument-index.
>> * The type of the surrounding element has multiple type arguments
>> and no index is given via type-argument-index.
>> * The same type argument of the surrounding element is configured
>> multiple times."
>>
>> Do you think this is clear enough or should we change the wording in some way?
>>
>
> Seems okay to me.
>
> Matt
>
>>
>>>
>>> Matt
>>>
>>>> Could you elaborate on that point a bit? What exactly is the use case for this?
>>>>
>>>> --Gunnar
>>>>
>>>>
>>>>
>>>> 2017-06-18 17:34 GMT+02:00 Matt Benson <mbenson at apache.org>:
>>>>> More thoughts in this vein:
>>>>>
>>>>> At first I thought that method overrides were the only place you might
>>>>> have > 1 container class represented, but why wouldn't we support
>>>>> e.g.:
>>>>>
>>>>> class Roles implements Set<@NotEmpty String> {
>>>>> ...
>>>>> }
>>>>>
>>>>> ? Then it'd be possible to extract container elements even at the bean level.
>>>>>
>>>>> Next, consider parameters. the spec currently says that wildcards
>>>>> aren't supported, but what about:
>>>>>
>>>>> <X extends Set<@NotEmpty String> & Iterable<@NotNull String>> void foo(X);
>>>>>
>>>>> In this way a parameter could expose multiple container types (pretend
>>>>> I used unrelated interfaces if that helps). We could save generic
>>>>> extends support for a future iteration of the spec, but semantically
>>>>> it makes sense to me so I'd expect we'll eventually have to do it in
>>>>> any case. I suppose you could even do fields if you annotated some
>>>>> type parameter of the bean.
>>>>>
>>>>> I also suspect that it will make sense to introduce a <container>
>>>>> element to the XML mapping schema with an optional type, to
>>>>> (optionally?) house the <container-element-type> elements.
>>>>>
>>>>> Matt
>>>>>
>>>>>
>>>>> On Fri, Jun 16, 2017 at 1:38 PM, Gunnar Morling <gunnar at hibernate.org> wrote:
>>>>>> 2017-06-16 14:06 GMT+02:00 Matt Benson <mbenson at apache.org>:
>>>>>>> On Fri, Jun 16, 2017 at 2:43 AM, Gunnar Morling <gunnar at hibernate.org> wrote:
>>>>>>>> Hi Matt,
>>>>>>>>
>>>>>>>> Thanks for bringing this up!
>>>>>>>>
>>>>>>>> I think adding ContainerElementTypeDescriptor#getContainerClass() is
>>>>>>>> one thing needed, but it's not enough. Currently we have
>>>>>>>> List<ContainerElementTypeDescriptor> getContainerElementTypes() on
>>>>>>>> PropertyDescriptor etc. which returns the container element
>>>>>>>> descriptors in the order of type arguments. This also doesn't really
>>>>>>>> fit for User#getRoles().
>>>>>>>>
>>>>>>>> So we could make this a Set<ContainerElementTypeDescriptor> instead,
>>>>>>>> and then each element from that set exposes containerClass and
>>>>>>>> typeArgumentIndex as you suggest.
>>>>>>>>
>>>>>>>> It's getting a bit tricky though to get the effective set of all
>>>>>>>> container element constraints. Eg. say we have:
>>>>>>>>
>>>>>>>> public interface IUserBase {
>>>>>>>> Set<@NotBlank String> getRoles();
>>>>>>>> }
>>>>>>>>
>>>>>>>> public interface IUser extends IUserBase {
>>>>>>>> Set<@NotEmpty String> getRoles();
>>>>>>>> }
>>>>>>>>
>>>>>>>> public class UserImpl implements IUser {
>>>>>>>> Roles getRoles();
>>>>>>>> }
>>>>>>>>
>>>>>>>> Then when querying
>>>>>>>>
>>>>>>>> validator.getConstraintsForClass( IUser.class )
>>>>>>>> .getConstraintsForProperty( "roles" )
>>>>>>>> .getContainerElementTypes();
>>>>>>>>
>>>>>>>> We'd get a set with these elements:
>>>>>>>>
>>>>>>>> - ContainerElementDescriptor(containerClass=IUser.class,
>>>>>>>> typeArgumentIndex=0, constraints={(type=NotEmpty,
>>>>>>>> defined=DEFINED_LOCALLY), (type=NotBlank,
>>>>>>>> defined=DEFINED_IN_HIERARCHY)}
>>>>>>>> - ContainerElementDescriptor(containerClass=User.class,
>>>>>>>> typeArgumentIndex=0, constraints={(type=NotBlank, defined=
>>>>>>>> DEFINED_LOCALLY)}
>>>>>>>>
>>>>>>>
>>>>>>> I was thinking that the containerClass would be Set.class.
>>>>>>
>>>>>> Yes, you are right of course.
>>>>>>
>>>>>>>
>>>>>>>> The descriptor for IUser would have to return the inherited @NotBlank
>>>>>>>> as per the defined semantics of getConstraintDescriptors(). So one
>>>>>>>> would have to examine all ContainerElementDescriptors and just collect
>>>>>>>> the ones that are DEFINED_LOCALLY to have the effective set without
>>>>>>>> duplicates.
>>>>>>>>
>>>>>>>
>>>>>>> I agree that the set of ContainerElementTypeDescriptors needs to be
>>>>>>> reduced to unique type parameters, but I'm not sure I'm following your
>>>>>>> argument above. My suggestion above (containerClass=Set.class) would
>>>>>>> give an easy way to identify what needs to be collapsed where: onto
>>>>>>> Set<[0]>. Consider this:
>>>>>>>
>>>>>>> public class Roles<S extends System> implements Set<String> {
>>>>>>> public S getSystem() {
>>>>>>> ...
>>>>>>> }
>>>>>>> ...
>>>>>>> }
>>>>>>>
>>>>>>> public class UserImpl implements IUser {
>>>>>>> public Roles<@ValidSystem UserSystem> getRoles() {
>>>>>>> ...
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> Here Roles would implement a custom container type on top of Iterable
>>>>>>> via Set, so the method would return separate ContainerTypeElements for
>>>>>>> Roles<[0]> and Set<[0]>. Does that make sense?
>>>>>>
>>>>>> Yes, it does.
>>>>>>
>>>>>> And in my previous example we'd have getContainerElementTypes() return
>>>>>> a set with one descriptor:
>>>>>>
>>>>>> ContainerElementDescriptor(containerClass=Set.class,
>>>>>> typeArgumentIndex=0, constraints={(type=NotEmpty, defined=
>>>>>> DEFINED_IN_HIERARCHY), (type=NotBlank, defined=DEFINED_IN_HIERARCHY)}
>>>>>>
>>>>>> (both constraints are defined in the hierarchy from the perspective of UserImpl)
>>>>>>
>>>>>>>
>>>>>>> Matt
>>>>>>>
>>>>>>>> The alternative would be to explicitly redeclare
>>>>>>>> getConstraintDescriptors() for ContainerElementDescriptor and say that
>>>>>>>> it doesn't expose constraints from the hierarchy. But this would make
>>>>>>>> it very difficult for the user to find all those constraints applying
>>>>>>>> to a given container element type (esp. as type arguments could switch
>>>>>>>> their order in the hierarchy). So that'd not be my preferred route.
>>>>>>>>
>>>>>>>> WDYT?
>>>>>>>>
>>>>>>>> I've filed https://hibernate.atlassian.net/browse/BVAL-655 for this issue.
>>>>>>>>
>>>>>>>> --Gunnar
>>>>>>>>
>>>>>>>>
>>>>>>>> 2017-06-15 17:38 GMT+02:00 Matt Benson <mbenson at apache.org>:
>>>>>>>>> The various descriptor types return constraint information from the
>>>>>>>>> entire hierarchy by default. However, in thinking about this I quickly
>>>>>>>>> encountered a puzzle which I cannot solve using the current version of
>>>>>>>>> the specification. Consider:
>>>>>>>>>
>>>>>>>>> public interface User {
>>>>>>>>> Set<@NotEmpty String> getRoles();
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> public class Roles implements Set<String> {
>>>>>>>>> ...
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> public class UserImpl implements User {
>>>>>>>>> @Override
>>>>>>>>> public Roles getRoles();
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> What should the metadata of UserImpl provide? The Iterable value
>>>>>>>>> extractor is applicable to Roles, but because the return value of the
>>>>>>>>> getter has been narrowed, there is no type parameter at this point in
>>>>>>>>> the hierarchy. It seems strange for a PropertyDescriptor of type Roles
>>>>>>>>> to return a ContainerElementTypeDescriptor with index 0. This example
>>>>>>>>> would AFAICT apply to e.g. javafx StringProperty as well, so it's
>>>>>>>>> probably a question we need to resolve, unless I have missed something
>>>>>>>>> that should be obvious. I think this could be solved if the CETD were
>>>>>>>>> to explicitly specify the type with which its argument index is
>>>>>>>>> associated. The alternative would be to require a custom
>>>>>>>>> ValueExtractor? This seems cumbersome and unnecessary.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Matt
>>>>>>>>> _______________________________________________
>>>>>>>>> beanvalidation-dev mailing list
>>>>>>>>> beanvalidation-dev at lists.jboss.org
>>>>>>>>> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
>>>>>>>> _______________________________________________
>>>>>>>> beanvalidation-dev mailing list
>>>>>>>> beanvalidation-dev at lists.jboss.org
>>>>>>>> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
>>>>>>> _______________________________________________
>>>>>>> beanvalidation-dev mailing list
>>>>>>> beanvalidation-dev at lists.jboss.org
>>>>>>> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
>>>>>> _______________________________________________
>>>>>> beanvalidation-dev mailing list
>>>>>> beanvalidation-dev at lists.jboss.org
>>>>>> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
>>>>> _______________________________________________
>>>>> beanvalidation-dev mailing list
>>>>> beanvalidation-dev at lists.jboss.org
>>>>> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
>>>> _______________________________________________
>>>> beanvalidation-dev mailing list
>>>> beanvalidation-dev at lists.jboss.org
>>>> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
>>> _______________________________________________
>>> beanvalidation-dev mailing list
>>> beanvalidation-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
>> _______________________________________________
>> beanvalidation-dev mailing list
>> beanvalidation-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
> _______________________________________________
> beanvalidation-dev mailing list
> beanvalidation-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
More information about the beanvalidation-dev
mailing list