From f67f98a1a7220e77f8d04405a879e1a051fc7d23 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Wed, 21 Jun 2023 15:56:50 +0200 Subject: [PATCH 1/2] Polishing --- ...nitDestroyAnnotationBeanPostProcessor.java | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/annotation/InitDestroyAnnotationBeanPostProcessor.java b/spring-beans/src/main/java/org/springframework/beans/factory/annotation/InitDestroyAnnotationBeanPostProcessor.java index bd30995a0c3..063b1e43432 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/annotation/InitDestroyAnnotationBeanPostProcessor.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/annotation/InitDestroyAnnotationBeanPostProcessor.java @@ -152,8 +152,8 @@ public class InitDestroyAnnotationBeanPostProcessor implements DestructionAwareB @Override - public void postProcessMergedBeanDefinition(RootBeanDefinition beanDefinition, Class beanType, String beanName) { - findLifecycleMetadata(beanDefinition, beanType); + public void postProcessMergedBeanDefinition(RootBeanDefinition beanDefinition, Class beanClass, String beanName) { + findLifecycleMetadata(beanDefinition, beanClass); } @Override @@ -173,8 +173,8 @@ public class InitDestroyAnnotationBeanPostProcessor implements DestructionAwareB return null; } - private LifecycleMetadata findLifecycleMetadata(RootBeanDefinition beanDefinition, Class beanType) { - LifecycleMetadata metadata = findLifecycleMetadata(beanType); + private LifecycleMetadata findLifecycleMetadata(RootBeanDefinition beanDefinition, Class beanClass) { + LifecycleMetadata metadata = findLifecycleMetadata(beanClass); metadata.checkInitDestroyMethods(beanDefinition); return metadata; } @@ -234,19 +234,19 @@ public class InitDestroyAnnotationBeanPostProcessor implements DestructionAwareB } - private LifecycleMetadata findLifecycleMetadata(Class clazz) { + private LifecycleMetadata findLifecycleMetadata(Class beanClass) { if (this.lifecycleMetadataCache == null) { // Happens after deserialization, during destruction... - return buildLifecycleMetadata(clazz); + return buildLifecycleMetadata(beanClass); } // Quick check on the concurrent map first, with minimal locking. - LifecycleMetadata metadata = this.lifecycleMetadataCache.get(clazz); + LifecycleMetadata metadata = this.lifecycleMetadataCache.get(beanClass); if (metadata == null) { synchronized (this.lifecycleMetadataCache) { - metadata = this.lifecycleMetadataCache.get(clazz); + metadata = this.lifecycleMetadataCache.get(beanClass); if (metadata == null) { - metadata = buildLifecycleMetadata(clazz); - this.lifecycleMetadataCache.put(clazz, metadata); + metadata = buildLifecycleMetadata(beanClass); + this.lifecycleMetadataCache.put(beanClass, metadata); } return metadata; } @@ -254,42 +254,42 @@ public class InitDestroyAnnotationBeanPostProcessor implements DestructionAwareB return metadata; } - private LifecycleMetadata buildLifecycleMetadata(final Class clazz) { - if (!AnnotationUtils.isCandidateClass(clazz, List.of(this.initAnnotationType, this.destroyAnnotationType))) { + private LifecycleMetadata buildLifecycleMetadata(final Class beanClass) { + if (!AnnotationUtils.isCandidateClass(beanClass, List.of(this.initAnnotationType, this.destroyAnnotationType))) { return this.emptyLifecycleMetadata; } List initMethods = new ArrayList<>(); List destroyMethods = new ArrayList<>(); - Class targetClass = clazz; + Class currentClass = beanClass; do { final List currInitMethods = new ArrayList<>(); final List currDestroyMethods = new ArrayList<>(); - ReflectionUtils.doWithLocalMethods(targetClass, method -> { + ReflectionUtils.doWithLocalMethods(currentClass, method -> { if (this.initAnnotationType != null && method.isAnnotationPresent(this.initAnnotationType)) { currInitMethods.add(new LifecycleMethod(method)); if (logger.isTraceEnabled()) { - logger.trace("Found init method on class [" + clazz.getName() + "]: " + method); + logger.trace("Found init method on class [" + beanClass.getName() + "]: " + method); } } if (this.destroyAnnotationType != null && method.isAnnotationPresent(this.destroyAnnotationType)) { currDestroyMethods.add(new LifecycleMethod(method)); if (logger.isTraceEnabled()) { - logger.trace("Found destroy method on class [" + clazz.getName() + "]: " + method); + logger.trace("Found destroy method on class [" + beanClass.getName() + "]: " + method); } } }); initMethods.addAll(0, currInitMethods); destroyMethods.addAll(currDestroyMethods); - targetClass = targetClass.getSuperclass(); + currentClass = currentClass.getSuperclass(); } - while (targetClass != null && targetClass != Object.class); + while (currentClass != null && currentClass != Object.class); return (initMethods.isEmpty() && destroyMethods.isEmpty() ? this.emptyLifecycleMetadata : - new LifecycleMetadata(clazz, initMethods, destroyMethods)); + new LifecycleMetadata(beanClass, initMethods, destroyMethods)); } @@ -311,7 +311,7 @@ public class InitDestroyAnnotationBeanPostProcessor implements DestructionAwareB */ private class LifecycleMetadata { - private final Class targetClass; + private final Class beanClass; private final Collection initMethods; @@ -323,10 +323,10 @@ public class InitDestroyAnnotationBeanPostProcessor implements DestructionAwareB @Nullable private volatile Set checkedDestroyMethods; - public LifecycleMetadata(Class targetClass, Collection initMethods, + public LifecycleMetadata(Class beanClass, Collection initMethods, Collection destroyMethods) { - this.targetClass = targetClass; + this.beanClass = beanClass; this.initMethods = initMethods; this.destroyMethods = destroyMethods; } @@ -339,7 +339,7 @@ public class InitDestroyAnnotationBeanPostProcessor implements DestructionAwareB beanDefinition.registerExternallyManagedInitMethod(methodIdentifier); checkedInitMethods.add(lifecycleMethod); if (logger.isTraceEnabled()) { - logger.trace("Registered init method on class [" + this.targetClass.getName() + "]: " + methodIdentifier); + logger.trace("Registered init method on class [" + this.beanClass.getName() + "]: " + methodIdentifier); } } } @@ -350,7 +350,7 @@ public class InitDestroyAnnotationBeanPostProcessor implements DestructionAwareB beanDefinition.registerExternallyManagedDestroyMethod(methodIdentifier); checkedDestroyMethods.add(lifecycleMethod); if (logger.isTraceEnabled()) { - logger.trace("Registered destroy method on class [" + this.targetClass.getName() + "]: " + methodIdentifier); + logger.trace("Registered destroy method on class [" + this.beanClass.getName() + "]: " + methodIdentifier); } } } From 0eb33d09ace20e73605e1c424820d806cb1ddad5 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Wed, 21 Jun 2023 16:18:12 +0200 Subject: [PATCH 2/2] Ensure package-private init/destroy methods are always invoked Prior to this commit, if an init/destroy method was package-private and declared in a superclass in a package different from the package in which the registered bean resided, a local init/destroy method with the same name would effectively "shadow" the method from the different package, resulting in only the local init/destroy method being invoked. This commit addresses this issue by tracking package-private init methods from different packages using their fully-qualified method names, analogous to the existing support for private init/destroy methods. Closes gh-30718 --- ...nitDestroyAnnotationBeanPostProcessor.java | 25 ++++++-- .../factory/support/RootBeanDefinition.java | 15 ++--- .../InitDestroyMethodLifecycleTests.java | 57 +++++++++++++------ .../lifecyclemethods/InitDestroyBean.java | 36 ++++++++++++ .../PackagePrivateInitDestroyBean.java | 34 +++++++++++ 5 files changed, 138 insertions(+), 29 deletions(-) create mode 100644 spring-context/src/test/java/org/springframework/context/annotation/lifecyclemethods/InitDestroyBean.java create mode 100644 spring-context/src/test/java/org/springframework/context/annotation/lifecyclemethods/PackagePrivateInitDestroyBean.java diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/annotation/InitDestroyAnnotationBeanPostProcessor.java b/spring-beans/src/main/java/org/springframework/beans/factory/annotation/InitDestroyAnnotationBeanPostProcessor.java index 063b1e43432..df1e11632ee 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/annotation/InitDestroyAnnotationBeanPostProcessor.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/annotation/InitDestroyAnnotationBeanPostProcessor.java @@ -269,13 +269,13 @@ public class InitDestroyAnnotationBeanPostProcessor implements DestructionAwareB ReflectionUtils.doWithLocalMethods(currentClass, method -> { if (this.initAnnotationType != null && method.isAnnotationPresent(this.initAnnotationType)) { - currInitMethods.add(new LifecycleMethod(method)); + currInitMethods.add(new LifecycleMethod(method, beanClass)); if (logger.isTraceEnabled()) { logger.trace("Found init method on class [" + beanClass.getName() + "]: " + method); } } if (this.destroyAnnotationType != null && method.isAnnotationPresent(this.destroyAnnotationType)) { - currDestroyMethods.add(new LifecycleMethod(method)); + currDestroyMethods.add(new LifecycleMethod(method, beanClass)); if (logger.isTraceEnabled()) { logger.trace("Found destroy method on class [" + beanClass.getName() + "]: " + method); } @@ -404,12 +404,12 @@ public class InitDestroyAnnotationBeanPostProcessor implements DestructionAwareB private final String identifier; - public LifecycleMethod(Method method) { + public LifecycleMethod(Method method, Class beanClass) { if (method.getParameterCount() != 0) { throw new IllegalStateException("Lifecycle annotation requires a no-arg method: " + method); } this.method = method; - this.identifier = (Modifier.isPrivate(method.getModifiers()) ? + this.identifier = (isPrivateOrNotVisible(method, beanClass) ? ClassUtils.getQualifiedMethodName(method) : method.getName()); } @@ -436,6 +436,23 @@ public class InitDestroyAnnotationBeanPostProcessor implements DestructionAwareB public int hashCode() { return this.identifier.hashCode(); } + + /** + * Determine if the supplied lifecycle {@link Method} is private or not + * visible to the supplied bean {@link Class}. + * @since 6.0.11 + */ + private static boolean isPrivateOrNotVisible(Method method, Class beanClass) { + int modifiers = method.getModifiers(); + if (Modifier.isPrivate(modifiers)) { + return true; + } + // Method is declared in a class that resides in a different package + // than the bean class and the method is neither public nor protected? + return (!method.getDeclaringClass().getPackageName().equals(beanClass.getPackageName()) && + !(Modifier.isPublic(modifiers) || Modifier.isProtected(modifiers))); + } + } } diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/RootBeanDefinition.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/RootBeanDefinition.java index 7f331d3b5f2..68c865c4d1b 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/RootBeanDefinition.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/RootBeanDefinition.java @@ -497,14 +497,15 @@ public class RootBeanDefinition extends AbstractBeanDefinition { /** * Register an externally managed configuration initialization method — - * for example, a method annotated with JSR-250's - * {@link jakarta.annotation.PostConstruct} annotation. - *

The supplied {@code initMethod} may be the - * {@linkplain Method#getName() simple method name} for non-private methods or the + * for example, a method annotated with JSR-250's {@link javax.annotation.PostConstruct} + * or Jakarta's {@link jakarta.annotation.PostConstruct} annotation. + *

The supplied {@code initMethod} may be a + * {@linkplain Method#getName() simple method name} or a * {@linkplain org.springframework.util.ClassUtils#getQualifiedMethodName(Method) - * qualified method name} for {@code private} methods. A qualified name is - * necessary for {@code private} methods in order to disambiguate between - * multiple private methods with the same name within a class hierarchy. + * qualified method name} for package-private and {@code private} methods. + * A qualified name is necessary for package-private and {@code private} methods + * in order to disambiguate between multiple such methods with the same name + * within a type hierarchy. */ public void registerExternallyManagedInitMethod(String initMethod) { synchronized (this.postProcessingLock) { diff --git a/spring-context/src/test/java/org/springframework/context/annotation/InitDestroyMethodLifecycleTests.java b/spring-context/src/test/java/org/springframework/context/annotation/InitDestroyMethodLifecycleTests.java index 13f5de72ca5..8ac8784b6a7 100644 --- a/spring-context/src/test/java/org/springframework/context/annotation/InitDestroyMethodLifecycleTests.java +++ b/spring-context/src/test/java/org/springframework/context/annotation/InitDestroyMethodLifecycleTests.java @@ -28,6 +28,8 @@ import org.springframework.beans.factory.InitializingBean; import org.springframework.beans.factory.annotation.InitDestroyAnnotationBeanPostProcessor; import org.springframework.beans.factory.support.DefaultListableBeanFactory; import org.springframework.beans.factory.support.RootBeanDefinition; +import org.springframework.context.annotation.lifecyclemethods.InitDestroyBean; +import org.springframework.context.annotation.lifecyclemethods.PackagePrivateInitDestroyBean; import static org.assertj.core.api.Assertions.assertThat; @@ -53,11 +55,11 @@ class InitDestroyMethodLifecycleTests { @Test void initDestroyMethods() { Class beanClass = InitDestroyBean.class; - DefaultListableBeanFactory beanFactory = createBeanFactoryAndRegisterBean(beanClass, "afterPropertiesSet", "destroy"); + DefaultListableBeanFactory beanFactory = createBeanFactoryAndRegisterBean(beanClass, "initMethod", "destroyMethod"); InitDestroyBean bean = beanFactory.getBean(InitDestroyBean.class); - assertThat(bean.initMethods).as("init-methods").containsExactly("afterPropertiesSet"); + assertThat(bean.initMethods).as("init-methods").containsExactly("initMethod"); beanFactory.destroySingletons(); - assertThat(bean.destroyMethods).as("destroy-methods").containsExactly("destroy"); + assertThat(bean.destroyMethods).as("destroy-methods").containsExactly("destroyMethod"); } @Test @@ -132,6 +134,26 @@ class InitDestroyMethodLifecycleTests { ); } + @Test + void jakartaAnnotationsCustomPackagePrivateInitDestroyMethodsWithTheSameMethodNames() { + Class beanClass = SubPackagePrivateInitDestroyBean.class; + DefaultListableBeanFactory beanFactory = createBeanFactoryAndRegisterBean(beanClass, "initMethod", "destroyMethod"); + SubPackagePrivateInitDestroyBean bean = beanFactory.getBean(SubPackagePrivateInitDestroyBean.class); + + assertThat(bean.initMethods).as("init-methods").containsExactly( + "PackagePrivateInitDestroyBean.postConstruct", + "SubPackagePrivateInitDestroyBean.postConstruct", + "initMethod" + ); + + beanFactory.destroySingletons(); + assertThat(bean.destroyMethods).as("destroy-methods").containsExactly( + "SubPackagePrivateInitDestroyBean.preDestroy", + "PackagePrivateInitDestroyBean.preDestroy", + "destroyMethod" + ); + } + @Test void allLifecycleMechanismsAtOnce() { Class beanClass = AllInOneBean.class; @@ -165,21 +187,6 @@ class InitDestroyMethodLifecycleTests { } - static class InitDestroyBean { - - final List initMethods = new ArrayList<>(); - final List destroyMethods = new ArrayList<>(); - - - public void afterPropertiesSet() throws Exception { - this.initMethods.add("afterPropertiesSet"); - } - - public void destroy() throws Exception { - this.destroyMethods.add("destroy"); - } - } - static class InitializingDisposableWithShadowedMethodsBean extends InitDestroyBean implements InitializingBean, DisposableBean { @@ -296,4 +303,18 @@ class InitDestroyMethodLifecycleTests { } } + static class SubPackagePrivateInitDestroyBean extends PackagePrivateInitDestroyBean { + + @PostConstruct + void postConstruct() { + this.initMethods.add("SubPackagePrivateInitDestroyBean.postConstruct"); + } + + @PreDestroy + void preDestroy() { + this.destroyMethods.add("SubPackagePrivateInitDestroyBean.preDestroy"); + } + + } + } diff --git a/spring-context/src/test/java/org/springframework/context/annotation/lifecyclemethods/InitDestroyBean.java b/spring-context/src/test/java/org/springframework/context/annotation/lifecyclemethods/InitDestroyBean.java new file mode 100644 index 00000000000..21c117c72bd --- /dev/null +++ b/spring-context/src/test/java/org/springframework/context/annotation/lifecyclemethods/InitDestroyBean.java @@ -0,0 +1,36 @@ +/* + * Copyright 2002-2023 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.context.annotation.lifecyclemethods; + +import java.util.ArrayList; +import java.util.List; + +public class InitDestroyBean { + + public final List initMethods = new ArrayList<>(); + public final List destroyMethods = new ArrayList<>(); + + + public void initMethod() { + this.initMethods.add("initMethod"); + } + + public void destroyMethod() { + this.destroyMethods.add("destroyMethod"); + } + +} diff --git a/spring-context/src/test/java/org/springframework/context/annotation/lifecyclemethods/PackagePrivateInitDestroyBean.java b/spring-context/src/test/java/org/springframework/context/annotation/lifecyclemethods/PackagePrivateInitDestroyBean.java new file mode 100644 index 00000000000..c37d6058726 --- /dev/null +++ b/spring-context/src/test/java/org/springframework/context/annotation/lifecyclemethods/PackagePrivateInitDestroyBean.java @@ -0,0 +1,34 @@ +/* + * Copyright 2002-2023 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.context.annotation.lifecyclemethods; + +import jakarta.annotation.PostConstruct; +import jakarta.annotation.PreDestroy; + +public class PackagePrivateInitDestroyBean extends InitDestroyBean { + + @PostConstruct + void postConstruct() { + this.initMethods.add("PackagePrivateInitDestroyBean.postConstruct"); + } + + @PreDestroy + void preDestroy() { + this.destroyMethods.add("PackagePrivateInitDestroyBean.preDestroy"); + } + +}