From 6f266145c7a9dce46872947b27a068ef66fbf49c Mon Sep 17 00:00:00 2001 From: nguyensach Date: Tue, 30 Mar 2021 12:31:09 +0900 Subject: [PATCH 1/2] Process additional profiles before config files processing Additional profiles were being processed after config file processing when legacy processing was used. This commit also restores the order in which additional profiles are added when legacy processing is used. Active profiles take precedence over additional profiles. See gh-25817 --- .../boot/SpringApplication.java | 12 +----- .../ConfigDataEnvironmentPostProcessor.java | 15 +++++++ .../boot/SpringApplicationTests.java | 12 ++++++ ...nfigDataEnvironmentPostProcessorTests.java | 19 +++++++++ ...leApplicationListenerLegacyReproTests.java | 12 ++++++ .../ConfigFileApplicationListenerTests.java | 41 +++++++++++++++++++ 6 files changed, 100 insertions(+), 11 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/SpringApplication.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/SpringApplication.java index f4ca8084f0..080d056ba7 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/SpringApplication.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/SpringApplication.java @@ -154,6 +154,7 @@ import org.springframework.web.context.support.StandardServletEnvironment; * @author Madhura Bhave * @author Brian Clozel * @author Ethan Rubinson + * @author Nguyen Bao Sach * @since 1.0.0 * @see #run(Class, String[]) * @see #run(Class[], String[]) @@ -373,7 +374,6 @@ public class SpringApplication { ConfigurationPropertySources.attach(environment); listeners.environmentPrepared(bootstrapContext, environment); DefaultPropertiesPropertySource.moveToEnd(environment); - configureAdditionalProfiles(environment); Assert.state(!environment.containsProperty("spring.main.environment-prefix"), "Environment prefix cannot be set via properties."); bindToSpringApplication(environment); @@ -558,16 +558,6 @@ public class SpringApplication { protected void configureProfiles(ConfigurableEnvironment environment, String[] args) { } - private void configureAdditionalProfiles(ConfigurableEnvironment environment) { - if (!CollectionUtils.isEmpty(this.additionalProfiles)) { - Set profiles = new LinkedHashSet<>(Arrays.asList(environment.getActiveProfiles())); - if (!profiles.containsAll(this.additionalProfiles)) { - profiles.addAll(this.additionalProfiles); - environment.setActiveProfiles(StringUtils.toStringArray(profiles)); - } - } - } - private void configureIgnoreBeanInfo(ConfigurableEnvironment environment) { if (System.getProperty(CachedIntrospectionResults.IGNORE_BEANINFO_PROPERTY_NAME) == null) { Boolean ignore = environment.getProperty("spring.beaninfo.ignore", Boolean.class, Boolean.TRUE); diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataEnvironmentPostProcessor.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataEnvironmentPostProcessor.java index 2c11110fc1..7df92a2548 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataEnvironmentPostProcessor.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataEnvironmentPostProcessor.java @@ -19,6 +19,8 @@ package org.springframework.boot.context.config; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.LinkedHashSet; +import java.util.Set; import java.util.function.Supplier; import org.apache.commons.logging.Log; @@ -34,6 +36,8 @@ import org.springframework.core.env.Environment; import org.springframework.core.io.DefaultResourceLoader; import org.springframework.core.io.ResourceLoader; import org.springframework.core.log.LogMessage; +import org.springframework.util.CollectionUtils; +import org.springframework.util.StringUtils; /** * {@link EnvironmentPostProcessor} that loads and applies {@link ConfigData} to Spring's @@ -41,6 +45,7 @@ import org.springframework.core.log.LogMessage; * * @author Phillip Webb * @author Madhura Bhave + * @author Nguyen Bao Sach * @since 2.4.0 */ public class ConfigDataEnvironmentPostProcessor implements EnvironmentPostProcessor, Ordered { @@ -99,6 +104,7 @@ public class ConfigDataEnvironmentPostProcessor implements EnvironmentPostProces catch (UseLegacyConfigProcessingException ex) { this.logger.debug(LogMessage.format("Switching to legacy config file processing [%s]", ex.getConfigurationProperty())); + configureAdditionalProfiles(environment, additionalProfiles); postProcessUsingLegacyApplicationListener(environment, resourceLoader); } } @@ -109,6 +115,15 @@ public class ConfigDataEnvironmentPostProcessor implements EnvironmentPostProces additionalProfiles, this.environmentUpdateListener); } + private void configureAdditionalProfiles(ConfigurableEnvironment environment, + Collection additionalProfiles) { + if (!CollectionUtils.isEmpty(additionalProfiles)) { + Set profiles = new LinkedHashSet<>(additionalProfiles); + profiles.addAll(Arrays.asList(environment.getActiveProfiles())); + environment.setActiveProfiles(StringUtils.toStringArray(profiles)); + } + } + private void postProcessUsingLegacyApplicationListener(ConfigurableEnvironment environment, ResourceLoader resourceLoader) { getLegacyListener().addPropertySources(environment, resourceLoader); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java index 8b8d437746..109f9fb8aa 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java @@ -147,6 +147,7 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; * @author Brian Clozel * @author Artsiom Yudovin * @author Marten Deinum + * @author Nguyen Bao Sach */ @ExtendWith(OutputCaptureExtension.class) class SpringApplicationTests { @@ -604,6 +605,17 @@ class SpringApplicationTests { assertThat(environment.getActiveProfiles()).containsExactly("bar", "spam", "foo"); } + @Test + void includeProfilesOrder() { + SpringApplication application = new SpringApplication(ExampleConfig.class); + application.setWebApplicationType(WebApplicationType.NONE); + ConfigurableEnvironment environment = new StandardEnvironment(); + application.setEnvironment(environment); + this.context = application.run("--spring.profiles.active=bar,spam", "--spring.profiles.include=foo"); + // Since Boot 2.4 included profiles should always be last + assertThat(environment.getActiveProfiles()).containsExactly("bar", "spam", "foo"); + } + @Test void addProfilesOrderWithProperties() { SpringApplication application = new SpringApplication(ExampleConfig.class); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigDataEnvironmentPostProcessorTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigDataEnvironmentPostProcessorTests.java index a3320116eb..6bec0e3624 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigDataEnvironmentPostProcessorTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigDataEnvironmentPostProcessorTests.java @@ -48,6 +48,7 @@ import static org.mockito.Mockito.verifyNoInteractions; * * @author Phillip Webb * @author Madhura Bhave + * @author Nguyen Bao Sach */ @ExtendWith(MockitoExtension.class) class ConfigDataEnvironmentPostProcessorTests { @@ -82,6 +83,7 @@ class ConfigDataEnvironmentPostProcessorTests { verify(this.postProcessor).getConfigDataEnvironment(any(), this.resourceLoaderCaptor.capture(), any()); verify(this.configDataEnvironment).processAndApply(); assertThat(this.resourceLoaderCaptor.getValue()).isInstanceOf(DefaultResourceLoader.class); + assertThat(this.environment.getActiveProfiles()).isEmpty(); } @Test @@ -93,6 +95,7 @@ class ConfigDataEnvironmentPostProcessorTests { verify(this.postProcessor).getConfigDataEnvironment(any(), this.resourceLoaderCaptor.capture(), any()); verify(this.configDataEnvironment).processAndApply(); assertThat(this.resourceLoaderCaptor.getValue()).isSameAs(resourceLoader); + assertThat(this.environment.getActiveProfiles()).isEmpty(); } @Test @@ -103,6 +106,7 @@ class ConfigDataEnvironmentPostProcessorTests { verify(this.postProcessor).getConfigDataEnvironment(any(), any(), this.additionalProfilesCaptor.capture()); verify(this.configDataEnvironment).processAndApply(); assertThat(this.additionalProfilesCaptor.getValue()).containsExactly("dev"); + assertThat(this.environment.getActiveProfiles()).isEmpty(); } @Test @@ -115,6 +119,21 @@ class ConfigDataEnvironmentPostProcessorTests { this.postProcessor.postProcessEnvironment(this.environment, this.application); verifyNoInteractions(this.configDataEnvironment); verify(legacyListener).addPropertySources(eq(this.environment), any(DefaultResourceLoader.class)); + assertThat(this.environment.getActiveProfiles()).isEmpty(); + } + + @Test + void postProcessEnvironmentWhenHasAdditionalProfilesViaProgrammaticallySettingAndUseLegacyProcessing() { + this.application.setAdditionalProfiles("dev"); + ConfigDataEnvironmentPostProcessor.LegacyConfigFileApplicationListener legacyListener = mock( + ConfigDataEnvironmentPostProcessor.LegacyConfigFileApplicationListener.class); + willThrow(new UseLegacyConfigProcessingException(null)).given(this.postProcessor) + .getConfigDataEnvironment(any(), any(), any()); + willReturn(legacyListener).given(this.postProcessor).getLegacyListener(); + this.postProcessor.postProcessEnvironment(this.environment, this.application); + verifyNoInteractions(this.configDataEnvironment); + verify(legacyListener).addPropertySources(eq(this.environment), any(DefaultResourceLoader.class)); + assertThat(this.environment.getActiveProfiles()).containsExactly("dev"); } @Test diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerLegacyReproTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerLegacyReproTests.java index 9c27a0f61f..a11173f2fe 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerLegacyReproTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerLegacyReproTests.java @@ -33,6 +33,7 @@ import static org.assertj.core.api.Assertions.assertThat; * * @author Phillip Webb * @author Dave Syer + * @author Nguyen Bao Sach */ @ExtendWith(UseLegacyProcessing.class) class ConfigFileApplicationListenerLegacyReproTests { @@ -167,6 +168,17 @@ class ConfigFileApplicationListenerLegacyReproTests { assertVersionProperty(this.context, "A", "C", "A"); } + @Test + void additionalProfilesViaProgrammaticallySetting() { + // gh-25704 + SpringApplication application = new SpringApplication(Config.class); + application.setWebApplicationType(WebApplicationType.NONE); + application.setAdditionalProfiles("dev"); + this.context = application.run(); + assertThat(this.context.getEnvironment().acceptsProfiles(Profiles.of("dev"))).isTrue(); + assertThat(this.context.getEnvironment().getProperty("my.property")).isEqualTo("fromdevpropertiesfile"); + } + private void assertVersionProperty(ConfigurableApplicationContext context, String expectedVersion, String... expectedActiveProfiles) { assertThat(context.getEnvironment().getActiveProfiles()).isEqualTo(expectedActiveProfiles); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java index f0ffcc0dc6..7c420c126a 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java @@ -70,6 +70,7 @@ import static org.assertj.core.api.Assertions.assertThatIllegalStateException; * @author Eddú Meléndez * @author Madhura Bhave * @author Scott Frederick + * @author Nguyen Bao Sach */ @Deprecated @ExtendWith({ OutputCaptureExtension.class, UseLegacyProcessing.class }) @@ -1150,6 +1151,46 @@ class ConfigFileApplicationListenerTests { assertThat(this.environment.getProperty("fourth.property")).isNull(); } + @Test + void additionalProfilesCanBeIncludedFromProgrammaticallySetting() { + // gh-25704 + SpringApplication application = new SpringApplication(Config.class); + application.setWebApplicationType(WebApplicationType.NONE); + application.setAdditionalProfiles("dev"); + this.context = application.run(); + // Active profile should win over default + assertThat(this.context.getEnvironment().getProperty("my.property")).isEqualTo("fromdevpropertiesfile"); + } + + @Test + void twoAdditionalProfilesCanBeIncludedFromProgrammaticallySetting() { + // gh-25704 + SpringApplication application = new SpringApplication(Config.class); + application.setWebApplicationType(WebApplicationType.NONE); + application.setAdditionalProfiles("other", "dev"); + this.context = application.run(); + assertThat(this.context.getEnvironment().getProperty("my.property")).isEqualTo("fromdevpropertiesfile"); + } + + @Test + void includeProfilesOrder() { + SpringApplication application = new SpringApplication(Config.class); + application.setWebApplicationType(WebApplicationType.NONE); + this.context = application.run("--spring.profiles.active=bar,spam", "--spring.profiles.include=foo"); + // Before Boot 2.4 included profiles should always be first + assertThat(this.context.getEnvironment().getActiveProfiles()).containsExactly("foo", "bar", "spam"); + } + + @Test + void addProfilesOrder() { + SpringApplication application = new SpringApplication(Config.class); + application.setWebApplicationType(WebApplicationType.NONE); + application.setAdditionalProfiles("foo"); + this.context = application.run("--spring.profiles.active=bar,spam"); + // Before Boot 2.4 additional profiles should always be first + assertThat(this.context.getEnvironment().getActiveProfiles()).containsExactly("foo", "bar", "spam"); + } + private Condition matchingPropertySource(final String sourceName) { return new Condition("environment containing property source " + sourceName) { From 2e1b20ce2b5aa0772dfa69dda68011e1e8b3b074 Mon Sep 17 00:00:00 2001 From: Madhura Bhave Date: Tue, 11 May 2021 13:01:15 -0700 Subject: [PATCH 2/2] Polish "Process additional profiles before config files processing" See gh-25817 --- .../boot/SpringApplication.java | 1 - .../boot/SpringApplicationTests.java | 11 --------- ...nfigDataEnvironmentPostProcessorTests.java | 12 +++++++--- ...leApplicationListenerLegacyReproTests.java | 11 --------- .../ConfigFileApplicationListenerTests.java | 23 +------------------ 5 files changed, 10 insertions(+), 48 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/SpringApplication.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/SpringApplication.java index 080d056ba7..e493ade245 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/SpringApplication.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/SpringApplication.java @@ -154,7 +154,6 @@ import org.springframework.web.context.support.StandardServletEnvironment; * @author Madhura Bhave * @author Brian Clozel * @author Ethan Rubinson - * @author Nguyen Bao Sach * @since 1.0.0 * @see #run(Class, String[]) * @see #run(Class[], String[]) diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java index 109f9fb8aa..783f9955e9 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java @@ -605,17 +605,6 @@ class SpringApplicationTests { assertThat(environment.getActiveProfiles()).containsExactly("bar", "spam", "foo"); } - @Test - void includeProfilesOrder() { - SpringApplication application = new SpringApplication(ExampleConfig.class); - application.setWebApplicationType(WebApplicationType.NONE); - ConfigurableEnvironment environment = new StandardEnvironment(); - application.setEnvironment(environment); - this.context = application.run("--spring.profiles.active=bar,spam", "--spring.profiles.include=foo"); - // Since Boot 2.4 included profiles should always be last - assertThat(environment.getActiveProfiles()).containsExactly("bar", "spam", "foo"); - } - @Test void addProfilesOrderWithProperties() { SpringApplication application = new SpringApplication(ExampleConfig.class); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigDataEnvironmentPostProcessorTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigDataEnvironmentPostProcessorTests.java index 6bec0e3624..b6924a3883 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigDataEnvironmentPostProcessorTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigDataEnvironmentPostProcessorTests.java @@ -83,7 +83,6 @@ class ConfigDataEnvironmentPostProcessorTests { verify(this.postProcessor).getConfigDataEnvironment(any(), this.resourceLoaderCaptor.capture(), any()); verify(this.configDataEnvironment).processAndApply(); assertThat(this.resourceLoaderCaptor.getValue()).isInstanceOf(DefaultResourceLoader.class); - assertThat(this.environment.getActiveProfiles()).isEmpty(); } @Test @@ -95,7 +94,6 @@ class ConfigDataEnvironmentPostProcessorTests { verify(this.postProcessor).getConfigDataEnvironment(any(), this.resourceLoaderCaptor.capture(), any()); verify(this.configDataEnvironment).processAndApply(); assertThat(this.resourceLoaderCaptor.getValue()).isSameAs(resourceLoader); - assertThat(this.environment.getActiveProfiles()).isEmpty(); } @Test @@ -106,6 +104,14 @@ class ConfigDataEnvironmentPostProcessorTests { verify(this.postProcessor).getConfigDataEnvironment(any(), any(), this.additionalProfilesCaptor.capture()); verify(this.configDataEnvironment).processAndApply(); assertThat(this.additionalProfilesCaptor.getValue()).containsExactly("dev"); + } + + @Test + void postProcessEnvironmentWhenNoActiveProfiles() { + willReturn(this.configDataEnvironment).given(this.postProcessor).getConfigDataEnvironment(any(), any(), any()); + this.postProcessor.postProcessEnvironment(this.environment, this.application); + verify(this.postProcessor).getConfigDataEnvironment(any(), this.resourceLoaderCaptor.capture(), any()); + verify(this.configDataEnvironment).processAndApply(); assertThat(this.environment.getActiveProfiles()).isEmpty(); } @@ -123,7 +129,7 @@ class ConfigDataEnvironmentPostProcessorTests { } @Test - void postProcessEnvironmentWhenHasAdditionalProfilesViaProgrammaticallySettingAndUseLegacyProcessing() { + void postProcessEnvironmentWhenHasAdditionalProfilesAndUseLegacyProcessing() { this.application.setAdditionalProfiles("dev"); ConfigDataEnvironmentPostProcessor.LegacyConfigFileApplicationListener legacyListener = mock( ConfigDataEnvironmentPostProcessor.LegacyConfigFileApplicationListener.class); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerLegacyReproTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerLegacyReproTests.java index a11173f2fe..6c3b1cfa64 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerLegacyReproTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerLegacyReproTests.java @@ -168,17 +168,6 @@ class ConfigFileApplicationListenerLegacyReproTests { assertVersionProperty(this.context, "A", "C", "A"); } - @Test - void additionalProfilesViaProgrammaticallySetting() { - // gh-25704 - SpringApplication application = new SpringApplication(Config.class); - application.setWebApplicationType(WebApplicationType.NONE); - application.setAdditionalProfiles("dev"); - this.context = application.run(); - assertThat(this.context.getEnvironment().acceptsProfiles(Profiles.of("dev"))).isTrue(); - assertThat(this.context.getEnvironment().getProperty("my.property")).isEqualTo("fromdevpropertiesfile"); - } - private void assertVersionProperty(ConfigurableApplicationContext context, String expectedVersion, String... expectedActiveProfiles) { assertThat(context.getEnvironment().getActiveProfiles()).isEqualTo(expectedActiveProfiles); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java index 7c420c126a..44ad34345e 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java @@ -1153,17 +1153,6 @@ class ConfigFileApplicationListenerTests { @Test void additionalProfilesCanBeIncludedFromProgrammaticallySetting() { - // gh-25704 - SpringApplication application = new SpringApplication(Config.class); - application.setWebApplicationType(WebApplicationType.NONE); - application.setAdditionalProfiles("dev"); - this.context = application.run(); - // Active profile should win over default - assertThat(this.context.getEnvironment().getProperty("my.property")).isEqualTo("fromdevpropertiesfile"); - } - - @Test - void twoAdditionalProfilesCanBeIncludedFromProgrammaticallySetting() { // gh-25704 SpringApplication application = new SpringApplication(Config.class); application.setWebApplicationType(WebApplicationType.NONE); @@ -1173,21 +1162,11 @@ class ConfigFileApplicationListenerTests { } @Test - void includeProfilesOrder() { - SpringApplication application = new SpringApplication(Config.class); - application.setWebApplicationType(WebApplicationType.NONE); - this.context = application.run("--spring.profiles.active=bar,spam", "--spring.profiles.include=foo"); - // Before Boot 2.4 included profiles should always be first - assertThat(this.context.getEnvironment().getActiveProfiles()).containsExactly("foo", "bar", "spam"); - } - - @Test - void addProfilesOrder() { + void activeProfilesShouldTakePrecedenceOverAdditionalProfiles() { SpringApplication application = new SpringApplication(Config.class); application.setWebApplicationType(WebApplicationType.NONE); application.setAdditionalProfiles("foo"); this.context = application.run("--spring.profiles.active=bar,spam"); - // Before Boot 2.4 additional profiles should always be first assertThat(this.context.getEnvironment().getActiveProfiles()).containsExactly("foo", "bar", "spam"); }