On Jun 21, 2017 2:58 AM, "Gunnar Morling" <gunnar@hibernate.org> wrote:
> 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>.

I suppose allowing a particular XML class configuration to target a superclass method in the suggested manner would confer an advantage not available to the annotation based config. Let's finish it up then. :-)

Matt


2017-06-20 22:36 GMT+02:00 Matt Benson <mbenson@apache.org>:
> On Tue, Jun 20, 2017 at 8:04 AM, Gunnar Morling <gunnar@hibernate.org> wrote:
>> 2017-06-19 17:50 GMT+02:00 Matt Benson <mbenson@apache.org>:
>>> On Mon, Jun 19, 2017 at 8:04 AM, Gunnar Morling <gunnar@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@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@hibernate.org> wrote:
>>>>>> 2017-06-16 14:06 GMT+02:00 Matt Benson <mbenson@apache.org>:
>>>>>>> On Fri, Jun 16, 2017 at 2:43 AM, Gunnar Morling <gunnar@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@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@lists.jboss.org
>>>>>>>>> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
>>>>>>>> _______________________________________________
>>>>>>>> beanvalidation-dev mailing list
>>>>>>>> beanvalidation-dev@lists.jboss.org
>>>>>>>> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
>>>>>>> _______________________________________________
>>>>>>> beanvalidation-dev mailing list
>>>>>>> beanvalidation-dev@lists.jboss.org
>>>>>>> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
>>>>>> _______________________________________________
>>>>>> beanvalidation-dev mailing list
>>>>>> beanvalidation-dev@lists.jboss.org
>>>>>> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
>>>>> _______________________________________________
>>>>> beanvalidation-dev mailing list
>>>>> beanvalidation-dev@lists.jboss.org
>>>>> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
>>>> _______________________________________________
>>>> beanvalidation-dev mailing list
>>>> beanvalidation-dev@lists.jboss.org
>>>> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
>>> _______________________________________________
>>> beanvalidation-dev mailing list
>>> beanvalidation-dev@lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
>> _______________________________________________
>> beanvalidation-dev mailing list
>> beanvalidation-dev@lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
> _______________________________________________
> beanvalidation-dev mailing list
> beanvalidation-dev@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
_______________________________________________
beanvalidation-dev mailing list
beanvalidation-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/beanvalidation-dev