From 97fd9a7c774a48c959c54682d8e3d21dc5ed1802 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 7c44b063c40..3c3124c4212 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[]) @@ -374,7 +375,6 @@ public class SpringApplication { ConfigurationPropertySources.attach(environment); listeners.environmentPrepared(bootstrapContext, environment); DefaultPropertiesPropertySource.moveToEnd(environment); - configureAdditionalProfiles(environment); bindToSpringApplication(environment); if (!this.isCustomEnvironment) { environment = new EnvironmentConverter(getClassLoader()).convertEnvironmentIfNecessary(environment, @@ -557,16 +557,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 2c11110fc10..7df92a25482 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 3d25098c467..c916bf92a57 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 4a0545b5861..42cf6d53dee 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 9c27a0f61f9..a11173f2fe0 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 f0ffcc0dc6b..7c420c126ae 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 eff024b0cef99139c3eaeb7a1abe26209f90b464 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 3c3124c4212..d243ddd6775 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 c916bf92a57..9b4700409f6 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 42cf6d53dee..dc6f56c182b 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 a11173f2fe0..6c3b1cfa648 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 7c420c126ae..44ad34345ed 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"); }