codeconsole commented on code in PR #16771:
URL: https://github.com/apache/grails-core/pull/16771#discussion_r4384564296
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -169,6 +171,84 @@ 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: a list or map of the nested maps that
splitting the key at every dot built there for
+ * each element, {@code rows[0]}, which hold the element as
application.yml gives it and are left as they are. The flat
+ * key {@code a.rows} then holds the same list or map, as the flat key of
a nested map holds that map, so a dotted key
+ * merged into it later is read through both. The flat keys of the
elements are left as they are.
+ */
+ 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)
+ int lastDot = containerKey.lastIndexOf('.')
+ if (lastDot < 0) {
+ // the write of the container has put it in the map that owns it
already
+ return
+ }
+ NavigableMap owner =
targetMap.navigateSubMap(containerKey.substring(0, lastDot).tokenize('.'),
false)
+ Object container = targetMap.get(containerKey)
+ if (owner == null) {
+ return
+ }
+ String name = containerKey.substring(lastDot + 1)
+ if (container instanceof List) {
+ List flatList = (List) container
+ List held = []
+ for (int i = 0; i < flatList.size(); i++) {
+ held.add(heldElement(owner.get("${name}[${i}]".toString()),
flatList.get(i)))
+ }
+ owner.put(name, held)
+ targetMap.put(containerKey, held)
+ } else if (container instanceof Map) {
+ Object current = owner.get(name)
+ NavigableMap held = current instanceof NavigableMap ?
(NavigableMap) current : new NavigableMap(owner.getRootConfig(),
(owner.getPath() + [name]).asImmutable())
+ for (Map.Entry entry in ((Map) container).entrySet()) {
+ held.put(String.valueOf(entry.key),
heldElement(owner.get("${name}[${entry.key}]".toString()), entry.value))
+ }
+ owner.put(name, held)
+ targetMap.put(containerKey, held)
+ }
+ }
+
+ /**
+ * Removes the keys that indexed keys and map subscripts wrote for the
entries of a list or map, such as
+ * {@code rows[0]} and {@code rows[0].name} for {@code rows}, so that a
subscript written after the list or map is
+ * replaced does not find the entries it replaced.
+ */
+ private static void removeSubscriptKeys(NavigableMap map, String key) {
+ String prefix = "${key}[".toString()
+ List<String> subscriptKeys = map.keySet().findAll { String k ->
k.startsWith(prefix) } as List<String>
+ for (String subscriptKey in subscriptKeys) {
+ map.remove(subscriptKey)
+ }
+ }
+
+ /**
+ * The element to hold: the nested map built for it, which also takes the
entries of the flat key's element, such as
+ * {@code db.url} beside {@code db}, as an element from application.yml
holds both, or the flat key's element itself.
+ */
+ private static Object heldElement(Object nested, Object flat) {
+ if (!(nested instanceof NavigableMap)) {
+ return flat
+ }
+ NavigableMap element = (NavigableMap) nested
+ if (flat instanceof Map) {
+ for (Map.Entry entry in ((Map) flat).entrySet()) {
+ String key = String.valueOf(entry.key)
+ if (!element.containsKey(key)) {
Review Comment:
Fixed in 38b6f4d134. The element an indexed key writes into is now held as a
nested map built again from the list's element, rather than filled in around
the nested map the indexed key built. After your two merges the element's `db`
holds `url`, `user` and `port`. `navigate`, the flat key `a.rows`,
`config.a.rows[0].db.url` and `getProperty('a.rows[0].db.url')` all read
`jdbc:new`; that last one was null on 7.0.x. "An indexed key written into a map
inside an element keeps the other entries of that map" in `NavigableMapSpec`
asserts the full `db`, and it fails before the commit.
Building the element from the list also covers your earlier three-step merge
without removing any keys. The owner's `rows[0]` is replaced with the rebuilt
element, so after it `getProperty('a.rows[0].name')` is `'new'` too.
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -271,6 +351,13 @@ class NavigableMap implements Map<String, Object>,
Cloneable {
}
targetMap.remove(sourceKey)
} else {
+ if (currentValue instanceof Collection || (currentValue
instanceof Map && !(sourceValue instanceof Map))) {
Review Comment:
Fixed in 38b6f4d134 by removing the cleanup, so the keys of a list's
elements stay as on 7.0.x. "An element of a list in application.yml is read by
its indexed key" in `PropertySourcesConfigSpec` loads your YAML and asserts
that `getProperty('app.rows[0].name')` is `first`,
`getProperty('app.rows[0].db.url')` is `jdbc:x`, and
`config.app.rows[0].db.url` reads the same. It fails before the commit.
Looking at how a YAML source is merged turned up a second problem from the
earlier commits, fixed in bed3ae249f. Each YAML source now carried the held
`rows` list in its nested `app` map, and merging a source replaced the list
below it. On 7.0.x the list was written element by element through `rows[0]`
and on. With `application-dev.yml` holding `rows: [{name: dev}]` over an
`application.yml` with two rows, `navigate('app', 'rows')` gave `[[name: dev]]`
instead of 7.0.x's `[[name: dev], [name: second]]`, and again after `refresh()`.
A held list now has its own list type, and merging a nested map skips it, so
the entries beside it write it as before. A list of values, such as `tags`, is
still merged as a whole, which gives `[dev]` as on 7.0.x. A `where:` in
`PropertySourcesConfigSpec` checks both against 7.0.x's results, before and
after `refresh()`.
One thing stays as on 7.0.x: after `merge(['a.rows[0].name': 'old'])` and
then `merge(['a.rows': [[name: 'new']]])` with no indexed write after it,
`getProperty('a.rows[0].name')` still answers `'old'`. Removing that key is
what broke the YAML case, and 7.0.x answers the same.
cf0cfd658b merges 7.0.x in now that #16769 has landed. Both modules' suites
pass.
--
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]
