Fix constructor binding issues

This commit fixes a few bugs related to constructor binding.
The ContructorFilter on the Bindable has been replaced with a
Binder level BinderConstructorProvider so that it can be used to
determine the constructor to use for nested properties as well.

Fixes gh-18810
Fixes gh-18670
Closes gh-18685
Closes gh-18894

Co-authored-by: Phillip Webb <[email protected]>

Author: mbhave

spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBean.java

@@ -18,15 +18,13 @@
 
 import java.lang.annotation.Annotation;
 import java.lang.reflect.AnnotatedElement;
-import java.lang.reflect.Constructor;
 import java.lang.reflect.Method;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicReference;
 
 import org.springframework.aop.support.AopUtils;
-import org.springframework.beans.BeanUtils;
 import org.springframework.beans.factory.NoSuchBeanDefinitionException;
 import org.springframework.beans.factory.config.BeanDefinition;
 import org.springframework.beans.factory.config.BeanPostProcessor;
@@ -37,7 +35,6 @@
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.ConfigurableApplicationContext;
 import org.springframework.context.annotation.Bean;
-import org.springframework.core.KotlinDetector;
 import org.springframework.core.ResolvableType;
 import org.springframework.core.annotation.MergedAnnotation;
 import org.springframework.core.annotation.MergedAnnotations;
