From 735051bf7d28a802598ec592ebcecaea2aeaf3dd Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Wed, 27 Jul 2022 11:33:06 +0200 Subject: [PATCH] Resolve infer destroy method at build-time This commit allows a RootBeanDefinition to resolve its infer destroy method if necessary. Contrary to BeanInstanceAdapter that uses the actual bean instance, the new method works against the type exposed in the bean definition. The AOT contribution of InitDestroyAnnotationBeanPostProcessor uses the new method to make sure the special '(inferred)' placeholder is handled prior to code generation. Closes gh-28215 --- ...nitDestroyAnnotationBeanPostProcessor.java | 1 + ...BeanDefinitionPropertiesCodeGenerator.java | 14 ++------ .../support/DisposableBeanAdapter.java | 15 ++++---- .../factory/support/RootBeanDefinition.java | 9 +++++ ...stroyAnnotationBeanPostProcessorTests.java | 26 +++++++++++++- ...efinitionPropertiesCodeGeneratorTests.java | 21 ++--------- .../support/RootBeanDefinitionTests.java | 35 +++++++++++++++++++ .../lifecycle/InferredDestroyBean.java | 26 ++++++++++++++ 8 files changed, 109 insertions(+), 38 deletions(-) create mode 100644 spring-beans/src/testFixtures/java/org/springframework/beans/testfixture/beans/factory/generator/lifecycle/InferredDestroyBean.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 1ac3014af3..6b7da1d5b2 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 @@ -159,6 +159,7 @@ public class InitDestroyAnnotationBeanPostProcessor implements DestructionAwareB @Override public BeanRegistrationAotContribution processAheadOfTime(RegisteredBean registeredBean) { RootBeanDefinition beanDefinition = registeredBean.getMergedBeanDefinition(); + beanDefinition.resolveDestroyMethodIfNecessary(); LifecycleMetadata metadata = findInjectionMetadata(beanDefinition, registeredBean.getBeanClass()); if (!CollectionUtils.isEmpty(metadata.initMethods)) { String[] initMethodNames = safeMerge(beanDefinition.getInitMethodNames(), metadata.initMethods); diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/aot/BeanDefinitionPropertiesCodeGenerator.java b/spring-beans/src/main/java/org/springframework/beans/factory/aot/BeanDefinitionPropertiesCodeGenerator.java index bd34cd24e5..d58d3745c0 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/aot/BeanDefinitionPropertiesCodeGenerator.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/aot/BeanDefinitionPropertiesCodeGenerator.java @@ -24,7 +24,6 @@ import java.lang.reflect.Method; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; -import java.util.List; import java.util.Map; import java.util.Objects; import java.util.function.BiFunction; @@ -140,23 +139,16 @@ class BeanDefinitionPropertiesCodeGenerator { private void addInitDestroyMethods(Builder builder, AbstractBeanDefinition beanDefinition, @Nullable String[] methodNames, String format) { - List filteredMethodNames = (!ObjectUtils.isEmpty(methodNames)) - ? Arrays.stream(methodNames).filter(this::isNotInferredMethod).toList() - : Collections.emptyList(); - if (!filteredMethodNames.isEmpty()) { + if (!ObjectUtils.isEmpty(methodNames)) { Class beanType = ClassUtils.getUserClass(beanDefinition.getResolvableType().toClass()); - filteredMethodNames.forEach(methodName -> addInitDestroyHint(beanType, methodName)); - CodeBlock arguments = filteredMethodNames.stream() + Arrays.stream(methodNames).forEach(methodName -> addInitDestroyHint(beanType, methodName)); + CodeBlock arguments = Arrays.stream(methodNames) .map(name -> CodeBlock.of("$S", name)) .collect(CodeBlock.joining(", ")); builder.addStatement(format, BEAN_DEFINITION_VARIABLE, arguments); } } - private boolean isNotInferredMethod(String candidate) { - return !AbstractBeanDefinition.INFER_METHOD.equals(candidate); - } - private void addInitDestroyHint(Class beanUserClass, String methodName) { Method method = ReflectionUtils.findMethod(beanUserClass, methodName); if (method != null) { diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/DisposableBeanAdapter.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/DisposableBeanAdapter.java index ad7f1489cb..374e65aa0d 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/DisposableBeanAdapter.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/DisposableBeanAdapter.java @@ -105,7 +105,7 @@ class DisposableBeanAdapter implements DisposableBean, Runnable, Serializable { this.invokeDisposableBean = (bean instanceof DisposableBean && !beanDefinition.hasAnyExternallyManagedDestroyMethod(DESTROY_METHOD_NAME)); - String[] destroyMethodNames = inferDestroyMethodsIfNecessary(bean, beanDefinition); + String[] destroyMethodNames = inferDestroyMethodsIfNecessary(bean.getClass(), beanDefinition); if (!ObjectUtils.isEmpty(destroyMethodNames) && !(this.invokeDisposableBean && DESTROY_METHOD_NAME.equals(destroyMethodNames[0])) && !beanDefinition.hasAnyExternallyManagedDestroyMethod(destroyMethodNames[0])) { @@ -325,7 +325,8 @@ class DisposableBeanAdapter implements DisposableBean, Runnable, Serializable { * @param beanDefinition the corresponding bean definition */ public static boolean hasDestroyMethod(Object bean, RootBeanDefinition beanDefinition) { - return (bean instanceof DisposableBean || inferDestroyMethodsIfNecessary(bean, beanDefinition) != null); + return (bean instanceof DisposableBean + || inferDestroyMethodsIfNecessary(bean.getClass(), beanDefinition) != null); } @@ -343,7 +344,7 @@ class DisposableBeanAdapter implements DisposableBean, Runnable, Serializable { * interfaces, reflectively calling the "close" method on implementing beans as well. */ @Nullable - private static String[] inferDestroyMethodsIfNecessary(Object bean, RootBeanDefinition beanDefinition) { + static String[] inferDestroyMethodsIfNecessary(Class target, RootBeanDefinition beanDefinition) { String[] destroyMethodNames = beanDefinition.getDestroyMethodNames(); if (destroyMethodNames != null && destroyMethodNames.length > 1) { return destroyMethodNames; @@ -352,23 +353,23 @@ class DisposableBeanAdapter implements DisposableBean, Runnable, Serializable { String destroyMethodName = beanDefinition.resolvedDestroyMethodName; if (destroyMethodName == null) { destroyMethodName = beanDefinition.getDestroyMethodName(); - boolean autoCloseable = (bean instanceof AutoCloseable); + boolean autoCloseable = (AutoCloseable.class.isAssignableFrom(target)); if (AbstractBeanDefinition.INFER_METHOD.equals(destroyMethodName) || (destroyMethodName == null && autoCloseable)) { // Only perform destroy method inference in case of the bean // not explicitly implementing the DisposableBean interface destroyMethodName = null; - if (!(bean instanceof DisposableBean)) { + if (!(DisposableBean.class.isAssignableFrom(target))) { if (autoCloseable) { destroyMethodName = CLOSE_METHOD_NAME; } else { try { - destroyMethodName = bean.getClass().getMethod(CLOSE_METHOD_NAME).getName(); + destroyMethodName = target.getMethod(CLOSE_METHOD_NAME).getName(); } catch (NoSuchMethodException ex) { try { - destroyMethodName = bean.getClass().getMethod(SHUTDOWN_METHOD_NAME).getName(); + destroyMethodName = target.getMethod(SHUTDOWN_METHOD_NAME).getName(); } catch (NoSuchMethodException ex2) { // no candidate destroy method found 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 72fd154fe3..7a50e6f015 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 @@ -550,6 +550,15 @@ public class RootBeanDefinition extends AbstractBeanDefinition { } } + /** + * Resolve the inferred destroy method if necessary. + * @since 6.0 + */ + public void resolveDestroyMethodIfNecessary() { + setDestroyMethodNames(DisposableBeanAdapter + .inferDestroyMethodsIfNecessary(getResolvableType().toClass(), this)); + } + /** * Register an externally managed configuration destruction method — * for example, a method annotated with JSR-250's diff --git a/spring-beans/src/test/java/org/springframework/beans/factory/annotation/InitDestroyAnnotationBeanPostProcessorTests.java b/spring-beans/src/test/java/org/springframework/beans/factory/annotation/InitDestroyAnnotationBeanPostProcessorTests.java index 0ce1ec542f..07faca0034 100644 --- a/spring-beans/src/test/java/org/springframework/beans/factory/annotation/InitDestroyAnnotationBeanPostProcessorTests.java +++ b/spring-beans/src/test/java/org/springframework/beans/factory/annotation/InitDestroyAnnotationBeanPostProcessorTests.java @@ -18,10 +18,12 @@ package org.springframework.beans.factory.annotation; import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.support.AbstractBeanDefinition; import org.springframework.beans.factory.support.DefaultListableBeanFactory; import org.springframework.beans.factory.support.RegisteredBean; import org.springframework.beans.factory.support.RootBeanDefinition; import org.springframework.beans.testfixture.beans.factory.generator.lifecycle.Destroy; +import org.springframework.beans.testfixture.beans.factory.generator.lifecycle.InferredDestroyBean; import org.springframework.beans.testfixture.beans.factory.generator.lifecycle.Init; import org.springframework.beans.testfixture.beans.factory.generator.lifecycle.InitDestroyBean; import org.springframework.beans.testfixture.beans.factory.generator.lifecycle.MultiInitDestroyBean; @@ -40,7 +42,7 @@ class InitDestroyAnnotationBeanPostProcessorTests { @Test void processAheadOfTimeWhenNoCallbackDoesNotMutateRootBeanDefinition() { - RootBeanDefinition beanDefinition = new RootBeanDefinition(String.class); + RootBeanDefinition beanDefinition = new RootBeanDefinition(NoInitDestroyBean.class); processAheadOfTime(beanDefinition); RootBeanDefinition mergedBeanDefinition = getMergedBeanDefinition(); assertThat(mergedBeanDefinition.getInitMethodNames()).isNull(); @@ -78,6 +80,26 @@ class InitDestroyAnnotationBeanPostProcessorTests { assertThat(mergedBeanDefinition.getDestroyMethodNames()).containsExactly("destroyMethod"); } + @Test + void processAheadOfTimeWhenHasInferredDestroyMethodAddsDestroyMethodName() { + RootBeanDefinition beanDefinition = new RootBeanDefinition(InferredDestroyBean.class); + beanDefinition.setDestroyMethodNames(AbstractBeanDefinition.INFER_METHOD); + processAheadOfTime(beanDefinition); + RootBeanDefinition mergedBeanDefinition = getMergedBeanDefinition(); + assertThat(mergedBeanDefinition.getInitMethodNames()).isNull(); + assertThat(mergedBeanDefinition.getDestroyMethodNames()).containsExactly("close"); + } + + @Test + void processAheadOfTimeWhenHasInferredDestroyMethodAndNoCandidateDoesNotMutateRootBeanDefinition() { + RootBeanDefinition beanDefinition = new RootBeanDefinition(NoInitDestroyBean.class); + beanDefinition.setDestroyMethodNames(AbstractBeanDefinition.INFER_METHOD); + processAheadOfTime(beanDefinition); + RootBeanDefinition mergedBeanDefinition = getMergedBeanDefinition(); + assertThat(mergedBeanDefinition.getInitMethodNames()).isNull(); + assertThat(mergedBeanDefinition.getDestroyMethodNames()).isNull(); + } + @Test void processAheadOfTimeWhenHasMultipleInitDestroyAnnotationsAddsAllMethodNames() { RootBeanDefinition beanDefinition = new RootBeanDefinition(MultiInitDestroyBean.class); @@ -110,4 +132,6 @@ class InitDestroyAnnotationBeanPostProcessorTests { return beanPostProcessor; } + static class NoInitDestroyBean {} + } diff --git a/spring-beans/src/test/java/org/springframework/beans/factory/aot/BeanDefinitionPropertiesCodeGeneratorTests.java b/spring-beans/src/test/java/org/springframework/beans/factory/aot/BeanDefinitionPropertiesCodeGeneratorTests.java index c9fdd0b8d7..d8725df503 100644 --- a/spring-beans/src/test/java/org/springframework/beans/factory/aot/BeanDefinitionPropertiesCodeGeneratorTests.java +++ b/spring-beans/src/test/java/org/springframework/beans/factory/aot/BeanDefinitionPropertiesCodeGeneratorTests.java @@ -36,7 +36,6 @@ import org.springframework.beans.factory.config.BeanReference; import org.springframework.beans.factory.config.ConstructorArgumentValues.ValueHolder; import org.springframework.beans.factory.config.RuntimeBeanNameReference; import org.springframework.beans.factory.config.RuntimeBeanReference; -import org.springframework.beans.factory.support.AbstractBeanDefinition; import org.springframework.beans.factory.support.DefaultListableBeanFactory; import org.springframework.beans.factory.support.ManagedList; import org.springframework.beans.factory.support.ManagedMap; @@ -225,9 +224,8 @@ class BeanDefinitionPropertiesCodeGeneratorTests { } @Test - void setInitMethodWhenSingleInferredInitMethod() { + void setInitMethodWhenNoInitMethod() { this.beanDefinition.setTargetType(InitDestroyBean.class); - this.beanDefinition.setInitMethodName(AbstractBeanDefinition.INFER_METHOD); compile((actual, compiled) -> assertThat(actual.getInitMethodNames()).isNull()); } @@ -241,13 +239,6 @@ class BeanDefinitionPropertiesCodeGeneratorTests { assertHasMethodInvokeHints(InitDestroyBean.class, methodNames); } - @Test - void setInitMethodWithInferredMethodFirst() { - this.beanDefinition.setInitMethodNames(AbstractBeanDefinition.INFER_METHOD, "init"); - compile((actual, compiled) -> assertThat(compiled.getSourceFile().getContent()) - .contains("beanDefinition.setInitMethodNames(\"init\");")); - } - @Test void setDestroyMethodWhenDestroyInitMethod() { this.beanDefinition.setTargetType(InitDestroyBean.class); @@ -260,9 +251,8 @@ class BeanDefinitionPropertiesCodeGeneratorTests { } @Test - void setDestroyMethodWhenSingleInferredInitMethod() { + void setDestroyMethodWhenNoDestroyMethod() { this.beanDefinition.setTargetType(InitDestroyBean.class); - this.beanDefinition.setDestroyMethodName(AbstractBeanDefinition.INFER_METHOD); compile((actual, compiled) -> assertThat(actual.getDestroyMethodNames()).isNull()); } @@ -277,13 +267,6 @@ class BeanDefinitionPropertiesCodeGeneratorTests { assertHasMethodInvokeHints(InitDestroyBean.class, methodNames); } - @Test - void setDestroyMethodWithInferredMethodFirst() { - this.beanDefinition.setDestroyMethodNames(AbstractBeanDefinition.INFER_METHOD, "destroy"); - compile((actual, compiled) -> assertThat(compiled.getSourceFile().getContent()) - .contains("beanDefinition.setDestroyMethodNames(\"destroy\");")); - } - private void assertHasMethodInvokeHints(Class beanType, String... methodNames) { assertThat(methodNames).allMatch(methodName -> RuntimeHintsPredicates.reflection() .onMethod(beanType, methodName).invoke() diff --git a/spring-beans/src/test/java/org/springframework/beans/factory/support/RootBeanDefinitionTests.java b/spring-beans/src/test/java/org/springframework/beans/factory/support/RootBeanDefinitionTests.java index 7e1a5481b5..ff39d5faeb 100644 --- a/spring-beans/src/test/java/org/springframework/beans/factory/support/RootBeanDefinitionTests.java +++ b/spring-beans/src/test/java/org/springframework/beans/factory/support/RootBeanDefinitionTests.java @@ -57,4 +57,39 @@ class RootBeanDefinitionTests { verify(instanceSupplier).getFactoryMethod(); } + @Test + void resolveDestroyMethodWithMatchingCandidateReplacedInferredVaue() { + RootBeanDefinition beanDefinition = new RootBeanDefinition(BeanWithCloseMethod.class); + beanDefinition.setDestroyMethodName(AbstractBeanDefinition.INFER_METHOD); + beanDefinition.resolveDestroyMethodIfNecessary(); + assertThat(beanDefinition.getDestroyMethodNames()).containsExactly("close"); + } + + @Test + void resolveDestroyMethodWithNoCandidateSetDestroyMethodNameToNull() { + RootBeanDefinition beanDefinition = new RootBeanDefinition(BeanWithNoDestroyMethod.class); + beanDefinition.setDestroyMethodName(AbstractBeanDefinition.INFER_METHOD); + beanDefinition.resolveDestroyMethodIfNecessary(); + assertThat(beanDefinition.getDestroyMethodNames()).isNull(); + } + + @Test + void resolveDestroyMethodWithNoResolvableType() { + RootBeanDefinition beanDefinition = new RootBeanDefinition(); + beanDefinition.setDestroyMethodName(AbstractBeanDefinition.INFER_METHOD); + beanDefinition.resolveDestroyMethodIfNecessary(); + assertThat(beanDefinition.getDestroyMethodNames()).isNull(); + } + + static class BeanWithCloseMethod { + + public void close() { + } + + } + + static class BeanWithNoDestroyMethod { + + } + } diff --git a/spring-beans/src/testFixtures/java/org/springframework/beans/testfixture/beans/factory/generator/lifecycle/InferredDestroyBean.java b/spring-beans/src/testFixtures/java/org/springframework/beans/testfixture/beans/factory/generator/lifecycle/InferredDestroyBean.java new file mode 100644 index 0000000000..ffa538b242 --- /dev/null +++ b/spring-beans/src/testFixtures/java/org/springframework/beans/testfixture/beans/factory/generator/lifecycle/InferredDestroyBean.java @@ -0,0 +1,26 @@ +/* + * Copyright 2002-2022 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.beans.testfixture.beans.factory.generator.lifecycle; + +public class InferredDestroyBean { + + public void close() { + + } +} + +