From 5ee6354356424ee48ff7b13751554f053c81598e Mon Sep 17 00:00:00 2001 From: Georgios Andrianakis Date: Mon, 6 Jul 2020 17:16:29 +0300 Subject: [PATCH] Allow Arc to deal with final methods of beans that needs to be proxied This is done in the same manner that is already present for interceptors and is very useful for Kotlin code where methods are final by default Fixes: #10290 --- docs/src/main/asciidoc/cdi-reference.adoc | 10 +- .../io/quarkus/arc/deployment/ArcConfig.java | 7 ++ .../quarkus/arc/deployment/ArcProcessor.java | 28 +++-- .../RequestScopedFinalMethodsTest.java | 106 ++++++++++++++++++ .../quarkus/arc/processor/BeanProcessor.java | 12 +- .../arc/processor/ClientProxyGenerator.java | 33 ++++-- .../io/quarkus/arc/processor/Methods.java | 66 +++++++---- 7 files changed, 218 insertions(+), 44 deletions(-) create mode 100644 extensions/arc/deployment/src/test/java/io/quarkus/arc/test/unproxyable/RequestScopedFinalMethodsTest.java diff --git a/docs/src/main/asciidoc/cdi-reference.adoc b/docs/src/main/asciidoc/cdi-reference.adoc index 0ad8ecc3a46a2..714ed4b91737b 100644 --- a/docs/src/main/asciidoc/cdi-reference.adoc +++ b/docs/src/main/asciidoc/cdi-reference.adoc @@ -647,7 +647,15 @@ class Services { * Private static methods are never intercepted * `InvocationContext#getTarget()` returns `null` for obvious reasons; therefore not all existing interceptors may behave correctly when intercepting static methods + -NOTE: Interceptors can use `InvocationContext.getMethod()` to detect static methods and adjust the behavior accordingly. +NOTE: Interceptors can use `InvocationContext.getMethod()` to detect static methods and adjust the behavior accordingly. + +=== Ability to handle 'final' classes and methods + +In normal CDI, classes that are marked as `final` and / or have `final` methods are not eligible for proxy creation, +which in turn means that interceptors and normal scoped beans don't work properly. +This situation is very common when trying to use CDI with alternative JVM languages like Kotlin where classes and methods are `final` by default. + +Quarkus however, can overcome these limitations when `quarkus.arc.transform-unproxyable-classes` is set to `true` (which is the default value). == Build Time Extension Points diff --git a/extensions/arc/deployment/src/main/java/io/quarkus/arc/deployment/ArcConfig.java b/extensions/arc/deployment/src/main/java/io/quarkus/arc/deployment/ArcConfig.java index 4babe0e9d06f9..af1377d3dfeb1 100644 --- a/extensions/arc/deployment/src/main/java/io/quarkus/arc/deployment/ArcConfig.java +++ b/extensions/arc/deployment/src/main/java/io/quarkus/arc/deployment/ArcConfig.java @@ -59,6 +59,13 @@ public class ArcConfig { * If set to true, the bytecode of unproxyable beans will be transformed. This ensures that a proxy/subclass * can be created properly. If the value is set to false, then an exception is thrown at build time indicating that a * subclass/proxy could not be created. + * + * Quarkus performs the following transformations when this setting is enabled: + * bytecodeTransformerConsumer = new Consumer() { - @Override - public void accept(BytecodeTransformer t) { - bytecodeTransformer.produce(new BytecodeTransformerBuildItem(t.getClassToTransform(), t.getVisitorFunction())); - } - }; + Consumer bytecodeTransformerConsumer = new BytecodeTransformerConsumer(bytecodeTransformer); observerRegistrationPhase.getBeanProcessor().initialize(bytecodeTransformerConsumer); return new ValidationPhaseBuildItem(observerRegistrationPhase.getBeanProcessor().validate(bytecodeTransformerConsumer), observerRegistrationPhase.getBeanProcessor()); @@ -410,7 +405,8 @@ public BeanContainerBuildItem generateResources(ArcRecorder recorder, ShutdownCo BuildProducer reflectiveFields, BuildProducer generatedClass, LiveReloadBuildItem liveReloadBuildItem, - BuildProducer generatedResource) throws Exception { + BuildProducer generatedResource, + BuildProducer bytecodeTransformer) throws Exception { for (ValidationErrorBuildItem validationError : validationErrors) { for (Throwable error : validationError.getValues()) { @@ -426,6 +422,8 @@ public BeanContainerBuildItem generateResources(ArcRecorder recorder, ShutdownCo liveReloadBuildItem.setContextObject(ExistingClasses.class, existingClasses); } + Consumer bytecodeTransformerConsumer = new BytecodeTransformerConsumer(bytecodeTransformer); + long start = System.currentTimeMillis(); List resources = beanProcessor.generateResources(new ReflectionRegistration() { @Override @@ -437,7 +435,7 @@ public void registerMethod(MethodInfo methodInfo) { public void registerField(FieldInfo fieldInfo) { reflectiveFields.produce(new ReflectiveFieldBuildItem(fieldInfo)); } - }, existingClasses.existingClasses); + }, existingClasses.existingClasses, bytecodeTransformerConsumer); for (ResourceOutput.Resource resource : resources) { switch (resource.getType()) { case JAVA_CLASS: @@ -603,4 +601,18 @@ public boolean test(T t) { static class ExistingClasses { Set existingClasses = new HashSet<>(); } + + private static class BytecodeTransformerConsumer implements Consumer { + + private final BuildProducer bytecodeTransformer; + + public BytecodeTransformerConsumer(BuildProducer bytecodeTransformer) { + this.bytecodeTransformer = bytecodeTransformer; + } + + @Override + public void accept(BytecodeTransformer t) { + bytecodeTransformer.produce(new BytecodeTransformerBuildItem(t.getClassToTransform(), t.getVisitorFunction())); + } + } } diff --git a/extensions/arc/deployment/src/test/java/io/quarkus/arc/test/unproxyable/RequestScopedFinalMethodsTest.java b/extensions/arc/deployment/src/test/java/io/quarkus/arc/test/unproxyable/RequestScopedFinalMethodsTest.java new file mode 100644 index 0000000000000..1b9f876002541 --- /dev/null +++ b/extensions/arc/deployment/src/test/java/io/quarkus/arc/test/unproxyable/RequestScopedFinalMethodsTest.java @@ -0,0 +1,106 @@ +package io.quarkus.arc.test.unproxyable; + +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; + +import javax.annotation.PostConstruct; +import javax.enterprise.context.Dependent; +import javax.enterprise.context.RequestScoped; +import javax.enterprise.event.Event; +import javax.enterprise.event.Observes; +import javax.inject.Inject; +import javax.inject.Singleton; + +import org.jboss.shrinkwrap.api.ShrinkWrap; +import org.jboss.shrinkwrap.api.spec.JavaArchive; +import org.junit.jupiter.api.Assertions; +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.InstanceHandle; +import io.quarkus.arc.ManagedContext; +import io.quarkus.test.QuarkusUnitTest; + +public class RequestScopedFinalMethodsTest { + + @RegisterExtension + public static QuarkusUnitTest container = new QuarkusUnitTest() + .setArchiveProducer(() -> ShrinkWrap.create(JavaArchive.class) + .addClasses(RequestScopedBean.class)); + + @Test + public void testRequestScopedBeanWorksProperly() { + ArcContainer container = Arc.container(); + ManagedContext requestContext = container.requestContext(); + requestContext.activate(); + + InstanceHandle handle = container.instance(RequestScopedBean.class); + Assertions.assertTrue(handle.isAvailable()); + + RequestScopedBean bean = handle.get(); + Assertions.assertNull(bean.getProp()); + bean.setProp(100); + Assertions.assertEquals(100, bean.getProp()); + + requestContext.terminate(); + requestContext.activate(); + + handle = container.instance(RequestScopedBean.class); + bean = handle.get(); + Assertions.assertTrue(handle.isAvailable()); + Assertions.assertNull(bean.getProp()); + } + + @RequestScoped + static class RequestScopedBean { + private Integer prop = null; + + public final Integer getProp() { + return prop; + } + + public final void setProp(Integer prop) { + this.prop = prop; + } + } + + @Dependent + static class StringProducer { + + @Inject + Event event; + + void fire(String value) { + event.fire(value); + } + + } + + @Singleton + static class StringObserver { + + private List events; + + @Inject + RequestScopedBean requestScopedBean; + + @PostConstruct + void init() { + events = new CopyOnWriteArrayList<>(); + } + + void observeSync(@Observes Integer value) { + Integer oldValue = requestScopedBean.getProp(); + Integer newValue = oldValue == null ? value : value + oldValue; + requestScopedBean.setProp(newValue); + events.add(newValue); + } + + List getEvents() { + return events; + } + + } +} diff --git a/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanProcessor.java b/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanProcessor.java index e4e15cd244c12..1b6a56b256da9 100644 --- a/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanProcessor.java +++ b/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanProcessor.java @@ -38,7 +38,7 @@ *
  • {@link #initialize(Consumer)}
    • *
  • {@link #validate(Consumer)}
    • *
  • {@link #processValidationErrors(io.quarkus.arc.processor.BeanDeploymentValidator.ValidationContext)}
    • - *
  • {@link #generateResources(ReflectionRegistration, Set)}
    • + *
  • {@link #generateResources(ReflectionRegistration, Set, Consumer)}
    • * */ public class BeanProcessor { @@ -64,6 +64,7 @@ public static Builder builder() { private final BeanDeployment beanDeployment; private final boolean generateSources; private final boolean allowMocking; + private final boolean transformUnproxyableClasses; // This predicate is used to filter annotations for InjectionPoint metadata // Note that we do create annotation literals for all annotations for an injection point that resolves to a @Dependent bean that injects the InjectionPoint metadata @@ -79,6 +80,7 @@ private BeanProcessor(Builder builder) { this.annotationLiterals = new AnnotationLiteralProcessor(builder.sharedAnnotationLiterals, applicationClassPredicate); this.generateSources = builder.generateSources; this.allowMocking = builder.allowMocking; + this.transformUnproxyableClasses = builder.transformUnproxyableClasses; // Initialize all build processors buildContext = new BuildContextImpl(); @@ -133,7 +135,8 @@ public void processValidationErrors(BeanDeploymentValidator.ValidationContext va BeanDeployment.processErrors(validationContext.getDeploymentProblems()); } - public List generateResources(ReflectionRegistration reflectionRegistration, Set existingClasses) + public List generateResources(ReflectionRegistration reflectionRegistration, Set existingClasses, + Consumer bytecodeTransformerConsumer) throws IOException { if (reflectionRegistration == null) { reflectionRegistration = this.reflectionRegistration; @@ -174,7 +177,8 @@ public List generateResources(ReflectionRegistration reflectionRegistr if (bean.getScope().isNormal()) { // Generate client proxy resources.addAll( - clientProxyGenerator.generate(bean, resource.getFullyQualifiedName())); + clientProxyGenerator.generate(bean, resource.getFullyQualifiedName(), + bytecodeTransformerConsumer, transformUnproxyableClasses)); } if (bean.isSubclassRequired()) { resources.addAll( @@ -234,7 +238,7 @@ public void accept(BytecodeTransformer transformer) { initialize(unsupportedBytecodeTransformer); ValidationContext validationContext = validate(unsupportedBytecodeTransformer); processValidationErrors(validationContext); - generateResources(null, new HashSet<>()); + generateResources(null, new HashSet<>(), unsupportedBytecodeTransformer); return beanDeployment; } diff --git a/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/ClientProxyGenerator.java b/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/ClientProxyGenerator.java index d4507c7b0183d..08bd3a582b9fa 100644 --- a/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/ClientProxyGenerator.java +++ b/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/ClientProxyGenerator.java @@ -25,9 +25,11 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Consumer; import java.util.function.Predicate; import javax.enterprise.context.ContextNotActiveException; import javax.enterprise.context.spi.Contextual; @@ -70,9 +72,12 @@ public ClientProxyGenerator(Predicate applicationClassPredicate, boolea * * @param bean * @param beanClassName Fully qualified class name + * @param bytecodeTransformerConsumer + * @param transformUnproxyableClasses whether or not unproxyable classes should be transformed * @return a collection of resources */ - Collection generate(BeanInfo bean, String beanClassName) { + Collection generate(BeanInfo bean, String beanClassName, + Consumer bytecodeTransformerConsumer, boolean transformUnproxyableClasses) { ResourceClassOutput classOutput = new ResourceClassOutput(applicationClassPredicate.test(bean.getBeanClass()), generateSources); @@ -127,7 +132,7 @@ Collection generate(BeanInfo bean, String beanClassName) { implementMockMethods(clientProxy); } - for (MethodInfo method : getDelegatingMethods(bean)) { + for (MethodInfo method : getDelegatingMethods(bean, bytecodeTransformerConsumer, transformUnproxyableClasses)) { MethodDescriptor originalMethodDescriptor = MethodDescriptor.of(method); MethodCreator forward = clientProxy.getMethodCreator(originalMethodDescriptor); @@ -302,23 +307,35 @@ void implementGetBean(ClassCreator clientProxy, FieldDescriptor beanField) { creator.returnValue(creator.readInstanceField(beanField, creator.getThis())); } - Collection getDelegatingMethods(BeanInfo bean) { + Collection getDelegatingMethods(BeanInfo bean, Consumer bytecodeTransformerConsumer, + boolean transformUnproxyableClasses) { Map methods = new HashMap<>(); if (bean.isClassBean()) { - Methods.addDelegatingMethods(bean.getDeployment().getIndex(), bean.getTarget().get().asClass(), - methods); + Set methodsFromWhichToRemoveFinal = new HashSet<>(); + ClassInfo classInfo = bean.getTarget().get().asClass(); + Methods.addDelegatingMethods(bean.getDeployment().getIndex(), classInfo, + methods, methodsFromWhichToRemoveFinal, transformUnproxyableClasses); + if (!methodsFromWhichToRemoveFinal.isEmpty()) { + String className = classInfo.name().toString(); + bytecodeTransformerConsumer.accept(new BytecodeTransformer(className, + new Methods.RemoveFinalFromMethod(className, methodsFromWhichToRemoveFinal))); + } } else if (bean.isProducerMethod()) { MethodInfo producerMethod = bean.getTarget().get().asMethod(); ClassInfo returnTypeClass = getClassByName(bean.getDeployment().getIndex(), producerMethod.returnType()); - Methods.addDelegatingMethods(bean.getDeployment().getIndex(), returnTypeClass, methods); + Methods.addDelegatingMethods(bean.getDeployment().getIndex(), returnTypeClass, methods, null, + transformUnproxyableClasses); } else if (bean.isProducerField()) { FieldInfo producerField = bean.getTarget().get().asField(); ClassInfo fieldClass = getClassByName(bean.getDeployment().getIndex(), producerField.type()); - Methods.addDelegatingMethods(bean.getDeployment().getIndex(), fieldClass, methods); + Methods.addDelegatingMethods(bean.getDeployment().getIndex(), fieldClass, methods, null, + transformUnproxyableClasses); } else if (bean.isSynthetic()) { - Methods.addDelegatingMethods(bean.getDeployment().getIndex(), bean.getImplClazz(), methods); + Methods.addDelegatingMethods(bean.getDeployment().getIndex(), bean.getImplClazz(), methods, null, + transformUnproxyableClasses); } + return methods.values(); } 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 1ace72fdb7763..52bde54593077 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 @@ -58,11 +58,12 @@ static boolean isSynthetic(MethodInfo method) { return (method.flags() & SYNTHETIC) != 0; } - static void addDelegatingMethods(IndexView index, ClassInfo classInfo, Map methods) { + static void addDelegatingMethods(IndexView index, ClassInfo classInfo, Map methods, + Set methodsFromWhichToRemoveFinal, boolean transformUnproxyableClasses) { // TODO support interfaces default methods if (classInfo != null) { for (MethodInfo method : classInfo.methods()) { - if (skipForClientProxy(method)) { + if (skipForClientProxy(method, transformUnproxyableClasses, methodsFromWhichToRemoveFinal)) { continue; } methods.computeIfAbsent(new Methods.MethodKey(method), key -> { @@ -82,19 +83,22 @@ static void addDelegatingMethods(IndexView index, ClassInfo classInfo, Map methodsFromWhichToRemoveFinal) { if (Modifier.isStatic(method.flags()) || Modifier.isPrivate(method.flags())) { return true; } @@ -108,6 +112,11 @@ private static boolean skipForClientProxy(MethodInfo method) { if (Modifier.isFinal(method.flags())) { String className = method.declaringClass().name().toString(); if (!className.startsWith("java.")) { + if (transformUnproxyableClasses && (methodsFromWhichToRemoveFinal != null)) { + methodsFromWhichToRemoveFinal.add(NameAndDescriptor.fromMethodInfo(method)); + return false; + } + LOGGER.warn(String.format( "Final method %s.%s() is ignored during proxy generation and should never be invoked upon the proxy instance!", className, method.name())); @@ -161,23 +170,8 @@ static Set addInterceptedMethodCandidates(BeanDeployment beanDeploym } if (!methodsFromWhichToRemoveFinal.isEmpty()) { bytecodeTransformerConsumer.accept( - new BytecodeTransformer(classInfo.name().toString(), new BiFunction() { - @Override - public ClassVisitor apply(String s, ClassVisitor classVisitor) { - return new ClassVisitor(Gizmo.ASM_API_VERSION, classVisitor) { - @Override - public MethodVisitor visitMethod(int access, String name, String descriptor, String signature, - String[] exceptions) { - if (methodsFromWhichToRemoveFinal.contains(new NameAndDescriptor(name, descriptor))) { - access = access & (~Opcodes.ACC_FINAL); - LOGGER.debug("final modifier removed from method " + name + " of class " - + classInfo.name().toString()); - } - return super.visitMethod(access, name, descriptor, signature, exceptions); - } - }; - } - })); + new BytecodeTransformer(classInfo.name().toString(), + new RemoveFinalFromMethod(classInfo.name().toString(), methodsFromWhichToRemoveFinal))); } if (classInfo.superClassType() != null) { ClassInfo superClassInfo = getClassByName(beanDeployment.getIndex(), classInfo.superName()); @@ -197,7 +191,7 @@ public MethodVisitor visitMethod(int access, String name, String descriptor, Str return finalMethodsFoundAndNotChanged; } - private static class NameAndDescriptor { + static class NameAndDescriptor { private final String name; private final String descriptor; @@ -349,4 +343,30 @@ static DotName toRawType(Type a) { } } + static class RemoveFinalFromMethod implements BiFunction { + + private final String classToTransform; + private final Set methodsFromWhichToRemoveFinal; + + public RemoveFinalFromMethod(String classToTransform, Set methodsFromWhichToRemoveFinal) { + this.classToTransform = classToTransform; + this.methodsFromWhichToRemoveFinal = methodsFromWhichToRemoveFinal; + } + + @Override + public ClassVisitor apply(String s, ClassVisitor classVisitor) { + return new ClassVisitor(Gizmo.ASM_API_VERSION, classVisitor) { + @Override + public MethodVisitor visitMethod(int access, String name, String descriptor, String signature, + String[] exceptions) { + if (methodsFromWhichToRemoveFinal.contains(new NameAndDescriptor(name, descriptor))) { + access = access & (~Opcodes.ACC_FINAL); + LOGGER.debug("final modifier removed from method " + name + " of class " + classToTransform); + } + return super.visitMethod(access, name, descriptor, signature, exceptions); + } + }; + } + } + }