From 06bb6bd1e1b6c8c44e2ebb83f70bc18a6573fc78 Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Wed, 21 Oct 2015 11:43:50 -0400 Subject: [PATCH 1/2] Fix logic affecting files loaded The problem fixed here is that the Loader keeps track of the profiles it is going to look at for loading files, but ignores any that were already active in the Environment if the listener is called initially with spring.profiles.active not empty. Closes gh-4261 --- .../config/ConfigFileApplicationListener.java | 20 +++++++++---------- .../ConfigFileApplicationListenerTests.java | 12 +++++++++++ 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigFileApplicationListener.java b/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigFileApplicationListener.java index 57f12bdcf82..b3c8ea160aa 100644 --- a/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigFileApplicationListener.java +++ b/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigFileApplicationListener.java @@ -314,17 +314,15 @@ public class ConfigFileApplicationListener maybeActivateProfiles( this.environment.getProperty(ACTIVE_PROFILES_PROPERTY)); } - else { - // Pre-existing active profiles set via Environment.setActiveProfiles() - // are additional profiles and config files are allowed to add more if - // they want to, so don't call addActiveProfiles() here. - List list = new ArrayList( - Arrays.asList(this.environment.getActiveProfiles())); - // Reverse them so the order is the same as from getProfilesForValue() - // (last one wins when properties are eventually resolved) - Collections.reverse(list); - this.profiles.addAll(list); - } + // Pre-existing active profiles set via Environment.setActiveProfiles() + // are additional profiles and config files are allowed to add more if + // they want to, so don't call addActiveProfiles() here. + List list = new ArrayList( + Arrays.asList(this.environment.getActiveProfiles())); + // Reverse them so the order is the same as from getProfilesForValue() + // (last one wins when properties are eventually resolved) + Collections.reverse(list); + this.profiles.addAll(list); // The default profile for these purposes is represented as null. We add it // last so that it is first out of the queue (active profiles will then diff --git a/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java b/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java index 474d4772723..a273cdb5756 100644 --- a/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java @@ -341,6 +341,18 @@ public class ConfigFileApplicationListenerTests { assertThat(property, equalTo("fromprofilepropertiesfile")); } + @Test + public void profilesAddedToEnvironmentAndViaProperty() throws Exception { + EnvironmentTestUtils.addEnvironment(this.environment, + "spring.profiles.active:foo"); + this.environment.addActiveProfile("dev"); + this.initializer.onApplicationEvent(this.event); + assertThat(this.environment.getActiveProfiles(), + equalTo(new String[] { "foo", "dev" })); + assertThat(this.environment.getProperty("my.property"), + equalTo("fromdevpropertiesfile")); + } + @Test public void yamlProfiles() throws Exception { this.initializer.setSearchNames("testprofiles"); From 7c1bf582621314d3143861fb1972cb3f83cc65f7 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Thu, 22 Oct 2015 13:46:09 +0200 Subject: [PATCH 2/2] Filter duplicate Improve the initial PR to include a filtering of the profiles that were already enabled via the `spring.profiles.active` property. Also add more tests to prove that each profile is loaded only once now. Closes gh-4273 --- .../main/asciidoc/spring-boot-features.adoc | 3 + .../config/ConfigFileApplicationListener.java | 37 ++++++-- .../ConfigFileApplicationListenerTests.java | 85 ++++++++++++++++--- 3 files changed, 109 insertions(+), 16 deletions(-) diff --git a/spring-boot-docs/src/main/asciidoc/spring-boot-features.adoc b/spring-boot-docs/src/main/asciidoc/spring-boot-features.adoc index 6400155c364..4310e0b937a 100644 --- a/spring-boot-docs/src/main/asciidoc/spring-boot-features.adoc +++ b/spring-boot-docs/src/main/asciidoc/spring-boot-features.adoc @@ -369,6 +369,9 @@ Profile specific properties are loaded from the same locations as standard ones irrespective of whether the profile-specific files are inside or outside your packaged jar. +If several profiles are specified, a last wins strategy applies. For example, profiles +specified by the `spring.active.profiles` property are added after those configured via +the `SpringApplication` API and therefore take precedence. [[boot-features-external-config-placeholders-in-properties]] diff --git a/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigFileApplicationListener.java b/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigFileApplicationListener.java index b3c8ea160aa..4578f2367e4 100644 --- a/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigFileApplicationListener.java +++ b/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigFileApplicationListener.java @@ -89,6 +89,7 @@ import org.springframework.validation.BindException; * * @author Dave Syer * @author Phillip Webb + * @author Stephane Nicoll */ public class ConfigFileApplicationListener implements ApplicationListener, Ordered { @@ -308,17 +309,19 @@ public class ConfigFileApplicationListener this.propertiesLoader = new PropertySourcesLoader(); this.profiles = Collections.asLifoQueue(new LinkedList()); this.activatedProfiles = false; + + Set initialActiveProfiles = null; if (this.environment.containsProperty(ACTIVE_PROFILES_PROPERTY)) { // Any pre-existing active profiles set via property sources (e.g. System // properties) take precedence over those added in config files. - maybeActivateProfiles( + initialActiveProfiles = maybeActivateProfiles( this.environment.getProperty(ACTIVE_PROFILES_PROPERTY)); } // Pre-existing active profiles set via Environment.setActiveProfiles() // are additional profiles and config files are allowed to add more if // they want to, so don't call addActiveProfiles() here. - List list = new ArrayList( - Arrays.asList(this.environment.getActiveProfiles())); + List list = filterEnvironmentProfiles(initialActiveProfiles != null + ? initialActiveProfiles : Collections.emptySet()); // Reverse them so the order is the same as from getProfilesForValue() // (last one wins when properties are eventually resolved) Collections.reverse(list); @@ -404,13 +407,36 @@ public class ConfigFileApplicationListener return propertySource; } - private void maybeActivateProfiles(Object value) { + /** + * Return the active profiles that have not been processed yet. + *

If a profile is enabled via both {@link #ACTIVE_PROFILES_PROPERTY} and + * {@link ConfigurableEnvironment#addActiveProfile(String)} it needs to be + * filtered so that the {@link #ACTIVE_PROFILES_PROPERTY} value takes + * precedence. + *

Concretely, if the "cloud" profile is enabled via the environment, + * it will take less precedence that any profile set via the + * {@link #ACTIVE_PROFILES_PROPERTY}. + * @param initialActiveProfiles the profiles that have been enabled via + * {@link #ACTIVE_PROFILES_PROPERTY} + * @return the additional profiles from the environment to enable + */ + private List filterEnvironmentProfiles(Set initialActiveProfiles) { + List additionalProfiles = new ArrayList(); + for (String profile : this.environment.getActiveProfiles()) { + if (!initialActiveProfiles.contains(profile)) { + additionalProfiles.add(profile); + } + } + return additionalProfiles; + } + + private Set maybeActivateProfiles(Object value) { if (this.activatedProfiles) { if (value != null) { this.debug.add("Profiles already activated, '" + value + "' will not be applied"); } - return; + return Collections.emptySet(); } Set profiles = getProfilesForValue(value); @@ -420,6 +446,7 @@ public class ConfigFileApplicationListener + StringUtils.collectionToCommaDelimitedString(profiles)); this.activatedProfiles = true; } + return profiles; } private void addIncludeProfiles(Object value) { diff --git a/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java b/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java index a273cdb5756..a649fbddc0c 100644 --- a/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2010-2014 the original author or authors. + * Copyright 2012-2015 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. @@ -27,20 +27,28 @@ import java.util.Collections; import java.util.List; import java.util.Properties; +import ch.qos.logback.classic.BasicConfigurator; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.LoggerContext; import org.hamcrest.Description; import org.hamcrest.Matcher; import org.hamcrest.TypeSafeDiagnosingMatcher; import org.junit.After; +import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; +import org.slf4j.LoggerFactory; import org.springframework.boot.SpringApplication; import org.springframework.boot.context.config.ConfigFileApplicationListener.ConfigurationPropertySources; import org.springframework.boot.context.event.ApplicationEnvironmentPreparedEvent; +import org.springframework.boot.context.event.ApplicationPreparedEvent; import org.springframework.boot.env.EnumerableCompositePropertySource; import org.springframework.boot.test.EnvironmentTestUtils; +import org.springframework.boot.test.OutputCapture; import org.springframework.context.ConfigurableApplicationContext; +import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Profile; import org.springframework.context.annotation.PropertySource; @@ -55,13 +63,8 @@ import org.springframework.core.io.ResourceLoader; import org.springframework.util.ReflectionUtils; import org.springframework.util.StringUtils; -import static org.hamcrest.Matchers.contains; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.not; -import static org.hamcrest.Matchers.notNullValue; -import static org.hamcrest.Matchers.nullValue; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertThat; +import static org.hamcrest.Matchers.*; +import static org.junit.Assert.*; /** * Tests for {@link ConfigFileApplicationListener}. @@ -81,6 +84,17 @@ public class ConfigFileApplicationListenerTests { @Rule public ExpectedException expected = ExpectedException.none(); + @Rule + public OutputCapture out = new OutputCapture(); + + @Before + public void resetLogging() { + LoggerContext loggerContext = ((Logger) LoggerFactory.getLogger(getClass())) + .getLoggerContext(); + loggerContext.reset(); + BasicConfigurator.configure(loggerContext); + } + @After public void cleanup() { System.clearProperty("the.property"); @@ -343,14 +357,63 @@ public class ConfigFileApplicationListenerTests { @Test public void profilesAddedToEnvironmentAndViaProperty() throws Exception { + // External profile takes precedence over profile added via the environment EnvironmentTestUtils.addEnvironment(this.environment, - "spring.profiles.active:foo"); + "spring.profiles.active:other"); this.environment.addActiveProfile("dev"); this.initializer.onApplicationEvent(this.event); - assertThat(this.environment.getActiveProfiles(), - equalTo(new String[] { "foo", "dev" })); + assertThat(Arrays.asList(this.environment.getActiveProfiles()), containsInAnyOrder("dev", "other")); + assertThat(this.environment.getProperty("my.property"), + equalTo("fromotherpropertiesfile")); + validateProfilePrecedence(null, "dev", "other"); + } + + @Test + public void profilesAddedToEnvironmentAndViaPropertyDuplicate() throws Exception { + EnvironmentTestUtils.addEnvironment(this.environment, + "spring.profiles.active:dev,other"); + this.environment.addActiveProfile("dev"); + this.initializer.onApplicationEvent(this.event); + assertThat(Arrays.asList(this.environment.getActiveProfiles()), containsInAnyOrder("dev", "other")); + assertThat(this.environment.getProperty("my.property"), + equalTo("fromotherpropertiesfile")); + validateProfilePrecedence(null, "dev", "other"); + } + + @Test + public void profilesAddedToEnvironmentAndViaPropertyDuplicateEnvironmentWins() throws Exception { + EnvironmentTestUtils.addEnvironment(this.environment, + "spring.profiles.active:other,dev"); + this.environment.addActiveProfile("other"); + this.initializer.onApplicationEvent(this.event); + assertThat(Arrays.asList(this.environment.getActiveProfiles()), containsInAnyOrder("dev", "other")); assertThat(this.environment.getProperty("my.property"), equalTo("fromdevpropertiesfile")); + validateProfilePrecedence(null, "other", "dev"); + } + + private void validateProfilePrecedence(String... profiles) { + this.initializer.onApplicationEvent(new ApplicationPreparedEvent( + new SpringApplication(), new String[0], new AnnotationConfigApplicationContext())); + String log = this.out.toString(); + + // First make sure that each profile got processed only once + for (String profile : profiles) { + assertThat("Wrong number of occurrences for profile '" + profile + "' --> " + log, + StringUtils.countOccurrencesOf(log, createLogForProfile(profile)), equalTo(1)); + } + // Make sure the order of loading is the right one + for (String profile : profiles) { + String line = createLogForProfile(profile); + int index = log.indexOf(line); + assertTrue("Loading profile '" + profile + "' not found in '" + log + "'", index != -1); + log = log.substring(index + line.length(), log.length()); + } + } + + private String createLogForProfile(String profile) { + String suffix = profile != null ? "-" + profile : ""; + return "Loaded config file 'classpath:/application" + suffix + ".properties'"; } @Test