[bv-dev] Spec question on ContainerElementTypeDescriptor

[Gunnar Morling]

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