Skip to content

Commit

Permalink
Intercepted subclasses generation - fix skipped methods evaluation
Browse files Browse the repository at this point in the history
- do not skip methods with a private param
- also do not evaluate methods that declare no interceptor bindings;
these methods are skipped automatically
- related to #19177

Co-authored-by: Ladislav Thon <[email protected]>
  • Loading branch information
mkouba and Ladicek committed Nov 29, 2021
1 parent ebd6d37 commit de75549
Show file tree
Hide file tree
Showing 3 changed files with 157 additions and 53 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ static Set<MethodInfo> addInterceptedMethodCandidates(BeanDeployment beanDeploym
Map<MethodKey, Set<AnnotationInstance>> candidates,
List<AnnotationInstance> classLevelBindings, Consumer<BytecodeTransformer> bytecodeTransformerConsumer,
boolean transformUnproxyableClasses) {
return addInterceptedMethodCandidates(beanDeployment, classInfo, classInfo, candidates, classLevelBindings,
return addInterceptedMethodCandidates(beanDeployment, classInfo, classInfo, candidates, Set.copyOf(classLevelBindings),
bytecodeTransformerConsumer, transformUnproxyableClasses,
new SubclassSkipPredicate(beanDeployment.getAssignabilityCheck()::isAssignableFrom,
beanDeployment.getBeanArchiveIndex()),
Expand All @@ -169,60 +169,30 @@ static Set<MethodInfo> addInterceptedMethodCandidates(BeanDeployment beanDeploym
static Set<MethodInfo> addInterceptedMethodCandidates(BeanDeployment beanDeployment, ClassInfo classInfo,
ClassInfo originalClassInfo,
Map<MethodKey, Set<AnnotationInstance>> candidates,
List<AnnotationInstance> classLevelBindings, Consumer<BytecodeTransformer> bytecodeTransformerConsumer,
Set<AnnotationInstance> classLevelBindings, Consumer<BytecodeTransformer> bytecodeTransformerConsumer,
boolean transformUnproxyableClasses, SubclassSkipPredicate skipPredicate, boolean ignoreMethodLevelBindings) {

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

for (MethodInfo method : classInfo.methods()) {
if (skipPredicate.test(method)) {
Set<AnnotationInstance> merged = mergeBindings(beanDeployment, originalClassInfo, classLevelBindings,
ignoreMethodLevelBindings, method);
if (merged.isEmpty() || skipPredicate.test(method)) {
continue;
}
Set<AnnotationInstance> merged = new HashSet<>();
if (ignoreMethodLevelBindings) {
merged.addAll(classLevelBindings);
} else {
Collection<AnnotationInstance> methodAnnnotations = beanDeployment.getAnnotations(method);
List<AnnotationInstance> methodLevelBindings = methodAnnnotations.stream()
.flatMap(a -> beanDeployment.extractInterceptorBindings(a).stream())
.collect(Collectors.toList());
if (Modifier.isPrivate(method.flags())
&& !methodLevelBindings.isEmpty()
&& !Annotations.contains(methodAnnnotations, DotNames.OBSERVES)
&& !Annotations.contains(methodAnnnotations, DotNames.OBSERVES_ASYNC)) {
if (methodLevelBindings.size() == 1) {
LOGGER.warnf("%s will have no effect on method %s.%s() because the method is private",
methodLevelBindings.iterator().next(), classInfo.name(), method.name());
} else {
LOGGER.warnf("Annotations %s will have no effect on method %s.%s() because the method is private",
methodLevelBindings.stream().map(AnnotationInstance::toString).collect(Collectors.joining(",")),
classInfo.name(), method.name());
}

}
merged.addAll(methodLevelBindings);
for (AnnotationInstance classLevelBinding : classLevelBindings) {
if (methodLevelBindings.isEmpty()
|| methodLevelBindings.stream().noneMatch(a -> classLevelBinding.name().equals(a.name()))) {
merged.add(classLevelBinding);
}
boolean addToCandidates = true;
if (Modifier.isFinal(method.flags())) {
if (transformUnproxyableClasses) {
methodsFromWhichToRemoveFinal.add(NameAndDescriptor.fromMethodInfo(method));
} else {
addToCandidates = false;
finalMethodsFoundAndNotChanged.add(method);
}
}
if (!merged.isEmpty()) {
boolean addToCandidates = true;
if (Modifier.isFinal(method.flags())) {
if (transformUnproxyableClasses) {
methodsFromWhichToRemoveFinal.add(NameAndDescriptor.fromMethodInfo(method));
} else {
addToCandidates = false;
finalMethodsFoundAndNotChanged.add(method);
}
}
if (addToCandidates) {
candidates.computeIfAbsent(new Methods.MethodKey(method), key -> merged);
}
if (addToCandidates) {
candidates.computeIfAbsent(new Methods.MethodKey(method), key -> merged);
}
}
skipPredicate.methodsProcessed();
Expand Down Expand Up @@ -255,6 +225,49 @@ static Set<MethodInfo> addInterceptedMethodCandidates(BeanDeployment beanDeploym
return finalMethodsFoundAndNotChanged;
}

private static Set<AnnotationInstance> mergeBindings(BeanDeployment beanDeployment, ClassInfo classInfo,
Set<AnnotationInstance> classLevelBindings, boolean ignoreMethodLevelBindings, MethodInfo method) {
if (ignoreMethodLevelBindings) {
return classLevelBindings;
}

Collection<AnnotationInstance> methodAnnotations = beanDeployment.getAnnotations(method);
if (methodAnnotations.isEmpty()) {
// No annotations declared on the method
return classLevelBindings;
}
List<AnnotationInstance> methodLevelBindings = new ArrayList<>();
for (AnnotationInstance annotation : methodAnnotations) {
methodLevelBindings.addAll(beanDeployment.extractInterceptorBindings(annotation));
}

Set<AnnotationInstance> merged;
if (methodLevelBindings.isEmpty()) {
merged = classLevelBindings;
} else {
merged = new HashSet<>(methodLevelBindings);
for (AnnotationInstance binding : classLevelBindings) {
if (methodLevelBindings.stream().noneMatch(a -> binding.name().equals(a.name()))) {
merged.add(binding);
}
}

if (Modifier.isPrivate(method.flags())
&& !Annotations.contains(methodAnnotations, DotNames.OBSERVES)
&& !Annotations.contains(methodAnnotations, DotNames.OBSERVES_ASYNC)) {
if (methodLevelBindings.size() == 1) {
LOGGER.warnf("%s will have no effect on method %s.%s() because the method is private",
methodLevelBindings.iterator().next(), classInfo.name(), method.name());
} else {
LOGGER.warnf("Annotations %s will have no effect on method %s.%s() because the method is private",
methodLevelBindings.stream().map(AnnotationInstance::toString).collect(Collectors.joining(",")),
classInfo.name(), method.name());
}
}
}
return merged;
}

static class NameAndDescriptor {
private final String name;
private final String descriptor;
Expand Down Expand Up @@ -536,17 +549,30 @@ public boolean test(MethodInfo method) {
if (!parameters.isEmpty() && (beanArchiveIndex != null)) {
String originalClassPackage = DotNames.packageName(originalClazz.name());
for (Type type : parameters) {
ClassInfo parameterClassInfo = beanArchiveIndex.getClassByName(type.name());
if (parameterClassInfo == null) {
continue; // hope for the best
if (type.kind() == Kind.PRIMITIVE) {
continue;
}
if (Modifier.isPrivate(parameterClassInfo.flags())) {
return true; // parameters whose class is private can not be loaded, as we would end up with IllegalAccessError when trying to access the use the load the class
DotName typeName = type.name();
if (type.kind() == Kind.ARRAY) {
typeName = type.asArrayType().component().name();
}
ClassInfo param = beanArchiveIndex.getClassByName(typeName);
if (param == null) {
LOGGER.warn(String.format(
"Parameter type info not available: %s - unable to validate the parameter type's visibility for method %s declared on %s",
type.name(), method.name(), method.declaringClass().name()));
continue;
}
if (!Modifier.isPublic(parameterClassInfo.flags())) {
// parameters whose class is package-private and the package is not the same as the package of the method for which we are checking can not be loaded,
// as we would end up with IllegalAccessError when trying to access the use the load the class
return !DotNames.packageName(parameterClassInfo.name()).equals(originalClassPackage);
if (Modifier.isPublic(param.flags()) || Modifier.isProtected(param.flags())) {
continue;
}
// e.g. parameters whose class is package-private and the package is not the same as the package of the method for which we are checking can not be loaded,
// as we would end up with IllegalAccessError when trying to access the use the load the class
if (!DotNames.packageName(param.name()).equals(originalClassPackage)) {
LOGGER.warn(String.format(
"A method %s() declared on %s has a non-public parameter of type %s which prevents it from being intercepted. Please change the parameter type visibility in order to make it intercepted.",
method.name(), method.declaringClass().name(), type));
return true;
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
package io.quarkus.arc.test.interceptors.nonpublicparams;

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

import io.quarkus.arc.Arc;
import io.quarkus.arc.ArcContainer;
import io.quarkus.arc.test.ArcTestContainer;
import io.quarkus.arc.test.interceptors.Simple;
import io.quarkus.arc.test.interceptors.nonpublicparams.charlie.Charlie;
import javax.annotation.Priority;
import javax.inject.Singleton;
import javax.interceptor.AroundInvoke;
import javax.interceptor.Interceptor;
import javax.interceptor.InvocationContext;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

public class NonPublicParametersTest {

@RegisterExtension
public ArcTestContainer container = new ArcTestContainer(Simple.class, SimpleBean.class,
SimpleInterceptor.class);

@Test
public void testInterception() {
ArcContainer arc = Arc.container();
SimpleBean simpleBean = arc.instance(SimpleBean.class).get();
assertEquals("FOO",
simpleBean.foo(null, new Bar(), new Baz(), null));
}

@Singleton
static class SimpleBean extends Charlie {

@Simple
String foo(Foo foo, Bar bar, Baz baz, CharlieParam charlie) {
return "foo";
}

@Simple
String fooArray(Foo[] foo, boolean isOk) {
return "foo";
}

private static class Foo {
}

}

static class Bar {

}

protected static class Baz {

}

@Simple
@Priority(1)
@Interceptor
public static class SimpleInterceptor {

@AroundInvoke
Object mySuperCoolAroundInvoke(InvocationContext ctx) throws Exception {
return ctx.proceed().toString().toUpperCase();
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package io.quarkus.arc.test.interceptors.nonpublicparams.charlie;

public class Charlie {

protected class CharlieParam {

}

}

0 comments on commit de75549

Please sign in to comment.