Straighten out profile ordering semantics

Here's what I think works best:

* Any profile in the Environment before application.yml
is processed takes precedence (i.e. it will be last in the
list of active profiles in the live app)

* Any profile in the Environment before SpringApplication
starts takes precedence (so any added on the command line or
with System properties come after ones added using
the SpringApplication API)

* The order of profiles in application.yml is irrelevant -
profiles are applied in the order they come out of
Environment.getActiveProfiles()

Fixes gh-342

spring-boot/src/main/java/org/springframework/boot/SpringApplication.java

@@ -415,9 +415,14 @@ public class SpringApplication {
 	 * @param environment the environment to configure
 	 */
 	protected void setupProfiles(ConfigurableEnvironment environment) {
+		Set<String> profiles = new LinkedHashSet<String>();
+		environment.getActiveProfiles(); // ensure they are initialized
+		// But these ones should go first (last wins in a property key clash)
 		for (String profile : this.profiles) {
-			environment.addActiveProfile(profile);
+			profiles.add(profile);
 		}
+		profiles.addAll(Arrays.asList(environment.getActiveProfiles()));
+		environment.setActiveProfiles(profiles.toArray(new String[profiles.size()]));
 	}
 
 	/**

spring-boot/src/main/java/org/springframework/boot/context/config/ConfigFileApplicationListener.java

@@ -264,11 +264,15 @@ public class ConfigFileApplicationListener implements
 
 		public void load() throws IOException {
 			this.propertiesLoader = new PropertySourcesLoader();
-			this.profiles = new LinkedList<String>();
-			this.profiles.add(null);
-			this.profiles.addAll(Arrays.asList(this.environment.getActiveProfiles()));
+			this.profiles = Collections.asLifoQueue(new LinkedList<String>());
 			this.activatedProfiles = false;
-			addActiveProfiles(this.environment.getProperty(ACTIVE_PROFILES_PROPERTY));
+
+			// Any pre-existing active profiles take precedence over those added in
+			// config files (unless latter are prefixed with "+").
+			addActiveProfiles(StringUtils.arrayToCommaDelimitedString(this.environment
+					.getActiveProfiles()));
+
+			this.profiles.add(null);
 
 			while (!this.profiles.isEmpty()) {
 				String profile = this.profiles.poll();
@@ -322,16 +326,29 @@ public class ConfigFileApplicationListener implements
 			String profiles = (property == null ? null : property.toString());
 			boolean profilesNotActivatedWhenCalled = !this.activatedProfiles;
 			for (String profile : asResolvedSet(profiles, null)) {
+				// A profile name prefixed with "+" is always added even if it is
+				// activated in a config file (without the "+" it can be disabled
+				// by an explicit Environment property set before the file was
+				// processed).
 				boolean addition = profile.startsWith("+");
 				profile = (addition ? profile.substring(1) : profile);
 				if (profilesNotActivatedWhenCalled || addition) {
 					this.profiles.add(profile);
-					this.environment.addActiveProfile(profile);
+					prependProfile(this.environment, profile);
 					this.activatedProfiles = true;
 				}
 			}
 		}
 
+		private void prependProfile(ConfigurableEnvironment environment, String profile) {
+			Set<String> profiles = new LinkedHashSet<String>();
+			environment.getActiveProfiles(); // ensure they are initialized
+			// But this one should go first (last wins in a property key clash)
+			profiles.add(profile);
+			profiles.addAll(Arrays.asList(environment.getActiveProfiles()));
+			environment.setActiveProfiles(profiles.toArray(new String[profiles.size()]));
+		}
+
 		public Set<String> getSearchLocations() {
 			Set<String> locations = new LinkedHashSet<String>();
 			locations.addAll(asResolvedSet(
@@ -361,10 +378,16 @@ public class ConfigFileApplicationListener implements
 		}
 
 		private Set<String> asResolvedSet(String value, String fallback) {
+			return asResolvedSet(value, fallback, true);
+		}
+
+		private Set<String> asResolvedSet(String value, String fallback, boolean reverse) {
 			List<String> list = Arrays.asList(StringUtils
 					.commaDelimitedListToStringArray(value != null ? this.environment
 							.resolvePlaceholders(value) : fallback));
-			Collections.reverse(list);
+			if (reverse) {
+				Collections.reverse(list);
+			}
 			return new LinkedHashSet<String>(list);
 		}
 

spring-boot/src/test/java/org/springframework/boot/ReproTests.java

@@ -27,6 +27,7 @@ import static org.junit.Assert.assertThat;
  * Tests to reproduce reported issues.
  * 
  * @author Phillip Webb
+ * @author Dave Syer
  */
 public class ReproTests {
 
@@ -44,23 +45,113 @@ public class ReproTests {
 	}
 
 	@Test
-	public void activeProfilesWithYaml() throws Exception {
-		// gh-322
+	public void activeProfilesWithYamlAndCommandLine() throws Exception {
+		// gh-322, gh-342
 		SpringApplication application = new SpringApplication(Config.class);
 		application.setWebEnvironment(false);
 		String configName = "--spring.config.name=activeprofilerepro";
 		assertVersionProperty(application.run(configName, "--spring.profiles.active=B"),
 				"B", "B");
+	}
+
+	@Test
+	public void activeProfilesWithYamlOnly() throws Exception {
+		// gh-322, gh-342
+		SpringApplication application = new SpringApplication(Config.class);
+		application.setWebEnvironment(false);
+		String configName = "--spring.config.name=activeprofilerepro";
 		assertVersionProperty(application.run(configName), "B", "B");
-		assertVersionProperty(application.run(configName, "--spring.profiles.active=A"),
-				"A", "A");
+	}
+
+	@Test
+	public void orderActiveProfilesWithYamlOnly() throws Exception {
+		// gh-322, gh-342
+		SpringApplication application = new SpringApplication(Config.class);
+		application.setWebEnvironment(false);
+		String configName = "--spring.config.name=activeprofilerepro-ordered";
+		assertVersionProperty(application.run(configName), "B", "A", "B");
+	}
+
+	@Test
+	public void commandLineBeatsProfilesWithYaml() throws Exception {
+		// gh-322, gh-342
+		SpringApplication application = new SpringApplication(Config.class);
+		application.setWebEnvironment(false);
+		String configName = "--spring.config.name=activeprofilerepro";
 		assertVersionProperty(application.run(configName, "--spring.profiles.active=C"),
 				"C", "C");
+	}
+
+	@Test
+	public void orderProfilesWithYaml() throws Exception {
+		// gh-322, gh-342
+		SpringApplication application = new SpringApplication(Config.class);
+		application.setWebEnvironment(false);
+		String configName = "--spring.config.name=activeprofilerepro";
 		assertVersionProperty(
-				application.run(configName, "--spring.profiles.active=A,C"), "A", "A",
+				application.run(configName, "--spring.profiles.active=A,C"), "C", "A",
 				"C");
+	}
+
+	@Test
+	public void reverseOrderOfProfilesWithYaml() throws Exception {
+		// gh-322, gh-342
+		SpringApplication application = new SpringApplication(Config.class);
+		application.setWebEnvironment(false);
+		String configName = "--spring.config.name=activeprofilerepro";
 		assertVersionProperty(
-				application.run(configName, "--spring.profiles.active=C,A"), "C", "C",
+				application.run(configName, "--spring.profiles.active=C,A"), "A", "C",
+				"A");
+	}
+
+	@Test
+	public void activeProfilesWithYamlAndCommandLineAndNoOverride() throws Exception {
+		// gh-322, gh-342
+		SpringApplication application = new SpringApplication(Config.class);
+		application.setWebEnvironment(false);
+		String configName = "--spring.config.name=activeprofilerepro-without-override";
+		assertVersionProperty(application.run(configName, "--spring.profiles.active=B"),
+				"B", "B");
+	}
+
+	@Test
+	public void activeProfilesWithYamlOnlyAndNoOverride() throws Exception {
+		// gh-322, gh-342
+		SpringApplication application = new SpringApplication(Config.class);
+		application.setWebEnvironment(false);
+		String configName = "--spring.config.name=activeprofilerepro-without-override";
+		assertVersionProperty(application.run(configName), null);
+	}
+
+	@Test
+	public void commandLineBeatsProfilesWithYamlAndNoOverride() throws Exception {
+		// gh-322, gh-342
+		SpringApplication application = new SpringApplication(Config.class);
+		application.setWebEnvironment(false);
+		String configName = "--spring.config.name=activeprofilerepro-without-override";
+		assertVersionProperty(application.run(configName, "--spring.profiles.active=C"),
+				"C", "C");
+	}
+
+	@Test
+	public void orderProfilesWithYamlAndNoOverride() throws Exception {
+		// gh-322, gh-342
+		SpringApplication application = new SpringApplication(Config.class);
+		application.setWebEnvironment(false);
+		String configName = "--spring.config.name=activeprofilerepro-without-override";
+		assertVersionProperty(
+				application.run(configName, "--spring.profiles.active=A,C"), "C", "A",
+				"C");
+	}
+
+	@Test
+	public void reverseOrderOfProfilesWithYamlAndNoOverride() throws Exception {
+		// gh-322, gh-342
+		SpringApplication application = new SpringApplication(Config.class);
+		application.setWebEnvironment(false);
+		String configName = "--spring.config.name=activeprofilerepro-without-override";
+		assertVersionProperty(
+				application.run(configName, "--spring.profiles.active=C,A"), "A", "C",
 				"A");
 	}
 

spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java

@@ -297,8 +297,8 @@ public class SpringApplicationTests {
 		ConfigurableEnvironment environment = new StandardEnvironment();
 		application.setEnvironment(environment);
 		application.run("--spring.profiles.active=bar");
-		// Command line arguably should always come last (not the case currently)
-		assertArrayEquals(new String[] { "bar", "foo" }, environment.getActiveProfiles());
+		// Command line should always come last
+		assertArrayEquals(new String[] { "foo", "bar" }, environment.getActiveProfiles());
 	}
 
 	@Test

spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java

@@ -207,30 +207,6 @@ public class ConfigFileApplicationListenerTests {
 		assertThat(this.environment.getActiveProfiles(), equalTo(new String[] { "prod" }));
 	}
 
-	@Test
-	public void yamlProfileOrdering() throws Exception {
-		this.initializer.setSearchNames("threeprofiles");
-		this.environment.setActiveProfiles("A", "C");
-		this.initializer.onApplicationEvent(this.event);
-		assertThat(this.environment.getProperty("version"), equalTo("C"));
-	}
-
-	@Test
-	public void yamlProfileOrderingReverse() throws Exception {
-		this.initializer.setSearchNames("threeprofiles");
-		this.environment.setActiveProfiles("C", "A");
-		this.initializer.onApplicationEvent(this.event);
-		assertThat(this.environment.getProperty("version"), equalTo("A"));
-	}
-
-	@Test
-	public void yamlProfileOrderingOverride() throws Exception {
-		this.initializer.setSearchNames("threeprofiles-with-override");
-		this.environment.setActiveProfiles("C", "A");
-		this.initializer.onApplicationEvent(this.event);
-		assertThat(this.environment.getProperty("version"), equalTo("B"));
-	}
-
 	@Test
 	public void specificNameAndProfileFromExistingSource() throws Exception {
 		EnvironmentTestUtils.addEnvironment(this.environment,

spring-boot/src/test/resources/activeprofilerepro-ordered.yml

@@ -1,5 +1,4 @@
----
-spring.profiles.active: B
+spring.profiles.active: A,B
 ---
 spring.profiles: A
 version: A
@@ -9,4 +8,4 @@ version: B
 ---
 spring.profiles: C
 version: C
----
+---