From 2e90b71c301f122d45dec036c403f00e9d0605b9 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Fri, 6 Jan 2023 16:49:54 -0800 Subject: [PATCH] Do not attempt constructor binding for items with an existing value Update `DefaultBindConstructorProvider` so that it no longer detects constructors when the `Bindable` has an existing value. This update allows us change `ConfigurationPropertiesBindingPostProcessor` and related classes so that all instances created from `@Bean` methods are treated as `JAVA_BEAN` bindings and will not be accidentally re-created using constructor binding. Fixes gh-33710 --- .../ConfigurationPropertiesBean.java | 42 +++++++++------ ...urationPropertiesBindingPostProcessor.java | 18 ++++--- .../context/properties/bind/Bindable.java | 32 +++++++++++- .../bind/DefaultBindConstructorProvider.java | 12 +++-- .../ConfigurationPropertiesTests.java | 51 ++++++++++++++++++- .../DefaultBindConstructorProviderTests.java | 10 +++- 6 files changed, 134 insertions(+), 31 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBean.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBean.java index 1d1ae6a7bf..a4ee220fb8 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBean.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBean.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. @@ -71,18 +71,13 @@ public final class ConfigurationPropertiesBean { private final BindMethod bindMethod; - private ConfigurationPropertiesBean(String name, Object instance, ConfigurationProperties annotation, - Bindable bindable) { - this(name, instance, annotation, bindable, BindMethod.get(bindable)); - } - private ConfigurationPropertiesBean(String name, Object instance, ConfigurationProperties annotation, Bindable bindTarget, BindMethod bindMethod) { this.name = name; this.instance = instance; this.annotation = annotation; this.bindTarget = bindTarget; - this.bindMethod = bindMethod; + this.bindMethod = (bindMethod != null) ? bindMethod : BindMethod.get(bindTarget); } /** @@ -168,7 +163,8 @@ public final class ConfigurationPropertiesBean { if (isConfigurationPropertiesBean(beanFactory, beanName)) { try { Object bean = beanFactory.getBean(beanName); - ConfigurationPropertiesBean propertiesBean = get(applicationContext, bean, beanName); + BindMethod bindMethod = getBindMethod(beanFactory, beanName); + ConfigurationPropertiesBean propertiesBean = get(applicationContext, bean, beanName, bindMethod); if (propertiesBean != null) { propertiesBeans.put(beanName, propertiesBean); } @@ -180,6 +176,16 @@ public final class ConfigurationPropertiesBean { return propertiesBeans; } + private static BindMethod getBindMethod(ConfigurableListableBeanFactory beanFactory, String beanName) { + try { + BeanDefinition beanDefinition = beanFactory.getBeanDefinition(beanName); + return (BindMethod) beanDefinition.getAttribute(BindMethod.class.getName()); + } + catch (NoSuchBeanDefinitionException ex) { + return null; + } + } + private static boolean isConfigurationPropertiesBean(ConfigurableListableBeanFactory beanFactory, String beanName) { try { if (beanFactory.getBeanDefinition(beanName).isAbstract()) { @@ -210,8 +216,13 @@ public final class ConfigurationPropertiesBean { * {@link ConfigurationProperties @ConfigurationProperties} */ public static ConfigurationPropertiesBean get(ApplicationContext applicationContext, Object bean, String beanName) { + return get(applicationContext, bean, beanName, null); + } + + private static ConfigurationPropertiesBean get(ApplicationContext applicationContext, Object bean, String beanName, + BindMethod bindMethod) { Method factoryMethod = findFactoryMethod(applicationContext, beanName); - return create(beanName, bean, bean.getClass(), factoryMethod); + return create(beanName, bean, bean.getClass(), factoryMethod, bindMethod); } private static Method findFactoryMethod(ApplicationContext applicationContext, String beanName) { @@ -260,13 +271,14 @@ public final class ConfigurationPropertiesBean { } static ConfigurationPropertiesBean forValueObject(Class beanClass, String beanName) { - ConfigurationPropertiesBean propertiesBean = create(beanName, null, beanClass, null); + ConfigurationPropertiesBean propertiesBean = create(beanName, null, beanClass, null, null); Assert.state(propertiesBean != null && propertiesBean.getBindMethod() == BindMethod.VALUE_OBJECT, () -> "Bean '" + beanName + "' is not a @ConfigurationProperties value object"); return propertiesBean; } - private static ConfigurationPropertiesBean create(String name, Object instance, Class type, Method factory) { + private static ConfigurationPropertiesBean create(String name, Object instance, Class type, Method factory, + BindMethod bindMethod) { ConfigurationProperties annotation = findAnnotation(instance, type, factory, ConfigurationProperties.class); if (annotation == null) { return null; @@ -277,13 +289,11 @@ public final class ConfigurationPropertiesBean { ResolvableType bindType = (factory != null) ? ResolvableType.forMethodReturnType(factory) : ResolvableType.forClass(type); Bindable bindable = Bindable.of(bindType).withAnnotations(annotations); - if (instance != null) { + bindMethod = (factory != null) ? BindMethod.JAVA_BEAN : bindMethod; + if (instance != null && bindMethod != BindMethod.VALUE_OBJECT) { bindable = bindable.withExistingValue(instance); } - if (factory != null) { - return new ConfigurationPropertiesBean(name, instance, annotation, bindable, BindMethod.JAVA_BEAN); - } - return new ConfigurationPropertiesBean(name, instance, annotation, bindable); + return new ConfigurationPropertiesBean(name, instance, annotation, bindable, bindMethod); } private static A findAnnotation(Object instance, Class type, Method factory, diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBindingPostProcessor.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBindingPostProcessor.java index f6d53b9224..9c27808ff1 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBindingPostProcessor.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBindingPostProcessor.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2021 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. @@ -75,12 +75,19 @@ public class ConfigurationPropertiesBindingPostProcessor @Override public Object postProcessBeforeInitialization(Object bean, String beanName) throws BeansException { - bind(ConfigurationPropertiesBean.get(this.applicationContext, bean, beanName)); + if (!hasBoundValueObject(beanName)) { + bind(ConfigurationPropertiesBean.get(this.applicationContext, bean, beanName)); + } return bean; } + private boolean hasBoundValueObject(String beanName) { + return this.registry.containsBeanDefinition(beanName) && BindMethod.VALUE_OBJECT + .equals(this.registry.getBeanDefinition(beanName).getAttribute(BindMethod.class.getName())); + } + private void bind(ConfigurationPropertiesBean bean) { - if (bean == null || hasBoundValueObject(bean.getName())) { + if (bean == null) { return; } Assert.state(bean.getBindMethod() == BindMethod.JAVA_BEAN, "Cannot bind @ConfigurationProperties for bean '" @@ -93,11 +100,6 @@ public class ConfigurationPropertiesBindingPostProcessor } } - private boolean hasBoundValueObject(String beanName) { - return this.registry.containsBeanDefinition(beanName) && BindMethod.VALUE_OBJECT - .equals(this.registry.getBeanDefinition(beanName).getAttribute(BindMethod.class.getName())); - } - /** * Register a {@link ConfigurationPropertiesBindingPostProcessor} bean if one is not * already registered. 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 e834577eb2..795179bf1a 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 @@ -1,5 +1,5 @@ /* - * Copyright 2012-2021 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. @@ -90,6 +90,18 @@ 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 @@ -182,7 +194,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) ? () -> existingValue : null; + Supplier value = (existingValue != null) ? new ExistingValueSupplier(existingValue) : null; return new Bindable<>(this.type, this.boxedType, value, this.annotations, this.bindRestrictions); } @@ -307,4 +319,20 @@ 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 ee67057f06..8f2c287355 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 @@ -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. @@ -39,12 +39,18 @@ class DefaultBindConstructorProvider implements BindConstructorProvider { @Override public Constructor getBindConstructor(Bindable bindable, boolean isNestedConstructorBinding) { - return getBindConstructor(bindable.getType().resolve(), isNestedConstructorBinding); + return getBindConstructor(bindable.getType().resolve(), bindable.hasExistingValue(), + isNestedConstructorBinding); } @Override public Constructor getBindConstructor(Class type, boolean isNestedConstructorBinding) { - if (type == null) { + 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); 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 4423341991..2eeeeefcbe 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 @@ -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. @@ -1113,6 +1113,14 @@ class ConfigurationPropertiesTests { assertThat(bean.getItem().getValue()).isEqualTo("foo"); } + @Test // gh-33710 + void loadWhenConstructorUsedInBeanMethodAndNotAsConstructorBinding() { + load(ConstructorUsedInBeanMethodConfiguration.class, "test.two=bound-2"); + ConstructorUsedInBeanMethod bean = this.context.getBean(ConstructorUsedInBeanMethod.class); + assertThat(bean.getOne()).isEqualTo("bean-method-1"); + assertThat(bean.getTwo()).isEqualTo("bound-2"); + } + private AnnotationConfigApplicationContext load(Class configuration, String... inlinedProperties) { return load(new Class[] { configuration }, inlinedProperties); } @@ -2847,4 +2855,45 @@ class ConfigurationPropertiesTests { } + @Configuration(proxyBeanMethods = false) + @EnableConfigurationProperties + static class ConstructorUsedInBeanMethodConfiguration { + + @Bean + @ConfigurationProperties("test") + ConstructorUsedInBeanMethod constructorUsedInBeanMethod() { + return new ConstructorUsedInBeanMethod("bean-method-1", "bean-method-2"); + } + + } + + static class ConstructorUsedInBeanMethod { + + private String one; + + private String two; + + ConstructorUsedInBeanMethod(String one, String two) { + this.one = one; + this.two = two; + } + + String getOne() { + return this.one; + } + + void setOne(String one) { + this.one = one; + } + + String getTwo() { + return this.two; + } + + void setTwo(String two) { + 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 118ce6105a..f2b9d7f63b 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 @@ -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. @@ -110,6 +110,14 @@ class DefaultBindConstructorProviderTests { } } + @Test + void getBindConstructorWhenHasExistingValueReturnsNull() { + OneConstructorWithConstructorBinding existingValue = new OneConstructorWithConstructorBinding("name", 123); + Bindable bindable = Bindable.of(OneConstructorWithConstructorBinding.class).withExistingValue(existingValue); + Constructor bindConstructor = this.provider.getBindConstructor(bindable, false); + assertThat(bindConstructor).isNull(); + } + static class OnlyDefaultConstructor { }