Rework @propertysource early parsing logic

Rework the @propertysource parsing logic recently changed in commit
7c60888 to deal with the same source appearing on a @configuration
class and an @import class.

Processing now occurs in a single sweep, with any previously added
sources being converted to a CompositePropertySource.

Issue: SPR-12115

spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java

@@ -32,6 +32,8 @@
 import java.util.Set;
 import java.util.Stack;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 import org.springframework.beans.BeanUtils;
 import org.springframework.beans.factory.Aware;
 import org.springframework.beans.factory.BeanClassLoaderAware;
@@ -68,8 +70,7 @@
 import org.springframework.core.type.classreading.MetadataReader;
 import org.springframework.core.type.classreading.MetadataReaderFactory;
 import org.springframework.core.type.filter.AssignableTypeFilter;
-import org.springframework.util.LinkedMultiValueMap;
-import org.springframework.util.MultiValueMap;
+import org.springframework.util.Assert;
 import org.springframework.util.StringUtils;
 
 /**
@@ -103,6 +104,8 @@ public int compare(DeferredImportSelectorHolder o1, DeferredImportSelectorHolder
 			};
 
 
+	private final Log logger = LogFactory.getLog(getClass());
+
 	private final MetadataReaderFactory metadataReaderFactory;
 
 	private final ProblemReporter problemReporter;
@@ -122,8 +125,7 @@ public int compare(DeferredImportSelectorHolder o1, DeferredImportSelectorHolder
 
 	private final Map<String, ConfigurationClass> knownSuperclasses = new HashMap<String, ConfigurationClass>();
 
-	private final MultiValueMap<Object, PropertySourceDescriptor> propertySources =
-			new LinkedMultiValueMap<Object, PropertySourceDescriptor>();
+	private final Set<String> propertySourceNames = new LinkedHashSet<String>();
 
 	private final ImportStack importStack = new ImportStack();
 
@@ -242,12 +244,15 @@ protected final SourceClass doProcessConfigurationClass(ConfigurationClass confi
 		// Process any @PropertySource annotations
 		for (AnnotationAttributes propertySource : AnnotationConfigUtils.attributesForRepeatable(
 				sourceClass.getMetadata(), PropertySources.class, org.springframework.context.annotation.PropertySource.class)) {
-			processPropertySource(propertySource);
+			if (!(this.environment instanceof ConfigurableEnvironment)) {
+				logger.warn("Ignoring @PropertySource annotation on "
+						+ sourceClass.getMetadata().getClassName()
+						+ "Reason: Environment must implement ConfigurableEnvironment");
+			} else {
+				processPropertySource(propertySource);
+			}
 		}
 
-		// Register PropertySources with Environment
-		registerPropertySources();
-
 		// Process any @ComponentScan annotations
 		AnnotationAttributes componentScan = AnnotationConfigUtils.attributesFor(sourceClass.getMetadata(), ComponentScan.class);
 		if (componentScan != null) {
@@ -315,71 +320,69 @@ private void processMemberClasses(ConfigurationClass configClass, SourceClass so
 	/**
 	 * Process the given <code>@PropertySource</code> annotation metadata.
 	 * @param propertySource metadata for the <code>@PropertySource</code> annotation found
+	 * @throws IOException if loading a property source failed
 	 */
