diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index d10e429d0..4c8df033f 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -2,7 +2,7 @@ name: PR on: pull_request: - branches: [ 4.3.x ] + branches: [ main, 4.3.x ] jobs: build: 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 fec51bedc..14a19b808 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 @@ -170,6 +170,18 @@ The `EnvironmentChangeEvent` covers a large class of refresh use cases, as long Note that those APIs are public and part of core Spring. You can verify that the changes are bound to `@ConfigurationProperties` beans by visiting the `/configprops` endpoint (a standard Spring Boot Actuator feature). For instance, a `DataSource` can have its `maxPoolSize` changed at runtime (the default `DataSource` created by Spring Boot is a `@ConfigurationProperties` bean) and grow capacity dynamically. + +When a `@ConfigurationProperties` bean is re-bound, its properties are first reset to their class-level defaults so that values for properties that have been removed from the `Environment` do not linger. +This reset also recurses into nested `@ConfigurationProperties` objects (for example a nested object that has no setter). +To avoid descending into object graphs that are not configuration properties, the reset never recurses into JDK types or types in the standard `jakarta.*` and `javax.*` API namespaces. +If you have a `@ConfigurationProperties` bean that holds a reference to some other library type that should not be recursed into (for example, a type with a cyclic object graph or one that holds internal state that must not be mutated), you can exclude it by setting `spring.cloud.refresh.never-reset-nested-types`. +The value is a comma-separated list of fully qualified class names or package prefixes, each matched against the type's class name with a prefix comparison: + +[source,properties] +---- +spring.cloud.refresh.never-reset-nested-types=com.example.MyClient,com.acme.sdk. +---- + Re-binding `@ConfigurationProperties` does not cover another large class of use cases, where you need more control over the refresh and where you need a change to be atomic over the whole `ApplicationContext`. To address those concerns, we have `@RefreshScope`. diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/ConfigurationPropertiesRebinderAutoConfiguration.java b/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/ConfigurationPropertiesRebinderAutoConfiguration.java index f827d0b05..959ec4dfe 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/ConfigurationPropertiesRebinderAutoConfiguration.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/ConfigurationPropertiesRebinderAutoConfiguration.java @@ -16,11 +16,13 @@ package org.springframework.cloud.autoconfigure; +import org.springframework.beans.factory.ObjectProvider; import org.springframework.beans.factory.SmartInitializingSingleton; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.autoconfigure.condition.SearchStrategy; import org.springframework.boot.context.properties.ConfigurationPropertiesBindingPostProcessor; +import org.springframework.cloud.autoconfigure.RefreshAutoConfiguration.RefreshProperties; import org.springframework.cloud.context.properties.ConfigurationPropertiesBeans; import org.springframework.cloud.context.properties.ConfigurationPropertiesRebinder; import org.springframework.context.ApplicationContext; @@ -50,9 +52,13 @@ public static ConfigurationPropertiesBeans configurationPropertiesBeans() { @Bean @ConditionalOnMissingBean(search = SearchStrategy.CURRENT) - public ConfigurationPropertiesRebinder configurationPropertiesRebinder(ConfigurationPropertiesBeans beans) { - ConfigurationPropertiesRebinder rebinder = new ConfigurationPropertiesRebinder(beans); - return rebinder; + public ConfigurationPropertiesRebinder configurationPropertiesRebinder(ConfigurationPropertiesBeans beans, + ObjectProvider refreshProperties) { + RefreshProperties properties = refreshProperties.getIfAvailable(); + if (properties != null) { + return new ConfigurationPropertiesRebinder(beans, properties); + } + return new ConfigurationPropertiesRebinder(beans); } @Override diff --git a/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/RefreshAutoConfiguration.java b/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/RefreshAutoConfiguration.java index 146f04d74..0a30471c4 100644 --- a/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/RefreshAutoConfiguration.java +++ b/spring-cloud-context/src/main/java/org/springframework/cloud/autoconfigure/RefreshAutoConfiguration.java @@ -18,6 +18,7 @@ import java.util.ArrayList; import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; import java.util.Set; @@ -138,6 +139,16 @@ public static class RefreshProperties { */ private List additionalPropertySourcesToRetain = new ArrayList<>(); + /** + * Fully qualified class name prefixes of nested configuration properties values + * that should never be recursively reset when a bean is rebound. JDK and standard + * API types are always skipped; use this to additionally exclude library types + * whose object graphs are cyclic or hold internal state that must not be mutated. + * Each entry is matched against the fully qualified class name using a prefix + * comparison, so either a package or an individual class name can be supplied. + */ + private Set neverResetNestedTypes = new LinkedHashSet<>(); + public List getAdditionalPropertySourcesToRetain() { return this.additionalPropertySourcesToRetain; } @@ -146,10 +157,19 @@ public void setAdditionalPropertySourcesToRetain(List additionalProperty this.additionalPropertySourcesToRetain = additionalPropertySourcesToRetain; } + public Set getNeverResetNestedTypes() { + return this.neverResetNestedTypes; + } + + public void setNeverResetNestedTypes(Set neverResetNestedTypes) { + this.neverResetNestedTypes = neverResetNestedTypes; + } + @Override public String toString() { return new ToStringCreator(this) .append("additionalPropertySourcesToRetain", additionalPropertySourcesToRetain) + .append("neverResetNestedTypes", neverResetNestedTypes) .toString(); } 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 363c8b961..386bf30cc 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 @@ -19,7 +19,9 @@ import java.beans.PropertyDescriptor; import java.lang.reflect.Modifier; import java.util.Collection; +import java.util.Collections; import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -37,6 +39,7 @@ import org.springframework.beans.BeansException; import org.springframework.beans.factory.BeanFactoryUtils; import org.springframework.boot.context.properties.ConfigurationProperties; +import org.springframework.cloud.autoconfigure.RefreshAutoConfiguration.RefreshProperties; import org.springframework.cloud.context.config.annotation.RefreshScope; import org.springframework.cloud.context.environment.EnvironmentChangeEvent; import org.springframework.cloud.util.ProxyUtils; @@ -75,8 +78,19 @@ public class ConfigurationPropertiesRebinder private Map errors = new ConcurrentHashMap<>(); + private final Set neverResetNestedTypes; + public ConfigurationPropertiesRebinder(ConfigurationPropertiesBeans beans) { + this(beans, Collections.emptySet()); + } + + public ConfigurationPropertiesRebinder(ConfigurationPropertiesBeans beans, RefreshProperties refreshProperties) { + this(beans, refreshProperties.getNeverResetNestedTypes()); + } + + private ConfigurationPropertiesRebinder(ConfigurationPropertiesBeans beans, Set neverResetNestedTypes) { this.beans = beans; + this.neverResetNestedTypes = neverResetNestedTypes; } @Override @@ -196,10 +210,14 @@ private void resetBeanToDefaults(Object bean) { + " for reset; skipping property reset", ex); return; } - resetProperties(bean, freshInstance); + resetProperties(bean, freshInstance, Collections.newSetFromMap(new IdentityHashMap<>())); } - private void resetProperties(Object bean, Object defaults) { + 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)) { + return; + } BeanWrapper target = new BeanWrapperImpl(bean); BeanWrapper defaultsWrapper = new BeanWrapperImpl(defaults); for (PropertyDescriptor pd : target.getPropertyDescriptors()) { @@ -227,8 +245,8 @@ else if (value instanceof Map map) { map.putAll(defaultMap); } } - else if (value != null && defaultValue != null && !BeanUtils.isSimpleValueType(value.getClass())) { - resetProperties(value, defaultValue); + else if (value != null && defaultValue != null && isResettableNestedType(value.getClass())) { + resetProperties(value, defaultValue, visited); } } } @@ -241,6 +259,51 @@ else if (value != null && defaultValue != null && !BeanUtils.isSimpleValueType(v } } + /** + * Determine whether a nested property value should be recursively reset. Only + * user-defined types are descended into. Recursing into JDK or standard API types + * (for example {@link javax.net.ssl.SSLContext}) is both unnecessary and unsafe: + * their object graphs may be cyclic, which previously led to a + * {@link StackOverflowError} (see gh-1698), and they may expose internal collections + * or maps that must not be cleared. Additional types can be excluded via the + * {@code spring.cloud.refresh.never-reset-nested-types} property. + */ + boolean isResettableNestedType(Class type) { + if (type.isArray() || BeanUtils.isSimpleValueType(type) || isJdkClass(type) || isStandardApiClass(type)) { + return false; + } + String typeName = type.getName(); + for (String excluded : this.neverResetNestedTypes) { + if (typeName.startsWith(excluded)) { + return false; + } + } + return true; + } + + /** + * Whether the given type is provided by the JDK itself. JDK types are loaded either + * by the bootstrap class loader (for example everything in {@code java.base}, which + * reports a {@code null} class loader) or by the platform class loader (the other + * platform modules); application and library types are loaded by the application + * class loader. + */ + private boolean isJdkClass(Class type) { + ClassLoader classLoader = type.getClassLoader(); + return classLoader == null || classLoader == ClassLoader.getPlatformClassLoader(); + } + + /** + * Whether the given type belongs to a standard API namespace (Jakarta EE or the + * legacy {@code javax} packages). These namespaces are reserved for specifications + * and, unlike the JDK modules, are loaded by the application class loader, so they + * are not detected by {@link #isJdkClass}. + */ + private boolean isStandardApiClass(Class type) { + String packageName = type.getPackageName(); + return packageName.startsWith("jakarta.") || packageName.startsWith("javax."); + } + @ManagedAttribute public Set getNeverRefreshable() { String neverRefresh = this.applicationContext.getEnvironment() diff --git a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderClearIntegrationTests.java b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderClearIntegrationTests.java index a2f4d06ba..1d583f915 100644 --- a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderClearIntegrationTests.java +++ b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderClearIntegrationTests.java @@ -21,6 +21,8 @@ import java.util.List; import java.util.Map; +import javax.net.ssl.SSLContext; + import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; @@ -154,6 +156,21 @@ public void testNestedPropertyWithoutSetterRestoredOnRemoval() { then(this.properties.getNested().getHost()).isEqualTo("default-host"); } + @Test + @DirtiesContext + public void testRebindWithJdkTypePropertyDoesNotStackOverflow() { + // gh-1698: a property whose value is a JDK type with a cyclic object graph (e.g. + // SSLContext) previously caused infinite recursion in resetProperties. + then(this.properties.getSslContext()).isNotNull(); + SSLContext original = this.properties.getSslContext(); + Map map = findTestProperties(); + map.remove("test.message"); + // Should not throw StackOverflowError + this.rebinder.rebind(); + // The JDK-typed property must be left untouched by the reset + then(this.properties.getSslContext()).isSameAs(original); + } + private Map findTestProperties() { for (PropertySource source : this.environment.getPropertySources()) { if (source.getName().toLowerCase().contains("test")) { @@ -206,6 +223,8 @@ protected static class TestProperties { private final NestedProperties nested = new NestedProperties(); + private transient SSLContext sslContext; + public String getMessage() { return this.message; } @@ -242,6 +261,19 @@ public NestedProperties getNested() { return this.nested; } + public synchronized SSLContext getSslContext() { + if (this.sslContext != null) { + return this.sslContext; + } + try { + this.sslContext = SSLContext.getDefault(); + } + catch (Exception ex) { + throw new IllegalStateException(ex); + } + return this.sslContext; + } + } protected static class NestedProperties { diff --git a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderResetExclusionIntegrationTests.java b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderResetExclusionIntegrationTests.java new file mode 100644 index 000000000..31be03098 --- /dev/null +++ b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderResetExclusionIntegrationTests.java @@ -0,0 +1,135 @@ +/* + * 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.Map; + +import org.junit.jupiter.api.Test; + +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.cloud.autoconfigure.ConfigurationPropertiesRebinderAutoConfiguration; +import org.springframework.cloud.autoconfigure.RefreshAutoConfiguration; +import org.springframework.cloud.context.properties.ConfigurationPropertiesRebinderResetExclusionIntegrationTests.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; + +/** + * Verifies that types listed in {@code spring.cloud.refresh.never-reset-nested-types} are + * not recursively reset during rebind. + * + * @author Ryan Baxter + */ +@SpringBootTest(classes = TestConfiguration.class, properties = { "test.nested.host=custom-host", + "spring.cloud.refresh.never-reset-nested-types=org.springframework.cloud.context.properties.ConfigurationPropertiesRebinderResetExclusionIntegrationTests$NestedProperties" }) +public class ConfigurationPropertiesRebinderResetExclusionIntegrationTests { + + @Autowired + private TestProperties properties; + + @Autowired + private ConfigurationPropertiesRebinder rebinder; + + @Autowired + private ConfigurableEnvironment environment; + + @Test + @DirtiesContext + public void excludedNestedTypeIsNotReset() { + then(this.properties.getNested().getHost()).isEqualTo("custom-host"); + // Remove the nested property and rebind + Map map = findTestProperties(); + map.remove("test.nested.host"); + this.rebinder.rebind(); + // Because the nested type is excluded, it is not descended into and its value is + // left untouched rather than being reset to the field default. + then(this.properties.getNested().getHost()).isEqualTo("custom-host"); + } + + 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 NestedProperties nested = new NestedProperties(); + + public NestedProperties getNested() { + return this.nested; + } + + } + + protected static class NestedProperties { + + private String host = "default-host"; + + public String getHost() { + return this.host; + } + + public void setHost(String host) { + this.host = host; + } + + } + +} diff --git a/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTests.java b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTests.java new file mode 100644 index 000000000..6ac56debc --- /dev/null +++ b/spring-cloud-context/src/test/java/org/springframework/cloud/context/properties/ConfigurationPropertiesRebinderTests.java @@ -0,0 +1,83 @@ +/* + * 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.List; + +import javax.net.ssl.SSLContext; +import javax.sql.DataSource; + +import jakarta.annotation.PostConstruct; +import jakarta.persistence.EntityManager; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.BDDAssertions.then; + +/** + * Unit tests for {@link ConfigurationPropertiesRebinder#isResettableNestedType}. + * + * @author Ryan Baxter + */ +public class ConfigurationPropertiesRebinderTests { + + private final ConfigurationPropertiesRebinder rebinder = new ConfigurationPropertiesRebinder( + new ConfigurationPropertiesBeans()); + + @Test + public void userDefinedTypeIsResettable() { + then(this.rebinder.isResettableNestedType(NestedProperties.class)).isTrue(); + } + + @Test + public void simpleValueAndArrayTypesAreNotResettable() { + then(this.rebinder.isResettableNestedType(String.class)).isFalse(); + then(this.rebinder.isResettableNestedType(Integer.class)).isFalse(); + then(this.rebinder.isResettableNestedType(NestedProperties[].class)).isFalse(); + } + + @Test + public void jdkTypesAreNotResettable() { + // Loaded by the bootstrap (java.base) or platform (java.sql) class loader + then(this.rebinder.isResettableNestedType(SSLContext.class)).isFalse(); + then(this.rebinder.isResettableNestedType(List.class)).isFalse(); + then(this.rebinder.isResettableNestedType(DataSource.class)).isFalse(); + } + + @Test + public void jakartaApiTypesAreNotResettable() { + // Loaded by the application class loader, so not detected as a JDK type by the + // class loader check; only caught by the standard API namespace check. + then(EntityManager.class.getClassLoader()).isNotNull(); + then(this.rebinder.isResettableNestedType(EntityManager.class)).isFalse(); + then(this.rebinder.isResettableNestedType(PostConstruct.class)).isFalse(); + } + + protected static class NestedProperties { + + private String host = "default-host"; + + public String getHost() { + return this.host; + } + + public void setHost(String host) { + this.host = host; + } + + } + +}