@@ -73,12 +70,12 @@ public final class ConfigurationPropertiesBean {
 	private final BindMethod bindMethod;
 
 	private ConfigurationPropertiesBean(String name, Object instance, ConfigurationProperties annotation,
-			Bindable<?> bindTarget, BindMethod bindMethod) {
+			Bindable<?> bindTarget) {
 		this.name = name;
 		this.instance = instance;
 		this.annotation = annotation;
 		this.bindTarget = bindTarget;
-		this.bindMethod = bindMethod;
+		this.bindMethod = BindMethod.forType(bindTarget.getType().resolve());
 	}
 
 	/**
@@ -264,16 +261,13 @@ private static ConfigurationPropertiesBean create(String name, Object instance,
 		Validated validated = findAnnotation(instance, type, factory, Validated.class);
 		Annotation[] annotations = (validated != null) ? new Annotation[] { annotation, validated }
 				: new Annotation[] { annotation };
-		Constructor<?> bindConstructor = BindMethod.findBindConstructor(type);
-		BindMethod bindMethod = (bindConstructor != null) ? BindMethod.VALUE_OBJECT : BindMethod.forClass(type);
 		ResolvableType bindType = (factory != null) ? ResolvableType.forMethodReturnType(factory)
 				: ResolvableType.forClass(type);
-		Bindable<Object> bindTarget = Bindable.of(bindType).withAnnotations(annotations)
-				.withConstructorFilter(ConfigurationPropertiesBean::isBindableConstructor);
+		Bindable<Object> bindTarget = Bindable.of(bindType).withAnnotations(annotations);
 		if (instance != null) {
 			bindTarget = bindTarget.withExistingValue(instance);
 		}
-		return new ConfigurationPropertiesBean(name, instance, annotation, bindTarget, bindMethod);
+		return new ConfigurationPropertiesBean(name, instance, annotation, bindTarget);
 	}
 
 	private static <A extends Annotation> A findAnnotation(Object instance, Class<?> type, Method factory,
@@ -298,15 +292,6 @@ private static <A extends Annotation> MergedAnnotation<A> findMergedAnnotation(A
 				: MergedAnnotation.missing();
 	}
 
-	private static boolean isBindableConstructor(Constructor<?> constructor) {
-		Class<?> declaringClass = constructor.getDeclaringClass();
-		Constructor<?> bindConstructor = BindMethod.findBindConstructor(declaringClass);
-		if (bindConstructor != null) {
-			return bindConstructor.equals(constructor);
-		}
-		return BindMethod.forClass(declaringClass) == BindMethod.VALUE_OBJECT;
-	}
-
 	/**
 	 * The binding method that is used for the bean.
 	 */
@@ -322,40 +307,9 @@ public enum BindMethod {
 		 */
 		VALUE_OBJECT;
 
-		static BindMethod forClass(Class<?> type) {
-			if (isConstructorBindingType(type) || findBindConstructor(type) != null) {
-				return VALUE_OBJECT;
-			}
-			return JAVA_BEAN;
-		}
-
-		private static boolean isConstructorBindingType(Class<?> type) {
-			return MergedAnnotations.from(type, SearchStrategy.TYPE_HIERARCHY_AND_ENCLOSING_CLASSES)
-					.isPresent(ConstructorBinding.class);
-		}
-
-		static Constructor<?> findBindConstructor(Class<?> type) {
-			if (KotlinDetector.isKotlinPresent() && KotlinDetector.isKotlinType(type)) {
-				Constructor<?> constructor = BeanUtils.findPrimaryConstructor(type);
-				if (constructor != null) {
-					return findBindConstructor(type, constructor);
-				}
-			}
-			return findBindConstructor(type, type.getDeclaredConstructors());
-		}
-
-		private static Constructor<?> findBindConstructor(Class<?> type, Constructor<?>... candidates) {
-			Constructor<?> constructor = null;
-			for (Constructor<?> candidate : candidates) {
-				if (MergedAnnotations.from(candidate).isPresent(ConstructorBinding.class)) {
-					Assert.state(candidate.getParameterCount() > 0,
-							type.getName() + " declares @ConstructorBinding on a no-args constructor");
-					Assert.state(constructor == null,
-							type.getName() + " has more than one @ConstructorBinding constructor");
-					constructor = candidate;
-				}
-			}
-			return constructor;
+		static BindMethod forType(Class<?> type) {
+			return (ConfigurationPropertiesBindConstructorProvider.INSTANCE.getBindConstructor(type) != null)
+					? VALUE_OBJECT : JAVA_BEAN;
 		}
 
 	}

spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBeanDefinitionValidator.java

@@ -54,7 +54,7 @@ public int getOrder() {
 
 	private void validate(ConfigurableListableBeanFactory beanFactory, String beanName) {
 		Class<?> beanClass = beanFactory.getType(beanName, false);
-		if (beanClass != null && BindMethod.forClass(beanClass) == BindMethod.VALUE_OBJECT) {
+		if (beanClass != null && BindMethod.forType(beanClass) == BindMethod.VALUE_OBJECT) {
 			throw new BeanCreationException(beanName,
 					"@EnableConfigurationProperties or @ConfigurationPropertiesScan must be used to add "
 							+ "@ConstructorBinding type " + beanClass.getName());

spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBeanRegistrar.java

@@ -88,7 +88,7 @@ private void registerBeanDefinition(String beanName, Class<?> type,
 	}
 
 	private BeanDefinition createBeanDefinition(String beanName, Class<?> type) {
-		if (BindMethod.forClass(type) == BindMethod.VALUE_OBJECT) {
+		if (BindMethod.forType(type) == BindMethod.VALUE_OBJECT) {
 			return new ConfigurationPropertiesValueObjectBeanDefinition(this.beanFactory, beanName, type);
 		}
 		GenericBeanDefinition definition = new GenericBeanDefinition();

spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBindConstructorProvider.java

@@ -0,0 +1,107 @@
+/*
+ * Copyright 2012-2019 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.boot.context.properties;
+
+import java.lang.reflect.Constructor;
+
+import org.springframework.beans.BeanUtils;
+import org.springframework.boot.context.properties.bind.BindConstructorProvider;
+import org.springframework.boot.context.properties.bind.Bindable;
+import org.springframework.core.KotlinDetector;
+import org.springframework.core.annotation.MergedAnnotations;
+import org.springframework.util.Assert;
+
+/**
+ * {@link BindConstructorProvider} used when binding
+ * {@link ConfigurationProperties @ConfigurationProperties}.
+ *
+ * @author Madhura Bhave
+ * @author Phillip Webb
+ */
+class ConfigurationPropertiesBindConstructorProvider implements BindConstructorProvider {
+
+	static final ConfigurationPropertiesBindConstructorProvider INSTANCE = new ConfigurationPropertiesBindConstructorProvider();
+
+	@Override
+	public Constructor<?> getBindConstructor(Bindable<?> bindable) {
+		return getBindConstructor(bindable.getType().resolve());
+	}
+
+	Constructor<?> getBindConstructor(Class<?> type) {
+		if (type == null) {
+			return null;
+		}
+		Constructor<?> constructor = findConstructorBindingAnnotatedConstructor(type);
+		if (constructor == null && isConstructorBindingAnnotatedType(type)) {
+			constructor = deduceBindConstructor(type);
+		}
+		return constructor;
+	}
+
+	private Constructor<?> findConstructorBindingAnnotatedConstructor(Class<?> type) {
+		if (isKotlinType(type)) {
+			Constructor<?> constructor = BeanUtils.findPrimaryConstructor(type);
+			if (constructor != null) {
+				return findAnnotatedConstructor(type, constructor);
+			}
+		}
+		return findAnnotatedConstructor(type, type.getDeclaredConstructors());
+	}
+
+	private Constructor<?> findAnnotatedConstructor(Class<?> type, Constructor<?>... candidates) {
+		Constructor<?> constructor = null;
+		for (Constructor<?> candidate : candidates) {
+			if (MergedAnnotations.from(candidate).isPresent(ConstructorBinding.class)) {
+				Assert.state(candidate.getParameterCount() > 0,
+						type.getName() + " declares @ConstructorBinding on a no-args constructor");
+				Assert.state(constructor == null,
+						type.getName() + " has more than one @ConstructorBinding constructor");
+				constructor = candidate;
+			}
+		}
+		return constructor;
+	}
+
+	private boolean isConstructorBindingAnnotatedType(Class<?> type) {
+		return MergedAnnotations.from(type, MergedAnnotations.SearchStrategy.TYPE_HIERARCHY_AND_ENCLOSING_CLASSES)
+				.isPresent(ConstructorBinding.class);
+	}
+
+	private Constructor<?> deduceBindConstructor(Class<?> type) {
+		if (isKotlinType(type)) {
+			return deducedKotlinBindConstructor(type);
+		}
+		Constructor<?>[] constructors = type.getDeclaredConstructors();
+		if (constructors.length == 1 && constructors[0].getParameterCount() > 0) {
+			return constructors[0];
+		}
+		return null;
+	}
+
+	private Constructor<?> deducedKotlinBindConstructor(Class<?> type) {
+		Constructor<?> primaryConstructor = BeanUtils.findPrimaryConstructor(type);
+		if (primaryConstructor != null && primaryConstructor.getParameterCount() > 0) {
+			return primaryConstructor;
+		}
+		return null;
+	}
+
+	private boolean isKotlinType(Class<?> type) {
+		return KotlinDetector.isKotlinPresent() && KotlinDetector.isKotlinType(type);
+	}
+
+}

spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBinder.java

@@ -151,7 +151,8 @@ private List<ConfigurationPropertiesBindHandlerAdvisor> getBindHandlerAdvisors()
 	private Binder getBinder() {
 		if (this.binder == null) {
 			this.binder = new Binder(getConfigurationPropertySources(), getPropertySourcesPlaceholdersResolver(),
-					getConversionService(), getPropertyEditorInitializer());
+					getConversionService(), getPropertyEditorInitializer(), null,
+					ConfigurationPropertiesBindConstructorProvider.INSTANCE);
 		}
 		return this.binder;
 	}

spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/BindConstructorProvider.java

@@ -0,0 +1,44 @@
+/*
+ * Copyright 2012-2019 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.boot.context.properties.bind;
+
+import java.lang.reflect.Constructor;
+
+/**
+ * Strategy interface used to determine a specific constructor to use when binding.
+ *
+ * @author Madhura Bhave
+ * @since 2.2.1
+ */
+@FunctionalInterface
+public interface BindConstructorProvider {
+
+	/**
+	 * Default {@link BindConstructorProvider} implementation that only returns a value
+	 * when there's a single constructor and when the bindable has no existing value.
+	 */
+	BindConstructorProvider DEFAULT = new DefaultBindConstructorProvider();
+
+	/**
+	 * Return the bind constructor to use for the given bindable, or {@code null} if
+	 * constructor binding is not supported.
+	 * @param bindable the bindable to check
+	 * @return the bind constructor or {@code null}
+	 */
+	Constructor<?> getBindConstructor(Bindable<?> bindable);
+
+}