This is an automated email from the ASF dual-hosted git repository. mattsicker pushed a commit to branch mean-bean-machine in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit a789af3811e6cf6eb3aa86052abcba53a8e1c7f3 Author: Matt Sicker <[email protected]> AuthorDate: Sat Jun 26 16:31:31 2021 -0500 Add tests and stable ordering for bean inheritance --- .../core/config/di/impl/DefaultBeanManager.java | 20 ++-- .../di/impl/DefaultInjectionTargetFactory.java | 90 +++++++++----- .../log4j/core/config/di/BeanManagerTest.java | 131 ++++++++++++++++++++- .../logging/log4j/plugins/di/SingletonScoped.java | 1 - 4 files changed, 204 insertions(+), 38 deletions(-) diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/di/impl/DefaultBeanManager.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/di/impl/DefaultBeanManager.java index c13f16e..a835ae0 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/di/impl/DefaultBeanManager.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/di/impl/DefaultBeanManager.java @@ -86,16 +86,20 @@ public class DefaultBeanManager implements BeanManager { for (final Class<?> beanClass : beanClasses) { final Bean<?> bean = isInjectable(beanClass) ? createBean(beanClass) : null; loadDisposerMethods(beanClass, bean); - for (final Method method : beanClass.getDeclaredMethods()) { - if (AnnotationUtil.isAnnotationPresent(method, Produces.class)) { - method.setAccessible(true); - loadedBeans.add(createBean(method, bean)); + for (Class<?> clazz = beanClass; clazz != null; clazz = clazz.getSuperclass()) { + for (final Method method : clazz.getDeclaredMethods()) { + if (AnnotationUtil.isAnnotationPresent(method, Produces.class)) { + method.setAccessible(true); + loadedBeans.add(createBean(method, bean)); + } } } - for (final Field field : beanClass.getDeclaredFields()) { - if (AnnotationUtil.isAnnotationPresent(field, Produces.class)) { - field.setAccessible(true); - loadedBeans.add(createBean(field, bean)); + for (Class<?> clazz = beanClass; clazz != null; clazz = clazz.getSuperclass()) { + for (final Field field : clazz.getDeclaredFields()) { + if (AnnotationUtil.isAnnotationPresent(field, Produces.class)) { + field.setAccessible(true); + loadedBeans.add(createBean(field, bean)); + } } } if (bean != null) { diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/di/impl/DefaultInjectionTargetFactory.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/di/impl/DefaultInjectionTargetFactory.java index f321cc1..4d99fd7 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/di/impl/DefaultInjectionTargetFactory.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/di/impl/DefaultInjectionTargetFactory.java @@ -29,6 +29,7 @@ import org.apache.logging.log4j.plugins.di.PreDestroy; import org.apache.logging.log4j.plugins.util.AnnotationUtil; import org.apache.logging.log4j.plugins.util.TypeUtil; +import java.lang.reflect.AccessibleObject; import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.lang.reflect.Method; @@ -36,7 +37,7 @@ import java.lang.reflect.Modifier; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; -import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; import java.util.stream.Collectors; @@ -55,33 +56,12 @@ class DefaultInjectionTargetFactory<T> implements InjectionTargetFactory<T> { public InjectionTarget<T> createInjectionTarget(final Bean<T> bean) { final Constructor<T> constructor = getInjectableConstructor(); final Collection<InjectionPoint> injectionPoints = - new HashSet<>(beanManager.createExecutableInjectionPoints(constructor, bean)); - for (final Field field : type.getDeclaredFields()) { - if (beanManager.isInjectable(field)) { - // TODO: if field is static, validate it's using an appropriate scope (singleton?) - field.setAccessible(true); - injectionPoints.add(beanManager.createFieldInjectionPoint(field, bean)); - } - } - final List<Method> methods = new ArrayList<>(); - for (final Method method : type.getDeclaredMethods()) { - methods.add(0, method); - if (!Modifier.isStatic(method.getModifiers()) && beanManager.isInjectable(method)) { - method.setAccessible(true); - injectionPoints.addAll(beanManager.createExecutableInjectionPoints(method, bean)); - } - } - // FIXME: verify these methods are ordered properly - final List<Method> postConstructMethods = methods.stream() - .filter(method -> AnnotationUtil.isAnnotationPresent(method, PostConstruct.class)) - .peek(method -> method.setAccessible(true)) - .collect(Collectors.toList()); - final List<Method> preDestroyMethods = methods.stream() - .filter(method -> AnnotationUtil.isAnnotationPresent(method, PreDestroy.class)) - .peek(method -> method.setAccessible(true)) - .collect(Collectors.toList()); + new LinkedHashSet<>(beanManager.createExecutableInjectionPoints(constructor, bean)); + // TODO: if field is static, validate it's using an appropriate scope (singleton?) + getInjectableFields().forEach(field -> injectionPoints.add(beanManager.createFieldInjectionPoint(field, bean))); + getInjectableMethods().forEach(method -> injectionPoints.addAll(beanManager.createExecutableInjectionPoints(method, bean))); return new DefaultInjectionTarget<>(injector, type, injectionPoints, constructor, - postConstructMethods, preDestroyMethods); + getPostConstructMethods(), getPreDestroyMethods()); } private Constructor<T> getInjectableConstructor() { @@ -121,4 +101,60 @@ class DefaultInjectionTargetFactory<T> implements InjectionTargetFactory<T> { throw new DefinitionException("No candidate constructors found for " + type); } } + + private List<Field> getInjectableFields() { + final List<Field> injectableFields = new ArrayList<>(); + for (Class<?> clazz = type; clazz != null; clazz = clazz.getSuperclass()) { + for (final Field field : clazz.getDeclaredFields()) { + if (beanManager.isInjectable(field)) { + injectableFields.add(field); + } + } + } + final Field[] fields = injectableFields.toArray(Field[]::new); + AccessibleObject.setAccessible(fields, true); + return List.of(fields); + } + + private List<Method> getInjectableMethods() { + final List<Method> injectableMethods = new ArrayList<>(); + for (Class<?> clazz = type; clazz != null; clazz = clazz.getSuperclass()) { + for (final Method method : clazz.getDeclaredMethods()) { + if (!Modifier.isStatic(method.getModifiers()) && beanManager.isInjectable(method)) { + injectableMethods.add(method); + } + } + } + final Method[] methods = injectableMethods.toArray(Method[]::new); + AccessibleObject.setAccessible(methods, true); + return List.of(methods); + } + + private List<Method> getPostConstructMethods() { + final List<Method> postConstructMethods = new ArrayList<>(); + for (Class<?> clazz = type; clazz != null; clazz = clazz.getSuperclass()) { + for (final Method method : clazz.getDeclaredMethods()) { + if (AnnotationUtil.isAnnotationPresent(method, PostConstruct.class)) { + postConstructMethods.add(0, method); + } + } + } + final Method[] methods = postConstructMethods.toArray(Method[]::new); + AccessibleObject.setAccessible(methods, true); + return List.of(methods); + } + + private List<Method> getPreDestroyMethods() { + final List<Method> preDestroyMethods = new ArrayList<>(); + for (Class<?> clazz = type; clazz != null; clazz = clazz.getSuperclass()) { + for (final Method method : clazz.getDeclaredMethods()) { + if (AnnotationUtil.isAnnotationPresent(method, PreDestroy.class)) { + preDestroyMethods.add(method); + } + } + } + final Method[] methods = preDestroyMethods.toArray(Method[]::new); + AccessibleObject.setAccessible(methods, true); + return List.of(methods); + } } diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/config/di/BeanManagerTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/config/di/BeanManagerTest.java index ec6364b..1432ea3 100644 --- a/log4j-core/src/test/java/org/apache/logging/log4j/core/config/di/BeanManagerTest.java +++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/config/di/BeanManagerTest.java @@ -30,6 +30,7 @@ import org.junit.jupiter.api.Test; import java.util.Optional; import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.IntStream; import static org.junit.jupiter.api.Assertions.*; @@ -166,7 +167,8 @@ class BeanManagerTest { } static class ScopeTestingBean { - @Named("dep") DependentBean field; + @Named("dep") + DependentBean field; DependentBean first; DependentBean second; @@ -473,7 +475,132 @@ class BeanManagerTest { } } + static class PostConstructBaseBean { + final int baseConstructorParameterA; + final int baseConstructorParameterB; + @Inject + int baseInjectedField; + int basePostConstructValue; + int baseInjectedParameterA; + int baseInjectedParameterB; + @Inject + IdGenerator idGenerator; + + @Inject + PostConstructBaseBean(int baseConstructorParameterA, int baseConstructorParameterB) { + this.baseConstructorParameterA = baseConstructorParameterA; + this.baseConstructorParameterB = baseConstructorParameterB; + } + + @PostConstruct + void setupBase() { + basePostConstructValue = idGenerator.nextId(); + } + + @Inject + void setValues(int e, int f) { + this.baseInjectedParameterA = e; + this.baseInjectedParameterB = f; + } + } + + static class PostConstructBean extends PostConstructBaseBean { + final int implConstructorParameter; + int implPostConstructValue; + int implInjectedParameter; + @Inject + int implInjectedField; + + @Inject + PostConstructBean(final int a, final int b, final int implConstructorParameter) { + super(a, b); + this.implConstructorParameter = implConstructorParameter; + } + + @PostConstruct + void setupBean() { + implPostConstructValue = idGenerator.nextId(); + } + + @Inject + void setImplInjectedParameter(int implInjectedParameter) { + this.implInjectedParameter = implInjectedParameter; + } + } + + @Test + void postConstructInheritanceOrdering() { + beanManager.loadAndValidateBeans(IdGenerator.class, PostConstructBean.class); + final Bean<PostConstructBean> bean = beanManager.getDefaultBean(PostConstructBean.class).orElseThrow(); + try (final var context = beanManager.createInitializationContext(null)) { + final PostConstructBean value = beanManager.getValue(bean, context); + int[] values = { + value.baseConstructorParameterA, + value.baseConstructorParameterB, + value.implConstructorParameter, + value.implInjectedField, + value.baseInjectedField, + value.implInjectedParameter, + value.baseInjectedParameterA, + value.baseInjectedParameterB, + value.basePostConstructValue, + value.implPostConstructValue + }; + assertAll(IntStream.range(0, values.length).mapToObj(i -> () -> assertEquals(i + 1, values[i]))); + } + } + + static class PreDestroyBaseBean { + @Inject + IdGenerator idGenerator; + int baseValue; + + @PreDestroy + void basePreDestroy() { + baseValue = idGenerator.nextId(); + } + } + + static class PreDestroyIntermediateBean extends PreDestroyBaseBean { + int intermediateValue; + + @PreDestroy + void intermediatePreDestroy() { + intermediateValue = idGenerator.nextId(); + } + } + + static class PreDestroyImplBean extends PreDestroyIntermediateBean { + int value; + + @PreDestroy + void tearDown() { + value = idGenerator.nextId(); + } + } + + @Test + void preDestroyInheritanceOrdering() { + beanManager.loadAndValidateBeans(IdGenerator.class, PreDestroyImplBean.class); + final Bean<PreDestroyImplBean> bean = beanManager.getDefaultBean(PreDestroyImplBean.class).orElseThrow(); + final PreDestroyImplBean value; + try (final var context = beanManager.createInitializationContext(bean)) { + value = beanManager.getValue(bean, context); + assertAll( + () -> assertEquals(0, value.value), + () -> assertEquals(0, value.intermediateValue), + () -> assertEquals(0, value.baseValue) + ); + } + assertAll( + () -> assertEquals(1, value.value), + () -> assertEquals(2, value.intermediateValue), + () -> assertEquals(3, value.baseValue) + ); + } + // TODO: add tests for other supported injection scenarios // TODO: add tests for hierarchical scopes - // TODO: add tests for @Named alias annotations like @PluginAttribute == @Named + // TODO: add tests for @Disposes + // TODO: add tests for injecting more specific types than the available ones } diff --git a/log4j-plugins/src/main/java/org/apache/logging/log4j/plugins/di/SingletonScoped.java b/log4j-plugins/src/main/java/org/apache/logging/log4j/plugins/di/SingletonScoped.java index 5aef475..53fafe0 100644 --- a/log4j-plugins/src/main/java/org/apache/logging/log4j/plugins/di/SingletonScoped.java +++ b/log4j-plugins/src/main/java/org/apache/logging/log4j/plugins/di/SingletonScoped.java @@ -24,7 +24,6 @@ import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; -// TODO: consider making this the default scope? @Retention(RetentionPolicy.RUNTIME) @Target({ElementType.TYPE, ElementType.FIELD, ElementType.METHOD}) @Documented
