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 gh-17385
Closes gh-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();