Skip to content

Commit

Permalink
Do not attempt constructor binding for items with an existing value
Browse files Browse the repository at this point in the history
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
  • Loading branch information
philwebb committed Jan 7, 2023
1 parent 2d372ed commit 2e90b71
Show file tree
Hide file tree
Showing 6 changed files with 134 additions and 31 deletions.
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -168,7 +163,8 @@ private static Map<String, ConfigurationPropertiesBean> getAll(ConfigurableAppli
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);
}
Expand All @@ -180,6 +176,16 @@ private static Map<String, ConfigurationPropertiesBean> getAll(ConfigurableAppli
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()) {
Expand Down Expand Up @@ -210,8 +216,13 @@ private static boolean isConfigurationPropertiesBean(ConfigurableListableBeanFac
* {@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) {
Expand Down Expand Up @@ -260,13 +271,14 @@ private static Method findFactoryMethodUsingReflection(ConfigurableListableBeanF
}

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;
Expand All @@ -277,13 +289,11 @@ private static ConfigurationPropertiesBean create(String name, Object instance,
ResolvableType bindType = (factory != null) ? ResolvableType.forMethodReturnType(factory)
: ResolvableType.forClass(type);
Bindable<Object> 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 extends Annotation> A findAnnotation(Object instance, Class<?> type, Method factory,
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -75,12 +75,19 @@ public int getOrder() {

@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 '"
Expand All @@ -93,11 +100,6 @@ private void bind(ConfigurationPropertiesBean bean) {
}
}

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.
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -90,6 +90,18 @@ public Supplier<T> getValue() {
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
Expand Down Expand Up @@ -182,7 +194,7 @@ public Bindable<T> withExistingValue(T existingValue) {
Assert.isTrue(
existingValue == null || this.type.isArray() || this.boxedType.resolve().isInstance(existingValue),
() -> "ExistingValue must be an instance of " + this.type);
Supplier<T> value = (existingValue != null) ? () -> existingValue : null;
Supplier<T> value = (existingValue != null) ? new ExistingValueSupplier(existingValue) : null;
return new Bindable<>(this.type, this.boxedType, value, this.annotations, this.bindRestrictions);
}

Expand Down Expand Up @@ -307,4 +319,20 @@ public enum BindRestriction {

}

private class ExistingValueSupplier implements Supplier<T> {

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;
}

}

}
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -1113,6 +1113,14 @@ void loadWhenBindingWithCustomConverterAndObjectToObjectMethod() {
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);
}
Expand Down Expand Up @@ -2847,4 +2855,45 @@ public WithPublicObjectToObjectMethod convert(String source) {

}

@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;
}

}

}
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -110,6 +110,14 @@ void getBindConstructorFromProxiedClassWithOneAutowiredConstructorReturnsNull()
}
}

@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 {

}
Expand Down

0 comments on commit 2e90b71

Please sign in to comment.