I added the validateProfiles() method to enforce stricter rules for Spring profile names.
Currently, only alphanumeric characters, hyphens (-), and underscores (_) are allowed. Please let me know if additional characters should be permitted - I'll update accordingly.
I've verified validation works in application.properties and @ActiveProfiles.
While @Profile doesn't perform validation, this seems acceptable since invalid profiles will fail during activation. Perhaps we could explore adding warnings as a potential enhancement in the future.
Note: this change may break applications using non-conforming profile names.
Fixes gh-34062
Comment From: mhalbritter
Hello! First of all, thank you for the PR!
This currently breaks:
org.springframework.boot.test.context.SpringBootContextLoaderTests#activeProfileWithCommaorg.springframework.boot.test.context.SpringBootContextLoaderTests#testPropertyValuesShouldTakePrecedenceWhenInlinedPropertiesPresentAndProfilesActive
I think we can remove activeProfileWithComma and use a profile without a comma in testPropertyValuesShouldTakePrecedenceWhenInlinedPropertiesPresentAndProfilesActive.
It also breaks:
org.springframework.boot.SpringApplicationTests#logsActiveProfilesWithoutProfileAndMultipleDefaults: Here we can remove the comma.org.springframework.boot.SpringApplicationTests#logsActiveProfilesWithMultipleProfiles: Same here.
Comment From: mhalbritter
Flagging this for team meeting as I want to talk about if the positive list is a good approach. Right now, it's very "western centric" by allowing only A-Z and digits.
I wonder if we should approach it like this:
private void validateProfiles(Profiles profiles) {
for (String profile : profiles) {
validateProfile(profile);
}
}
private void validateProfile(String profile) {
Assert.notNull(profile, "Profile must not be null");
Assert.hasText(profile, "Profile must not be empty");
Assert.state(!profile.startsWith("-") && !profile.startsWith("_"),
() -> String.format("Invalid profile '%s': must not start with '-' or '_'", profile));
Assert.state(!profile.endsWith("-") && !profile.endsWith("_"),
() -> String.format("Invalid profile '%s': must not end with '-' or '_'", profile));
profile.codePoints().forEach((codePoint) -> {
if (codePoint == '-' || codePoint == '_' || Character.isLetterOrDigit(codePoint)) {
return;
}
throw new IllegalStateException(String.format("Invalid profile '%s': must contain only letters or digits or '-' or '_'", profile));
});
}
This would also block ,, ( etc. but lets more non-ascii letters and digits through.
Comment From: YangSiJun528
I incorporated the feedback provided. @mhalbritter
- Fixed the broken tests
- Fixed validateProfiles to allow non-ascii letters
Comment From: YangSiJun528
To test empty string and null cases for active profile, I used mock(Environment.class) because AbstractEnvironment#setActiveProfiles() blocks direct testing. However, using mocks for testing is challenging because Profiles is used in many places.
Is there a better way to handle this testing?
I've included example tests in a demo branch (f4ed4d1490fdb085062aff200dba13a6250c57c0).
Comment From: YangSiJun528
@mhalbritter Gentle ping. 😉
Comment From: mhalbritter
@YangSiJun528 We need to wait until the main branch has been switched to 3.5.x. Right now, it's on 3.4.x and this PR is targeted for 3.5.x. Hang in there, your contribution won't be lost :)
Comment From: YangSiJun528
I understood. Thank you for letting me know.