Skip to content

Commit

Permalink
ArC: fix inheritance of interceptor binding annotations
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Ladicek committed May 23, 2023
1 parent d42f912 commit f5a8f7d
Show file tree
Hide file tree
Showing 6 changed files with 187 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -797,7 +797,7 @@ private void putLifecycleInterceptors(Map<InterceptionType, InterceptionInfo> li

private void addClassLevelBindings(ClassInfo targetClass, Collection<AnnotationInstance> bindings) {
List<AnnotationInstance> 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
Expand All @@ -808,22 +808,27 @@ private void addClassLevelBindings(ClassInfo targetClass, Collection<AnnotationI
}
for (StereotypeInfo stereotype : Beans.stereotypesWithTransitive(stereotypes,
beanDeployment.getStereotypesMap())) {
doAddClassLevelBindings(stereotype.getTarget(), bindings, skip);
doAddClassLevelBindings(stereotype.getTarget(), bindings, skip, false);
}
}
}

// bindings whose class name is present in `skip` are ignored (this is used to ignore bindings on stereotypes
// when the original class has a binding of the same type)
private void doAddClassLevelBindings(ClassInfo classInfo, Collection<AnnotationInstance> bindings, Set<DotName> skip) {
private void doAddClassLevelBindings(ClassInfo classInfo, Collection<AnnotationInstance> bindings, Set<DotName> 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);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,18 +183,20 @@ private static Set<MethodInfo> 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<AnnotationInstance> 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 {
Expand All @@ -203,7 +205,7 @@ private static Set<MethodInfo> addInterceptedMethodCandidates(BeanDeployment bea
}
}
if (addToCandidates) {
candidates.computeIfAbsent(new Methods.MethodKey(method), key -> bindings);
candidates.putIfAbsent(key, bindings);
}
}
skipPredicate.methodsProcessed();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,6 @@
<exclude name="testDestroyForSameCreationalContextOnly"/>
</methods>
</class>
<class name="org.jboss.cdi.tck.tests.interceptors.definition.inheritance.InterceptorBindingInheritanceTest">
<methods>
<exclude name="testInterceptorBindingDirectlyInheritedFromManagedBean"/>
<exclude name="testMethodInterceptorBindingDirectlyInheritedFromManagedBean"/>
<exclude name="testMethodInterceptorBindingDirectlyNotInheritedFromManagedBean"/>
<exclude name="testMethodInterceptorBindingIndirectlyInheritedFromManagedBean"/>
<exclude name="testInterceptorBindingIndirectlyInheritedFromManagedBean"/>
<exclude name="testMethodInterceptorBindingIndirectlyNotInheritedFromManagedBean"/>
</methods>
</class>
<class name="org.jboss.cdi.tck.tests.lookup.injectionpoint.InjectionPointTest">
<methods>
<!-- https://github.com/jakartaee/cdi-tck/issues/459 -->
Expand Down
Original file line number Diff line number Diff line change
@@ -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();
}
}
}
Original file line number Diff line number Diff line change
@@ -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();
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -15,5 +16,6 @@
@Retention(RUNTIME)
@Documented
@InterceptorBinding
@Inherited
public @interface InheritedClassLevel {
}

0 comments on commit f5a8f7d

Please sign in to comment.