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

nguyensach · 2021-03-29T02:18:03Z

In post-processing the given environment of ConfigDataEnvironmentPostProcessor, add SpringApplication's additional profiles that are set programmatically into the given environment when legacy processing is used.

This commit updates the following areas:

Add configureAdditionalProfiles method into ConfigDataEnvironmentPostProcessor, that add the additional profiles into the given environment.
When legacy processing is used, call configureAdditionalProfiles method to add the additional profiles into the given environment.
Remove unnecessary configureAdditionalProfiles method in SpringApplication.
Supplement unit test for SpringApplicationTests.

Fixes #25704
Fixes #26189

...oot/src/main/java/org/springframework/boot/context/config/ConfigFileApplicationListener.java

wilkinsona · 2021-03-29T06:53:32Z

Thanks for the pull request, @nguyensach. Additional profiles configured via SpringApplication are activated by including them in the Environment's active profiles:

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

Lines 557 to 565 in d1aaee4

    
           private void configureAdditionalProfiles(ConfigurableEnvironment environment) { 
        
           	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)); 
        
           		} 
        
           	} 
        
           }

IIRC, this means that they should be picked up by ConfigFileApplicationListener without any special treatment. I believe this was the case in Spring Boot 2.3 and, ideally, we'd make it the case in Spring Boot 2.4 as well. If you explored this approach and it didn't work out, can you please explain what stopped it from working?

nguyensach · 2021-03-30T05:34:08Z

@wilkinsona

Thanks for your information!

If you explored this approach and it didn't work out, can you please explain what stopped it from working?

In Spring Boot 2.4, when legacy processing is used and additional profiles configured via SpringApplication, it is not worked as expected because of the method configureProfiles of SpringApplication.

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

Lines 552 to 562 in 4ad87a8

    
           	/** 
        
           	 * Configure which profiles are active (or active by default) for this application 
        
           	 * environment. Additional profiles may be activated during configuration file 
        
           	 * processing via the {@code spring.profiles.active} property. 
        
           	 * @param environment this application's environment 
        
           	 * @param args arguments passed to the {@code run} method 
        
           	 * @see #configureEnvironment(ConfigurableEnvironment, String[]) 
        
           	 * @see org.springframework.boot.context.config.ConfigFileApplicationListener 
        
           	 */ 
        
           	protected void configureProfiles(ConfigurableEnvironment environment, String[] args) { 
        
           	}

In Spring Boot 2.3, the implementation of the method configureProfiles of SpringApplication is like below.

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

Lines 513 to 526 in 73cab9c

    
           	/** 
        
           	 * Configure which profiles are active (or active by default) for this application 
        
           	 * environment. Additional profiles may be activated during configuration file 
        
           	 * processing via the {@code spring.profiles.active} property. 
        
           	 * @param environment this application's environment 
        
           	 * @param args arguments passed to the {@code run} method 
        
           	 * @see #configureEnvironment(ConfigurableEnvironment, String[]) 
        
           	 * @see org.springframework.boot.context.config.ConfigFileApplicationListener 
        
           	 */ 
        
           	protected void configureProfiles(ConfigurableEnvironment environment, String[] args) { 
        
           		Set<String> profiles = new LinkedHashSet<>(this.additionalProfiles); 
        
           		profiles.addAll(Arrays.asList(environment.getActiveProfiles())); 
        
           		environment.setActiveProfiles(StringUtils.toStringArray(profiles)); 
        
           	}

In Spring Boot 2.3, by the method configureEnvironment of SpringApplication that calls the method configureProfiles, additional profiles configured via SpringApplication are activated by including them in the Environment's active profiles. Then, that Environment is passed into ConfigFileApplicationListener at the below point on ApplicationEnvironmentPreparedEvent event.

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

Line 345 in 73cab9c

listeners.environmentPrepared(environment);

Then, in ConfigFileApplicationListener, the additional profiles are loaded into the Environment's propertySources, therefore, in Spring Boot 2.3, the properties of the additional profiles is got by the Environment's getProperty.

However, In Spring Boot 2.4, the method configureProfiles of SpringApplication is empty, therefore, the additional profiles configured via SpringApplication are not activated by including them in the Environment's active profiles before jumping into ConfigFileApplicationListener. So, the additional profiles are not loaded into the Environment's propertySources.

IIRC, this means that they should be picked up by ConfigFileApplicationListener without any special treatment.

In Spring Boot 2.4, when using legacy processing, the additional profiles are not picked up by ConfigFileApplicationListener without any special treatment.

