codeconsole commented on code in PR #16770:
URL: https://github.com/apache/grails-core/pull/16770#discussion_r4383231272
##########
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:
Done in d4f63ff354. The spec merges `[app: [name: 'demo']]` before the
placeholder, so without ca3943ae7f the export, the subscript and `getProperty`
all fail, not only the subscript. The description's example now merges into an
existing section too, so its `toProperties()` line shows the raw value it
really gave.
##########
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:
Done in d4f63ff354. The feature is now a `where:` with an "in a nested map"
row and an "as a dotted key" row, and each row asserts that `navigate`, the
export, the subscript and `getProperty` read one value. The read count stays in
its own feature, for a nested map only, which is where "resolved once" holds.
The commit message and the description both say that a dotted key is resolved
twice and that both places now hold the second result.
##########
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:
That's the line #16769 changes, with the same prefix (f54b426242), so I left
it to that PR rather than change it in both. I checked the two together by
cherry-picking f54b426242 onto this branch. After `config.merge([grails:
['gorm.url': 'http://${host}']])`, `config['grails.gorm.url']`,
`navigate('grails', 'gorm', 'url')`, `getProperty('grails.gorm.url')` and
`toProperties()` all give the same value, `getProperty('gorm.url')` is null,
and the root holds no `gorm.url`. The description now points to #16769 for this
shape.
--
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]
