jdaugherty commented on code in PR #16769:
URL: https://github.com/apache/grails-core/pull/16769#discussion_r4382494379
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -159,7 +159,9 @@ class NavigableMap implements Map<String, Object>,
Cloneable {
def pathParts = keyParts[0..-2]
Map actualTarget = targetMap.navigateSubMap(pathParts as
List, true)
sourceKey = keyParts[-1]
- mergeMapEntry(rootMap, pathParts.join('.'), actualTarget,
sourceKey, sourceValue, parseFlatKeys)
+ // the flat key of the entry is its full path: that of the
map holding the dotted key, then the key
+ String nestedPath = path ?
"${path}.${pathParts.join('.')}" : pathParts.join('.')
+ mergeMapEntry(rootMap, nestedPath, actualTarget,
sourceKey, sourceValue, parseFlatKeys)
Review Comment:
`nestedPath` is also the path that the `sourceValue instanceof Map` branch
of `mergeMapEntry` recurses with, so this changes more than the scalar case the
specs cover. On 7.0.x:
- `merge([x: ['a.b': [c: 'v']]], true)` also wrote root keys `a.b` and
`a.b.c`
- `merge([x: ['a.b': ['c.d': 'v']]], true)` also wrote `a.b`, `a.b.c.d` and
`c.d`
- `merge([x: ['a.b.c': 'v']], true)` also wrote `a.b.c`
With this change none of those keys are written, and root `x.a.b` is now the
same map instance that `navigate('x', 'a', 'b')` returns (before, it was a
different one). Could you cover these too, for example as a `where:` table in
`NavigableMapSpec` that asserts the root key set?
##########
grails-core/src/test/groovy/org/grails/config/PropertySourcesConfigSpec.groovy:
##########
@@ -116,4 +116,21 @@ class PropertySourcesConfigSpec extends Specification {
then: 'the second value is not merged with the first value'
cfg.foo.bar == ['two': '2']
}
+
+ void 'a dotted key inside a nested map is exported under its full path and
overwrites no other property'() {
+ given: "a map holding a property and, in a nested map, a dotted key
that alone names the same property"
+ def config = new PropertySourcesConfig([a: [b: 'root'], x: ['a.b':
'nested'], logging: [level: ['org.springframework': 'DEBUG']]])
+
+ expect: "each property is read and exported under its own name"
+ config.getProperty('a.b') == 'root'
Review Comment:
With the `NavigableMap` change reverted, this feature still passes this line
and line 128: `getProperty('a.b')` and `toProperties().getProperty('a.b')` both
answered `root` on 7.0.x as well. `getProperty` walks the nested maps first,
and `toProperties()` visits the `a` map after the flat `a.b` entry, so the
nested value wins there too. The feature only fails at line 133, on the
`logging` half. The read the overwrite did break is `config['a.b']`, which
answered `nested`. Could you add `config['a.b'] == 'root'` here, so the
overwrite half fails without the fix as well?
The overwrite does reach `getProperty` when `a` has no `b` of its own. On
7.0.x, `new PropertySourcesConfig([a: [c: '1'], x: ['a.b': 'nested']])`
answered `nested` for `getProperty('a.b')`, `toProperties().getProperty('a.b')`
and `navigate('a', 'b')`; with this PR all three answer `null`. That would make
a good second case.
--
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]
