[bv-dev] Spec question on ContainerElementTypeDescriptor

Matt Benson mbenson at apache.org
Tue Jun 20 16:36:59 EDT 2017 

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

More information about the beanvalidation-dev mailing list