Skip reset to default functionality when a bean contains more than one constructor

docs/modules/ROOT/pages/spring-cloud-commons/application-context-services.adoc

@@ -182,6 +182,10 @@ The value is a comma-separated list of fully qualified class names or package pr
 spring.cloud.refresh.never-reset-nested-types=com.example.MyClient,com.acme.sdk.
 ----
 
+NOTE: The reset to class-level defaults only happens for a bean whose class declares a no-argument constructor as its *only* constructor.
+If a `@ConfigurationProperties` bean's class declares a no-argument constructor alongside another constructor that takes arguments (for example, a no-arg constructor kept only for a framework's benefit, next to a constructor that takes a collaborator and uses it to compute the bean's real defaults), the reset is skipped entirely for that bean, and only the values already carried by the `Environment` are re-bound on top of whatever the bean currently holds.
+This is deliberate: instantiating such a class through its no-argument constructor would not reproduce the defaults the bean was actually constructed with, and resetting to it would wipe out values that no property source ever carried.
+
 NOTE: Re-binding mutates the `@ConfigurationProperties` bean's fields in place, destroying and re-initializing the same instance rather than swapping it out for a new one.
 Concurrent rebinds of the same bean are serialized internally, but this does not make the bean safe to read from other threads while a rebind is in progress: a concurrent reader can observe transient, partially-updated state (for example, a property briefly reset to its class-level default before the new value is applied).
 If your application needs a consistent view of a bean's properties across a refresh, use `@RefreshScope` instead, which serializes reads against refreshes for beans in that scope.

spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java

