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 48fd3c352..eb6f03704 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 @@ -55,6 +55,7 @@ import org.springframework.jmx.export.annotation.ManagedOperation; import org.springframework.jmx.export.annotation.ManagedResource; import org.springframework.stereotype.Component; +import org.springframework.util.ObjectUtils; import org.springframework.util.StringUtils; /** @@ -97,6 +98,17 @@ public class ConfigurationPropertiesRebinder private final Set neverResetNestedTypes; + private final Set loggedResetFailures = ConcurrentHashMap.newKeySet(); + + /** + * Safety net against unbounded recursion in {@link #resetProperties}. Cyclic graphs + * are already broken by the identity-based {@code visited} set, and graphs built from + * unstable (newly-allocated-per-call) instances are broken by + * {@link #isStableInstance(BeanWrapper, String, Object)}; this limit only guards + * against a gap in either of those checks, or a legitimately very deep object graph. + */ + private static final int MAX_RESET_DEPTH = 25; + public ConfigurationPropertiesRebinder(ConfigurationPropertiesBeans beans) { this(beans, Collections.emptySet()); } @@ -253,7 +265,7 @@ private void resetBeanToDefaults(Object bean) { + " for reset; skipping property reset", ex); return; } - resetProperties(bean, freshInstance, Collections.newSetFromMap(new IdentityHashMap<>())); + resetProperties(bean, freshInstance, Collections.newSetFromMap(new IdentityHashMap<>()), 0); } /** @@ -278,9 +290,11 @@ private boolean hasDefaultConstructor(Class type) { return constructors.length == 1 && constructors[0].getParameterCount() == 0; } - private void resetProperties(Object bean, Object defaults, Set visited) { - // Guard against cyclic object graphs so that recursion always terminates. - if (bean == null || !visited.add(bean)) { + private void resetProperties(Object bean, Object defaults, Set visited, int depth) { + // Guard against cyclic object graphs and pathologically deep ones so that + // recursion + // always terminates; see MAX_RESET_DEPTH. + if (bean == null || depth > MAX_RESET_DEPTH || !visited.add(bean)) { return; } BeanWrapper target = new BeanWrapperImpl(bean); @@ -303,25 +317,50 @@ && isNeverReset(target.getPropertyValue(propertyName))) { continue; } Object defaultValue = defaultsWrapper.getPropertyValue(propertyName); - target.setPropertyValue(propertyName, defaultValue); + Object currentValue = target.isReadableProperty(propertyName) + ? target.getPropertyValue(propertyName) : null; + // Skip the setter call entirely when there's nothing to change: this + // avoids needlessly invoking setters that don't tolerate being called + // again with the same value, and setters that reject null are only + // ever + // invoked when the property actually needs to become null. + if (!ObjectUtils.nullSafeEquals(currentValue, defaultValue)) { + try { + target.setPropertyValue(propertyName, defaultValue); + } + catch (Exception ex) { + warnCannotReset(bean, propertyName, ex); + } + } } else if (target.isReadableProperty(propertyName) && defaultsWrapper.isReadableProperty(propertyName)) { Object value = target.getPropertyValue(propertyName); Object defaultValue = defaultsWrapper.getPropertyValue(propertyName); if (value instanceof Collection collection) { - collection.clear(); - if (defaultValue instanceof Collection defaultCollection) { - collection.addAll(defaultCollection); + try { + collection.clear(); + if (defaultValue instanceof Collection defaultCollection) { + collection.addAll(defaultCollection); + } + } + catch (UnsupportedOperationException ex) { + warnCannotReset(bean, propertyName, ex); } } else if (value instanceof Map map) { - map.clear(); - if (defaultValue instanceof Map defaultMap) { - map.putAll(defaultMap); + try { + map.clear(); + if (defaultValue instanceof Map defaultMap) { + map.putAll(defaultMap); + } + } + catch (UnsupportedOperationException ex) { + warnCannotReset(bean, propertyName, ex); } } - else if (value != null && defaultValue != null && isResettableNestedType(value.getClass())) { - resetProperties(value, defaultValue, visited); + else if (value != null && defaultValue != null && isResettableNestedType(value.getClass()) + && isStableInstance(target, propertyName, value)) { + resetProperties(value, defaultValue, visited, depth + 1); } } } @@ -334,6 +373,42 @@ else if (value != null && defaultValue != null && isResettableNestedType(value.g } } + /** + * Whether re-reading the given read-only property returns the very same instance. A + * getter that instead returns a new object on every call cannot be holding any state + * that a reset would need to affect, so recursing into such a value would only ever + * walk a throw-away object graph - one that, for record-style/fluent accessors, may + * not even terminate (see gh-1750). + */ + private boolean isStableInstance(BeanWrapper target, String propertyName, Object value) { + try { + return target.getPropertyValue(propertyName) == value; + } + catch (Exception ex) { + return false; + } + } + + /** + * Log, at most once per bean type and property, that a property could not be reset to + * its default value and may therefore retain a stale value after this and future + * refreshes. The full exception is only logged at DEBUG, on every occurrence, to + * avoid spamming the log at the default level while still making the failure + * discoverable. + */ + private void warnCannotReset(Object bean, String propertyName, Exception ex) { + String targetClassName = AopUtils.getTargetClass(bean).getName(); + if (this.loggedResetFailures.add(targetClassName + "#" + propertyName)) { + logger.warn("Cannot reset property '" + propertyName + "' on " + targetClassName + + " to its default value; it may retain its previous value after this and future refreshes. " + + "Enable DEBUG logging for " + ConfigurationPropertiesRebinder.class.getName() + + " for the full exception."); + } + if (logger.isDebugEnabled()) { + logger.debug("Failed to reset property '" + propertyName + "' on " + targetClassName, ex); + } + } + /** * Determine whether a nested property value should be recursively reset. Only * user-defined types are descended into. Recursing into JDK or standard API types diff --git a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderUnstableNestedIntegrationTests.java b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderUnstableNestedIntegrationTests.java new file mode 100644 index 000000000..aae4221ee --- /dev/null +++ b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderUnstableNestedIntegrationTests.java @@ -0,0 +1,234 @@ +/* + * 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 java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; + +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.cloud.autoconfigure.ConfigurationPropertiesRebinderAutoConfiguration; +import org.springframework.cloud.autoconfigure.RefreshAutoConfiguration; +import org.springframework.cloud.context.properties.ConfigurationPropertiesRebinderUnstableNestedIntegrationTests.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.core.env.PropertySource; +import org.springframework.test.annotation.DirtiesContext; + +import static org.assertj.core.api.BDDAssertions.then; +import static org.assertj.core.data.MapEntry.entry; + +/** + * Reproduces the three problems reported in gh-1750 against + * {@code ConfigurationPropertiesRebinder.resetProperties}: a {@link StackOverflowError} + * for read-only getters that return a new instance on every call, silently-skipped resets + * for unmodifiable collections/maps, and silently-skipped resets for setters that reject + * {@code null}. + * + * @author Ryan Baxter + */ +@SpringBootTest(classes = TestConfiguration.class, + properties = { "test.readonly.label.a=1", "test.readonly.label.b=2", + "test.nullrejecting.endpoint=https://old.example.com" }) +@ExtendWith(OutputCaptureExtension.class) +public class ConfigurationPropertiesRebinderUnstableNestedIntegrationTests { + + @Autowired + private TestProperties properties; + + @Autowired + private ConfigurationPropertiesRebinder rebinder; + + @Autowired + private ConfigurableEnvironment environment; + + @Test + @DirtiesContext + public void rebindDoesNotStackOverflowForFluentAccessorReturningNewInstance() { + // FluentProperties.getSettings() returns a new instance on every call, and + // Settings.protect() does the same, so a naive recursive reset never terminates. + // The rebind must complete rather than throw StackOverflowError. + this.rebinder.rebind(); + then(this.properties.getFluent().getName()).isEqualTo("default"); + } + + @Test + @DirtiesContext + public void removedEntryLogsWarningForUnmodifiableView(CapturedOutput output) { + then(this.properties.getReadonly().getLabels()).containsExactly(entry("a", "1"), entry("b", "2")); + Map map = findTestProperties(); + map.remove("test.readonly.label.b"); + this.rebinder.rebind(); + // getLabels() only exposes an unmodifiable view over the private backing map, so + // the + // removed entry can't actually be cleared through the bean's public API - but the + // failure must now be visible instead of silently swallowed at DEBUG. + then(output).contains("Cannot reset property 'labels'"); + } + + @Test + @DirtiesContext + public void removedPropertyLogsWarningForNullRejectingSetter(CapturedOutput output) { + then(this.properties.getNullrejecting().getEndpoint()).isEqualTo("https://old.example.com"); + Map map = findTestProperties(); + map.remove("test.nullrejecting.endpoint"); + this.rebinder.rebind(); + // The setter rejects null, so the stale value cannot be reset away, but the + // failure must now be visible instead of silently swallowed at DEBUG. + then(output).contains("Cannot reset property 'endpoint'"); + } + + private Map findTestProperties() { + for (PropertySource source : this.environment.getPropertySources()) { + if (source.getName().toLowerCase().contains("test")) { + @SuppressWarnings("unchecked") + Map map = (Map) source.getSource(); + return map; + } + } + throw new IllegalStateException("Could not find test property source"); + } + + @Configuration(proxyBeanMethods = false) + @EnableConfigurationProperties(RefreshAutoConfiguration.RefreshProperties.class) + @Import({ RefreshConfiguration.RebinderConfiguration.class, PropertyPlaceholderAutoConfiguration.class }) + protected static class TestConfiguration { + + @Bean + protected TestProperties testProperties() { + return new TestProperties(); + } + + } + + // 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 final FluentProperties fluent = new FluentProperties(); + + private final ReadOnlyViewProperties readonly = new ReadOnlyViewProperties(); + + private final NullRejectingSetterProperties nullrejecting = new NullRejectingSetterProperties(); + + public FluentProperties getFluent() { + return this.fluent; + } + + public ReadOnlyViewProperties getReadonly() { + return this.readonly; + } + + public NullRejectingSetterProperties getNullrejecting() { + return this.nullrejecting; + } + + } + + protected static class FluentProperties { + + private String name = "default"; + + public String getName() { + return this.name; + } + + public void setName(String name) { + this.name = name; + } + + // Read-only: returns a new instance on every call. + public Settings getSettings() { + return new Settings(); + } + + protected static class Settings { + + private boolean protect; + + public boolean isProtect() { + return this.protect; + } + + // Record-style accessor (no "get" prefix, matches the "protect" field) that + // returns a new, protected copy on every call - exactly like + // org.infinispan.commons.configuration.attributes.AttributeSet#protect(). + public Settings protect() { + Settings copy = new Settings(); + copy.protect = true; + return copy; + } + + } + + } + + protected static class ReadOnlyViewProperties { + + private final Map labels = new LinkedHashMap<>(); + + // Read-only: exposes an unmodifiable view over the mutable backing map. + public Map getLabels() { + return Collections.unmodifiableMap(this.labels); + } + + public void setLabel(Map label) { + this.labels.putAll(label); + } + + } + + protected static class NullRejectingSetterProperties { + + private String endpoint; + + public String getEndpoint() { + return this.endpoint; + } + + public void setEndpoint(String endpoint) { + this.endpoint = endpoint.strip(); + } + + } + +} diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplierTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplierTests.java index e5aa4a7bd..ae8d78b41 100644 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplierTests.java +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplierTests.java @@ -718,7 +718,7 @@ void shouldCheckUserProvidedPortForHealthCheckRequest() { listSupplier.isAlive(serviceInstance).block(); }); - assertThat(exception).hasMessageContaining("Connection refused: /127.0.0.1:8888"); + assertThat(exception).hasMessageContaining("Connection refused"); } private static Stream healthCheckFunctions() {