jdaugherty commented on code in PR #16771:
URL: https://github.com/apache/grails-core/pull/16771#discussion_r4382482026
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -169,6 +171,28 @@ class NavigableMap implements Map<String, Object>,
Cloneable {
}
}
+ /**
+ * Holds the list or map that an indexed key with a property after the
index, such as {@code a.rows[0].name}, writes
+ * under its flat key, {@code a.rows}, in the nested map that owns it too,
so that {@code navigate('a', 'rows')} and
+ * {@code config.a.rows} read it; splitting the key at every dot wrote
only an entry named after the element there,
+ * {@code rows[0]}, which the flat keys of the element are read through
and which is left as it is.
+ */
+ private void holdIndexedContainer(NavigableMap targetMap, String
indexedKey) {
+ int subscriptStart = indexedKey.indexOf('[')
+ int subscriptEnd = subscriptStart > -1 ? indexedKey.indexOf(']',
subscriptStart) : -1
+ if (subscriptEnd < 0 || indexedKey.indexOf('.', subscriptEnd) < 0) {
+ return
+ }
+ String containerKey = indexedKey.substring(0, subscriptStart)
+ Object container = targetMap.get(containerKey)
+ int lastDot = containerKey.lastIndexOf('.')
+ NavigableMap owner = lastDot > -1 ?
targetMap.navigateSubMap(containerKey.substring(0, lastDot).tokenize('.'),
false) : targetMap
+ if (owner == null || owner.is(owner.getRootConfig()) || !(container
instanceof List || container instanceof Map)) {
Review Comment:
The map branch (the description's `app.mappings[foo].name`) has no test, and
since it holds the flat key's `LinkedHashMap`, a later dotted key under the
same container drops it again:
```groovy
config.merge(['app.mappings[foo].name': 'x', 'app.mappings.bar': 'y'])
config.navigate('app', 'mappings') // [bar:y]
config.get('app.mappings') // [foo:[name:x]]
```
`navigateSubMap` only descends into a `NavigableMap`, so `app.mappings.bar`
replaces the held map with a new one, and the nested map and the flat key
disagree as they did on 7.0.x. Written the other way round, both hold `foo` and
`bar`. A miss under it also throws now: `config.app.mappings.bar.deeper` after
merging only `app.mappings[foo].name` is a NullPointerException, where 7.0.x
returned a null-safe navigator.
Could you add a test for a map-keyed key, including a sibling dotted key
merged after it?
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -169,6 +171,28 @@ class NavigableMap implements Map<String, Object>,
Cloneable {
}
}
+ /**
+ * Holds the list or map that an indexed key with a property after the
index, such as {@code a.rows[0].name}, writes
+ * under its flat key, {@code a.rows}, in the nested map that owns it too,
so that {@code navigate('a', 'rows')} and
+ * {@code config.a.rows} read it; splitting the key at every dot wrote
only an entry named after the element there,
+ * {@code rows[0]}, which the flat keys of the element are read through
and which is left as it is.
+ */
+ private void holdIndexedContainer(NavigableMap targetMap, String
indexedKey) {
+ int subscriptStart = indexedKey.indexOf('[')
+ int subscriptEnd = subscriptStart > -1 ? indexedKey.indexOf(']',
subscriptStart) : -1
+ if (subscriptEnd < 0 || indexedKey.indexOf('.', subscriptEnd) < 0) {
+ return
+ }
+ String containerKey = indexedKey.substring(0, subscriptStart)
+ Object container = targetMap.get(containerKey)
+ int lastDot = containerKey.lastIndexOf('.')
+ NavigableMap owner = lastDot > -1 ?
targetMap.navigateSubMap(containerKey.substring(0, lastDot).tokenize('.'),
false) : targetMap
Review Comment:
When `containerKey` has no dot, the first `mergeMapEntry` in `mergeMaps` has
already put this same list under that name in `targetMap`, so the `: targetMap`
branch only writes back what is there, and the root check exists only for that
branch. `if (lastDot < 0) { return }` would cover both.
That is also why the "in a nested map" row in `NavigableMapSpec` passes on
7.0.x without this change, so it does not cover the fix.
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -169,6 +171,28 @@ class NavigableMap implements Map<String, Object>,
Cloneable {
}
}
+ /**
+ * Holds the list or map that an indexed key with a property after the
index, such as {@code a.rows[0].name}, writes
+ * under its flat key, {@code a.rows}, in the nested map that owns it too,
so that {@code navigate('a', 'rows')} and
+ * {@code config.a.rows} read it; splitting the key at every dot wrote
only an entry named after the element there,
+ * {@code rows[0]}, which the flat keys of the element are read through
and which is left as it is.
+ */
+ private void holdIndexedContainer(NavigableMap targetMap, String
indexedKey) {
+ int subscriptStart = indexedKey.indexOf('[')
+ int subscriptEnd = subscriptStart > -1 ? indexedKey.indexOf(']',
subscriptStart) : -1
+ if (subscriptEnd < 0 || indexedKey.indexOf('.', subscriptEnd) < 0) {
+ return
+ }
+ String containerKey = indexedKey.substring(0, subscriptStart)
+ Object container = targetMap.get(containerKey)
+ int lastDot = containerKey.lastIndexOf('.')
+ NavigableMap owner = lastDot > -1 ?
targetMap.navigateSubMap(containerKey.substring(0, lastDot).tokenize('.'),
false) : targetMap
+ if (owner == null || owner.is(owner.getRootConfig()) || !(container
instanceof List || container instanceof Map)) {
+ return
+ }
+ owner.put(containerKey.substring(lastDot + 1), container)
Review Comment:
This holds the flat key's list, whose elements are plain `LinkedHashMap`s
that keep the rest of the key as one flat entry. A property one level into an
element reads fine, but nothing deeper does, and a miss inside an element now
throws instead of returning a null-safe navigator. With a Boot-style source
(application.properties, system properties and command-line arguments all reach
`merge(map, true)` through `PropertySourcesConfig`):
```groovy
def sources = new MutablePropertySources()
sources.addFirst(new MapPropertySource('application.properties',
['app.rows[0].name': 'first', 'app.rows[0].db.url': 'jdbc:x',
'app.rows[1].name': 'second']))
def config = new PropertySourcesConfig(sources)
config.navigate('app', 'rows') // [[name:first, db.url:jdbc:x],
[name:second]]
config.app.rows[0].db.url // 7.0.x: NullSafeNavigator here:
NullPointerException
config.app.rows[0].db.url ?: 'default' // 7.0.x: 'default' here:
NullPointerException
config.app.rows[1].missing.deeper // 7.0.x: NullSafeNavigator here:
NullPointerException
```
The same list read from application.yml comes back with `NavigableMap`
elements (`[name:first, db:[url:jdbc:x], db.url:jdbc:x]`), so there
`config.app.rows[0].db.url` is `jdbc:x` and a miss stays null-safe. The
`rows[0]` entries this merge already builds in the owner hold each element in
that nested form (`rows[0]:[name:first, db:[url:jdbc:x]]`), which may be one
way to get there.
Could the held list give the same elements as application.yml does, with a
test for a property two levels into an element and for a miss inside one?
--
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]