After rethinking about order of the additional profiles, the included profiles, the other actived profiles in the ConfigFileApplicationListener's initializeProfiles method, I fixed this PR. Could you help me to review my PR?

By the way, in my PR, the CI is fail at the gradle's spring-boot-project:spring-boot:checkstyleMain task. I run format task in my local pc but it is not auto correct for me. Is there any way to auto fix the issues of the checkstyleMain.

nguyensach · 2021-03-30T05:43:20Z

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

@@ -558,16 +558,6 @@ protected void configurePropertySources(ConfigurableEnvironment environment, Str
 	protected void configureProfiles(ConfigurableEnvironment environment, String[] args) {
 	}

-	private void configureAdditionalProfiles(ConfigurableEnvironment environment) {


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.

wilkinsona · 2021-03-30T06:58:12Z

@philwebb could you take a look at this one please?

wilkinsona · 2021-03-30T06:59:28Z

By the way, in my PR, the CI is fail at the gradle's spring-boot-project:spring-boot:checkstyleMain task. I run format task in my local pc but it is not auto correct for me. Is there any way to auto fix the issues of the checkstyleMain.

There's no auto-fix for Checkstyle problems, unfortunately. Looking at the diff, I suspect the problem is due to the changes to the imports. They need to be kept in their current ordering and not use wildcards.

nguyensach · 2021-03-30T07:10:51Z

By the way, in my PR, the CI is fail at the gradle's spring-boot-project:spring-boot:checkstyleMain task. I run format task in my local pc but it is not auto correct for me. Is there any way to auto fix the issues of the checkstyleMain.

There's no auto-fix for Checkstyle problems, unfortunately. Looking at the diff, I suspect the problem is due to the changes to the imports. They need to be kept in their current ordering and not use wildcards.

@wilkinsona Thanks for your response! I will fix it manually.

In post-processing the given environment of ConfigDataEnvironmentPostProcessor, add SpringApplication's additional profiles that are set by programmatically into environment when legacy processing is used. This commit updates the following areas: - Add configureAdditionalProfiles method into ConfigDataEnvironmentPostProcessor, that add the additional profiles into the given environment. - When legacy processing is used, call configureAdditionalProfiles method to add the additional profiles into the given environment. - Remove unnecessary configureAdditionalProfiles method in SpringApplication. - Supplement unit test for SpringApplicationTests. Closes spring-projects#25704

mbhave · 2021-04-30T23:48:44Z

@nguyensach This change makes sense to me. I have left some comments. Can you please take a look?

nguyensach · 2021-05-01T01:11:17Z

This change makes sense to me. I have left some comments.

@mbhave
I'm sorry! I don't see any of your comments. Can you please check it?

mbhave · 2021-04-30T23:37:26Z

...a/org/springframework/boot/context/config/ConfigFileApplicationListenerLegacyReproTests.java

+		this.context = application.run();
+		assertThat(this.context.getEnvironment().acceptsProfiles(Profiles.of("dev"))).isTrue();
+		assertThat(this.context.getEnvironment().getProperty("my.property")).isEqualTo("fromdevpropertiesfile");
+	}


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

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.

mbhave · 2021-04-30T23:38:03Z

...rc/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java

+		application.setWebApplicationType(WebApplicationType.NONE);
+		application.setAdditionalProfiles("other", "dev");
+		this.context = application.run();
+		assertThat(this.context.getEnvironment().getProperty("my.property")).isEqualTo("fromdevpropertiesfile");


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

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?

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

@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.

mbhave · 2021-04-30T23:39:21Z

...rc/test/java/org/springframework/boot/context/config/ConfigFileApplicationListenerTests.java

+		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");


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.

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

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

@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.

mbhave · 2021-04-30T23:40:09Z

...st/java/org/springframework/boot/context/config/ConfigDataEnvironmentPostProcessorTests.java

@@ -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();


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

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

mbhave · 2021-04-30T23:46:18Z

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

+		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");
+	}


This test also seems unrelated to this change.

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

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

@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?

mbhave · 2021-04-30T23:48:05Z

...st/java/org/springframework/boot/context/config/ConfigDataEnvironmentPostProcessorTests.java

+		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");


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).

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

mbhave · 2021-05-03T16:56:42Z

@nguyensach Sorry about that! I forgot to click on submit 🤦🏽‍♀️ Hopefully you can see them now.

nguyensach · 2021-05-03T23:30:48Z

@mbhave Thanks for your review! I replied to all your comments.

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

See gh-25817

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

See gh-25817

mbhave · 2021-05-11T22:40:03Z

Thanks for the PR @nguyensach. This has now been merged into main along with this polish commit.

