jdaugherty commented on code in PR #16770:
URL: https://github.com/apache/grails-core/pull/16770#discussion_r4382487693
##########
grails-core/src/test/groovy/org/grails/config/PropertySourcesConfigSpec.groovy:
##########
@@ -116,4 +117,38 @@ class PropertySourcesConfigSpec extends Specification {
then: 'the second value is not merged with the first value'
cfg.foo.bar == ['two': '2']
}
+
+ void 'a placeholder in a merged value is resolved once, for the flat key
and the nested entry alike'() {
+ given: "a property source that answers a new value each time it is
read"
+ def counter = new CountingPropertySource()
+ def config = new PropertySourcesConfig(counter)
+
+ when: "a map holding a placeholder is merged"
+ config.merge([app: [url: 'http://${host}']])
+
+ then: "the dotted key, the nested map and the export all read the one
resolved value"
+ config.getProperty('app.url') == 'http://host1'
+ config['app.url'] == 'http://host1'
+ config.navigate('app', 'url') == 'http://host1'
+ config.toProperties().getProperty('app.url') == 'http://host1'
Review Comment:
On 7.0.x this assertion already passes, and so does the `getProperty` one:
only `config['app.url']` fails without the fix. On a fresh merge the flat
`app.url` key is put into the root before `app`, so `toProperties()` writes the
raw value first and then overwrites it with the nested one. The export only
came out raw when the section already existed:
```groovy
config.merge([app: [name: 'x']])
config.merge([app: [url: 'http://${host}']])
config.toProperties().getProperty('app.url') // 'http://${host}' on 7.0.x,
'http://host1' here
```
Could the spec merge into an existing `app` section first, so the export
assertion also fails on the old code? The `toProperties()` line in the
description has the same caveat: for the example as written it was already
`'http://example.com'`.
##########
grails-core/src/test/groovy/org/grails/config/PropertySourcesConfigSpec.groovy:
##########
@@ -116,4 +117,38 @@ class PropertySourcesConfigSpec extends Specification {
then: 'the second value is not merged with the first value'
cfg.foo.bar == ['two': '2']
}
+
+ void 'a placeholder in a merged value is resolved once, for the flat key
and the nested entry alike'() {
+ given: "a property source that answers a new value each time it is
read"
+ def counter = new CountingPropertySource()
+ def config = new PropertySourcesConfig(counter)
+
+ when: "a map holding a placeholder is merged"
+ config.merge([app: [url: 'http://${host}']])
+
+ then: "the dotted key, the nested map and the export all read the one
resolved value"
+ config.getProperty('app.url') == 'http://host1'
+ config['app.url'] == 'http://host1'
+ config.navigate('app', 'url') == 'http://host1'
+ config.toProperties().getProperty('app.url') == 'http://host1'
+ counter.readCount() == 1
Review Comment:
This holds for a nested map, but not for a dotted key, the shape a flattened
config arrives in (`ConfigObject.flatten()`, property sources).
`config.merge(['app.url': 'http://${host}'])` resolves the placeholder twice,
once for the root `app.url` entry and once in the nested pass, and the second
result ends up in both places: `host2` from every read, `readCount() == 2`. So
the two places agree, which is the point of the fix, but "resolved once" in the
feature name and the commit message only holds for nested maps.
Could the spec also cover the dotted-key shape, for example as a `where:`
row, asserting that the four reads agree rather than counting the reads?
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -271,10 +271,12 @@ class NavigableMap implements Map<String, Object>,
Cloneable {
}
targetMap.remove(sourceKey)
} else {
+ mergeMapEntry(targetMap, sourceKey, newValue)
if (path) {
- rootMap.put("${path}.${sourceKey}".toString(), newValue)
+ // the flat key holds the value the nested entry holds, as
the hook stored it, so a placeholder in it
+ // is resolved once, for both
+ rootMap.put("${path}.${sourceKey}".toString(),
targetMap.get(sourceKey))
Review Comment:
Not caused by this change, but it's the one shape where the flat key and the
nested entry still disagree after it, so noting it here (the line is 162,
outside the diff): a dotted key inside a nested map. The nested pass gets
`pathParts.join('.')` as its path without the enclosing `path`, so for
```groovy
config.merge([grails: ['gorm.url': 'http://${host}']])
```
`grails.gorm.url` keeps the first resolution, the nested entry gets the
second, and the second also lands under a stray root key:
```groovy
config['grails.gorm.url'] // 'http://host1'
config.navigate('grails', 'gorm', 'url') // 'http://host2'
config.getProperty('gorm.url') // 'http://host2'
```
Prefixing the enclosing path at line 162 (`path ?
"${path}.${pathParts.join('.')}".toString() : pathParts.join('.')`) makes all
three agree and drops the stray key, and the grails-bootstrap and grails-core
tests pass with it. 8.0.x has the same code. YAML doesn't reach this, since
Spring flattens it before the merge; it takes a nested map holding a dotted key
passed to `merge`. Fine as a follow-up issue if you'd rather keep this PR to
the one line.
--
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]
