Re: [bv-dev] Spec question on ContainerElementTypeDescriptor

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