From 5ad0d49ec1bad5071b328cba6fd6710a9c709fff Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Fri, 19 May 2023 12:37:51 +0100 Subject: [PATCH] Fix hints for `@Bean` config props that could be constructor bound Previously, if a `@ConfigurationProperties`-annotated `@Bean` method returned a type that looked like it could be constructor bound, the registered runtime hints were incorrect. With only the bean's class to work with, the hints registrar would incorrectly determine that the type would be constructor bound and would not register the hints required for Java bean binding. This commit updates the registrar to allow the caller to provide a Bindable which knows both what should be bound and how it should be bound, thereby allowing the registrar to generate the correct hints. The tests for the AOT processor have also been updated to remove duplication of the tests in BindableRuntimeHintsRegistrarTests and to focus on the contribution creating Bindable instances with the bind method that is required to produce the correct reflection hints. Closes gh-35564 Co-authored-by: Phillip Webb --- ...BeanFactoryInitializationAotProcessor.java | 31 +-- .../bind/BindableRuntimeHintsRegistrar.java | 59 ++++-- ...actoryInitializationAotProcessorTests.java | 189 ++++++++++++++++-- .../BindableRuntimeHintsRegistrarTests.java | 2 +- 4 files changed, 235 insertions(+), 46 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBeanFactoryInitializationAotProcessor.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBeanFactoryInitializationAotProcessor.java index c7bf287643..3786fc3aca 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBeanFactoryInitializationAotProcessor.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBeanFactoryInitializationAotProcessor.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2022 the original author or authors. + * Copyright 2012-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. @@ -24,9 +24,10 @@ import org.springframework.beans.factory.aot.BeanFactoryInitializationAotContrib import org.springframework.beans.factory.aot.BeanFactoryInitializationAotProcessor; import org.springframework.beans.factory.aot.BeanFactoryInitializationCode; import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; +import org.springframework.boot.context.properties.bind.BindMethod; +import org.springframework.boot.context.properties.bind.Bindable; import org.springframework.boot.context.properties.bind.BindableRuntimeHintsRegistrar; import org.springframework.util.ClassUtils; -import org.springframework.util.CollectionUtils; /** * {@link BeanFactoryInitializationAotProcessor} that contributes runtime hints for @@ -43,36 +44,38 @@ class ConfigurationPropertiesBeanFactoryInitializationAotProcessor implements Be public ConfigurationPropertiesReflectionHintsContribution processAheadOfTime( ConfigurableListableBeanFactory beanFactory) { String[] beanNames = beanFactory.getBeanNamesForAnnotation(ConfigurationProperties.class); - List> types = new ArrayList<>(); + List> bindables = new ArrayList<>(); for (String beanName : beanNames) { Class beanType = beanFactory.getType(beanName, false); if (beanType != null) { - types.add(ClassUtils.getUserClass(beanType)); + BindMethod bindMethod = beanFactory.containsBeanDefinition(beanName) + ? (BindMethod) beanFactory.getBeanDefinition(beanName).getAttribute(BindMethod.class.getName()) + : null; + bindables.add(Bindable.of(ClassUtils.getUserClass(beanType)) + .withBindMethod((bindMethod != null) ? bindMethod : BindMethod.JAVA_BEAN)); } } - if (!CollectionUtils.isEmpty(types)) { - return new ConfigurationPropertiesReflectionHintsContribution(types); - } - return null; + return (!bindables.isEmpty()) ? new ConfigurationPropertiesReflectionHintsContribution(bindables) : null; } static final class ConfigurationPropertiesReflectionHintsContribution implements BeanFactoryInitializationAotContribution { - private final Iterable> types; + private final List> bindables; - private ConfigurationPropertiesReflectionHintsContribution(Iterable> types) { - this.types = types; + private ConfigurationPropertiesReflectionHintsContribution(List> bindables) { + this.bindables = bindables; } @Override public void applyTo(GenerationContext generationContext, BeanFactoryInitializationCode beanFactoryInitializationCode) { - BindableRuntimeHintsRegistrar.forTypes(this.types).registerHints(generationContext.getRuntimeHints()); + BindableRuntimeHintsRegistrar.forBindables(this.bindables) + .registerHints(generationContext.getRuntimeHints()); } - Iterable> getTypes() { - return this.types; + Iterable> getBindables() { + return this.bindables; } } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/BindableRuntimeHintsRegistrar.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/BindableRuntimeHintsRegistrar.java index 0ed6fecc29..ebb002a151 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/BindableRuntimeHintsRegistrar.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/BindableRuntimeHintsRegistrar.java @@ -24,6 +24,7 @@ import java.util.Collection; import java.util.HashSet; import java.util.Map; import java.util.Set; +import java.util.stream.Stream; import java.util.stream.StreamSupport; import kotlin.jvm.JvmClassMappingKt; @@ -51,8 +52,8 @@ import org.springframework.util.ReflectionUtils; * {@link RuntimeHintsRegistrar} that can be used to register {@link ReflectionHints} for * {@link Bindable} types, discovering any nested type it may expose through a property. *

- * This class can be used as a base-class, or instantiated using the {@code forTypes} - * factory methods. + * This class can be used as a base-class, or instantiated using the {@code forTypes} and + * {@code forBindables} factory methods. * * @author Andy Wilkinson * @author Moritz Halbritter @@ -62,14 +63,23 @@ import org.springframework.util.ReflectionUtils; */ public class BindableRuntimeHintsRegistrar implements RuntimeHintsRegistrar { - private final Class[] types; + private final Bindable[] bindables; /** * Create a new {@link BindableRuntimeHintsRegistrar} for the specified types. * @param types the types to process */ protected BindableRuntimeHintsRegistrar(Class... types) { - this.types = types; + this(Stream.of(types).map(Bindable::of).toArray(Bindable[]::new)); + } + + /** + * Create a new {@link BindableRuntimeHintsRegistrar} for the specified bindables. + * @param bindables the bindables to process + * @since 3.0.8 + */ + protected BindableRuntimeHintsRegistrar(Bindable... bindables) { + this.bindables = bindables; } @Override @@ -83,8 +93,8 @@ public class BindableRuntimeHintsRegistrar implements RuntimeHintsRegistrar { */ public void registerHints(RuntimeHints hints) { Set> compiledWithoutParameters = new HashSet<>(); - for (Class type : this.types) { - new Processor(type, compiledWithoutParameters).process(hints.reflection()); + for (Bindable bindable : this.bindables) { + new Processor(bindable, compiledWithoutParameters).process(hints.reflection()); } if (!compiledWithoutParameters.isEmpty()) { throw new MissingParametersCompilerArgumentException(compiledWithoutParameters); @@ -110,6 +120,27 @@ public class BindableRuntimeHintsRegistrar implements RuntimeHintsRegistrar { return new BindableRuntimeHintsRegistrar(types); } + /** + * Create a new {@link BindableRuntimeHintsRegistrar} for the specified bindables. + * @param bindables the bindables to process + * @return a new {@link BindableRuntimeHintsRegistrar} instance + * @since 3.0.8 + */ + public static BindableRuntimeHintsRegistrar forBindables(Iterable> bindables) { + Assert.notNull(bindables, "Bindables must not be null"); + return forBindables(StreamSupport.stream(bindables.spliterator(), false).toArray(Bindable[]::new)); + } + + /** + * Create a new {@link BindableRuntimeHintsRegistrar} for the specified bindables. + * @param bindables the bindables to process + * @return a new {@link BindableRuntimeHintsRegistrar} instance + * @since 3.0.8 + */ + public static BindableRuntimeHintsRegistrar forBindables(Bindable... bindables) { + return new BindableRuntimeHintsRegistrar(bindables); + } + /** * Processor used to register the hints. */ @@ -136,15 +167,17 @@ public class BindableRuntimeHintsRegistrar implements RuntimeHintsRegistrar { private final Set> compiledWithoutParameters; - Processor(Class type, Set> compiledWithoutParameters) { - this(type, false, new HashSet<>(), compiledWithoutParameters); + Processor(Bindable bindable, Set> compiledWithoutParameters) { + this(bindable, false, new HashSet<>(), compiledWithoutParameters); } - private Processor(Class type, boolean nestedType, Set> seen, + private Processor(Bindable bindable, boolean nestedType, Set> seen, Set> compiledWithoutParameters) { - this.type = type; - this.bindConstructor = BindConstructorProvider.DEFAULT.getBindConstructor(Bindable.of(type), nestedType); - this.bean = JavaBeanBinder.BeanProperties.of(Bindable.of(type)); + this.type = bindable.getType().getRawClass(); + this.bindConstructor = (bindable.getBindMethod() != BindMethod.JAVA_BEAN) + ? BindConstructorProvider.DEFAULT.getBindConstructor(bindable.getType().resolve(), nestedType) + : null; + this.bean = JavaBeanBinder.BeanProperties.of(bindable); this.seen = seen; this.compiledWithoutParameters = compiledWithoutParameters; } @@ -235,7 +268,7 @@ public class BindableRuntimeHintsRegistrar implements RuntimeHintsRegistrar { } private void processNested(Class type, ReflectionHints hints) { - new Processor(type, true, this.seen, this.compiledWithoutParameters).process(hints); + new Processor(Bindable.of(type), true, this.seen, this.compiledWithoutParameters).process(hints); } private Class getComponentClass(ResolvableType type) { diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBeanFactoryInitializationAotProcessorTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBeanFactoryInitializationAotProcessorTests.java index 1b45fc8e5d..984cfaf454 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBeanFactoryInitializationAotProcessorTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBeanFactoryInitializationAotProcessorTests.java @@ -18,22 +18,26 @@ package org.springframework.boot.context.properties; import java.util.stream.Stream; +import org.assertj.core.api.AbstractAssert; +import org.assertj.core.api.AssertProvider; +import org.assertj.core.error.BasicErrorMessageFactory; import org.junit.jupiter.api.Test; -import org.springframework.aot.generate.GenerationContext; import org.springframework.aot.hint.TypeHint; import org.springframework.aot.hint.TypeReference; import org.springframework.aot.test.generate.TestGenerationContext; import org.springframework.beans.factory.aot.AotServices; import org.springframework.beans.factory.aot.BeanFactoryInitializationAotProcessor; -import org.springframework.beans.factory.aot.BeanFactoryInitializationCode; import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; import org.springframework.beans.factory.support.DefaultListableBeanFactory; -import org.springframework.beans.factory.support.RootBeanDefinition; import org.springframework.boot.context.properties.ConfigurationPropertiesBeanFactoryInitializationAotProcessor.ConfigurationPropertiesReflectionHintsContribution; +import org.springframework.boot.context.properties.bind.BindMethod; +import org.springframework.boot.context.properties.bind.Bindable; +import org.springframework.context.annotation.AnnotationConfigApplicationContext; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.mock; /** * Tests for {@link ConfigurationPropertiesBeanFactoryInitializationAotProcessor}. @@ -41,6 +45,7 @@ import static org.mockito.Mockito.mock; * @author Stephane Nicoll * @author Moritz Halbritter * @author Sebastien Deleuze + * @author Andy Wilkinson */ class ConfigurationPropertiesBeanFactoryInitializationAotProcessorTests { @@ -58,44 +63,192 @@ class ConfigurationPropertiesBeanFactoryInitializationAotProcessorTests { } @Test - void processManuallyRegisteredSingleton() { + void manuallyRegisteredSingletonBindsAsJavaBean() { DefaultListableBeanFactory beanFactory = new DefaultListableBeanFactory(); beanFactory.registerSingleton("test", new SampleProperties()); ConfigurationPropertiesReflectionHintsContribution contribution = process(beanFactory); - assertThat(contribution.getTypes()).containsExactly(SampleProperties.class); + assertThat(singleBindable(contribution)).hasBindMethod(BindMethod.JAVA_BEAN).hasType(SampleProperties.class); assertThat(typeHints(contribution).map(TypeHint::getType)) .containsExactly(TypeReference.of(SampleProperties.class)); } @Test - void processDefinedBean() { - ConfigurationPropertiesReflectionHintsContribution contribution = process(SampleProperties.class); - assertThat(contribution.getTypes()).containsExactly(SampleProperties.class); + void javaBeanConfigurationPropertiesBindAsJavaBean() { + ConfigurationPropertiesReflectionHintsContribution contribution = process(EnableJavaBeanProperties.class); + assertThat(singleBindable(contribution)).hasBindMethod(BindMethod.JAVA_BEAN).hasType(JavaBeanProperties.class); assertThat(typeHints(contribution).map(TypeHint::getType)) - .containsExactly(TypeReference.of(SampleProperties.class)); + .containsExactly(TypeReference.of(JavaBeanProperties.class)); } - Stream typeHints(ConfigurationPropertiesReflectionHintsContribution contribution) { - GenerationContext generationContext = new TestGenerationContext(); - contribution.applyTo(generationContext, mock(BeanFactoryInitializationCode.class)); + @Test + void constructorBindingConfigurationPropertiesBindAsValueObject() { + ConfigurationPropertiesReflectionHintsContribution contribution = process( + EnableConstructorBindingProperties.class); + assertThat(singleBindable(contribution)).hasBindMethod(BindMethod.VALUE_OBJECT) + .hasType(ConstructorBindingProperties.class); + assertThat(typeHints(contribution).map(TypeHint::getType)) + .containsExactly(TypeReference.of(ConstructorBindingProperties.class)); + } + + @Test + void possibleConstructorBindingPropertiesDefinedThroughBeanMethodBindAsJavaBean() { + ConfigurationPropertiesReflectionHintsContribution contribution = process( + PossibleConstructorBindingPropertiesBeanMethodConfiguration.class); + assertThat(singleBindable(contribution)).hasBindMethod(BindMethod.JAVA_BEAN) + .hasType(PossibleConstructorBindingProperties.class); + assertThat(typeHints(contribution).map(TypeHint::getType)) + .containsExactly(TypeReference.of(PossibleConstructorBindingProperties.class)); + } + + @Test + void possibleConstructorBindingPropertiesDefinedThroughEnabledAnnotationBindAsValueObject() { + ConfigurationPropertiesReflectionHintsContribution contribution = process( + EnablePossibleConstructorBindingProperties.class); + assertThat(singleBindable(contribution)).hasBindMethod(BindMethod.VALUE_OBJECT) + .hasType(PossibleConstructorBindingProperties.class); + assertThat(typeHints(contribution).map(TypeHint::getType)) + .containsExactly(TypeReference.of(PossibleConstructorBindingProperties.class)); + } + + private Stream typeHints(ConfigurationPropertiesReflectionHintsContribution contribution) { + TestGenerationContext generationContext = new TestGenerationContext(); + contribution.applyTo(generationContext, null); return generationContext.getRuntimeHints().reflection().typeHints(); } - private ConfigurationPropertiesReflectionHintsContribution process(Class... types) { - DefaultListableBeanFactory beanFactory = new DefaultListableBeanFactory(); - for (Class type : types) { - beanFactory.registerBeanDefinition(type.getName(), new RootBeanDefinition(type)); + private ConfigurationPropertiesReflectionHintsContribution process(Class config) { + try (AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(config)) { + return process(context.getBeanFactory()); } - return process(beanFactory); } private ConfigurationPropertiesReflectionHintsContribution process(ConfigurableListableBeanFactory beanFactory) { return this.processor.processAheadOfTime(beanFactory); } + private BindableAssertProvider singleBindable(ConfigurationPropertiesReflectionHintsContribution contribution) { + assertThat(contribution.getBindables()).hasSize(1); + return new BindableAssertProvider(contribution.getBindables().iterator().next()); + } + @ConfigurationProperties("test") static class SampleProperties { } + @EnableConfigurationProperties(JavaBeanProperties.class) + static class EnableJavaBeanProperties { + + } + + @ConfigurationProperties("java-bean") + static class JavaBeanProperties { + + private String value; + + String getValue() { + return this.value; + } + + void setValue(String value) { + this.value = value; + } + + } + + @EnableConfigurationProperties(ConstructorBindingProperties.class) + static class EnableConstructorBindingProperties { + + } + + @ConfigurationProperties("constructor-binding") + static class ConstructorBindingProperties { + + private final String value; + + ConstructorBindingProperties(String value) { + this.value = value; + } + + String getValue() { + return this.value; + } + + } + + @Configuration(proxyBeanMethods = false) + static class PossibleConstructorBindingPropertiesBeanMethodConfiguration { + + @Bean + @ConfigurationProperties(prefix = "bean-method") + PossibleConstructorBindingProperties possibleConstructorBindingProperties() { + return new PossibleConstructorBindingProperties("alpha"); + } + + } + + @EnableConfigurationProperties(PossibleConstructorBindingProperties.class) + static class EnablePossibleConstructorBindingProperties { + + } + + @ConfigurationProperties("possible-constructor-binding") + static class PossibleConstructorBindingProperties { + + private String value; + + PossibleConstructorBindingProperties(String arg) { + + } + + String getValue() { + return this.value; + } + + void setValue(String value) { + this.value = value; + } + + } + + static class BindableAssertProvider implements AssertProvider { + + private final Bindable bindable; + + BindableAssertProvider(Bindable bindable) { + this.bindable = bindable; + } + + @Override + public BindableAssert assertThat() { + return new BindableAssert(this.bindable); + } + + } + + static class BindableAssert extends AbstractAssert> { + + BindableAssert(Bindable bindable) { + super(bindable, BindableAssert.class); + } + + BindableAssert hasBindMethod(BindMethod bindMethod) { + if (this.actual.getBindMethod() != bindMethod) { + throwAssertionError( + new BasicErrorMessageFactory("Expected %s to have bind method %s but bind method was %s", + this.actual, bindMethod, this.actual.getBindMethod())); + } + return this; + } + + BindableAssert hasType(Class type) { + if (!type.equals(this.actual.getType().resolve())) { + throwAssertionError(new BasicErrorMessageFactory("Expected %s to have type %s but type was %s", + this.actual, type, this.actual.getType().resolve())); + } + return this; + } + + } + } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/BindableRuntimeHintsRegistrarTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/BindableRuntimeHintsRegistrarTests.java index d0aed438f2..e42cba5840 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/BindableRuntimeHintsRegistrarTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/BindableRuntimeHintsRegistrarTests.java @@ -75,7 +75,7 @@ class BindableRuntimeHintsRegistrarTests { @Test void registerHintsWhenNoClasses() { RuntimeHints runtimeHints = new RuntimeHints(); - BindableRuntimeHintsRegistrar registrar = new BindableRuntimeHintsRegistrar(); + BindableRuntimeHintsRegistrar registrar = new BindableRuntimeHintsRegistrar(new Class[0]); registrar.registerHints(runtimeHints); assertThat(runtimeHints.reflection().typeHints()).isEmpty(); }