Restore support for non-EnumerablePropertySource in PropertySourcesPlaceholderConfigurer

Commit 3295289 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 spring-projectsgh-17385 Closes spring-projectsgh-34861

Author: sbrannen

spring-context/src/main/java/org/springframework/context/support/PropertySourcesPlaceholderConfigurer.java

@@ -24,7 +24,6 @@
 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 void postProcessBeanFactory(ConfigurableListableBeanFactory beanFactory)
 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 PropertySources getAppliedPropertySources() throws IllegalStateException
 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<ConfigurableEnvironment> {
+
+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}.
+ * <p>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<Environment> {
+
+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 + "}";
+}
+}
+
 }

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.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 @@ public Object getProperty(String key) {
 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<String> 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();