nguyensach force-pushed the enable_additional_profiles_in_2.4 branch from 1aa39f5 to bd4fa3c Compare March 29, 2021 02:21

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 29, 2021

cuspymd reviewed Mar 29, 2021

View reviewed changes

...oot/src/main/java/org/springframework/boot/context/config/ConfigFileApplicationListener.java Outdated Show resolved Hide resolved

wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Mar 29, 2021

nguyensach force-pushed the enable_additional_profiles_in_2.4 branch 2 times, most recently from 4002310 to 5ba2234 Compare March 30, 2021 03:31

nguyensach changed the title ~~Add additional profiles to ConfigFileApplicationListener~~ Add additional profiles into environment when legacy processing is used Mar 30, 2021

spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 30, 2021

nguyensach commented Mar 30, 2021

View reviewed changes

wilkinsona requested a review from philwebb March 30, 2021 06:57

nguyensach force-pushed the enable_additional_profiles_in_2.4 branch from 5ba2234 to aaf7092 Compare March 30, 2021 11:51

philwebb self-assigned this Apr 6, 2021

nguyensach mentioned this pull request Apr 22, 2021

SpringApplication additional profiles and active profiles ordering changed with Spring Boot 2.4 #26189

Closed

mbhave self-assigned this Apr 30, 2021

mbhave added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Apr 30, 2021

spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 1, 2021

snicoll added the for: team-attention An issue we'd like other members of the team to review label May 3, 2021

philwebb removed the for: team-attention An issue we'd like other members of the team to review label May 3, 2021

mbhave reviewed May 3, 2021

View reviewed changes

mbhave added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels May 3, 2021

spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 3, 2021

mbhave added a commit that referenced this pull request May 11, 2021

Polish "Process additional profiles before config files processing"

2e1b20c

See gh-25817

mbhave closed this in 38ef6b5 May 11, 2021

mbhave changed the title ~~Add additional profiles into environment when legacy processing is used~~ Additional profiles are processed too late when legacy processing is used May 11, 2021

mbhave added this to the 2.4.x milestone May 11, 2021

mbhave mentioned this pull request May 11, 2021

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

Closed

mbhave added a commit that referenced this pull request May 11, 2021

Polish "Process additional profiles before config files processing"

eff024b

See gh-25817

mbhave modified the milestones: 2.4.x, 2.4.6 May 11, 2021

mbhave mentioned this pull request May 11, 2021

SpringApplication.setAdditionalProfiles, application-XXX.yml and SpringBoot2.4 #25704

Closed

nguyensach deleted the enable_additional_profiles_in_2.4 branch May 12, 2021 01:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nguyensach commented Mar 29, 2021 •

edited

wilkinsona commented Mar 29, 2021

nguyensach commented Mar 30, 2021

nguyensach Mar 30, 2021 •

edited

wilkinsona commented Mar 30, 2021

wilkinsona commented Mar 30, 2021

nguyensach commented Mar 30, 2021

mbhave commented Apr 30, 2021

nguyensach commented May 1, 2021

mbhave Apr 30, 2021

nguyensach May 3, 2021

mbhave Apr 30, 2021

nguyensach May 3, 2021

mbhave May 4, 2021

nguyensach May 5, 2021

mbhave Apr 30, 2021

nguyensach May 3, 2021

mbhave May 4, 2021

nguyensach May 5, 2021

mbhave Apr 30, 2021

nguyensach May 3, 2021

mbhave Apr 30, 2021

nguyensach May 3, 2021

mbhave May 4, 2021

nguyensach May 5, 2021 •

edited

mbhave Apr 30, 2021

nguyensach May 3, 2021 •

edited

mbhave commented May 3, 2021

nguyensach commented May 3, 2021

mbhave commented May 11, 2021

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

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

Conversation

nguyensach commented Mar 29, 2021 • edited

wilkinsona commented Mar 29, 2021

nguyensach commented Mar 30, 2021

nguyensach Mar 30, 2021 • edited

Choose a reason for hiding this comment

wilkinsona commented Mar 30, 2021

wilkinsona commented Mar 30, 2021

nguyensach commented Mar 30, 2021

mbhave commented Apr 30, 2021

nguyensach commented May 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyensach May 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyensach May 3, 2021 • edited

Choose a reason for hiding this comment

mbhave commented May 3, 2021

nguyensach commented May 3, 2021

mbhave commented May 11, 2021

nguyensach commented Mar 29, 2021 •

edited

nguyensach Mar 30, 2021 •

edited

nguyensach May 5, 2021 •

edited

nguyensach May 3, 2021 •

edited