From 4599ea0acbbc85fcb33003ed853ce7f8d7c22ded Mon Sep 17 00:00:00 2001 From: Ladislav Thon Date: Tue, 14 Nov 2023 13:05:02 +0100 Subject: [PATCH] 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. --- .../invokable/AbstractInvokerBuilder.java | 3 ++ .../weld/invokable/MethodHandleUtils.java | 25 +++++++------ .../extension/translator/BeanInfoImpl.java | 36 ++++++++++++++++--- .../translator/ExtensionInvoker.java | 14 +++++++- .../translator/ReflectionMembers.java | 8 +++-- .../SyntheticComponentBuilderBase.java | 9 ++++- 6 files changed, 72 insertions(+), 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 index a755d76ddc6..c480d3755c0 100644 --- 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 index 61ac07769a8..81f3abd8615 100644 --- 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,14 +48,16 @@ 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); @@ -61,13 +65,8 @@ static MethodHandle createMethodHandle(Method method) { } 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/weld-lite-extension-translator/src/main/java/org/jboss/weld/lite/extension/translator/BeanInfoImpl.java b/weld-lite-extension-translator/src/main/java/org/jboss/weld/lite/extension/translator/BeanInfoImpl.java index 722cf811d87..877eeac0378 100644 --- a/weld-lite-extension-translator/src/main/java/org/jboss/weld/lite/extension/translator/BeanInfoImpl.java +++ b/weld-lite-extension-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 injectionPoints() { @Override public InvokerBuilder 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/weld-lite-extension-translator/src/main/java/org/jboss/weld/lite/extension/translator/ExtensionInvoker.java b/weld-lite-extension-translator/src/main/java/org/jboss/weld/lite/extension/translator/ExtensionInvoker.java index ec3311c3547..84feb53852c 100644 --- a/weld-lite-extension-translator/src/main/java/org/jboss/weld/lite/extension/translator/ExtensionInvoker.java +++ b/weld-lite-extension-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 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/weld-lite-extension-translator/src/main/java/org/jboss/weld/lite/extension/translator/ReflectionMembers.java b/weld-lite-extension-translator/src/main/java/org/jboss/weld/lite/extension/translator/ReflectionMembers.java index f9d50c2e54f..84a0bba5835 100644 --- a/weld-lite-extension-translator/src/main/java/org/jboss/weld/lite/extension/translator/ReflectionMembers.java +++ b/weld-lite-extension-translator/src/main/java/org/jboss/weld/lite/extension/translator/ReflectionMembers.java @@ -26,17 +26,19 @@ static Set allFields(Class clazz) { } private static void forEachSuperclass(Class clazz, Consumer> action) { + // an interface may be inherited multiple times, but we only want to process it once + Set> alreadySeen = new HashSet<>(); Queue> 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/weld-lite-extension-translator/src/main/java/org/jboss/weld/lite/extension/translator/SyntheticComponentBuilderBase.java b/weld-lite-extension-translator/src/main/java/org/jboss/weld/lite/extension/translator/SyntheticComponentBuilderBase.java index 7c599183bdd..0d1ec9fa5e6 100644 --- a/weld-lite-extension-translator/src/main/java/org/jboss/weld/lite/extension/translator/SyntheticComponentBuilderBase.java +++ b/weld-lite-extension-translator/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> { final Map 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(); } }