From f5a8f7d58a8fcf1ef4a4aaf4fc2854d0c6eb95b7 Mon Sep 17 00:00:00 2001 From: Ladislav Thon Date: Fri, 3 Mar 2023 18:08:36 +0100 Subject: [PATCH] ArC: fix inheritance of interceptor binding annotations Class-level interceptor bindings declared on a superclass should only be taken into account when they are `@Inherited`. ArC however inherits all interceptor bindings from superclasses, even if they are _not_ declared `@Inherited`. This commit fixes that in the strict mode, to not affect Quarkus security (which assumes that security annotations are inherited, even if they are not). Method-level annotations are never inherited. This means that the lowest override of a method determines whether method-level interceptor binding annotations are present. ArC used to behave differently. If a method on a class had no interceptor binding annotations but overrode a method from a superclass that _did_ have interceptor binding annotations, the method-level annotations from the superclass were inherited. This commit fixes that. --- .../io/quarkus/arc/processor/BeanInfo.java | 13 ++- .../io/quarkus/arc/processor/Methods.java | 14 +-- .../src/test/resources/testng.xml | 10 -- .../inherited/InheritedBindingOnBeanTest.java | 96 +++++++++++++++++++ ...itedMethodsWithInterceptorBindingTest.java | 72 ++++++++++++++ .../InheritedClassLevel.java | 2 + 6 files changed, 187 insertions(+), 20 deletions(-) create mode 100644 independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/bindings/inherited/InheritedBindingOnBeanTest.java create mode 100644 independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/bindings/inherited/InheritedMethodsWithInterceptorBindingTest.java diff --git a/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanInfo.java b/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanInfo.java index 160e9cb46cba8e..9e904309f8882a 100644 --- a/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanInfo.java +++ b/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanInfo.java @@ -797,7 +797,7 @@ private void putLifecycleInterceptors(Map li private void addClassLevelBindings(ClassInfo targetClass, Collection bindings) { List classLevelBindings = new ArrayList<>(); - doAddClassLevelBindings(targetClass, classLevelBindings, Set.of()); + doAddClassLevelBindings(targetClass, classLevelBindings, Set.of(), false); bindings.addAll(classLevelBindings); if (!stereotypes.isEmpty()) { // interceptor binding declared on a bean class replaces an interceptor binding of the same type @@ -808,22 +808,27 @@ private void addClassLevelBindings(ClassInfo targetClass, Collection bindings, Set skip) { + private void doAddClassLevelBindings(ClassInfo classInfo, Collection bindings, Set skip, + boolean onlyInherited) { beanDeployment.getAnnotations(classInfo).stream() .filter(a -> beanDeployment.getInterceptorBinding(a.name()) != null) .filter(a -> !skip.contains(a.name())) + .filter(a -> !onlyInherited + || beanDeployment.hasAnnotation(beanDeployment.getInterceptorBinding(a.name()), DotNames.INHERITED)) .forEach(bindings::add); if (classInfo.superClassType() != null && !classInfo.superClassType().name().equals(DotNames.OBJECT)) { ClassInfo superClass = getClassByName(beanDeployment.getBeanArchiveIndex(), classInfo.superName()); if (superClass != null) { - doAddClassLevelBindings(superClass, bindings, skip); + // proper interceptor binding inheritance only in strict mode, due to Quarkus expecting security + // annotations (such as `@RolesAllowed`) to be inherited, even though they are not `@Inherited` + doAddClassLevelBindings(superClass, bindings, skip, beanDeployment.strictCompatibility); } } } diff --git a/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/Methods.java b/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/Methods.java index c6c3c7e4df94a4..5f57163fe681e4 100644 --- a/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/Methods.java +++ b/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/Methods.java @@ -183,18 +183,20 @@ private static Set addInterceptedMethodCandidates(BeanDeployment bea skipPredicate.startProcessing(classInfo, originalClassInfo); for (MethodInfo method : classInfo.methods()) { + MethodKey key = new MethodKey(method); + if (candidates.containsKey(key)) { + continue; + } + // Note that we must merge the bindings first Set bindings = mergeBindings(beanDeployment, originalClassInfo, classLevelBindings, ignoreMethodLevelBindings, method, noClassInterceptorsMethods); - if (bindings.isEmpty() && !targetHasAroundInvokes) { - // No bindings found and target class does not declare around invoke interceptor methods - continue; - } + boolean possiblyIntercepted = !bindings.isEmpty() || targetHasAroundInvokes; if (skipPredicate.test(method)) { continue; } boolean addToCandidates = true; - if (Modifier.isFinal(method.flags())) { + if (Modifier.isFinal(method.flags()) && possiblyIntercepted) { if (transformUnproxyableClasses && !isNoninterceptableKotlinMethod(method)) { methodsFromWhichToRemoveFinal.add(NameAndDescriptor.fromMethodInfo(method)); } else { @@ -203,7 +205,7 @@ private static Set addInterceptedMethodCandidates(BeanDeployment bea } } if (addToCandidates) { - candidates.computeIfAbsent(new Methods.MethodKey(method), key -> bindings); + candidates.putIfAbsent(key, bindings); } } skipPredicate.methodsProcessed(); diff --git a/independent-projects/arc/tcks/cdi-tck-runner/src/test/resources/testng.xml b/independent-projects/arc/tcks/cdi-tck-runner/src/test/resources/testng.xml index 08f5a9f8f05d12..bd5e206ab1c720 100644 --- a/independent-projects/arc/tcks/cdi-tck-runner/src/test/resources/testng.xml +++ b/independent-projects/arc/tcks/cdi-tck-runner/src/test/resources/testng.xml @@ -52,16 +52,6 @@ - - - - - - - - - - diff --git a/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/bindings/inherited/InheritedBindingOnBeanTest.java b/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/bindings/inherited/InheritedBindingOnBeanTest.java new file mode 100644 index 00000000000000..6f08c6cf413891 --- /dev/null +++ b/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/bindings/inherited/InheritedBindingOnBeanTest.java @@ -0,0 +1,96 @@ +package io.quarkus.arc.test.interceptors.bindings.inherited; + +import static java.lang.annotation.ElementType.METHOD; +import static java.lang.annotation.ElementType.TYPE; +import static java.lang.annotation.RetentionPolicy.RUNTIME; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.lang.annotation.Documented; +import java.lang.annotation.Inherited; +import java.lang.annotation.Retention; +import java.lang.annotation.Target; + +import jakarta.annotation.Priority; +import jakarta.inject.Singleton; +import jakarta.interceptor.AroundInvoke; +import jakarta.interceptor.Interceptor; +import jakarta.interceptor.InterceptorBinding; +import jakarta.interceptor.InvocationContext; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.quarkus.arc.Arc; +import io.quarkus.arc.test.ArcTestContainer; + +public class InheritedBindingOnBeanTest { + @RegisterExtension + public ArcTestContainer container = new ArcTestContainer.Builder() + .beanClasses(MyBean.class, FooBinding.class, BarBinding.class, FooInterceptor.class, BarInterceptor.class) + .strictCompatibility(true) // correct interceptor binding inheritance + .build(); + + @Test + public void testInterception() { + MyBean bean = Arc.container().instance(MyBean.class).get(); + assertNotNull(bean); + bean.doSomething(); + assertTrue(FooInterceptor.intercepted); + assertFalse(BarInterceptor.intercepted); + } + + @FooBinding + @BarBinding + static class MySuperclass { + } + + @Singleton + static class MyBean extends MySuperclass { + void doSomething() { + } + } + + @Target({ TYPE, METHOD }) + @Retention(RUNTIME) + @Documented + @InterceptorBinding + @Inherited + @interface FooBinding { + } + + @Target({ TYPE, METHOD }) + @Retention(RUNTIME) + @Documented + @InterceptorBinding + // not @Inherited + @interface BarBinding { + } + + @FooBinding + @Interceptor + @Priority(1) + static class FooInterceptor { + static boolean intercepted = false; + + @AroundInvoke + Object intercept(InvocationContext ctx) throws Exception { + intercepted = true; + return ctx.proceed(); + } + } + + @BarBinding + @Interceptor + @Priority(1) + static class BarInterceptor { + static boolean intercepted = false; + + @AroundInvoke + Object intercept(InvocationContext ctx) throws Exception { + intercepted = true; + return ctx.proceed(); + } + } +} diff --git a/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/bindings/inherited/InheritedMethodsWithInterceptorBindingTest.java b/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/bindings/inherited/InheritedMethodsWithInterceptorBindingTest.java new file mode 100644 index 00000000000000..590648eaba2de3 --- /dev/null +++ b/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/bindings/inherited/InheritedMethodsWithInterceptorBindingTest.java @@ -0,0 +1,72 @@ +package io.quarkus.arc.test.interceptors.bindings.inherited; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +import jakarta.annotation.Priority; +import jakarta.enterprise.context.Dependent; +import jakarta.interceptor.AroundInvoke; +import jakarta.interceptor.Interceptor; +import jakarta.interceptor.InterceptorBinding; +import jakarta.interceptor.InvocationContext; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.quarkus.arc.Arc; +import io.quarkus.arc.test.ArcTestContainer; + +public class InheritedMethodsWithInterceptorBindingTest { + @RegisterExtension + ArcTestContainer container = new ArcTestContainer(MyBean.class, MyInterceptorBinding.class, MyInterceptor.class); + + @Test + public void test() { + MyBean bean = Arc.container().instance(MyBean.class).get(); + assertEquals("foobar", bean.foobar()); + assertEquals("intercepted: foobar", bean.foobarNotInherited()); + } + + static class MySuperclass { + @MyInterceptorBinding + String foobar() { + return "this should be ignored"; + } + + @MyInterceptorBinding + String foobarNotInherited() { + return "foobar"; + } + } + + @Dependent + static class MyBean extends MySuperclass { + @Override + String foobar() { + return "foobar"; + } + } + + @Target({ ElementType.TYPE, ElementType.METHOD }) + @Retention(RetentionPolicy.RUNTIME) + @Documented + @InterceptorBinding + @interface MyInterceptorBinding { + } + + @MyInterceptorBinding + @Priority(1) + @Interceptor + static class MyInterceptor { + @AroundInvoke + public Object intercept(InvocationContext ctx) throws Exception { + return "intercepted: " + ctx.proceed(); + } + } + +} diff --git a/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/noclassinterceptors/InheritedClassLevel.java b/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/noclassinterceptors/InheritedClassLevel.java index a844883ff8152c..299571408e9f16 100644 --- a/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/noclassinterceptors/InheritedClassLevel.java +++ b/independent-projects/arc/tests/src/test/java/io/quarkus/arc/test/interceptors/noclassinterceptors/InheritedClassLevel.java @@ -6,6 +6,7 @@ import static java.lang.annotation.RetentionPolicy.RUNTIME; import java.lang.annotation.Documented; +import java.lang.annotation.Inherited; import java.lang.annotation.Retention; import java.lang.annotation.Target; @@ -15,5 +16,6 @@ @Retention(RUNTIME) @Documented @InterceptorBinding +@Inherited public @interface InheritedClassLevel { }