Rely more on AnnotedTypes in CDI Extension again

[jungm]

For the related discussion see https://lists.apache.org/thread/3py3htgx0l4bd46tsd90l138xqy65s7t

@rmannibucau sorry it took me a bit to get back on this but please take a look if this is what you had in mind. I'm not really sure how I should read type hierarchy from AnnotatedTypes, all ways I found to do that lead through java.lang.reflect.* land

[rmannibucau]

AFAIK this is not needed since the model already has all methods, constructors and inherited annotations (rest looks ok)

[jungm]

technically yes, but e.g. annotations on method parameters are lost if the method is being overwritten and inherited over multiple levels (see CdiConstraintOnlyOnParentClassTest, it fails if I remove processing superclass/interfaces)

[rmannibucau]

I assume it was copied from a tck? how do you remove the constraint if you have to respect the parent mode? Last time I checked the spec the liskov principle was enforced and a child can relax a parent constraint (not the opposite) so a not null param in an interface can be null in a class so we should really stick to CDI model and ensure parent annotations are there using CDI cause like that you can just end up in unpracticable cases no?

[jungm]

as far as I understand the spec says: "child must not strengthen preconditions or weaken postconditions", so one must not declare constraints on parameters in an overwritten method (https://jakarta.ee/specifications/bean-validation/3.1/jakarta-validation-spec-3.1#constraintdeclarationvalidationprocess-methodlevelconstraints-inheritance)

but also: "A constraint declaration can be placed on an interface. For a given class, constraint declarations held on superclasses as well as interfaces are evaluated by the Jakarta Validation provider." (https://jakarta.ee/specifications/bean-validation/3.1/jakarta-validation-spec-3.1#constraintdeclarationvalidationprocess-inheritance)

won't that mean that constraints are always inherited? actually, I am failing to see how a subtype would even explicitly weaken preconditions, I could only find examples of strengthening postconditions in the spec

[rmannibucau]

hmm, both parts kind of contradict themselves if I'm reading it right and in CDI integration it is worse since all the inheritance is in Annotated* model, no more in plain reflection at all so think we should exclude and challenge (if desired) this test and just not loop to parents. (likely with a comment explaining the model is not reflection based + liskov principle is the opposite of this)

[jungm]

IMO the example in the test should work:

• GreetingService defines a method with preconditions
• GreetingServiceImpl overrides it, one could argue it weakens preconditions but there is also no way for it to "re enforce" the precondition from the parent. I would assume that to be happening by default because of §3.1
• IntermediateGreetingService/LastGreetingService do not overwrite the method anymore, so nothing on preconditions/postconditions should change

This is also how the validation ends up working, but the algorithm in the CDI extension must IMO look at the entire type hierarchy to decide whether or not to enable the BValInterceptor. And I think it is better to have false positives than false negatives at detecting what needs the interceptor (of course the goal should be to already be 100% aligned with the spec in the algorithm in BValExtension)

[rmannibucau]

one could argue it weakens preconditions but there is also no way for it to "re enforce" the precondition from the parent

this is allowed and this is why we must not check the parents IMHO, what you can't relax is the returned type contract, if the result is not null then you cant return null but if you accept a not null param it is ok to accept a null param since you do not break any parent use cases, you enable more.

I think it is better to have false positives than false negatives

well there are multiple things there:

1. we speak about hundreds (thousands) vs dozens so it is not a small thing - the bval filter is there for that, also note it applies to applications not using bval at all or for a few beans
2. it is not only about "false" detection but also about not possible use cases (relaxing a param must be possible and is enabled by cdi api so bval-cdi must respect it)

while using pure reflection it is ok-ish since everything is kind of immutable, using cdi where all the model is mutable this is very limiting to do it so I wouldn't even try

maybe worse case we use a system prop disabled by default and enabled in tck module but I would really make the extension fast, detection is already not crazy fast due to the spec rules which doesn't enforce an interceptor binding IIRC (if done it wouldn't be a real issue)

[jungm]

note this wasn't originally about a TCK issue but something tomee users noted; https://issues.apache.org/jira/projects/TOMEE/issues/TOMEE-4449

[rmannibucau]

maybe a compromise is to do it only for explicit methods and skip it intentionally for all others

so something like:

if (ValidateOnExecution is none on type) { return ;}
foreach (potentiallyexecutable : type.getall()) {
  // if explicit only handle *this* method in parents - note that we cache parents in a lazily initialized list to not recompute it twice if not needed, maybe something like Map<MethodSignature, List<Executable>/*in parents only*/>)
  model = ...;
  if (ValidateOnExecution) { model = handleoverrideparent(model, potentiallyexecutale); }
  handleModel(model);
}

means all the totally implicit (no ValidateOnExecution) cases will be skipped as before, but also means the explicit cases will work as intended with almost no penalty

[guewin]

If I may add my 2 cents.
I would also interpret the spec in the same way as @rmannibucau , in essence, Liskovs Principle has to be followed and precondition weakening should be possible. Thus, for retaining an interface method constraint, the implementation also has to declare the very same constraint, otherwise its a valid constraint weakening and the parent constraint should be ignored.
The rules from §5.6.5 seem to be ambiguous or incomplete for this case, and should be clarified.

In this regard, issues like https://issues.apache.org/jira/projects/TOMEE/issues/TOMEE-4449 seem to be not-spec-compliant.

How about a configurable switch? Like already proposed in https://issues.apache.org/jira/projects/BVAL/issues/BVAL-175.
It seems like BVal considered the parent method constraints till version 2.0.2, so keeping this behaviour configurable, but have the spec compliant way as default, i.e. parent constraints are not considered to support the weakening case. Since both ways also differ in performance, a switch looks like a good compromise, so that users can choose what they prefer.