Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Additional profiles are processed too late when legacy processing is used #25817

Closed
wants to merge 1 commit into from
Closed

Additional profiles are processed too late when legacy processing is used #25817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 1 addition & 11 deletions ...ng-boot-project/spring-boot/src/main/java/org/springframework/boot/SpringApplication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@
* @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[])
Expand Down Expand Up @@ -373,7 +374,6 @@ private ConfigurableEnvironment prepareEnvironment(SpringApplicationRunListeners
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);
Expand Down Expand Up @@ -558,16 +558,6 @@ protected void configurePropertySources(ConfigurableEnvironment environment, Str
protected void configureProfiles(ConfigurableEnvironment environment, String[] args) {
}

private void configureAdditionalProfiles(ConfigurableEnvironment environment) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The additional profiles are included in the Environment's active profiles at ConfigDataEnvironmentPostProcessor when legacy processing is used, or at ConfigDataEnvironment as below

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

Line 348 in 4ad87a8

this.environment.setActiveProfiles(StringUtils.toStringArray(profiles.getActive()));

Therefore, configureAdditionalProfiles becomes unnecessary.

if (!CollectionUtils.isEmpty(this.additionalProfiles)) {
Set<String> 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);
Expand Down
15 changes: 15 additions & 0 deletions ...main/java/org/springframework/boot/context/config/ConfigDataEnvironmentPostProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
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;
Expand All @@ -34,13 +36,16 @@
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
* {@link Environment}.
*
* @author Phillip Webb
* @author Madhura Bhave
* @author Nguyen Bao Sach
* @since 2.4.0
*/
public class ConfigDataEnvironmentPostProcessor implements EnvironmentPostProcessor, Ordered {
Expand Down Expand Up @@ -99,6 +104,7 @@ void postProcessEnvironment(ConfigurableEnvironment environment, ResourceLoader
catch (UseLegacyConfigProcessingException ex) {
this.logger.debug(LogMessage.format("Switching to legacy config file processing [%s]",
ex.getConfigurationProperty()));
configureAdditionalProfiles(environment, additionalProfiles);
postProcessUsingLegacyApplicationListener(environment, resourceLoader);
}
}
Expand All @@ -109,6 +115,15 @@ ConfigDataEnvironment getConfigDataEnvironment(ConfigurableEnvironment environme
additionalProfiles, this.environmentUpdateListener);
}

private void configureAdditionalProfiles(ConfigurableEnvironment environment,
Collection<String> additionalProfiles) {
if (!CollectionUtils.isEmpty(additionalProfiles)) {
Set<String> 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);
Expand Down
12 changes: 12 additions & 0 deletions ...ot-project/spring-boot/src/test/java/org/springframework/boot/SpringApplicationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@
* @author Brian Clozel
* @author Artsiom Yudovin
* @author Marten Deinum
* @author Nguyen Bao Sach
*/
@ExtendWith(OutputCaptureExtension.class)
class SpringApplicationTests {
Expand Down Expand Up @@ -604,6 +605,17 @@ void addProfilesOrder() {
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");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test also seems unrelated to this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As my above reply, Could I add this test to another PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we already cover the active and include case in this test.

Copy link
Contributor Author

@nguyensach nguyensach May 5, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbhave
In this test case, I want to test active and included profiles when using Boot 2.4 or later without using legacy processing.
It seems that the order of the active and included profiles has to be changed since boot 2.4 or later without using legacy processing.
Could I add this test to another PR?


@Test
void addProfilesOrderWithProperties() {
SpringApplication application = new SpringApplication(ExampleConfig.class);
Expand Down
19 changes: 19 additions & 0 deletions ...java/org/springframework/boot/context/config/ConfigDataEnvironmentPostProcessorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
*
* @author Phillip Webb
* @author Madhura Bhave
* @author Nguyen Bao Sach
*/
@ExtendWith(MockitoExtension.class)
class ConfigDataEnvironmentPostProcessorTests {
Expand Down Expand Up @@ -82,6 +83,7 @@ void postProcessEnvironmentWhenNoLoaderCreatesDefaultLoaderInstance() {
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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nguyensach What is the reason for adding this (and similiar ones below) assertion here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ConfigDataEnvironmentPostProcessor, I added the below code.

https://github.com/spring-projects/spring-boot/pull/25817/files#diff-73c99017027ef3e130148a82d7075ffa9e95826b56aef5b495f1e48c1f58fac3R118-R125

It adds the additional profiles to the environment's active profiles, so I want to check the environment's active profiles here

}

@Test
Expand All @@ -93,6 +95,7 @@ void postProcessEnvironmentWhenCustomLoaderUsesSpecifiedLoaderInstance() {
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
Expand All @@ -103,6 +106,7 @@ void postProcessEnvironmentWhenHasAdditionalProfilesOnSpringApplicationUsesAddit
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
Expand All @@ -115,6 +119,21 @@ void postProcessEnvironmentWhenUseLegacyProcessingSwitchesToLegacyMethod() {
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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of just checking if the environment contains the additional profiles (which it does even today because SpringApplication configures them in configureAdditionalProfiles), we should check if the profiles are actually processed by ConfigFileApplicationListener).

Copy link
Contributor Author

@nguyensach nguyensach May 3, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the scope of ConfigDataEnvironmentPostProcessor, I am only checking if the environment contains the additional profiles.

we should check if the profiles are actually processed by ConfigFileApplicationListener

In order to check it, I added two below tests.
https://github.com/spring-projects/spring-boot/pull/25817/files#diff-82a4d071283a2194fc5c0e8cffbdddbeda4e53d4e5676138f93b46fcaa16e516R1154-R1163

https://github.com/spring-projects/spring-boot/pull/25817/files#diff-82a4d071283a2194fc5c0e8cffbdddbeda4e53d4e5676138f93b46fcaa16e516R1184-R1192

}

@Test
Expand Down
12 changes: 12 additions & 0 deletions ...rg/springframework/boot/context/config/ConfigFileApplicationListenerLegacyReproTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
*
* @author Phillip Webb
* @author Dave Syer
* @author Nguyen Bao Sach
*/
@ExtendWith(UseLegacyProcessing.class)
class ConfigFileApplicationListenerLegacyReproTests {
Expand Down Expand Up @@ -167,6 +168,17 @@ void reverseOrderOfProfilesWithYamlAndNoOverride() {
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");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this test as it is essentially a duplicate of the test below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Line 32 in 0893df4

* Tests to reproduce reported issues.

According to the above comment, this test reproduces reported issues. Like you commented, it is essentially a duplicate of another test, so I will remove it.


private void assertVersionProperty(ConfigurableApplicationContext context, String expectedVersion,
String... expectedActiveProfiles) {
assertThat(context.getEnvironment().getActiveProfiles()).isEqualTo(expectedActiveProfiles);
Expand Down
41 changes: 41 additions & 0 deletions ...test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
* @author Eddú Meléndez
* @author Madhura Bhave
* @author Scott Frederick
* @author Nguyen Bao Sach
*/
@Deprecated
@ExtendWith({ OutputCaptureExtension.class, UseLegacyProcessing.class })
Expand Down Expand Up @@ -1150,6 +1151,46 @@ void locationsWithWildcardFilesShouldIgnoreHiddenDirectories() {
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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't seem to be giving us much either and I don't think it is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only want to test when there are many additional profiles rather than one.
In this test case, I want to check that which additional profile should win.

Maybe this test isn't related to the bug. Could I add this test to another PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, we don't need the test for the single additional profile and we can keep this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbhave

In that case, we don't need the test for the single additional profile and we can keep this one.

You don't need the test for the multiple additional profiles, right?
If so, I will delete this test case.

}

@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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't seem to be related to the bug that this issue is trying to address. The issue is about additionalProfiles and I don't think this test is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test isn't related to the bug. However, I think that we are lacking test cases of the order included profiles and active profiles. So, Could I add this test to another PR?

Because of lacking test cases of the order additional profiles and active profiles, it results in #26189

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to be testing active and additional profiles though. It's testing active and included profiles.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbhave
#25817 (comment)
As your above comment, there is already a test case that tests active and included profiles when using legacy processing, so I will delete this test case.

}

@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<ConfigurableEnvironment> matchingPropertySource(final String sourceName) {
return new Condition<ConfigurableEnvironment>("environment containing property source " + sourceName) {

Expand Down