diff --git a/environments/se/tests/src/test/java/org/jboss/weld/environment/se/test/discovery/withAnnotations/VerifyingExtension.java b/environments/se/tests/src/test/java/org/jboss/weld/environment/se/test/discovery/withAnnotations/VerifyingExtension.java index 8d99d3b97db..b939adfe845 100644 --- a/environments/se/tests/src/test/java/org/jboss/weld/environment/se/test/discovery/withAnnotations/VerifyingExtension.java +++ b/environments/se/tests/src/test/java/org/jboss/weld/environment/se/test/discovery/withAnnotations/VerifyingExtension.java @@ -18,6 +18,9 @@ import static org.junit.Assert.assertNull; +import java.util.HashSet; +import java.util.Set; + import jakarta.enterprise.event.Observes; import jakarta.enterprise.inject.Stereotype; import jakarta.enterprise.inject.spi.AnnotatedType; @@ -32,6 +35,10 @@ public class VerifyingExtension implements Extension { private AnnotatedType myBeanMetaType; + private final Set> markedTypes = new HashSet<>(); + + private final Set> inheritedMarkedTypes = new HashSet<>(); + void processMyBean(@Observes @WithAnnotations(Named.class) ProcessAnnotatedType event) { assertNull(myBeanType); this.myBeanType = event.getAnnotatedType(); @@ -42,6 +49,15 @@ void processMyBeanMeta(@Observes @WithAnnotations(Stereotype.class) ProcessAnnot this.myBeanMetaType = event.getAnnotatedType(); } + void processMarkedType(@Observes @WithAnnotations(WithAnnotationsTest.Marker.class) ProcessAnnotatedType event) { + markedTypes.add(event.getAnnotatedType().getJavaClass()); + } + + void processInheritedMarkedType( + @Observes @WithAnnotations(WithAnnotationsTest.InheritedMarker.class) ProcessAnnotatedType event) { + inheritedMarkedTypes.add(event.getAnnotatedType().getJavaClass()); + } + AnnotatedType getMyBeanType() { return myBeanType; } @@ -50,4 +66,12 @@ AnnotatedType getMyBeanMetaType() { return myBeanMetaType; } + Set> getMarkedTypes() { + return markedTypes; + } + + Set> getInheritedMarkedTypes() { + return inheritedMarkedTypes; + } + } diff --git a/environments/se/tests/src/test/java/org/jboss/weld/environment/se/test/discovery/withAnnotations/WithAnnotationsTest.java b/environments/se/tests/src/test/java/org/jboss/weld/environment/se/test/discovery/withAnnotations/WithAnnotationsTest.java index e3c558054b8..5e4756e0d40 100644 --- a/environments/se/tests/src/test/java/org/jboss/weld/environment/se/test/discovery/withAnnotations/WithAnnotationsTest.java +++ b/environments/se/tests/src/test/java/org/jboss/weld/environment/se/test/discovery/withAnnotations/WithAnnotationsTest.java @@ -19,6 +19,12 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import java.lang.annotation.Inherited; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.util.Set; + +import jakarta.enterprise.context.Dependent; import jakarta.enterprise.inject.spi.Extension; import org.jboss.arquillian.container.se.api.ClassPath; @@ -41,6 +47,7 @@ public static Archive getDeployment() { return ClassPath.builder() .add(ShrinkWrap.create(BeanArchive.class, Utils.getDeploymentNameAsHash(WithAnnotationsTest.class)) .addPackage(WithAnnotationsTest.class.getPackage()) + .addClasses(WithAnnotationsTest.class.getDeclaredClasses()) .addAsServiceProvider(Extension.class, VerifyingExtension.class)) .build(); } @@ -59,4 +66,32 @@ public void testWithAnnotationsOnDefaultMethod() { } } + + @Test + public void testWithAnnotationsRespectsClassAnnotationInheritance() { + try (WeldContainer container = new Weld().scanClasspathEntries().initialize()) { + VerifyingExtension extension = container.getBeanManager().getExtension(VerifyingExtension.class); + assertEquals(Set.of(Base.class), extension.getMarkedTypes()); + assertEquals(Set.of(Base.class, Child.class), extension.getInheritedMarkedTypes()); + } + } + + @Retention(RetentionPolicy.RUNTIME) + public @interface Marker { + } + + @Inherited + @Retention(RetentionPolicy.RUNTIME) + public @interface InheritedMarker { + } + + @Dependent + @Marker + @InheritedMarker + public static class Base { + } + + @Dependent + public static class Child extends Base { + } } diff --git a/impl/src/main/java/org/jboss/weld/annotated/enhanced/jlr/EnhancedAnnotatedTypeImpl.java b/impl/src/main/java/org/jboss/weld/annotated/enhanced/jlr/EnhancedAnnotatedTypeImpl.java index 3b1ec168571..1f04e029fe6 100644 --- a/impl/src/main/java/org/jboss/weld/annotated/enhanced/jlr/EnhancedAnnotatedTypeImpl.java +++ b/impl/src/main/java/org/jboss/weld/annotated/enhanced/jlr/EnhancedAnnotatedTypeImpl.java @@ -25,8 +25,10 @@ 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.stream.Collectors; import jakarta.enterprise.event.Observes; import jakarta.enterprise.event.ObservesAsync; @@ -290,45 +292,52 @@ protected EnhancedAnnotatedTypeImpl(SlimAnnotatedType annotatedType, SetMultimap, Annotation> declaredMetaAnnotationMap = SetMultimap.newSetMultimap(); processMetaAnnotations(declaredMetaAnnotationMap, declaredAnnotationMap.values(), classTransformer, true); this.declaredMetaAnnotationMap = Multimaps.unmodifiableMultimap(declaredMetaAnnotationMap); - this.overriddenMethods = getOverriddenMethods(this, methodsTemp); + Map, List>> methodsByDeclaringClass = groupMethodsByDeclaringClass( + methodsTemp); + this.overriddenMethods = getOverriddenMethods(this, methodsByDeclaringClass); // WELD-1548 remove all overriden methods except for those which are overriden by a bridge method - methodsTemp.removeAll(getOverriddenMethods(this, methodsTemp, true)); + methodsTemp.removeAll(getOverriddenMethods(this, methodsByDeclaringClass, true)); this.methods = methodsTemp; this.annotatedMethods = buildAnnotatedMethodMultimap(this.methods); this.annotatedMethodsByAnnotatedParameters = buildAnnotatedParameterMethodMultimap(this.methods); } - protected Set> getOverriddenMethods(EnhancedAnnotatedType annotatedType, + protected Map, List>> groupMethodsByDeclaringClass( Set> methods) { - return getOverriddenMethods(annotatedType, methods, false); + return methods.stream().collect(Collectors.groupingBy(method -> method.getJavaMember().getDeclaringClass())); + } + + protected Set> getOverriddenMethods(EnhancedAnnotatedType annotatedType, + Map, List>> methodsByDeclaringClass) { + return getOverriddenMethods(annotatedType, methodsByDeclaringClass, false); } /** * * @param annotatedType - * @param methods + * @param methodsByDeclaringClass * @param skipOverridingBridgeMethods If set to true the returning set will not contain methods overriden by a * bridge method * @return the set of overriden methods */ protected Set> getOverriddenMethods(EnhancedAnnotatedType annotatedType, - Set> methods, boolean skipOverridingBridgeMethods) { + Map, List>> methodsByDeclaringClass, + boolean skipOverridingBridgeMethods) { Set> overriddenMethods = new HashSet>(); Multimap seenMethods = SetMultimap.newSetMultimap(); for (Class clazz = annotatedType.getJavaClass(); clazz != null && clazz != Object.class; clazz = clazz.getSuperclass()) { - for (EnhancedAnnotatedMethod method : methods) { - if (method.getJavaMember().getDeclaringClass().equals(clazz)) { - if (skipOverridingBridgeMethods && method.getJavaMember().isBridge()) { - continue; - } - if (isOverridden(method, seenMethods)) { - overriddenMethods.add(method); - } - seenMethods.put(method.getSignature(), method.getPackage()); + for (EnhancedAnnotatedMethod method : methodsByDeclaringClass.getOrDefault(clazz, + Collections.emptyList())) { + if (skipOverridingBridgeMethods && method.getJavaMember().isBridge()) { + continue; + } + if (isOverridden(method, seenMethods)) { + overriddenMethods.add(method); } + seenMethods.put(method.getSignature(), method.getPackage()); } } return immutableSetView(overriddenMethods); diff --git a/impl/src/main/java/org/jboss/weld/annotated/enhanced/jlr/EnhancedAnnotationImpl.java b/impl/src/main/java/org/jboss/weld/annotated/enhanced/jlr/EnhancedAnnotationImpl.java index 7356a4f37fc..c9c1b3fa317 100644 --- a/impl/src/main/java/org/jboss/weld/annotated/enhanced/jlr/EnhancedAnnotationImpl.java +++ b/impl/src/main/java/org/jboss/weld/annotated/enhanced/jlr/EnhancedAnnotationImpl.java @@ -20,6 +20,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; @@ -87,9 +88,16 @@ protected EnhancedAnnotationImpl(SlimAnnotatedType annotatedType, } } + @Override + protected Map, List>> groupMethodsByDeclaringClass( + Set> methods) { + return Collections.emptyMap(); + } + @Override protected Set> getOverriddenMethods(EnhancedAnnotatedType annotatedType, - Set> methods, boolean skipOverridingBridgeMethods) { + Map, List>> methodsByDeclaringClass, + boolean skipOverridingBridgeMethods) { return Collections.emptySet(); } diff --git a/impl/src/main/java/org/jboss/weld/bootstrap/events/RequiredAnnotationDiscovery.java b/impl/src/main/java/org/jboss/weld/bootstrap/events/RequiredAnnotationDiscovery.java index cd361321db4..96cb72a5db6 100644 --- a/impl/src/main/java/org/jboss/weld/bootstrap/events/RequiredAnnotationDiscovery.java +++ b/impl/src/main/java/org/jboss/weld/bootstrap/events/RequiredAnnotationDiscovery.java @@ -21,9 +21,13 @@ import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.lang.reflect.Method; +import java.util.HashSet; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import org.jboss.weld.annotated.slim.backed.BackedAnnotatedType; -import org.jboss.weld.bootstrap.api.Service; +import org.jboss.weld.bootstrap.api.helpers.AbstractBootstrapService; import org.jboss.weld.resources.ReflectionCache; import org.jboss.weld.util.reflection.Reflections; @@ -34,10 +38,16 @@ * @author Jozef Hartinger * */ -public class RequiredAnnotationDiscovery implements Service { +public class RequiredAnnotationDiscovery extends AbstractBootstrapService { private final ReflectionCache cache; + /** + * Complete annotation sets from searches that reached the end without a match. Successful searches + * short-circuit without publishing a partial set. + */ + private final ConcurrentMap, Set>> matchingAnnotations = new ConcurrentHashMap<>(); + public RequiredAnnotationDiscovery(ReflectionCache cache) { this.cache = cache; } @@ -81,39 +91,36 @@ public RequiredAnnotationDiscovery(ReflectionCache cache) { * @return */ public boolean containsAnnotation(BackedAnnotatedType annotatedType, Class requiredAnnotation) { + Set> cached = matchingAnnotations.get(annotatedType.getJavaClass()); + if (cached != null) { + return cached.contains(requiredAnnotation); + } + Set> found = new HashSet<>(); // class level annotations - if (containsAnnotation(annotatedType.getAnnotations(), requiredAnnotation, true)) { + if (containsAnnotations(annotatedType.getAnnotations(), requiredAnnotation, found)) { return true; } for (Class clazz = annotatedType.getJavaClass(); clazz != null && clazz != Object.class; clazz = clazz.getSuperclass()) { // fields for (Field field : clazz.getDeclaredFields()) { - if (containsAnnotations(cache.getAnnotations(field), requiredAnnotation)) { + if (containsAnnotations(cache.getAnnotations(field), requiredAnnotation, found)) { return true; } } // constructors for (Constructor constructor : clazz.getDeclaredConstructors()) { - if (containsAnnotations(cache.getAnnotations(constructor), requiredAnnotation)) { + if (containsAnnotations(cache.getAnnotations(constructor), requiredAnnotation, found) + || containsParameterAnnotations(constructor.getParameterAnnotations(), requiredAnnotation, found)) { return true; } - for (Annotation[] parameterAnnotations : constructor.getParameterAnnotations()) { - if (containsAnnotations(parameterAnnotations, requiredAnnotation)) { - return true; - } - } } // methods for (Method method : clazz.getDeclaredMethods()) { - if (containsAnnotations(cache.getAnnotations(method), requiredAnnotation)) { + if (containsAnnotations(cache.getAnnotations(method), requiredAnnotation, found) + || containsParameterAnnotations(method.getParameterAnnotations(), requiredAnnotation, found)) { return true; } - for (Annotation[] parameterAnnotations : method.getParameterAnnotations()) { - if (containsAnnotations(parameterAnnotations, requiredAnnotation)) { - return true; - } - } } } @@ -121,28 +128,31 @@ public boolean containsAnnotation(BackedAnnotatedType annotatedType, Class interfaceClazz : Reflections.getInterfaceClosure(annotatedType.getJavaClass())) { for (Method method : interfaceClazz.getDeclaredMethods()) { if (Reflections.isDefault(method)) { - if (containsAnnotations(cache.getAnnotations(method), requiredAnnotation)) { + if (containsAnnotations(cache.getAnnotations(method), requiredAnnotation, found) + || containsParameterAnnotations(method.getParameterAnnotations(), requiredAnnotation, found)) { return true; } - for (Annotation[] parameterAnnotations : method.getParameterAnnotations()) { - if (containsAnnotations(parameterAnnotations, requiredAnnotation)) { - return true; - } - } } } } + matchingAnnotations.putIfAbsent(annotatedType.getJavaClass(), found); return false; } - private boolean containsAnnotations(Annotation[] annotations, Class requiredAnnotation) { - return containsAnnotation(annotations, requiredAnnotation, true); + private boolean containsParameterAnnotations(Annotation[][] parameterAnnotations, + Class requiredAnnotation, Set> found) { + for (Annotation[] annotations : parameterAnnotations) { + if (containsAnnotations(annotations, requiredAnnotation, found)) { + return true; + } + } + return false; } - private boolean containsAnnotation(Annotation[] annotations, Class requiredAnnotation, - boolean checkMetaAnnotations) { + private boolean containsAnnotations(Annotation[] annotations, Class requiredAnnotation, + Set> found) { for (Annotation annotation : annotations) { - if (containsAnnotation(annotation, requiredAnnotation, checkMetaAnnotations)) { + if (containsAnnotation(annotation, requiredAnnotation, found)) { return true; } } @@ -150,14 +160,10 @@ private boolean containsAnnotation(Annotation[] annotations, Class annotations, - Class requiredAnnotation) { - return containsAnnotation(annotations, requiredAnnotation, true); - } - - private boolean containsAnnotation(Iterable annotations, - Class requiredAnnotation, boolean checkMetaAnnotations) { + Class requiredAnnotation, + Set> found) { for (Annotation annotation : annotations) { - if (containsAnnotation(annotation, requiredAnnotation, checkMetaAnnotations)) { + if (containsAnnotation(annotation, requiredAnnotation, found)) { return true; } } @@ -165,18 +171,23 @@ private boolean containsAnnotation(Iterable annotations, } private boolean containsAnnotation(Annotation annotation, Class requiredAnnotation, - boolean checkMetaAnnotations) { + Set> found) { Class annotationType = annotation.annotationType(); if (requiredAnnotation.equals(annotationType)) { return true; } - if (checkMetaAnnotations && containsAnnotation(cache.getAnnotations(annotationType), requiredAnnotation, false)) { - return true; + found.add(annotationType); + for (Annotation metaAnnotation : cache.getAnnotations(annotationType)) { + if (requiredAnnotation.equals(metaAnnotation.annotationType())) { + return true; + } + found.add(metaAnnotation.annotationType()); } return false; } @Override - public void cleanup() { + public void cleanupAfterBoot() { + matchingAnnotations.clear(); } } diff --git a/impl/src/test/java/org/jboss/weld/annotated/enhanced/jlr/EnhancedAnnotatedTypeOverriddenMethodsTest.java b/impl/src/test/java/org/jboss/weld/annotated/enhanced/jlr/EnhancedAnnotatedTypeOverriddenMethodsTest.java new file mode 100644 index 00000000000..eff3b2d617f --- /dev/null +++ b/impl/src/test/java/org/jboss/weld/annotated/enhanced/jlr/EnhancedAnnotatedTypeOverriddenMethodsTest.java @@ -0,0 +1,68 @@ +/* + * Copyright The Weld Authors + * SPDX-License-Identifier: Apache-2.0 + */ +package org.jboss.weld.annotated.enhanced.jlr; + +import static org.junit.Assert.assertEquals; + +import java.lang.reflect.Method; +import java.util.Collection; +import java.util.Set; +import java.util.stream.Collectors; + +import jakarta.enterprise.inject.spi.AnnotatedMethod; + +import org.jboss.weld.annotated.enhanced.EnhancedAnnotatedType; +import org.jboss.weld.annotated.slim.AnnotatedTypeIdentifier; +import org.jboss.weld.bootstrap.api.helpers.RegistrySingletonProvider; +import org.jboss.weld.metadata.TypeStore; +import org.jboss.weld.resources.ClassTransformer; +import org.jboss.weld.resources.ReflectionCacheFactory; +import org.jboss.weld.resources.SharedObjectCache; +import org.junit.Test; + +public class EnhancedAnnotatedTypeOverriddenMethodsTest { + + static class Base { + public Object value() { + return null; + } + } + + static class Middle extends Base { + @Override + public String value() { + return ""; + } + } + + static class Leaf extends Middle { + @Override + public String value() { + return ""; + } + } + + private final TypeStore typeStore = new TypeStore(); + private final ClassTransformer transformer = new ClassTransformer(typeStore, new SharedObjectCache(), + ReflectionCacheFactory.newInstance(typeStore), RegistrySingletonProvider.STATIC_INSTANCE); + + private EnhancedAnnotatedType enhancedType(Class javaClass) { + return transformer.getEnhancedAnnotatedType(javaClass, + AnnotatedTypeIdentifier.NULL_BDA_ID); + } + + private static Set nonBridgeMethods(Collection> methods) { + return methods.stream().map(AnnotatedMethod::getJavaMember).filter(method -> !method.isBridge()) + .collect(Collectors.toSet()); + } + + @Test + public void resolvesCovariantOverridesAcrossTheHierarchy() throws NoSuchMethodException { + EnhancedAnnotatedType leaf = enhancedType(Leaf.class); + assertEquals(Set.of(Leaf.class.getDeclaredMethod("value")), nonBridgeMethods(leaf.getEnhancedMethods())); + assertEquals(Set.of(Base.class.getDeclaredMethod("value"), Middle.class.getDeclaredMethod("value"), + Leaf.class.getDeclaredMethod("value")), nonBridgeMethods(leaf.getMethods())); + } +} diff --git a/impl/src/test/java/org/jboss/weld/bootstrap/events/RequiredAnnotationDiscoveryTest.java b/impl/src/test/java/org/jboss/weld/bootstrap/events/RequiredAnnotationDiscoveryTest.java new file mode 100644 index 00000000000..a980a997119 --- /dev/null +++ b/impl/src/test/java/org/jboss/weld/bootstrap/events/RequiredAnnotationDiscoveryTest.java @@ -0,0 +1,79 @@ +/* + * Copyright The Weld Authors + * SPDX-License-Identifier: Apache-2.0 + */ +package org.jboss.weld.bootstrap.events; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.lang.annotation.Annotation; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.reflect.AnnotatedElement; +import java.util.Set; + +import org.jboss.weld.annotated.slim.AnnotatedTypeIdentifier; +import org.jboss.weld.annotated.slim.backed.BackedAnnotatedType; +import org.jboss.weld.metadata.TypeStore; +import org.jboss.weld.resources.DefaultReflectionCache; +import org.jboss.weld.resources.SharedObjectCache; +import org.junit.Test; + +public class RequiredAnnotationDiscoveryTest { + + @Retention(RetentionPolicy.RUNTIME) + @interface OnClass { + } + + @Retention(RetentionPolicy.RUNTIME) + @interface OnField { + } + + @OnClass + static class Bean { + @OnField + String field; + } + + static class CountingReflectionCache extends DefaultReflectionCache { + int annotationReads; + + CountingReflectionCache() { + super(new TypeStore()); + } + + @Override + public Set getAnnotations(AnnotatedElement element) { + annotationReads++; + return super.getAnnotations(element); + } + } + + private final CountingReflectionCache cache = new CountingReflectionCache(); + private final RequiredAnnotationDiscovery discovery = new RequiredAnnotationDiscovery(cache); + private final BackedAnnotatedType type = BackedAnnotatedType.of(Bean.class, new SharedObjectCache(), cache, + "test", AnnotatedTypeIdentifier.NULL_BDA_ID); + + @Test + public void earlyMatchStopsReflectionWithoutCachingAnIncompleteSet() { + type.getAnnotations(); + cache.annotationReads = 0; + + assertTrue(discovery.containsAnnotation(type, OnClass.class)); + assertEquals("A direct match must not inspect meta-annotations or members", 0, cache.annotationReads); + assertTrue(discovery.containsAnnotation(type, OnField.class)); + } + + @Test + public void completedSearchCachesBothMatchesAndMisses() { + assertFalse(discovery.containsAnnotation(type, Deprecated.class)); + cache.annotationReads = 0; + + assertTrue(discovery.containsAnnotation(type, OnClass.class)); + assertTrue(discovery.containsAnnotation(type, OnField.class)); + assertFalse(discovery.containsAnnotation(type, FunctionalInterface.class)); + assertEquals("A completed search must avoid reflection for subsequent queries", 0, cache.annotationReads); + } +}