improvements to the method invokers implementation

Includes one tiny improvement to the Language Model implementation that should be clarified in the next version of the specification.
weld · Nov 14, 2023 · 4599ea0 · 4599ea0
1 parent 8c1438f
commit 4599ea0
Show file tree

Hide file tree

Showing 6 changed files with 72 additions and 23 deletions.
diff --git a/impl/src/main/java/org/jboss/weld/invokable/AbstractInvokerBuilder.java b/impl/src/main/java/org/jboss/weld/invokable/AbstractInvokerBuilder.java
@@ -137,6 +137,9 @@ private boolean requiresCleanup() {
 
         MethodHandle mh = MethodHandleUtils.createMethodHandle(method);
 
+        // single, array-typed parameter at the end for variable arity methods
+        mh = mh.asFixedArity();
+
         // instance transformer
         if (instanceTransformer != null && !isStaticMethod) {
             MethodHandle instanceTransformerMethod = MethodHandleUtils.createMethodHandleFromTransformer(method,

diff --git a/impl/src/main/java/org/jboss/weld/invokable/MethodHandleUtils.java b/impl/src/main/java/org/jboss/weld/invokable/MethodHandleUtils.java
@@ -4,6 +4,7 @@
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodHandles;
 import java.lang.reflect.Constructor;
+import java.lang.reflect.Executable;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.lang.reflect.Type;
@@ -12,9 +13,10 @@
 import java.util.function.Consumer;
 
 import jakarta.enterprise.inject.spi.BeanManager;
-import jakarta.enterprise.inject.spi.DeploymentException;
 import jakarta.enterprise.invoke.Invoker;
 
+import org.jboss.weld.exceptions.DeploymentException;
+
 class MethodHandleUtils {
     private MethodHandleUtils() {
     }
@@ -46,28 +48,25 @@ private MethodHandleUtils() {
         }
     }
 
-    static MethodHandle createMethodHandle(Method method) {
-        MethodHandles.Lookup lookup = Modifier.isPublic(method.getModifiers())
-                && Modifier.isPublic(method.getDeclaringClass().getModifiers())
-                        ? MethodHandles.publicLookup()
-                        : MethodHandles.lookup();
+    private static MethodHandles.Lookup lookupFor(Executable method) throws IllegalAccessException {
+        if (Modifier.isPublic(method.getModifiers()) && Modifier.isPublic(method.getDeclaringClass().getModifiers())) {
+            return MethodHandles.publicLookup();
+        }
+        return MethodHandles.privateLookupIn(method.getDeclaringClass(), MethodHandles.lookup());
+    }
 
+    static MethodHandle createMethodHandle(Method method) {
         try {
-            return lookup.unreflect(method);
+            return lookupFor(method).unreflect(method);
         } catch (ReflectiveOperationException e) {
             // TODO proper exception handling
             throw new RuntimeException(e);
         }
     }
 
     static MethodHandle createMethodHandle(Constructor<?> constructor) {
-        MethodHandles.Lookup lookup = Modifier.isPublic(constructor.getModifiers())
-                && Modifier.isPublic(constructor.getDeclaringClass().getModifiers())
-                        ? MethodHandles.publicLookup()
-                        : MethodHandles.lookup();
-
         try {
-            return lookup.unreflectConstructor(constructor);
+            return lookupFor(constructor).unreflectConstructor(constructor);
         } catch (ReflectiveOperationException e) {
             // TODO proper exception handling
             throw new RuntimeException(e);

diff --git a/...nsion-translator/src/main/java/org/jboss/weld/lite/extension/translator/BeanInfoImpl.java b/...nsion-translator/src/main/java/org/jboss/weld/lite/extension/translator/BeanInfoImpl.java
@@ -1,5 +1,6 @@
 package org.jboss.weld.lite.extension.translator;
 
+import java.lang.reflect.Modifier;
 import java.util.Collection;
 import java.util.stream.Collectors;
 
@@ -12,6 +13,7 @@
 import jakarta.enterprise.inject.build.compatible.spi.StereotypeInfo;
 import jakarta.enterprise.inject.spi.AnnotatedMethod;
 import jakarta.enterprise.inject.spi.BeanManager;
+import jakarta.enterprise.inject.spi.Decorator;
 import jakarta.enterprise.inject.spi.InjectionPoint;
 import jakarta.enterprise.invoke.InvokerBuilder;
 import jakarta.enterprise.lang.model.AnnotationInfo;
@@ -20,6 +22,7 @@
 import jakarta.enterprise.lang.model.declarations.MethodInfo;
 import jakarta.enterprise.lang.model.types.Type;
 
+import org.jboss.weld.exceptions.DeploymentException;
 import org.jboss.weld.invokable.InvokerInfoBuilder;
 import org.jboss.weld.lite.extension.translator.util.reflection.AnnotatedTypes;
 
@@ -151,18 +154,41 @@ public Collection<InjectionPointInfo> injectionPoints() {
 
     @Override
     public InvokerBuilder<InvokerInfo> createInvoker(MethodInfo methodInfo) {
+        if (!isClassBean()) {
+            throw new DeploymentException("Cannot build invoker for a bean which is not a managed bean: " + this);
+        }
+        if (isInterceptor()) {
+            throw new DeploymentException("Cannot build invoker for an interceptor: " + this);
+        }
+        if (cdiBean instanceof Decorator) { // not representable in BCE, but can theoretically happen
+            throw new DeploymentException("Cannot build invoker for a decorator: " + this);
+        }
+
         if (methodInfo.isConstructor()) {
-            // TODO better exception
-            throw new IllegalArgumentException(
-                    "Constructor methods are not valid candidates for Invokers. MethodInfo: " + methodInfo);
+            throw new DeploymentException("Cannot build invoker for a constructor: " + methodInfo);
+        }
+        if (Modifier.isPrivate(methodInfo.modifiers())) {
+            throw new DeploymentException("Cannot build invoker for a private method: " + methodInfo);
         }
+        if ("java.lang.Object".equals(methodInfo.declaringClass().name())
+                && !"toString".equals(methodInfo.name())) {
+            throw new DeploymentException("Cannot build invoker for a method declared on java.lang.Object: " + methodInfo);
+        }
+
         if (methodInfo instanceof MethodInfoImpl) {
             // at this point we can be sure it is a Method, not a Constructor, so we cast it
             AnnotatedMethod<?> cdiMethod = (AnnotatedMethod<?>) ((MethodInfoImpl) methodInfo).cdiDeclaration;
+
+            // verify that the `methodInfo` belongs to this bean
+            // TODO inefficient when many invokers are created for methods of a single bean
+            if (!ReflectionMembers.allMethods(cdiBean.getBeanClass()).contains(cdiMethod.getJavaMember())) {
+                throw new DeploymentException("Method does not belong to bean " + cdiBean.getBeanClass().getName()
+                        + ": " + methodInfo);
+            }
+
             return new InvokerInfoBuilder<>(cdiBean.getBeanClass(), cdiMethod.getJavaMember(), bm);
         } else {
-            // TODO better exception
-            throw new IllegalArgumentException("Custom implementations of MethodInfo are not supported!");
+            throw new DeploymentException("Custom implementations of MethodInfo are not supported!");
         }
     }
 

diff --git a/...n-translator/src/main/java/org/jboss/weld/lite/extension/translator/ExtensionInvoker.java b/...n-translator/src/main/java/org/jboss/weld/lite/extension/translator/ExtensionInvoker.java
@@ -12,6 +12,8 @@
 import jakarta.annotation.Priority;
 import jakarta.enterprise.inject.build.compatible.spi.BuildCompatibleExtension;
 import jakarta.enterprise.inject.build.compatible.spi.SkipIfPortableExtensionPresent;
+import jakarta.enterprise.inject.spi.DefinitionException;
+import jakarta.enterprise.inject.spi.DeploymentException;
 import jakarta.interceptor.Interceptor;
 
 import org.jboss.weld.lite.extension.translator.logging.LiteExtensionTranslatorLogger;
@@ -116,7 +118,17 @@ void callExtensionMethod(java.lang.reflect.Method method, List<Object> arguments
         Class<?> extensionClass = extensionClasses.get(method.getDeclaringClass().getName());
         Object extensionClassInstance = extensionClassInstances.get(extensionClass);
 
-        method.invoke(extensionClassInstance, arguments.toArray());
+        try {
+            method.invoke(extensionClassInstance, arguments.toArray());
+        } catch (InvocationTargetException e) {
+            if (e.getCause() instanceof DefinitionException) {
+                throw (DefinitionException) e.getCause();
+            } else if (e.getCause() instanceof DeploymentException) {
+                throw (DeploymentException) e.getCause();
+            } else {
+                throw e;
+            }
+        }
     }
 
     void clear() {

diff --git a/...-translator/src/main/java/org/jboss/weld/lite/extension/translator/ReflectionMembers.java b/...-translator/src/main/java/org/jboss/weld/lite/extension/translator/ReflectionMembers.java
@@ -26,17 +26,19 @@ static Set<java.lang.reflect.Field> allFields(Class<?> clazz) {
     }
 
     private static void forEachSuperclass(Class<?> clazz, Consumer<Class<?>> action) {
+        // an interface may be inherited multiple times, but we only want to process it once
+        Set<Class<?>> alreadySeen = new HashSet<>();
         Queue<Class<?>> workQueue = new ArrayDeque<>();
         workQueue.add(clazz);
         while (!workQueue.isEmpty()) {
             Class<?> item = workQueue.remove();
-
-            if (item.equals(Object.class)) {
+            if (alreadySeen.contains(item)) {
                 continue;
             }
+            alreadySeen.add(item);
 
             Class<?> superclass = item.getSuperclass();
-            if (superclass != null) {
+            if (superclass != null && !Object.class.equals(superclass)) {
                 workQueue.add(superclass);
             }
             workQueue.addAll(Arrays.asList(item.getInterfaces()));

diff --git a/...src/main/java/org/jboss/weld/lite/extension/translator/SyntheticComponentBuilderBase.java b/...src/main/java/org/jboss/weld/lite/extension/translator/SyntheticComponentBuilderBase.java
@@ -5,9 +5,12 @@
 import java.util.Map;
 
 import jakarta.enterprise.inject.build.compatible.spi.InvokerInfo;
+import jakarta.enterprise.invoke.Invoker;
 import jakarta.enterprise.lang.model.AnnotationInfo;
 import jakarta.enterprise.lang.model.declarations.ClassInfo;
 
+import org.jboss.weld.invokable.InvokerImpl;
+
 public class SyntheticComponentBuilderBase<THIS extends SyntheticComponentBuilderBase<THIS>> {
     final Map<String, Object> params = new HashMap<>();
 
@@ -130,7 +133,11 @@ public THIS withParam(String key, InvokerInfo value) {
     }
 
     public THIS withParam(String key, InvokerInfo[] value) {
-        this.params.put(key, value);
+        Invoker<?, ?>[] array = new Invoker<?, ?>[value.length];
+        for (int i = 0; i < value.length; i++) {
+            array[i] = (InvokerImpl<?, ?>) value[i];
+        }
+        this.params.put(key, array);
         return self();
     }
 }