Skip to content

Commit

Permalink
ArC: add tests for business method interceptors on target classes and…
Browse files Browse the repository at this point in the history
… fix some issues

Co-authored-by: Martin Kouba <[email protected]>
  • Loading branch information
Ladicek and mkouba committed Apr 20, 2023
1 parent f181c27 commit 1adf1b7
Show file tree
Hide file tree
Showing 13 changed files with 604 additions and 44 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -619,8 +619,8 @@ private Map<MethodInfo, InterceptionInfo> initInterceptedMethods(List<Throwable>
}
}

Set<MethodInfo> finalMethods = Methods.addInterceptedMethodCandidates(beanDeployment, target.get().asClass(),
candidates, classLevelBindings, bytecodeTransformerConsumer, transformUnproxyableClasses, aroundInvokes);
Set<MethodInfo> finalMethods = Methods.addInterceptedMethodCandidates(this, candidates, classLevelBindings,
bytecodeTransformerConsumer, transformUnproxyableClasses);
if (!finalMethods.isEmpty()) {
String additionalError = "";
if (finalMethods.stream().anyMatch(KotlinUtils::isNoninterceptableKotlinMethod)) {
Expand Down Expand Up @@ -668,7 +668,8 @@ private Map<MethodInfo, DecorationInfo> initDecoratedMethods() {
ClassInfo classInfo = target.get().asClass();
addDecoratedMethods(candidates, classInfo, classInfo, bound,
new SubclassSkipPredicate(beanDeployment.getAssignabilityCheck()::isAssignableFrom,
beanDeployment.getBeanArchiveIndex(), beanDeployment.getObserverAndProducerMethods()));
beanDeployment.getBeanArchiveIndex(), beanDeployment.getObserverAndProducerMethods(),
beanDeployment.getAnnotationStore()));

Map<MethodInfo, DecorationInfo> decoratedMethods = new HashMap<>(candidates.size());
for (Entry<MethodKey, DecorationInfo> entry : candidates.entrySet()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -650,24 +650,25 @@ static List<MethodInfo> getCallbacks(ClassInfo beanClass, DotName annotation, In
}

static List<MethodInfo> getAroundInvokes(ClassInfo beanClass, BeanDeployment deployment) {
List<MethodInfo> methods = new ArrayList<>();
AnnotationStore store = deployment.getAnnotationStore();
Set<String> processed = new HashSet<>();
List<MethodInfo> methods = new ArrayList<>();

List<MethodInfo> allMethods = new ArrayList<>();
ClassInfo aClass = beanClass;
while (aClass != null) {
int aroundInvokesFound = 0;
for (MethodInfo method : aClass.methods()) {
if (Modifier.isStatic(method.flags())) {
continue;
}
if (store.hasAnnotation(method, DotNames.AROUND_INVOKE) && !processed.contains(method.name())) {
methods.add(method);
if (store.hasAnnotation(method, DotNames.AROUND_INVOKE)) {
InterceptorInfo.addInterceptorMethod(allMethods, methods, method);
if (++aroundInvokesFound > 1) {
throw new DefinitionException(
"Multiple @AroundInvoke interceptor methods declared on class: " + aClass);
}
}
allMethods.add(method);
}
DotName superTypeName = aClass.superName();
aClass = superTypeName == null || DotNames.OBJECT.equals(superTypeName) ? null
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,28 +247,30 @@ public int compareTo(InterceptorInfo other) {
return getTarget().toString().compareTo(other.getTarget().toString());
}

private void addInterceptorMethod(List<MethodInfo> allMethods, List<MethodInfo> interceptorMethods, MethodInfo method) {
static void addInterceptorMethod(List<MethodInfo> allMethods, List<MethodInfo> interceptorMethods, MethodInfo method) {
validateSignature(method);
if (!isInterceptorMethodOverriden(allMethods, method)) {
interceptorMethods.add(method);
}
}

private boolean isInterceptorMethodOverriden(Iterable<MethodInfo> allMethods, MethodInfo method) {
static boolean isInterceptorMethodOverriden(Iterable<MethodInfo> allMethods, MethodInfo method) {
for (MethodInfo m : allMethods) {
if (m.name().equals(method.name()) && m.parametersCount() == 1
&& (m.parameterType(0).name().equals(DotNames.INVOCATION_CONTEXT)
|| m.parameterType(0).name().equals(DotNames.ARC_INVOCATION_CONTEXT))) {
if (m.name().equals(method.name()) && hasInterceptorMethodParameter(m)) {
return true;
}
}
return false;
}

private MethodInfo validateSignature(MethodInfo method) {
List<Type> parameters = method.parameterTypes();
if (parameters.size() != 1 || !(parameters.get(0).name().equals(DotNames.INVOCATION_CONTEXT)
|| parameters.get(0).name().equals(DotNames.ARC_INVOCATION_CONTEXT))) {
static boolean hasInterceptorMethodParameter(MethodInfo method) {
return method.parametersCount() == 1
&& (method.parameterType(0).name().equals(DotNames.INVOCATION_CONTEXT)
|| method.parameterType(0).name().equals(DotNames.ARC_INVOCATION_CONTEXT));
}

private static MethodInfo validateSignature(MethodInfo method) {
if (!hasInterceptorMethodParameter(method)) {
throw new IllegalStateException(
"An interceptor method must accept exactly one parameter of type jakarta.interceptor.InvocationContext: "
+ method + " declared on " + method.declaringClass());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ final class Methods {
public static final String TO_STRING = "toString";

static final Set<String> IGNORED_METHODS = Set.of(INIT, CLINIT);
static final List<DotName> OBSERVER_PRODUCER_ANNOTATIONS = List.of(DotNames.OBSERVES, DotNames.OBSERVES_ASYNC,
DotNames.PRODUCES);

private Methods() {
}
Expand Down Expand Up @@ -149,36 +151,40 @@ static boolean isObjectToString(MethodInfo method) {
return method.declaringClass().name().equals(DotNames.OBJECT) && method.name().equals(TO_STRING);
}

static Set<MethodInfo> addInterceptedMethodCandidates(BeanDeployment beanDeployment, ClassInfo classInfo,
static Set<MethodInfo> addInterceptedMethodCandidates(BeanInfo bean,
Map<MethodKey, Set<AnnotationInstance>> candidates,
List<AnnotationInstance> classLevelBindings, Consumer<BytecodeTransformer> bytecodeTransformerConsumer,
boolean transformUnproxyableClasses, List<MethodInfo> aroundInvokes) {
boolean transformUnproxyableClasses) {
BeanDeployment beanDeployment = bean.getDeployment();
ClassInfo classInfo = bean.getTarget().get().asClass();
return addInterceptedMethodCandidates(beanDeployment, classInfo, classInfo, candidates, Set.copyOf(classLevelBindings),
bytecodeTransformerConsumer, transformUnproxyableClasses,
new SubclassSkipPredicate(beanDeployment.getAssignabilityCheck()::isAssignableFrom,
beanDeployment.getBeanArchiveIndex(), beanDeployment.getObserverAndProducerMethods()),
false, new HashSet<>(), aroundInvokes);
beanDeployment.getBeanArchiveIndex(), beanDeployment.getObserverAndProducerMethods(),
beanDeployment.getAnnotationStore()),
false, new HashSet<>(), bean.hasAroundInvokes());
}

private static Set<MethodInfo> addInterceptedMethodCandidates(BeanDeployment beanDeployment, ClassInfo classInfo,
ClassInfo originalClassInfo,
Map<MethodKey, Set<AnnotationInstance>> candidates,
Set<AnnotationInstance> classLevelBindings, Consumer<BytecodeTransformer> bytecodeTransformerConsumer,
boolean transformUnproxyableClasses, SubclassSkipPredicate skipPredicate, boolean ignoreMethodLevelBindings,
Set<MethodKey> noClassInterceptorsMethods, List<MethodInfo> aroundInvokes) {
Set<MethodKey> noClassInterceptorsMethods, boolean targetHasAroundInvokes) {

Set<NameAndDescriptor> methodsFromWhichToRemoveFinal = new HashSet<>();
Set<MethodInfo> finalMethodsFoundAndNotChanged = new HashSet<>();
skipPredicate.startProcessing(classInfo, originalClassInfo);

for (MethodInfo method : classInfo.methods()) {
if (aroundInvokes.contains(method)) {
// Around invoke method declared in the target class hierarchy
// 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;
}
Set<AnnotationInstance> merged = mergeBindings(beanDeployment, originalClassInfo, classLevelBindings,
ignoreMethodLevelBindings, method, noClassInterceptorsMethods);
if ((merged.isEmpty() && aroundInvokes.isEmpty()) || skipPredicate.test(method)) {
if (skipPredicate.test(method)) {
continue;
}
boolean addToCandidates = true;
Expand All @@ -191,7 +197,7 @@ private static Set<MethodInfo> addInterceptedMethodCandidates(BeanDeployment bea
}
}
if (addToCandidates) {
candidates.computeIfAbsent(new Methods.MethodKey(method), key -> merged);
candidates.computeIfAbsent(new Methods.MethodKey(method), key -> bindings);
}
}
skipPredicate.methodsProcessed();
Expand All @@ -208,7 +214,7 @@ private static Set<MethodInfo> addInterceptedMethodCandidates(BeanDeployment bea
finalMethodsFoundAndNotChanged
.addAll(addInterceptedMethodCandidates(beanDeployment, superClassInfo, classInfo, candidates,
classLevelBindings, bytecodeTransformerConsumer, transformUnproxyableClasses, skipPredicate,
ignoreMethodLevelBindings, noClassInterceptorsMethods, aroundInvokes));
ignoreMethodLevelBindings, noClassInterceptorsMethods, targetHasAroundInvokes));
}
}

Expand All @@ -218,7 +224,7 @@ private static Set<MethodInfo> addInterceptedMethodCandidates(BeanDeployment bea
//interfaces can't have final methods
addInterceptedMethodCandidates(beanDeployment, interfaceInfo, originalClassInfo, candidates,
classLevelBindings, bytecodeTransformerConsumer, transformUnproxyableClasses,
skipPredicate, true, noClassInterceptorsMethods, aroundInvokes);
skipPredicate, true, noClassInterceptorsMethods, targetHasAroundInvokes);
}
}
return finalMethodsFoundAndNotChanged;
Expand Down Expand Up @@ -274,9 +280,7 @@ private static Set<AnnotationInstance> mergeBindings(BeanDeployment beanDeployme
}

if (Modifier.isPrivate(method.flags())
&& !Annotations.contains(methodAnnotations, DotNames.PRODUCES)
&& !Annotations.contains(methodAnnotations, DotNames.OBSERVES)
&& !Annotations.contains(methodAnnotations, DotNames.OBSERVES_ASYNC)) {
&& !Annotations.containsAny(methodAnnotations, OBSERVER_PRODUCER_ANNOTATIONS)) {
String message;
if (methodLevelBindings.size() == 1) {
message = String.format("%s will have no effect on method %s.%s() because the method is private.",
Expand Down Expand Up @@ -442,24 +446,28 @@ public MethodVisitor visitMethod(int access, String name, String descriptor, Str
* This stateful predicate can be used to skip methods that should not be added to the generated subclass.
* <p>
* Don't forget to call {@link SubclassSkipPredicate#startProcessing(ClassInfo, ClassInfo)} before the methods are processed
* and
* {@link SubclassSkipPredicate#methodsProcessed()} afterwards.
* and {@link SubclassSkipPredicate#methodsProcessed()} afterwards.
*/
static class SubclassSkipPredicate implements Predicate<MethodInfo> {

private static final List<DotName> INTERCEPTOR_ANNOTATIONS = List.of(DotNames.AROUND_INVOKE, DotNames.POST_CONSTRUCT,
DotNames.PRE_DESTROY);

private final BiFunction<Type, Type, Boolean> assignableFromFun;
private final IndexView beanArchiveIndex;
private final Set<MethodInfo> producersAndObservers;
private final AnnotationStore annotationStore;
private ClassInfo clazz;
private ClassInfo originalClazz;
private List<MethodInfo> regularMethods;
private Set<MethodInfo> bridgeMethods = new HashSet<>();

public SubclassSkipPredicate(BiFunction<Type, Type, Boolean> assignableFromFun, IndexView beanArchiveIndex,
Set<MethodInfo> producersAndObservers) {
Set<MethodInfo> producersAndObservers, AnnotationStore annotationStore) {
this.assignableFromFun = assignableFromFun;
this.beanArchiveIndex = beanArchiveIndex;
this.producersAndObservers = producersAndObservers;
this.annotationStore = annotationStore;
}

void startProcessing(ClassInfo clazz, ClassInfo originalClazz) {
Expand Down Expand Up @@ -487,28 +495,34 @@ public boolean test(MethodInfo method) {
// Skip bridge methods that have a corresponding "implementation method" on the same class
// The algorithm we use to detect these methods is best effort, i.e. there might be use cases where the detection fails
return hasImplementation(method);
} else if (method.isSynthetic()) {
}
if (method.isSynthetic()) {
// Skip non-bridge synthetic methods
return true;
}
if (Modifier.isPrivate(method.flags()) && !producersAndObservers.contains(method)) {
// Skip a private method that is not and observer or producer
return true;
}
if (method.hasAnnotation(DotNames.POST_CONSTRUCT) || method.hasAnnotation(DotNames.PRE_DESTROY)) {
// @PreDestroy and @PostConstruct methods declared on the bean are NOT candidates for around invoke interception
if (Modifier.isStatic(method.flags())) {
return true;
}
if (isOverridenByBridgeMethod(method)) {
if (IGNORED_METHODS.contains(method.name())) {
return true;
}
if (Modifier.isStatic(method.flags())) {
if (method.declaringClass().name().equals(DotNames.OBJECT)) {
return true;
}
if (IGNORED_METHODS.contains(method.name())) {
if (annotationStore.hasAnyAnnotation(method, INTERCEPTOR_ANNOTATIONS)) {
// @AroundInvoke, @PreDestroy and @PostConstruct methods declared on the bean are NOT candidates for around invoke interception
return true;
}
if (method.declaringClass().name().equals(DotNames.OBJECT)) {
if (InterceptorInfo.hasInterceptorMethodParameter(method)
&& InterceptorInfo.isInterceptorMethodOverriden(regularMethods, method)) {
// Has exactly one param InvocationContext/ArcInvocationContext and is overriden
return true;
}
if (isOverridenByBridgeMethod(method)) {
return true;
}
if (Modifier.isInterface(clazz.flags()) && Modifier.isInterface(method.declaringClass().flags())
Expand All @@ -527,7 +541,7 @@ public boolean test(MethodInfo method) {
}
DotName typeName = type.name();
if (type.kind() == Kind.ARRAY) {
Type componentType = type.asArrayType().component();
Type componentType = type.asArrayType().constituent();
if (componentType.kind() == Kind.PRIMITIVE) {
continue;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public void testPredicate() throws IOException {
IndexView index = Index.of(Base.class, Submarine.class, Long.class, Number.class);
AssignabilityCheck assignabilityCheck = new AssignabilityCheck(index, null);
SubclassSkipPredicate predicate = new SubclassSkipPredicate(assignabilityCheck::isAssignableFrom, null,
Collections.emptySet());
Collections.emptySet(), new AnnotationStore(Collections.emptyList(), null));

ClassInfo submarineClass = index.getClassByName(DotName.createSimple(Submarine.class.getName()));
predicate.startProcessing(submarineClass, submarineClass);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package io.quarkus.arc.test.interceptors.targetclass;

import static org.junit.jupiter.api.Assertions.assertEquals;

import jakarta.inject.Singleton;
import jakarta.interceptor.AroundInvoke;
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.ArcContainer;
import io.quarkus.arc.test.ArcTestContainer;

public class AroundInvokeOnTargetClassAndManySuperclassesWithOverridesTest {
@RegisterExtension
public ArcTestContainer container = new ArcTestContainer(MyBean.class);

@Test
public void test() {
ArcContainer arc = Arc.container();
MyBean bean = arc.instance(MyBean.class).get();
assertEquals("super-intercepted: intercepted: foobar", bean.doSomething(42));
}

static class Alpha {
@AroundInvoke
Object intercept(InvocationContext ctx) throws Exception {
return "this should not be called as the method is overridden in MyBean";
}
}

static class Bravo extends Alpha {
@AroundInvoke
Object specialIntercept(InvocationContext ctx) throws Exception {
return "this should not be called as the method is overridden in Charlie";
}
}

static class Charlie extends Bravo {
@AroundInvoke
Object superIntercept(InvocationContext ctx) throws Exception {
return "super-intercepted: " + ctx.proceed();
}

@Override
Object specialIntercept(InvocationContext ctx) {
return "this is not an interceptor method";
}
}

@Singleton
static class MyBean extends Charlie {
String doSomething(int param) {
return "foobar";
}

@AroundInvoke
Object intercept(InvocationContext ctx) throws Exception {
return "intercepted: " + ctx.proceed();
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package io.quarkus.arc.test.interceptors.targetclass;

import static org.junit.jupiter.api.Assertions.assertEquals;

import jakarta.inject.Singleton;
import jakarta.interceptor.AroundInvoke;
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.ArcContainer;
import io.quarkus.arc.test.ArcTestContainer;

public class AroundInvokeOnTargetClassAndSuperclassesTest {
@RegisterExtension
public ArcTestContainer container = new ArcTestContainer(MyBean.class);

@Test
public void test() {
ArcContainer arc = Arc.container();
MyBean bean = arc.instance(MyBean.class).get();
assertEquals("super-intercepted: intercepted: foobar", bean.doSomething());
}

static class MyBeanSuperclass {
@AroundInvoke
Object superIntercept(InvocationContext ctx) throws Exception {
return "super-intercepted: " + ctx.proceed();
}
}

@Singleton
static class MyBean extends MyBeanSuperclass {
String doSomething() {
return "foobar";
}

@AroundInvoke
Object intercept(InvocationContext ctx) throws Exception {
return "intercepted: " + ctx.proceed();
}
}
}
Loading

0 comments on commit 1adf1b7

Please sign in to comment.