Re: [bv-dev] Spec question on ContainerElementTypeDescriptor

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.

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