Simplify handling of ValueExtractors in Hibernate Validator extension

[yrodiere]

Draft for now, using this as a way to demonstrate a problem while addressing #36831.

[mkouba]

I don't think this is necessary. For parameterized types we always compare the raw type, i.e. if you use beanTypes(DotName.createSimple(ValueExtractor.class)) then if your bean implements ValueExtractor<String> it should be matched.

[yrodiere]

Right... except that parameterized types using wildcards are ignored in compliance with the CDI spec, as you explained in #36831 (comment).

Which means beanTypes(DotName.createSimple(ValueExtractor.class)) won't match a bean implementing ValueExtractor<List<?>> for example.

Which extensions may need something like this when they have no control over whether implementations will use wildcards. In the case of Hibernate Validator using wildcards in ValueExtractor implementations is even one of the recommended solutions, so... :/

Though this method should probably be renamed to translate this... ?

[mkouba]

Yes, the name should not contain beanTypes at all.

Nevertheless, I believe that it's not a very common functionality so a custom Predicate<BeanInfo> in the HV extension would be a better fit.

[yrodiere]

Yes, the name should not contain beanTypes at all.

Sure. Though it's problematic that "bean type" is a very generic wording for a very specific thing in CDI :/ I suspect most people using beanTypes(...) methods, like me, didn't realize that CDI Bean types are not the same as the Java types of objects registered as CDI beans.

Nevertheless, I believe that it's not a very common functionality

I already have a second extension needing this with Hibernate ORM and the TenantResolver, as you know: #36831

I just had a quick look at the current code in Quarkus, and I suspect we should be using it in other places as well:

• For AttributeConverter<X, Y>: what if a bean implements AttributeConverter<List<? extends Number>, String>?

  quarkus/extensions/hibernate-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java

  Line 615 in 3fc255f

  unremovableBeans.produce(UnremovableBeanBuildItem.beanTypes(AttributeConverter.class));

• For BeanMarshaller<T>, EnumMarshaller<E>, MessageMarshaller<T>, RawProtobufMarshaller<T>: same idea.

  quarkus/extensions/infinispan-client/deployment/src/main/java/io/quarkus/infinispan/client/deployment/InfinispanClientProcessor.java

  Line 463 in d7842f9

  return UnremovableBeanBuildItem.beanTypes(BaseMarshaller.class, EnumMarshaller.class, MessageMarshaller.class,

• For ServiceDiscoveryProvider<T>, LoadBalancerProvider<T>, ServiceRegistrarProvider<T, MetadataKeyType>, ServiceRegistrarLoader<MetadataKeyType>: same idea.

  quarkus/extensions/smallrye-stork/deployment/src/main/java/io/quarkus/stork/deployment/SmallRyeStorkProcessor.java

  Lines 51 to 57 in 9bbfe72

  	return UnremovableBeanBuildItem.beanTypes(
  	DotName.createSimple(ServiceDiscoveryProvider.class),
  	DotName.createSimple(ServiceDiscoveryLoader.class),
  	DotName.createSimple(LoadBalancerProvider.class),
  	DotName.createSimple(LoadBalancerLoader.class),
  	DotName.createSimple(ServiceRegistrarProvider.class),
  	DotName.createSimple(ServiceRegistrarLoader.class));

Really, I'm pretty sure most extensions using beanTypes(...) were written without knowing that it wouldn't work with wildcard types. I.e., they should have been using something like this, and just didn't because they assumed beanTypes(...) would work intuitively, but it doesn't as soon as one uses wildcards, and nobody reported the problem yet.

So I agree applications ending up with a problem because of wildcards should be rare (except for Hibernate Validator). But I disagree this is an uncommon functionality, because pretty much every extension needing to register unremovable generic interfaces would need something like this, whether their authors know it or not.

[mkouba]

But I disagree this is an uncommon functionality, because pretty much every extension needing to register unremovable generic interfaces would need something like this, whether their authors know it or not.

Well, type variables are not a problem. And I'm not sure if wildcard type arguments are very common. But I agree that it's confusing for those who don't care about the CDI spec (i.e. majority of users ;-).

[yrodiere]

Right, and by the way I'm not saying I (or extension maintainers, or application developers) don't care about the CDI spec. Just that we may not be aware of every edge case that it covers, especially those that are not particularly intuitive.

Now I'll try to be more constructive... let's continue the conversation there: #36831 (comment), where I suggested we should have a way to make CDI use the raw type as a bean type for beans that implement a parameterized type, but for specific raw types only of course. I suggested a build item, Matej suggested an annotation which extensions could apply through bytecode transformation, which I guess could be triggered by a build item as well. Could you please chime in there when you have some time?