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.

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

Allow recursive profile group references #24327

Closed
wants to merge 3 commits into from

Conversation

@dreis2211
Copy link
Contributor

@dreis2211 dreis2211 commented Dec 3, 2020

Hi,

this PR fixes #24319 by removing the current profile from group profiles in order to avoid an endless loop when expanding/flattening the profile list.

Cheers,
Christoph

@@ -128,7 +130,7 @@ private boolean hasExplicit(Supplier<String[]> supplier, String propertyValue, S
}
List<String> reversed = new ArrayList<>(list);
Collections.reverse(reversed);
return Collections.unmodifiableList(reversed);
return reversed;

This comment has been minimized.

@dreis2211

dreis2211 Dec 3, 2020
Author Contributor

Alternatively, we could do a stream/filter approach, but the unmodifiable list seemed not needed as the result is only used above and cannot be modified from the outside.

@mbhave
Copy link
Contributor

@mbhave mbhave commented Dec 3, 2020

I'm not sure if just removing the current profile is enough. I think we'd end up with the same issue if we had a configuration like:

spring.profiles.group.a=e,f
spring.profiles.group.e=a,x,y

Maybe we should throw an exception in this case since it is an invalid configuration.

@dreis2211
Copy link
Contributor Author

@dreis2211 dreis2211 commented Dec 4, 2020

@mbhave Good catch - I covered the additional case now.

Which sort of exception do you want to be thrown here? IllegalStateException or a new one? Just let me know if I should add the exception and if so which type. The "maybe" suggests that you're not sure yet.

@philwebb
Copy link
Member

@philwebb philwebb commented Dec 4, 2020

I think an IllegalStateException would be fine. We can always change it later if we need to.

@dreis2211
Copy link
Contributor Author

@dreis2211 dreis2211 commented Dec 4, 2020

@philwebb & @mbhave I added the IllegalStateException. Let me know if this is what you had in mind.

@dreis2211 dreis2211 changed the title Ignore recursive reference in profile groups Fail on recursive references in profile groups Dec 7, 2020
philwebb added a commit that referenced this pull request Dec 9, 2020
Update `Profiles` group expansion logic to fail if recursive
references are found.

See gh-24327
@philwebb philwebb closed this in a2a7bd5 Dec 9, 2020
@philwebb philwebb modified the milestones: 2.4.x, 2.4.1 Dec 9, 2020
@philwebb
Copy link
Member

@philwebb philwebb commented Dec 9, 2020

Thanks once again @dreis2211! I've merged this into master with a minor update to make the message catch duplicates in the expanded set as well as the stack.

@philwebb philwebb reopened this Dec 9, 2020
@philwebb
Copy link
Member

@philwebb philwebb commented Dec 9, 2020

Reopening because I just realized that a user might want to have the same expansion in different groups:

	@Test
	void test() {
		MockEnvironment environment = new MockEnvironment();
		environment.setProperty("spring.profiles.active", "a,b,c");
		environment.setProperty("spring.profiles.group.a", "e,f");
		environment.setProperty("spring.profiles.group.e", "f,g");
		Binder binder = Binder.get(environment);
		assertThatIllegalStateException().isThrownBy(() -> new Profiles(environment, binder, null))
				.withMessageContaining("Profiles could not be resolved. Remove profile 'a' from group: 'e'");
	}
@philwebb philwebb changed the title Fail on recursive references in profile groups Allow recusive profile group references Dec 9, 2020
@philwebb philwebb changed the title Allow recusive profile group references Allow recursive profile group references Dec 9, 2020
@philwebb philwebb requested a review from mbhave Dec 9, 2020
philwebb added a commit that referenced this pull request Dec 9, 2020
Update the original fix for issue #24327 so that recursive elements
are tolerated rather than fail.

See gh-24327
@philwebb
Copy link
Member

@philwebb philwebb commented Dec 9, 2020

After some more thought, I think it's best if we tolerate duplicates rather than throwing an exception. I've changed things again in commit bef5fe2.

@mbhave @dreis2211 I'd appreciate a review if you have time. (I'll leave the issue open for now)

@dreis2211
Copy link
Contributor Author

@dreis2211 dreis2211 commented Dec 9, 2020

Looks good to me, @philwebb

@philwebb philwebb closed this Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants