From 84b13f07488ed220ce1ab8c5148306f72dde727e Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Mon, 9 Jan 2023 16:49:33 -0800 Subject: [PATCH] Refine constructor detection logic when binding to existing values Update `DefaultBindConstructorProvider` so that deduced constructors are not used if there is an existing value. Prior to this commit, constructor detection logic was not compatible with earlier versions of Spring Boot. With Spring Boot 3.0.1, given a class of the following form: @ConfigurationProperties(prefix = "example") public class ExampleProperties { @NestedConfigurationProperty private final NestedProperty nested = new NestedProperty( "Default", "default"); public NestedProperty getNested() { return nested; } } If `NestedProperty` has a single constructor with arguments, constructor binding would be used. In Spring Boot 2.x, setter injection would have been used. The updated code now only uses constructor injection if an explicit `@ConstructorBinding` annotation is present, or if there is no existing value. Fixes gh-33409 See gh-33710 --- .../context/properties/bind/Bindable.java | 30 +----------- .../bind/DefaultBindConstructorProvider.java | 47 ++++++++++++------- .../ConfigurationPropertiesTests.java | 40 ++++++++++++++-- .../DefaultBindConstructorProviderTests.java | 19 +++++++- 4 files changed, 83 insertions(+), 53 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/Bindable.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/Bindable.java index 795179bf1a0..67bff4f621d 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/Bindable.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/Bindable.java @@ -90,18 +90,6 @@ public final class Bindable { return this.value; } - /** - * Return if the bindable is for an existing non-null value. If this method return - * true then {@link #getValue()} will return a {@link Supplier} that never returns - * {@code null}. - * @return if the {@link Bindable} is for an existing value - * @since 3.0.2 - * @see #withExistingValue(Object) - */ - public boolean hasExistingValue() { - return this.value instanceof ExistingValueSupplier; - } - /** * Return any associated annotations that could affect binding. * @return the associated annotations @@ -194,7 +182,7 @@ public final class Bindable { Assert.isTrue( existingValue == null || this.type.isArray() || this.boxedType.resolve().isInstance(existingValue), () -> "ExistingValue must be an instance of " + this.type); - Supplier value = (existingValue != null) ? new ExistingValueSupplier(existingValue) : null; + Supplier value = (existingValue != null) ? () -> existingValue : null; return new Bindable<>(this.type, this.boxedType, value, this.annotations, this.bindRestrictions); } @@ -319,20 +307,4 @@ public final class Bindable { } - private class ExistingValueSupplier implements Supplier { - - private final T value; - - ExistingValueSupplier(T value) { - Assert.notNull(value, "Value must not be null"); - this.value = value; - } - - @Override - public T get() { - return this.value; - } - - } - } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/DefaultBindConstructorProvider.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/DefaultBindConstructorProvider.java index 8f2c2873554..d5bf68a7834 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/DefaultBindConstructorProvider.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/DefaultBindConstructorProvider.java @@ -39,25 +39,19 @@ class DefaultBindConstructorProvider implements BindConstructorProvider { @Override public Constructor getBindConstructor(Bindable bindable, boolean isNestedConstructorBinding) { - return getBindConstructor(bindable.getType().resolve(), bindable.hasExistingValue(), + Constructors constructors = Constructors.getConstructors(bindable.getType().resolve(), isNestedConstructorBinding); + if (constructors.getBind() != null && constructors.isDeducedBindConstructor()) { + if (bindable.getValue() != null && bindable.getValue().get() != null) { + return null; + } + } + return constructors.getBind(); } @Override public Constructor getBindConstructor(Class type, boolean isNestedConstructorBinding) { - return getBindConstructor(type, false, isNestedConstructorBinding); - } - - private Constructor getBindConstructor(Class type, boolean hasExistingValue, - boolean isNestedConstructorBinding) { - if (type == null || hasExistingValue) { - return null; - } - Constructors constructors = Constructors.getConstructors(type); - if (constructors.getBind() != null || isNestedConstructorBinding) { - Assert.state(!constructors.hasAutowired(), - () -> type.getName() + " declares @ConstructorBinding and @Autowired constructor"); - } + Constructors constructors = Constructors.getConstructors(type, isNestedConstructorBinding); return constructors.getBind(); } @@ -66,13 +60,18 @@ class DefaultBindConstructorProvider implements BindConstructorProvider { */ static final class Constructors { + private static final Constructors NONE = new Constructors(false, null, false); + private final boolean hasAutowired; private final Constructor bind; - private Constructors(boolean hasAutowired, Constructor bind) { + private final boolean deducedBindConstructor; + + private Constructors(boolean hasAutowired, Constructor bind, boolean deducedBindConstructor) { this.hasAutowired = hasAutowired; this.bind = bind; + this.deducedBindConstructor = deducedBindConstructor; } boolean hasAutowired() { @@ -83,18 +82,32 @@ class DefaultBindConstructorProvider implements BindConstructorProvider { return this.bind; } - static Constructors getConstructors(Class type) { + boolean isDeducedBindConstructor() { + return this.deducedBindConstructor; + } + + static Constructors getConstructors(Class type, boolean isNestedConstructorBinding) { + if (type == null) { + return NONE; + } boolean hasAutowiredConstructor = isAutowiredPresent(type); Constructor[] candidates = getCandidateConstructors(type); MergedAnnotations[] candidateAnnotations = getAnnotations(candidates); + boolean deducedBindConstructor = false; Constructor bind = getConstructorBindingAnnotated(type, candidates, candidateAnnotations); if (bind == null && !hasAutowiredConstructor) { bind = deduceBindConstructor(type, candidates); + deducedBindConstructor = bind != null; } if (bind == null && !hasAutowiredConstructor && isKotlinType(type)) { bind = deduceKotlinBindConstructor(type); + deducedBindConstructor = bind != null; + } + if (bind != null || isNestedConstructorBinding) { + Assert.state(!hasAutowiredConstructor, + () -> type.getName() + " declares @ConstructorBinding and @Autowired constructor"); } - return new Constructors(hasAutowiredConstructor, bind); + return new Constructors(hasAutowiredConstructor, bind, deducedBindConstructor); } private static boolean isAutowiredPresent(Class type) { diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesTests.java index dce89e318ad..de44144c607 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesTests.java @@ -1116,11 +1116,19 @@ class ConfigurationPropertiesTests { @Test // gh-33710 void loadWhenConstructorUsedInBeanMethodAndNotAsConstructorBinding() { load(ConstructorUsedInBeanMethodConfiguration.class, "test.two=bound-2"); - ConstructorUsedInBeanMethod bean = this.context.getBean(ConstructorUsedInBeanMethod.class); + ConstructorUsedDirectly bean = this.context.getBean(ConstructorUsedDirectly.class); assertThat(bean.getOne()).isEqualTo("bean-method-1"); assertThat(bean.getTwo()).isEqualTo("bound-2"); } + @Test // gh-33409 + void loadWhenConstructorUsedInNestedPropertyAndNotAsConstructorBinding() { + load(ConstructorUsedInNestedPropertyConfiguration.class, "test.nested.two=bound-2"); + ConstructorUsedInNestedProperty bean = this.context.getBean(ConstructorUsedInNestedProperty.class); + assertThat(bean.getNested().getOne()).isEqualTo("nested-1"); + assertThat(bean.getNested().getTwo()).isEqualTo("bound-2"); + } + private AnnotationConfigApplicationContext load(Class configuration, String... inlinedProperties) { return load(new Class[] { configuration }, inlinedProperties); } @@ -2861,19 +2869,41 @@ class ConfigurationPropertiesTests { @Bean @ConfigurationProperties("test") - ConstructorUsedInBeanMethod constructorUsedInBeanMethod() { - return new ConstructorUsedInBeanMethod("bean-method-1", "bean-method-2"); + ConstructorUsedDirectly constructorUsedInBeanMethod() { + return new ConstructorUsedDirectly("bean-method-1", "bean-method-2"); + } + + } + + @Configuration(proxyBeanMethods = false) + @EnableConfigurationProperties(ConstructorUsedInNestedProperty.class) + static class ConstructorUsedInNestedPropertyConfiguration { + + } + + @ConfigurationProperties("test") + static class ConstructorUsedInNestedProperty { + + @NestedConfigurationProperty + private ConstructorUsedDirectly nested = new ConstructorUsedDirectly("nested-1", "nested-2"); + + ConstructorUsedDirectly getNested() { + return this.nested; + } + + void setNested(ConstructorUsedDirectly nested) { + this.nested = nested; } } - static class ConstructorUsedInBeanMethod { + static class ConstructorUsedDirectly { private String one; private String two; - ConstructorUsedInBeanMethod(String one, String two) { + ConstructorUsedDirectly(String one, String two) { this.one = one; this.two = two; } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/DefaultBindConstructorProviderTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/DefaultBindConstructorProviderTests.java index dfe420aca4d..6d09fe98a5e 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/DefaultBindConstructorProviderTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/DefaultBindConstructorProviderTests.java @@ -111,11 +111,19 @@ class DefaultBindConstructorProviderTests { } @Test - void getBindConstructorWhenHasExistingValueReturnsNull() { + void getBindConstructorWhenHasExistingValueAndOneConstructorWithoutAnnotationsReturnsNull() { + OneConstructorWithoutAnnotations existingValue = new OneConstructorWithoutAnnotations("name", 123); + Bindable bindable = Bindable.of(OneConstructorWithoutAnnotations.class).withExistingValue(existingValue); + Constructor bindConstructor = this.provider.getBindConstructor(bindable, false); + assertThat(bindConstructor).isNull(); + } + + @Test + void getBindConstructorWhenHasExistingValueAndOneConstructorWithConstructorBindingReturnsConstructor() { OneConstructorWithConstructorBinding existingValue = new OneConstructorWithConstructorBinding("name", 123); Bindable bindable = Bindable.of(OneConstructorWithConstructorBinding.class).withExistingValue(existingValue); Constructor bindConstructor = this.provider.getBindConstructor(bindable, false); - assertThat(bindConstructor).isNull(); + assertThat(bindConstructor).isNotNull(); } static class OnlyDefaultConstructor { @@ -185,6 +193,13 @@ class DefaultBindConstructorProviderTests { } + static class OneConstructorWithoutAnnotations { + + OneConstructorWithoutAnnotations(String name, int age) { + } + + } + static class TwoConstructorsWithBothConstructorBinding { @ConstructorBinding