@@ -232,11 +232,12 @@ private boolean rebind(String name, ApplicationContext appContext) {
 	private void resetBeanToDefaults(Object bean) {
 		Class<?> targetClass = AopUtils.getTargetClass(bean);
 		if (!hasDefaultConstructor(targetClass)) {
-			// Beans that have no default constructor (for example constructor-bound beans
-			// or beans with required dependencies) cannot be instantiated to obtain their
-			// defaults, so the reset is skipped. The bean is still re-bound from the
-			// Environment afterwards; only reverting removed properties to their defaults
-			// is skipped.
+			// Beans whose no-arg constructor (if any) is not their only constructor
+			// (for example constructor-bound beans, beans with required dependencies, or
+			// beans with an extra no-arg constructor kept only for a framework's benefit)
+			// cannot be instantiated to obtain trustworthy defaults, so the reset is
+			// skipped. The bean is still re-bound from the Environment afterwards; only
+			// reverting removed properties to their defaults is skipped.
 			if (logger.isDebugEnabled()) {
 				logger.debug("No default constructor for " + targetClass.getName()
 						+ "; skipping property reset before rebinding");
@@ -256,17 +257,25 @@ private void resetBeanToDefaults(Object bean) {
 	}
 
 	/**
-	 * Whether the given type declares a no-argument constructor (of any visibility),
+	 * Whether the given type's <em>only</em> declared constructor is a no-argument one,
 	 * which is what {@link BeanUtils#instantiateClass(Class)} needs to build a defaults
-	 * template.
+	 * template that is actually representative of the bean's defaults.
+	 * <p>
+	 * A no-arg constructor that sits alongside another, parameterized constructor is
+	 * deliberately not enough here, regardless of its visibility: some beans declare one
+	 * purely for a framework's benefit (for example Jackson deserialization) next to
+	 * another constructor that takes collaborators and computes the bean's real defaults
+	 * (for example from the current host). Instantiating the type through the no-arg
+	 * constructor then produces a template with the wrong defaults - typically
+	 * {@code null} or zero-valued fields - for anything the other constructor would have
+	 * computed, and resetting the bean to that template before rebinding wipes out values
+	 * no property source ever carried (see gh-1733). Only a type whose sole constructor
+	 * takes no arguments can be assumed to have one intended, complete way of producing a
+	 * default instance.
 	 */
 	private boolean hasDefaultConstructor(Class<?> type) {
-		for (Constructor<?> constructor : type.getDeclaredConstructors()) {
-			if (constructor.getParameterCount() == 0) {
-				return true;
-			}
-		}
-		return false;
+		Constructor<?>[] constructors = type.getDeclaredConstructors();
+		return constructors.length == 1 && constructors[0].getParameterCount() == 0;
 	}
 
 	private void resetProperties(Object bean, Object defaults, Set<Object> visited) {

spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderMultipleConstructorsIntegrationTests.java

@@ -0,0 +1,146 @@
+/*
+ * Copyright 2012-present 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.cloud.context.properties;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.autoconfigure.context.PropertyPlaceholderAutoConfiguration;
+import org.springframework.boot.context.properties.ConfigurationProperties;
+import org.springframework.boot.context.properties.EnableConfigurationProperties;
+import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.boot.test.system.CapturedOutput;
+import org.springframework.boot.test.system.OutputCaptureExtension;
+import org.springframework.boot.test.util.TestPropertyValues;
+import org.springframework.cloud.autoconfigure.ConfigurationPropertiesRebinderAutoConfiguration;
+import org.springframework.cloud.autoconfigure.RefreshAutoConfiguration;
+import org.springframework.cloud.context.properties.ConfigurationPropertiesRebinderMultipleConstructorsIntegrationTests.TestConfiguration;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.Import;
+import org.springframework.core.env.ConfigurableEnvironment;
+import org.springframework.test.annotation.DirtiesContext;
+
+import static org.assertj.core.api.BDDAssertions.then;
+
+/**
+ * Verifies that a {@code @ConfigurationProperties} bean which declares a no-argument
+ * constructor <em>alongside</em> another constructor is rebound without discarding values
+ * that were computed by the constructor actually used to create it (for example from a
+ * collaborator), even though no property source carries those values. Reproduces
+ * <a href="https://github.com/spring-cloud/spring-cloud-commons/issues/1733">gh-1733</a>,
+ * where {@code EurekaInstanceConfigBean} - which declares a private no-arg constructor
+ * next to the public one that computes {@code ipAddress} and {@code hostname} - had those
+ * fields wiped out on every refresh.
+ *
+ * @author Ryan Baxter
+ */
+@SpringBootTest(classes = TestConfiguration.class, properties = "test.message=Hello")
+@ExtendWith(OutputCaptureExtension.class)
+public class ConfigurationPropertiesRebinderMultipleConstructorsIntegrationTests {
+
+	@Autowired
+	private TestProperties properties;
+
+	@Autowired
+	private ConfigurationPropertiesRebinder rebinder;
+
+	@Autowired
+	private ConfigurableEnvironment environment;
+
+	@Test
+	@DirtiesContext
+	public void rebindPreservesValueComputedByThePublicConstructor(CapturedOutput output) {
+		then(this.properties.getMessage()).isEqualTo("Hello");
+		then(this.properties.getComputed()).isEqualTo("computed-value");
+		// Change a property that the bean does carry and rebind, exactly as an
+		// EnvironmentChangeEvent-triggered refresh would.
+		TestPropertyValues.of("test.message=World").applyTo(this.environment);
+		this.rebinder.rebind();
+		// Rebinding still applies the new value...
+		then(this.properties.getMessage()).isEqualTo("World");
+		// ...but does not wipe out a value no property source ever carried, just
+		// because the bean also declares a no-arg constructor besides the one that
+		// actually computed the value.
+		then(this.properties.getComputed()).isEqualTo("computed-value");
+		then(output).doesNotContain("Cannot create default instance");
+	}
+
+	@Configuration(proxyBeanMethods = false)
+	@EnableConfigurationProperties
+	@Import({ RefreshConfiguration.RebinderConfiguration.class, PropertyPlaceholderAutoConfiguration.class })
+	protected static class TestConfiguration {
+
+		@Bean
+		protected TestProperties testProperties() {
+			return new TestProperties("computed-value");
+		}
+
+	}
+
+	// Hack out a protected inner class for testing
+	protected static class RefreshConfiguration extends RefreshAutoConfiguration {
+
+		@Configuration(proxyBeanMethods = false)
+		protected static class RebinderConfiguration extends ConfigurationPropertiesRebinderAutoConfiguration {
+
+			public RebinderConfiguration(ApplicationContext context) {
+				super(context);
+			}
+
+		}
+
+	}
+
+	@ConfigurationProperties("test")
+	protected static class TestProperties {
+
+		private String computed;
+
+		private String message;
+
+		// Declared for a framework's benefit only (for example bean deserialization);
+		// it does not perform the computation that the constructor below does, which
+		// is exactly the trap described in gh-1733.
+		private TestProperties() {
+		}
+
+		public TestProperties(String computed) {
+			this.computed = computed;
+		}
+
+		public String getComputed() {
+			return this.computed;
+		}
+
+		public void setComputed(String computed) {
+			this.computed = computed;
+		}
+
+		public String getMessage() {
+			return this.message;
+		}
+
+		public void setMessage(String message) {
+			this.message = message;
+		}
+
+	}
+
+}