diff --git a/docs/modules/ROOT/pages/spring-cloud-commons/application-context-services.adoc b/docs/modules/ROOT/pages/spring-cloud-commons/application-context-services.adoc index 8677999cb..aa9822dfe 100644 --- a/docs/modules/ROOT/pages/spring-cloud-commons/application-context-services.adoc +++ b/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. diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java b/spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java index d6dcadc8e..7aa846959 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinder.java +++ b/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 only 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. + *

+ * 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 visited) { diff --git a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderMultipleConstructorsIntegrationTests.java b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderMultipleConstructorsIntegrationTests.java new file mode 100644 index 000000000..49116702f --- /dev/null +++ b/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 alongside 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 + * gh-1733, + * 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; + } + + } + +}