matrei commented on code in PR #16547:
URL: https://github.com/apache/grails-core/pull/16547#discussion_r4218397978
##########
grails-core/src/main/groovy/org/grails/config/yaml/YamlPropertySourceLoader.java:
##########
@@ -58,9 +62,17 @@ public List<PropertySource<?>> load(String name, Resource
resource) throws IOExc
public List<PropertySource<?>> load(String name, Resource resource,
List<String> filteredKeys) throws IOException {
setResources(resource);
+ // Select source documents once; merging resolved configuration must
not re-evaluate JVM profiles.
+ final List<String> activeProfiles = Arrays.asList(
+
StringUtils.tokenizeToStringArray(System.getProperty("spring.profiles.active",
""), ","));
setDocumentMatchers((DocumentMatcher) properties -> {
- final String profile = properties.getProperty("spring.profiles");
- return profile == null ||
profile.equalsIgnoreCase(System.getProperty("spring.profiles.active")) ?
MatchStatus.FOUND : MatchStatus.NOT_FOUND;
+ final String[] profiles = profileSelectors(properties,
"spring.config.activate.on-profile");
+ final String[] legacyProfiles = profileSelectors(properties,
"spring.profiles");
+ final boolean matchesProfile = profiles.length == 0 ||
Review Comment:
Minor, not blocking. Boot binds `on-profile: dev,test` to `String[] {"dev",
"test"}`, so in Boot it means either profile. Here it becomes a single profile
named `dev,test`. That can never match, because the active list is split on
commas, so the document is silently skipped. Sequences are already treated as
alternatives to match Boot's `String[]` binding. Splitting scalars with
`StringUtils.commaDelimitedListToStringArray` would make both forms behave like
Boot, and the doc paragraph about commas could go.
It would also restore one 7.0.x case: `spring.profiles: dev,alpha` with
`-Dspring.profiles.active=dev,alpha` used to match by exact string. It doesn't
match anymore, and the `'dev,alpha' | 'dev,alpha' | 'default'` row locks that
in.
##########
grails-core/src/main/groovy/org/grails/config/yaml/YamlPropertySourceLoader.java:
##########
@@ -58,9 +62,17 @@ public List<PropertySource<?>> load(String name, Resource
resource) throws IOExc
public List<PropertySource<?>> load(String name, Resource resource,
List<String> filteredKeys) throws IOException {
setResources(resource);
+ // Select source documents once; merging resolved configuration must
not re-evaluate JVM profiles.
Review Comment:
This loader is registered in `spring.factories`, so Boot's config-data
processing loads `application.yml` through it. The selected document's
`spring.config.activate.on-profile` value ends up in the merged `NavigableMap`
source. Boot then evaluates that value again against its own active profiles
and drops the whole source if it doesn't match.
That was harmless while only positive selectors could be selected, because
Boot's active profiles always include the system property. Negation breaks it.
Take this example:
```yaml
sample.base: base
---
spring.config.activate.on-profile: '!production'
sample.message: non-production
```
Without `-Dspring.profiles.active`, the loader selects the second document.
`GrailsApp.configureEnvironment` adds the Grails environment name as an active
profile, so in production Boot sees `production`, rejects `!production` and
drops the entire file. `sample.base` then resolves to `null`. The same happens
with `--spring.profiles.active=prod` or `SPRING_PROFILES_ACTIVE=prod` and
`'!prod'`. On 7.0.x the document was skipped and `sample.base` was still `base`.
At this point the documents have already been selected, so stripping the
selector keys (scalar and indexed, for both selectors) before merging fixes it:
```java
loaded.forEach(map -> {
map.keySet().removeIf(YamlPropertySourceLoader::isProfileSelectorKey);
...
```
```java
private static boolean isProfileSelectorKey(String name) {
return name.equals("spring.config.activate.on-profile") ||
isIndexedKey(name, "spring.config.activate.on-profile") ||
name.equals("spring.profiles") || isIndexedKey(name,
"spring.profiles");
}
```
This also fixes an older problem. When a legacy `spring.profiles` document
is selected, startup currently fails with Boot's
`InvalidConfigDataPropertyException` ("Property 'spring.profiles' ... is
invalid and should be replaced with 'spring.config.activate.on-profile'"). Yet
the guide now documents the legacy selector as supported. With the keys
stripped, both cases work and the existing `org.grails.config` /
`org.grails.core.cfg` specs still pass.
The current specs call the loader directly. A test that goes through
`SpringApplication` (with a profile added in `configureEnvironment`, like
`GrailsApp` does) would cover this path.
##########
grails-doc/src/en/guide/conf/config.adoc:
##########
@@ -53,6 +53,35 @@ For example:
my.tmp.dir = "${userHome}/.grails/tmp"
----
+=== Profile-specific YAML Documents
+
+Use `spring.config.activate.on-profile` to load a YAML document only when its
profile condition matches. Grails evaluates these conditions against the
comma-separated profiles in the JVM's `spring.profiles.active` system property.
For example, `-Dspring.profiles.active=dev,alpha` activates both `dev` and
`alpha`; whitespace around each name is ignored.
Review Comment:
Could we spell out that profiles activated any other way are not considered
here? That includes `SPRING_PROFILES_ACTIVE`, `--spring.profiles.active` and
the Grails environment name that `GrailsApp` adds. Even with the fix above, a
`'!prod'` document still applies when `prod` comes from the environment
variable, which is easy to trip over.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
