You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@grails.apache.org by "jdaugherty (via GitHub)" <gi...@apache.org> on 2026/10/07 02:04:48 UTC
[PR] Fix configuration snapshot merges with Spring profile metadata [grails-core]
jdaugherty opened a new pull request, #16547:
URL: https://github.com/apache/grails-core/pull/16547
## Summary
Addresses #15840 on `7.0.x` by keeping Spring profile document selection out of ordinary configuration merges.
`NavigableMap.merge` previously applied Spring profile filtering recursively to every map. This could silently discard an entire configuration snapshot:
- A nested `spring.profiles` settings map, such as `[active: 'alpha']`, was treated as a legacy profile selector, emptying the backup constructed with `PropertySourcesConfig(Map)`.
- A saved configuration containing `spring.config.activate.on-profile=alpha` was rejected during restoration if another application changed the JVM-wide `spring.profiles.active` property to `beta`.
The latter failure reproduces across independent application classloaders with separate `Holders` classes. The same reproduction passes on Grails 6.2.3 and fails on released Grails 7.0.0 and 7.1.0.
## Changes
- Select modern and legacy profile-specific YAML documents in `YamlPropertySourceLoader` before merging their values.
- Make ordinary map/configuration merges preserve the supplied values, including nested profile settings and already-resolved profile metadata.
- Replace tests of the private profile-filter helper with public YAML-loader tests for nested/dotted selectors, absent/matching/nonmatching profiles, settings maps, and loader reuse.
- Add backup/clear/restore and isolated-classloader regression tests, plus coverage for nested application data resembling profile metadata.
- Document runtime merge semantics in the configuration guide.
The public method signatures and dependency versions are unchanged. The reporter's exact Tomcat deployment has not been reproduced; this fixes the demonstrated profile-filtering defect that can cause the reported configuration loss.
## Validation
- Clean Java 17.0.18 builds of both affected modules:
```sh
./gradlew :grails-bootstrap:clean :grails-core:clean \
:grails-bootstrap:build :grails-core:build --rerun-tasks --continue \
--console=plain --quiet --max-workers=4 -PmaxTestParallel=2
```
**352 tests, zero failures, three existing skips.**
- Repository-wide `codeStyle` and `buildHealth`, plus `:grails-bootstrap:validateDependencyVersions` and `:grails-core:validateDependencyVersions`: passed. Build health reported non-fatal dependency advice only.
- RAT license audit: passed with a local init script excluding generated Eclipse/language-server metadata and compiled `bin` output. The stock task initially scanned those generated files; no repository exclusions were changed.
- `git diff --check`: passed.
The newer `aggregateViolations` / `grails-test-report` verification setup is not available on `7.0.x`; this uses the branch's existing checks and module test reports. A repository-wide test suite was not run.
## AI assistance
OpenCode was used for investigation, implementation, review of the diff, and test execution.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix configuration snapshot merges with Spring profile metadata [grails-core]
Posted by "matrei (via GitHub)" <gi...@apache.org>.
matrei commented on PR #16547:
URL: https://github.com/apache/grails-core/pull/16547#issuecomment-6031294735
I added it as part of the Spring 6 / Boot 3 upgrade (#13545). When I switched our test `application.yml` from `spring.profiles` to `spring.config.activate.on-profile`, `NavigableMapSpringProfilesSpec` failed because the `DocumentMatcher` in `YamlPropertySourceLoader` only looked at `spring.profiles`. I found the old `shouldSkipBlock` in `NavigableMap`, whose call had been removed in 03bef04 back in 2018, assumed that was an oversight and wired it back in. In hindsight that was the wrong layer: the profile check belongs in the loader's document matcher, not in every `merge`. So Grails 4–6 never filtered on merge, which lines up with 6.2.3 working.
Moving it into the loader looks right to me. Two small things:
- The legacy `spring.profiles` comparison went from `equalsIgnoreCase` to `equals`. Should we keep it case-insensitive to avoid a behaviour change in a patch release?
- Not new, but neither version handles profile expressions (`a | b`, `!prod`) or multiple active profiles (`dev,alpha`). Maybe a follow-up using `Profiles.of(...)`?
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix configuration snapshot merges with Spring profile metadata [grails-core]
Posted by "jamesfredley (via GitHub)" <gi...@apache.org>.
jamesfredley commented on code in PR #16547:
URL: https://github.com/apache/grails-core/pull/16547#discussion_r4214605124
##########
grails-core/src/main/groovy/org/grails/config/yaml/YamlPropertySourceLoader.java:
##########
@@ -58,9 +61,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 profile = properties.getProperty("spring.config.activate.on-profile", "").trim();
Review Comment:
Rechecked a643247dd2. The loader collects indexed sequence entries and evaluates them as alternative selectors. YamlSpringProfileSpec and ExternalConfigSpec fail if a sequence is ignored.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix configuration snapshot merges with Spring profile metadata [grails-core]
Posted by "sbglasius (via GitHub)" <gi...@apache.org>.
sbglasius commented on code in PR #16547:
URL: https://github.com/apache/grails-core/pull/16547#discussion_r4209451381
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -148,10 +148,6 @@ class NavigableMap implements Map<String, Object>, Cloneable {
Map sourceMap,
boolean parseFlatKeys) {
Review Comment:
Removing the profile check here also removes it for non-YAML sources. `GroovyConfigPropertySourceLoader` calls `NavigableMap.merge(configObject, false)`, so an `application.groovy` with `spring.profiles = 'prod'` or `spring.config.activate.on-profile = 'prod'` used to be skipped when `prod` was inactive. It is now merged in every environment, because only the YAML loader filters by profile.
If dropping this is intentional, it would help to say in the docs that Groovy config doesn't support document-level profile selectors, and to add a test for it.
##########
grails-core/src/main/groovy/org/grails/config/yaml/YamlPropertySourceLoader.java:
##########
@@ -58,9 +61,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 profile = properties.getProperty("spring.config.activate.on-profile");
+ final String legacyProfile = properties.getProperty("spring.profiles");
+ final boolean matchesProfile = profile == null || profile.isEmpty() ||
Review Comment:
A whitespace-only selector (e.g. `spring.profiles: ' '`) passes the `isEmpty()` guard, and `Profiles.of(" ")` throws `IllegalArgumentException: Invalid profile expression [ ]: must contain text` (checked against spring-core 7.0.8), which aborts config loading. Trimming the values before the empty check avoids it.
Also, a comma list such as `spring.profiles: 'dev,test'` is parsed as one profile named `dev,test` and never matches. The old `equalsIgnoreCase` check didn't match it either, so this isn't a regression, but the docs only mention commas for the active profiles.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix configuration snapshot merges with Spring profile metadata [grails-core]
Posted by "jamesfredley (via GitHub)" <gi...@apache.org>.
jamesfredley commented on code in PR #16547:
URL: https://github.com/apache/grails-core/pull/16547#discussion_r4214605124
##########
grails-core/src/main/groovy/org/grails/config/yaml/YamlPropertySourceLoader.java:
##########
@@ -58,9 +61,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 profile = properties.getProperty("spring.config.activate.on-profile", "").trim();
Review Comment:
Rechecked a643247dd2. The loader collects indexed sequence entries and evaluates them as alternative selectors. YamlSpringProfileSpec and ExternalConfigSpec fail if a sequence is ignored.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix configuration snapshot merges with Spring profile metadata [grails-core]
Posted by "jamesfredley (via GitHub)" <gi...@apache.org>.
jamesfredley commented on code in PR #16547:
URL: https://github.com/apache/grails-core/pull/16547#discussion_r4214590760
##########
grails-core/src/main/groovy/org/grails/config/yaml/YamlPropertySourceLoader.java:
##########
@@ -58,9 +61,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 profile = properties.getProperty("spring.config.activate.on-profile", "").trim();
Review Comment:
Rechecked a643247dd2. The loader collects indexed sequence entries and evaluates them as alternative selectors. YamlSpringProfileSpec and ExternalConfigSpec fail if a sequence is ignored. Oracle and Codex both came back clean on this head.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix configuration snapshot merges with Spring profile metadata [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty merged PR #16547:
URL: https://github.com/apache/grails-core/pull/16547
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix configuration snapshot merges with Spring profile metadata [grails-core]
Posted by "jamesfredley (via GitHub)" <gi...@apache.org>.
jamesfredley commented on code in PR #16547:
URL: https://github.com/apache/grails-core/pull/16547#discussion_r4210586848
##########
grails-core/src/main/groovy/org/grails/config/yaml/YamlPropertySourceLoader.java:
##########
@@ -58,9 +61,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 profile = properties.getProperty("spring.config.activate.on-profile", "").trim();
Review Comment:
A sequence such as `spring.config.activate.on-profile: [alpha, beta]` is flattened to `spring.config.activate.on-profile[0]` and `[1]`. This lookup asks for the unindexed name, receives the default empty string, and treats the document as unrestricted. With only `gamma` active the inactive document is still merged, including YAML loaded through `grails.config.locations`.
Read the indexed selector values and evaluate them before treating a missing unindexed key as "always on". The new whitespace rows do not cover a sequence. The same gap applies to a sequence-valued `spring.profiles`.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix configuration snapshot merges with Spring profile metadata [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty commented on PR #16547:
URL: https://github.com/apache/grails-core/pull/16547#issuecomment-6040608748
@matrei I believe this is addressed and ready for you
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix configuration snapshot merges with Spring profile metadata [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16547:
URL: https://github.com/apache/grails-core/pull/16547#issuecomment-6041704064
## ✅ All tests passed ✅
🏷️ Commit: 0aa7bf735f4983c5fa0caf0c8fae4081ce0d5308
▶️ Tests: 51970 executed
⚪️ Checks: 56/56 completed
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix configuration snapshot merges with Spring profile metadata [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16547:
URL: https://github.com/apache/grails-core/pull/16547#issuecomment-6046487927
## ✅ All tests passed ✅
🏷️ Commit: a643247dd2303f0edaffa55caba891a0015f3a9b
▶️ Tests: 51979 executed
⚪️ Checks: 56/56 completed
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix configuration snapshot merges with Spring profile metadata [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty commented on code in PR #16547:
URL: https://github.com/apache/grails-core/pull/16547#discussion_r4210770052
##########
grails-core/src/main/groovy/org/grails/config/yaml/YamlPropertySourceLoader.java:
##########
@@ -58,9 +61,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 profile = properties.getProperty("spring.config.activate.on-profile", "").trim();
Review Comment:
Fixed in a643247dd2. The matcher now collects the indexed entries (`key[0]`, `key[1]`, ...) alongside the scalar value for both `spring.config.activate.on-profile` and `spring.profiles`, and treats them as alternatives, matching Spring Boot's `String[]` binding. Each entry may be a profile expression; blank entries are ignored, so a sequence restricts the document unless every entry is blank. Unrelated indexed keys such as `spring.profiles.include[0]` are not treated as selectors.
Tests in `YamlSpringProfileSpec` cover flow and block sequences for both selectors in dotted and nested form (including your `[alpha, beta]` with only `gamma` active), case sensitivity, a sequence combined with a scalar selector, and an invalid expression inside a sequence. `ExternalConfigSpec` adds a `grails.config.locations` case. The new rows fail against the previous loader. The guide now documents sequence selectors.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix configuration snapshot merges with Spring profile metadata [grails-core]
Posted by "jamesfredley (via GitHub)" <gi...@apache.org>.
jamesfredley commented on code in PR #16547:
URL: https://github.com/apache/grails-core/pull/16547#discussion_r4214605124
##########
grails-core/src/main/groovy/org/grails/config/yaml/YamlPropertySourceLoader.java:
##########
@@ -58,9 +61,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 profile = properties.getProperty("spring.config.activate.on-profile", "").trim();
Review Comment:
Rechecked a643247dd2. The loader collects indexed sequence entries and evaluates them as alternative selectors. YamlSpringProfileSpec and ExternalConfigSpec fail if a sequence is ignored. Oracle and Codex both came back clean on this head.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix configuration snapshot merges with Spring profile metadata [grails-core]
Posted by "jamesfredley (via GitHub)" <gi...@apache.org>.
jamesfredley commented on code in PR #16547:
URL: https://github.com/apache/grails-core/pull/16547#discussion_r4214580080
##########
grails-core/src/main/groovy/org/grails/config/yaml/YamlPropertySourceLoader.java:
##########
@@ -58,9 +61,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 profile = properties.getProperty("spring.config.activate.on-profile", "").trim();
Review Comment:
Rechecked a643247dd2. The loader collects indexed sequence entries and evaluates them as alternative selectors. YamlSpringProfileSpec and ExternalConfigSpec fail if a sequence is ignored. Oracle and Codex both came back clean on this head.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix configuration snapshot merges with Spring profile metadata [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty commented on PR #16547:
URL: https://github.com/apache/grails-core/pull/16547#issuecomment-6029415955
@matrei It appears the exclusion logic broke functionality in 7. Do you know why you originally added it in 7?
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix configuration snapshot merges with Spring profile metadata [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16547:
URL: https://github.com/apache/grails-core/pull/16547#issuecomment-6030258300
## ✅ All tests passed ✅
🏷️ Commit: e57bd1359aaa5eb464d8a0ff89b21eead96bb1f4
▶️ Tests: 51655 executed
⚪️ Checks: 54/54 completed
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix configuration snapshot merges with Spring profile metadata [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty commented on code in PR #16547:
URL: https://github.com/apache/grails-core/pull/16547#discussion_r4221024911
##########
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:
Fixed in b92b61d8df. Once documents are selected, the loader removes `spring.config.activate.on-profile` and `spring.profiles` (scalar and indexed keys) before merging, so Boot no longer evaluates them again.
The new `GrailsAppYamlProfileSpec` starts `GrailsApp` against an `application.yml`. It covers `'!production'` with `grails.env=production`, `'!prod'` with `--spring.profiles.active=prod`, and selected legacy `spring.profiles` documents, both scalar and sequence. Before the change, the negated cases resolved `sample.base` to `null` and the legacy cases failed with `InvalidConfigDataPropertyException`.
##########
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:
Done in b92b61d8df. Scalar selectors are now split with `StringUtils.commaDelimitedListToStringArray`, and each entry is an alternative, as with a sequence. Blank entries are ignored. The `'dev,alpha'` rows now select the document, and I added rows such as `'beta, dev & alpha'` and `'!prod,!test'`. Sequence entries are not split further, which matches Boot's indexed binding. The guide paragraph now describes commas as alternatives.
##########
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:
Added a NOTE in b92b61d8df. It says only the `spring.profiles.active` system property affects document selection, lists `SPRING_PROFILES_ACTIVE`, `--spring.profiles.active` and the Grails environment name as sources that are not considered, and includes the `'!prod'` example.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix configuration snapshot merges with Spring profile metadata [grails-core]
Posted by "matrei (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org