Re: [bv-dev] Spec question on ContainerElementTypeDescriptor

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?

> >> 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>

... 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>

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?

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