-	private void processPropertySource(AnnotationAttributes propertySource) {
+	private void processPropertySource(AnnotationAttributes propertySource) throws IOException {
 		String name = propertySource.getString("name");
 		String[] locations = propertySource.getStringArray("value");
 		boolean ignoreResourceNotFound = propertySource.getBoolean("ignoreResourceNotFound");
-		if (locations.length == 0) {
-			throw new IllegalArgumentException("At least one @PropertySource(value) location is required");
-		}
+		Assert.isTrue(locations.length > 0, "At least one @PropertySource(value) location is required");
 		for (String location : locations) {
-			this.propertySources.add((StringUtils.hasText(name) ? name : this.propertySources.size()),
-					new PropertySourceDescriptor(location, ignoreResourceNotFound));
-		}
-	}
-
-	/**
-	 * Register all discovered property sources with the {@link Environment}.
-	 * @throws IOException in case of property loading failure
-	 */
-	private void registerPropertySources() throws IOException {
-		if (!(this.environment instanceof ConfigurableEnvironment)) {
-			return;
-		}
-		MutablePropertySources envPropertySources = ((ConfigurableEnvironment) this.environment).getPropertySources();
-
-		String lastName = null;
-		for (Map.Entry<Object, List<PropertySourceDescriptor>> entry : this.propertySources.entrySet()) {
-			Object key = entry.getKey();
-			String name = (key instanceof String ? (String) key : null);
-
-			List<PropertySourceDescriptor> descriptors = entry.getValue();
-			List<ResourcePropertySource> resources = new ArrayList<ResourcePropertySource>(descriptors.size());
-
-			for (PropertySourceDescriptor descriptor : descriptors) {
-				try {
-					String resolvedLocation = this.environment.resolveRequiredPlaceholders(descriptor.location);
-					Resource resource = this.resourceLoader.getResource(resolvedLocation);
-					ResourcePropertySource ps = new ResourcePropertySource(resource);
-					resources.add(ps);
-					if (name == null) {
-						name = ps.getName();
-					}
-				}
-				catch (IllegalArgumentException ex) {
-					// from resolveRequiredPlaceholders
-					if (!descriptor.ignoreResourceNotFound) {
-						throw ex;
-					}
+			try {
+				processPropertySourceLocation(name, location);
+			}
+			catch (IllegalArgumentException ex) {
+				// from resolveRequiredPlaceholders
+				if (!ignoreResourceNotFound) {
+					throw ex;
 				}
-				catch (FileNotFoundException ex) {
-					// from ResourcePropertySource constructor
-					if (!descriptor.ignoreResourceNotFound) {
-						throw ex;
-					}
+			}
+			catch (FileNotFoundException ex) {
+				// from ResourcePropertySource constructor
+				if (!ignoreResourceNotFound) {
+					throw ex;
 				}
 			}
+		}
+	}
 
-			PropertySource<?> ps = collatePropertySources(name, resources);
-			if (lastName != null) {
-				envPropertySources.addBefore(lastName, ps);
+	private void processPropertySourceLocation(String name, String location) throws IOException, FileNotFoundException {
+		String resolvedLocation = this.environment.resolveRequiredPlaceholders(location);
+		Resource resource = this.resourceLoader.getResource(resolvedLocation);
+		AnnotationPropertySource propertySource = (StringUtils.hasText(name) ?
+				new AnnotationPropertySource(name, resource) : new AnnotationPropertySource(resource));
+		addPropertySource(propertySource);
+	}
+
+	private void addPropertySource(AnnotationPropertySource propertySource) {
+		String name = propertySource.getName();
+		MutablePropertySources propertySources = ((ConfigurableEnvironment) this.environment).getPropertySources();
+		if (propertySources.contains(name) && this.propertySourceNames.contains(name)) {
+			// We've already added a version, we need to extend it
+			PropertySource<?> existing = propertySources.get(name);
+			if (existing instanceof CompositePropertySource) {
+				((CompositePropertySource) existing).addFirstPropertySource(
+						propertySource.resourceNamedPropertySource());
 			}
 			else {
-				envPropertySources.addLast(ps);
+				if (existing instanceof AnnotationPropertySource) {
+					existing = ((AnnotationPropertySource) existing).resourceNamedPropertySource();
+				}
+				CompositePropertySource composite = new CompositePropertySource(name);
+				composite.addPropertySource(propertySource.resourceNamedPropertySource());
+				composite.addPropertySource(existing);
+				propertySources.replace(name, composite);
 			}
-			lastName = name;
 		}
+		else {
+			if(this.propertySourceNames.isEmpty()) {
+				propertySources.addLast(propertySource);
+			} else {
+				String firstProcessed = this.propertySourceNames.iterator().next();
+				propertySources.addBefore(firstProcessed, propertySource);
+			}
+		}
+		this.propertySourceNames.add(name);
 	}
 
 	/**
@@ -529,17 +532,6 @@ public Set<ConfigurationClass> getConfigurationClasses() {
 		return this.configurationClasses.keySet();
 	}
 
-	private PropertySource<?> collatePropertySources(String name, List<ResourcePropertySource> propertySources) {
-		if (propertySources.size() == 1) {
-			return propertySources.get(0).withName(name);
-		}
-		CompositePropertySource result = new CompositePropertySource(name);
-		for (int i = propertySources.size() - 1; i >= 0; i--) {
-			result.addPropertySource(propertySources.get(i));
-		}
-		return result;
-	}
-
 
 	ImportRegistry getImportRegistry() {
 		return this.importStack;
@@ -825,19 +817,6 @@ public String toString() {
 	}
 
 
-	private static class PropertySourceDescriptor {
-
-		public PropertySourceDescriptor(String location, boolean ignoreResourceNotFound) {
-			this.location = location;
-			this.ignoreResourceNotFound = ignoreResourceNotFound;
-		}
-
-		public final String location;
-
-		public final boolean ignoreResourceNotFound;
-	}
-
-
 	/**
 	 * {@link Problem} registered upon detection of a circular {@link Import}.
 	 */
@@ -852,4 +831,34 @@ public CircularImportProblem(ConfigurationClass attemptedImport, Stack<Configura
 		}
 	}
 
+
+	private static class AnnotationPropertySource extends ResourcePropertySource {
+
+		/** The resource name or null if the same as getName() */
+		private final String resourceName;
+
+		public AnnotationPropertySource(Resource resource) throws IOException {
+			super(resource);
+			this.resourceName = null;
+		}
+
+		public AnnotationPropertySource(String name, Resource resource) throws IOException {
+			super(name, resource);
+			this.resourceName = getNameForResource(resource);
+		}
+
+		protected AnnotationPropertySource(String name, Map<String, Object> source) {
+			super(name, source);
+			this.resourceName = null;
+		}
+
+		public AnnotationPropertySource resourceNamedPropertySource() {
+			if (this.resourceName == null) {
+				return this;
+			}
+			return new AnnotationPropertySource(this.resourceName, getSource());
+		}
+
+	}
+
 }

spring-context/src/test/java/org/springframework/context/annotation/PropertySourceAnnotationTests.java

@@ -210,6 +210,14 @@ public void withIgnoredPropertySource() {
 		assertThat(ctx.getEnvironment().containsProperty("from.p2"), is(true));
 	}
 
+	@Test
+	public void withSameSourceImportedInDifferentOrder() throws Exception {
+		AnnotationConfigApplicationContext ctx =new AnnotationConfigApplicationContext(ConfigWithSameSourceImportedInDifferentOrder.class);
+		assertThat(ctx.getEnvironment().containsProperty("from.p1"), is(true));
+		assertThat(ctx.getEnvironment().containsProperty("from.p2"), is(true));
+		assertThat(ctx.getEnvironment().getProperty("testbean.name"), equalTo("p2TestBean"));
+	}
+
 
 	@Configuration
 	@PropertySource(value="classpath:${unresolvable}/p1.properties")
@@ -354,4 +362,22 @@ static class ConfigWithIgnoredPropertySource {
 	static class ConfigWithEmptyResourceLocations {
 	}
 
+	@Import({ ConfigImportedWithSameSourceImportedInDifferentOrder.class })
+	@PropertySources({
+		@PropertySource("classpath:org/springframework/context/annotation/p1.properties"),
+		@PropertySource("classpath:org/springframework/context/annotation/p2.properties")
+	})
+	@Configuration
+	public static class ConfigWithSameSourceImportedInDifferentOrder {
+
+	}
+
+	@Configuration
+	@PropertySources({
+		@PropertySource("classpath:org/springframework/context/annotation/p2.properties"),
+		@PropertySource("classpath:org/springframework/context/annotation/p1.properties")
+	})
+	public static class ConfigImportedWithSameSourceImportedInDifferentOrder {
+	}
+
 }

spring-core/src/main/java/org/springframework/core/env/CompositePropertySource.java

@@ -16,7 +16,9 @@
 
 package org.springframework.core.env;
 
+import java.util.ArrayList;
 import java.util.LinkedHashSet;
+import java.util.List;
 import java.util.Set;
 
 /**
@@ -25,6 +27,7 @@
  * share the same name, e.g. when multiple values are supplied to {@code @PropertySource}.
  *
  * @author Chris Beams
+ * @author Phillip Webb
  * @since 3.1.1
  */
 public class CompositePropertySource extends PropertySource<Object> {
@@ -56,6 +59,13 @@ public void addPropertySource(PropertySource<?> propertySource) {
 		this.propertySources.add(propertySource);
 	}
 
+	public void addFirstPropertySource(PropertySource<?> propertySource) {
+		List<PropertySource<?>> exisiting = new ArrayList<PropertySource<?>>(this.propertySources);
+		this.propertySources.clear();
+		this.propertySources.add(propertySource);
+		this.propertySources.addAll(exisiting);
+	}
+
 	@Override
 	public String toString() {
 		return String.format("%s [name='%s', propertySources=%s]",

spring-core/src/main/java/org/springframework/core/io/support/ResourcePropertySource.java

@@ -115,7 +115,7 @@ public ResourcePropertySource(String location) throws IOException {
 		this(new DefaultResourceLoader().getResource(location));
 	}
 
-	private ResourcePropertySource(String name, Map<String, Object> source) {
+	protected ResourcePropertySource(String name, Map<String, Object> source) {
 		super(name, source);
 	}
 
@@ -137,7 +137,7 @@ public ResourcePropertySource withName(String name) {
 	 * empty, return the class name of the resource plus its identity hash code.
 	 * @see org.springframework.core.io.Resource#getDescription()
 	 */
-	private static String getNameForResource(Resource resource) {
+	protected static String getNameForResource(Resource resource) {
 		String name = resource.getDescription();
 		if (!StringUtils.hasText(name)) {
 			name = resource.getClass().getSimpleName() + "@" + System.identityHashCode(resource);

spring-core/src/test/java/org/springframework/core/env/CompositePropertySourceTests.java

@@ -0,0 +1,46 @@
+/*
+ * Copyright 2002-2014 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
+ *
+ * http://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.core.env;
+
+import java.util.Collections;
+
+import org.junit.Test;
+
+import static org.hamcrest.Matchers.*;
+import static org.junit.Assert.*;
+
+/**
+ * Tests for {@link CompositePropertySource}.
+ * @author Phillip Webb
+ */
+public class CompositePropertySourceTests {
+
+	@Test
+	public void addFirst() {
+		PropertySource<?> p1 = new MapPropertySource("p1", Collections.emptyMap());
+		PropertySource<?> p2 = new MapPropertySource("p2", Collections.emptyMap());
+		PropertySource<?> p3 = new MapPropertySource("p3", Collections.emptyMap());
+		CompositePropertySource composite = new CompositePropertySource("c");
+		composite.addPropertySource(p2);
+		composite.addPropertySource(p3);
+		composite.addPropertySource(p1);
+		composite.addFirstPropertySource(p1);
+		assertThat(composite.toString(), containsString("MapPropertySource [name='p1'], "
+				+ "MapPropertySource [name='p2'], MapPropertySource [name='p3']"));
+	}
+
+}