Skip to content

Rely more on AnnotedTypes in CDI Extension again #191

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jungm wants to merge 1 commit into
base: master
Choose a base branch
from
+28 −37
Open

Rely more on AnnotedTypes in CDI Extension again #191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 27 additions & 35 deletions bval-jsr/src/main/java/org/apache/bval/cdi/BValExtension.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,8 @@
package org.apache.bval.cdi;

import java.lang.annotation.Annotation;
import java.lang.reflect.AnnotatedElement;
import java.lang.reflect.Executable;
import java.lang.reflect.Modifier;
import java.lang.reflect.Parameter;
import java.lang.reflect.Type;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
Expand All @@ -39,6 +35,11 @@
import jakarta.enterprise.event.Observes;
import jakarta.enterprise.inject.spi.AfterBeanDiscovery;
import jakarta.enterprise.inject.spi.AfterDeploymentValidation;
import jakarta.enterprise.inject.spi.Annotated;
import jakarta.enterprise.inject.spi.AnnotatedCallable;
import jakarta.enterprise.inject.spi.AnnotatedConstructor;
import jakarta.enterprise.inject.spi.AnnotatedMethod;
import jakarta.enterprise.inject.spi.AnnotatedParameter;
import jakarta.enterprise.inject.spi.AnnotatedType;
import jakarta.enterprise.inject.spi.Bean;
import jakarta.enterprise.inject.spi.BeanManager;
Expand Down Expand Up @@ -70,7 +71,8 @@
public class BValExtension implements Extension {
private static final Logger LOGGER = Logger.getLogger(BValExtension.class.getName());

public static final AnnotatedTypeFilter DEFAULT_ANNOTATED_TYPE_FILTER =
// protected so can be accessed in tests
protected static final AnnotatedTypeFilter DEFAULT_ANNOTATED_TYPE_FILTER =
annotatedType -> !annotatedType.getJavaClass().getName().startsWith("org.apache.bval.");

private static AnnotatedTypeFilter annotatedTypeFilter = DEFAULT_ANNOTATED_TYPE_FILTER;
Expand Down Expand Up @@ -119,7 +121,7 @@ public void addBvalBinding(final @Observes BeforeBeanDiscovery beforeBeanDiscove
}

// @WithAnnotations(ValidateOnExecution.class) doesn't check interfaces so not enough
public <A> void processAnnotatedType(final @Observes ProcessAnnotatedType<A> pat) {
public <A> void processAnnotatedType(final @Observes ProcessAnnotatedType<A> pat, BeanManager beanManager) {
if (!isExecutableValidationEnabled) {
return;
}
Expand All @@ -132,28 +134,31 @@ public <A> void processAnnotatedType(final @Observes ProcessAnnotatedType<A> pat
final int modifiers = javaClass.getModifiers();
if (!javaClass.isInterface() && !javaClass.isAnonymousClass() && !Modifier.isFinal(modifiers) && !Modifier.isAbstract(modifiers)) {
try {
Queue<Class<?>> toProcess = new LinkedList<>();
toProcess.add(annotatedType.getJavaClass());
Queue<AnnotatedType<?>> toProcess = new LinkedList<>();
toProcess.add(annotatedType);

while (!toProcess.isEmpty()) {
Class<?> now = toProcess.poll();
Executable[] methods = now.getMethods();
Executable[] constructors = now.getConstructors();
AnnotatedType<?> now = toProcess.poll();
Set<? extends AnnotatedMethod<?>> methods = now.getMethods();
Set<? extends AnnotatedConstructor<?>> constructors = now.getConstructors();

if (hasValidation(now)
|| hasValidation(methods) || hasValidation(constructors)
|| hasParamsWithValidation(methods) || hasParamsWithValidation(constructors)) {
|| methods.stream().anyMatch(this::hasValidation)
|| constructors.stream().anyMatch(this::hasValidation)
|| methods.stream().anyMatch(this::hasParamsWithValidation)
|| constructors.stream().anyMatch(this::hasParamsWithValidation)) {
pat.setAnnotatedType(new BValAnnotatedType<>(annotatedType));

break;
}

// Nothing found, collect superclass/interface and repeat (See BVAL-222)
if (now.getSuperclass() != Object.class && now.getSuperclass() != null) {
toProcess.add(now.getSuperclass());
Class<?> superclass = now.getJavaClass().getSuperclass();
Copy link
Contributor

@rmannibucau rmannibucau May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jungm jungm May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@rmannibucau rmannibucau May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@jungm jungm May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@rmannibucau rmannibucau May 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

@jungm jungm May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@rmannibucau rmannibucau May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

@jungm jungm May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@rmannibucau rmannibucau May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@guewin guewin May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (superclass != Object.class && superclass != null) {
toProcess.add(beanManager.createAnnotatedType(superclass));
}

toProcess.addAll(Arrays.asList(now.getInterfaces()));
for (Class<?> iface : now.getJavaClass().getInterfaces()) {
toProcess.add(beanManager.createAnnotatedType(iface));
}
}
} catch (final Exception e) {
if (e instanceof ValidationException) {
Expand All @@ -165,7 +170,6 @@ public <A> void processAnnotatedType(final @Observes ProcessAnnotatedType<A> pat
}
}
}

public <A> void processBean(final @Observes ProcessBean<A> processBeanEvent) {
if (validatorFound && validatorFactoryFound) {
return;
Expand Down Expand Up @@ -220,29 +224,17 @@ public void afterStart(@Observes final AfterDeploymentValidation clearEvent) {
notBValAnnotation.clear();
}

private boolean hasValidation(final AnnotatedElement[] elements) {
for (AnnotatedElement element : elements) {
if (hasValidation(element)) {
private boolean hasParamsWithValidation(AnnotatedCallable<?> callable) {
for (AnnotatedParameter<?> param : callable.getParameters()) {
if (hasValidation(param)) {
return true;
}
}

return false;
}

private boolean hasParamsWithValidation(Executable[] executables) {
for (Executable executable : executables) {
for (Parameter param : executable.getParameters()) {
if (hasValidation(param)) {
return true;
}
}
}

return false;
}

private boolean hasValidation(final AnnotatedElement element) {
private boolean hasValidation(final Annotated element) {
for (Annotation annotation : element.getAnnotations()) {
final Class<? extends Annotation> type = annotation.annotationType();
if (type == ValidateOnExecution.class || type == Valid.class) {
Expand Down
3 changes: 1 addition & 2 deletions ...i/CdiConstraintOnlyOnParentClassTest.java → ...i/CdiConstraintOnlyOnParentClassTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,12 @@
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.bval.jsr.cdi;
package org.apache.bval.cdi;

import jakarta.enterprise.context.ApplicationScoped;
import jakarta.inject.Inject;
import jakarta.validation.ConstraintViolationException;
import jakarta.validation.constraints.NotNull;
import org.apache.bval.cdi.BValExtension;
import org.jboss.arquillian.container.test.api.Deployment;
import org.jboss.arquillian.junit.Arquillian;
import org.jboss.shrinkwrap.api.ShrinkWrap;
Expand Down