From 3096bb6d0ce090c773de0c4cdbd514dd034ae39b Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Sun, 11 May 2025 13:24:40 +0200 Subject: [PATCH 1/2] Polishing --- ...ertySourcesPlaceholderConfigurerTests.java | 55 ++++++++----------- 1 file changed, 24 insertions(+), 31 deletions(-) diff --git a/spring-context/src/test/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurerTests.java b/spring-context/src/test/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurerTests.java index 9fee2b56e9..81ccd73e46 100644 --- a/spring-context/src/test/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurerTests.java +++ b/spring-context/src/test/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurerTests.java @@ -21,6 +21,8 @@ import java.util.Properties; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.springframework.beans.factory.BeanCreationException; import org.springframework.beans.factory.BeanDefinitionStoreException; @@ -90,14 +92,29 @@ class PropertySourcesPlaceholderConfigurerTests { assertThat(bf.getBean(TestBean.class).getName()).isEqualTo("foo"); } - @Test - void localPropertiesOverrideFalse() { - localPropertiesOverride(false); - } + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void localPropertiesOverride(boolean override) { + DefaultListableBeanFactory bf = new DefaultListableBeanFactory(); + bf.registerBeanDefinition("testBean", + genericBeanDefinition(TestBean.class) + .addPropertyValue("name", "${foo}") + .getBeanDefinition()); - @Test - void localPropertiesOverrideTrue() { - localPropertiesOverride(true); + PropertySourcesPlaceholderConfigurer ppc = new PropertySourcesPlaceholderConfigurer(); + + ppc.setLocalOverride(override); + ppc.setProperties(new Properties() {{ + setProperty("foo", "local"); + }}); + ppc.setEnvironment(new MockEnvironment().withProperty("foo", "enclosing")); + ppc.postProcessBeanFactory(bf); + if (override) { + assertThat(bf.getBean(TestBean.class).getName()).isEqualTo("local"); + } + else { + assertThat(bf.getBean(TestBean.class).getName()).isEqualTo("enclosing"); + } } @Test @@ -283,30 +300,6 @@ class PropertySourcesPlaceholderConfigurerTests { assertThat(bf.getBean(TestBean.class).getName()).isEqualTo("bar"); } - @SuppressWarnings("serial") - private void localPropertiesOverride(boolean override) { - DefaultListableBeanFactory bf = new DefaultListableBeanFactory(); - bf.registerBeanDefinition("testBean", - genericBeanDefinition(TestBean.class) - .addPropertyValue("name", "${foo}") - .getBeanDefinition()); - - PropertySourcesPlaceholderConfigurer ppc = new PropertySourcesPlaceholderConfigurer(); - - ppc.setLocalOverride(override); - ppc.setProperties(new Properties() {{ - setProperty("foo", "local"); - }}); - ppc.setEnvironment(new MockEnvironment().withProperty("foo", "enclosing")); - ppc.postProcessBeanFactory(bf); - if (override) { - assertThat(bf.getBean(TestBean.class).getName()).isEqualTo("local"); - } - else { - assertThat(bf.getBean(TestBean.class).getName()).isEqualTo("enclosing"); - } - } - @Test void customPlaceholderPrefixAndSuffix() { PropertySourcesPlaceholderConfigurer ppc = new PropertySourcesPlaceholderConfigurer(); From ebb44a836805653497072b47127345189dc2df5f Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Sun, 11 May 2025 13:33:37 +0200 Subject: [PATCH 2/2] Restore support for non-EnumerablePropertySource in PropertySourcesPlaceholderConfigurer Commit 3295289e17 fixed a number issues with placeholder resolution in PropertySourcesPlaceholderConfigurer. However, in doing so, it replaced a raw PropertySource with a CompositePropertySource which implements EnumerablePropertySource. Consequently, all property sources registered in the Environment must now implement EnumerablePropertySource (which is not an actual requirement). Otherwise, invocations of getPropertyNames() on the CompositePropertySource result in an IllegalStateException, and that is a breaking change which resulted in numerous build failures within the Spring portfolio. To address that regression, this commit introduces a private ConfigurableEnvironmentPropertySource in PropertySourcesPlaceholderConfigurer which is a "raw" PropertySource that delegates directly to the PropertySources in a ConfigurableEnvironment. This commit also extracts the raw PropertySource for direct Environment delegation into a new FallbackEnvironmentPropertySource. See gh-17385 Closes gh-34861 --- .../PropertySourcesPlaceholderConfigurer.java | 93 +++++++++++++++---- ...ertySourcesPlaceholderConfigurerTests.java | 58 ++++++++++++ 2 files changed, 133 insertions(+), 18 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurer.java b/spring-context/src/main/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurer.java index a0b89f3d91..f5a6d3c33f 100644 --- a/spring-context/src/main/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurer.java +++ b/spring-context/src/main/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurer.java @@ -24,7 +24,6 @@ import org.springframework.beans.factory.BeanInitializationException; import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; import org.springframework.beans.factory.config.PlaceholderConfigurerSupport; import org.springframework.context.EnvironmentAware; -import org.springframework.core.env.CompositePropertySource; import org.springframework.core.env.ConfigurableEnvironment; import org.springframework.core.env.ConfigurablePropertyResolver; import org.springframework.core.env.Environment; @@ -133,23 +132,10 @@ public class PropertySourcesPlaceholderConfigurer extends PlaceholderConfigurerS if (this.propertySources == null) { this.propertySources = new MutablePropertySources(); if (this.environment != null) { - PropertySource environmentPropertySource; - if (this.environment instanceof ConfigurableEnvironment configurableEnvironment) { - environmentPropertySource = new CompositePropertySource(ENVIRONMENT_PROPERTIES_PROPERTY_SOURCE_NAME, - configurableEnvironment.getPropertySources()); - } - else { - // Fallback code path that should never apply in a regular scenario, since the - // Environment in the ApplicationContext should always be a ConfigurableEnvironment. - environmentPropertySource = - new PropertySource<>(ENVIRONMENT_PROPERTIES_PROPERTY_SOURCE_NAME, this.environment) { - @Override - @Nullable - public Object getProperty(String key) { - return super.source.getProperty(key); - } - }; - } + PropertySource environmentPropertySource = + (this.environment instanceof ConfigurableEnvironment configurableEnvironment ? + new ConfigurableEnvironmentPropertySource(configurableEnvironment) : + new FallbackEnvironmentPropertySource(this.environment)); this.propertySources.addLast(environmentPropertySource); } try { @@ -232,4 +218,75 @@ public class PropertySourcesPlaceholderConfigurer extends PlaceholderConfigurerS return this.appliedPropertySources; } + + /** + * Custom {@link PropertySource} that delegates to the + * {@link ConfigurableEnvironment#getPropertySources() PropertySources} in a + * {@link ConfigurableEnvironment}. + * @since 6.2.7 + */ + private static class ConfigurableEnvironmentPropertySource extends PropertySource { + + private final PropertySources propertySources; + + + ConfigurableEnvironmentPropertySource(ConfigurableEnvironment environment) { + super(ENVIRONMENT_PROPERTIES_PROPERTY_SOURCE_NAME, environment); + this.propertySources = environment.getPropertySources(); + } + + + @Override + @Nullable + public Object getProperty(String name) { + for (PropertySource propertySource : this.propertySources) { + Object candidate = propertySource.getProperty(name); + if (candidate != null) { + return candidate; + } + } + return null; + } + + @Override + public boolean containsProperty(String name) { + for (PropertySource propertySource : this.propertySources) { + if (propertySource.containsProperty(name)) { + return true; + } + } + return false; + } + + @Override + public String toString() { + return "ConfigurableEnvironmentPropertySource {propertySources=" + this.propertySources + "}"; + } + } + + /** + * Fallback {@link PropertySource} that delegates to a raw {@link Environment}. + *

Should never apply in a regular scenario, since the {@code Environment} + * in an {@code ApplicationContext} should always be a {@link ConfigurableEnvironment}. + * @since 6.2.7 + */ + private static class FallbackEnvironmentPropertySource extends PropertySource { + + FallbackEnvironmentPropertySource(Environment environment) { + super(ENVIRONMENT_PROPERTIES_PROPERTY_SOURCE_NAME, environment); + } + + + @Override + @Nullable + public Object getProperty(String name) { + return super.source.getProperty(name); + } + + @Override + public String toString() { + return "FallbackEnvironmentPropertySource {environment=" + super.source + "}"; + } + } + } diff --git a/spring-context/src/test/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurerTests.java b/spring-context/src/test/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurerTests.java index 81ccd73e46..24325f9552 100644 --- a/spring-context/src/test/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurerTests.java +++ b/spring-context/src/test/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurerTests.java @@ -16,6 +16,9 @@ package org.springframework.context.support; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; import java.util.Optional; import java.util.Properties; @@ -34,6 +37,7 @@ import org.springframework.context.annotation.AnnotationConfigApplicationContext import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.convert.support.DefaultConversionService; +import org.springframework.core.env.EnumerablePropertySource; import org.springframework.core.env.MutablePropertySources; import org.springframework.core.env.PropertySource; import org.springframework.core.env.StandardEnvironment; @@ -300,6 +304,60 @@ class PropertySourcesPlaceholderConfigurerTests { assertThat(bf.getBean(TestBean.class).getName()).isEqualTo("bar"); } + @Test // gh-34861 + void withEnumerableAndNonEnumerablePropertySourcesInTheEnvironmentAndLocalProperties() { + DefaultListableBeanFactory bf = new DefaultListableBeanFactory(); + bf.registerBeanDefinition("testBean", + genericBeanDefinition(TestBean.class) + .addPropertyValue("name", "${foo:bogus}") + .addPropertyValue("jedi", "${local:false}") + .getBeanDefinition()); + + // 1) MockPropertySource is an EnumerablePropertySource. + MockPropertySource mockPropertySource = new MockPropertySource("mockPropertySource") + .withProperty("foo", "${bar}"); + + // 2) PropertySource is not an EnumerablePropertySource. + PropertySource rawPropertySource = new PropertySource<>("rawPropertySource", new Object()) { + @Override + public Object getProperty(String key) { + return ("bar".equals(key) ? "quux" : null); + } + }; + + MockEnvironment env = new MockEnvironment(); + env.getPropertySources().addFirst(mockPropertySource); + env.getPropertySources().addLast(rawPropertySource); + + PropertySourcesPlaceholderConfigurer ppc = new PropertySourcesPlaceholderConfigurer(); + ppc.setEnvironment(env); + // 3) Local properties are stored in a PropertiesPropertySource which is an EnumerablePropertySource. + ppc.setProperties(new Properties() {{ + setProperty("local", "true"); + }}); + ppc.postProcessBeanFactory(bf); + + // Verify all properties can be resolved via the Environment. + assertThat(env.getProperty("foo")).isEqualTo("quux"); + assertThat(env.getProperty("bar")).isEqualTo("quux"); + + // Verify that placeholder resolution works. + TestBean testBean = bf.getBean(TestBean.class); + assertThat(testBean.getName()).isEqualTo("quux"); + assertThat(testBean.isJedi()).isTrue(); + + // Verify that the presence of a non-EnumerablePropertySource does not prevent + // accessing EnumerablePropertySources via getAppliedPropertySources(). + List propertyNames = new ArrayList<>(); + for (PropertySource propertySource : ppc.getAppliedPropertySources()) { + if (propertySource instanceof EnumerablePropertySource enumerablePropertySource) { + Collections.addAll(propertyNames, enumerablePropertySource.getPropertyNames()); + } + } + // Should not contain "foo" or "bar" from the Environment. + assertThat(propertyNames).containsOnly("local"); + } + @Test void customPlaceholderPrefixAndSuffix() { PropertySourcesPlaceholderConfigurer ppc = new PropertySourcesPlaceholderConfigurer();