codeconsole commented on code in PR #16771:
URL: https://github.com/apache/grails-core/pull/16771#discussion_r4395783432
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -171,6 +177,158 @@ 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. Each element that is a map is held as a
nested map, which the owner also holds
+ * under the indexed name, {@code rows[0]}, and which holds the element as
application.yml gives it. 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.
+ */
+ 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)
+ String written = indexedKey.substring(subscriptStart + 1, subscriptEnd)
+ String writtenField = indexedKey.substring(subscriptEnd + 2)
+ if (container instanceof List) {
+ List flatList = (List) container
+ List held = new HeldList()
+ for (int i = 0; i < flatList.size(); i++) {
+ String index = String.valueOf(i)
+ held.add(heldElement(targetMap, owner, containerKey, name,
index, flatList.get(i), index == written ? writtenField : null))
+ }
+ 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()) {
+ String key = String.valueOf(entry.key)
+ held.put(key, heldElement(targetMap, owner, containerKey,
name, key, entry.value, key == written ? writtenField : null))
+ }
+ owner.put(name, held)
+ targetMap.put(containerKey, held)
Review Comment:
Fixed in f02b0f1287. The flat keys are left as 7.0.x wrote them, and only
the owner holds the held list or map. `grails.cors.mappings` is the plain map
again, so Spring Boot binds the mapping.
`GrailsCorsConfigurationSpec` in `grails-web-url-mappings` now loads the
guide's YAML through the Grails `YamlPropertySourceLoader` and binds
`grails.cors` into `GrailsCorsConfiguration` with `Binder`. It checks that
`corsConfigurations` holds only `/api/**`, with `allowedOrigins`
`[http://localhost:5000]`. At 6ed67380ea it fails with the `/**` fallback.
`NavigableMapPropertySourceSpec` now checks that the flat keys of an indexed
list and map are listed and hold the plain list and map, as before.
To check that nothing else the environment sees has moved, I compared the
root entries a `NavigableMapPropertySource` presents against 7.0.x. The
comparison covered every sequence of up to three writes from eleven kinds:
indexed keys into a list and a map, a subscript after a subscript, lists merged
whole and as nested maps, dotted keys into a held map, a dotted property after
a map subscript, and scalar list entries. They are identical in all but 31 of
the 1463 sequences. Those 31 are the intended fix for `merge(['a.b.c.d':
'old'])` followed by `merge(['a[b].c.d': 'new'])`: the flat key `a.b.c.d` now
holds `'new'`.
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -171,6 +177,158 @@ 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. Each element that is a map is held as a
nested map, which the owner also holds
+ * under the indexed name, {@code rows[0]}, and which holds the element as
application.yml gives it. 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.
+ */
+ 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)
Review Comment:
Fixed in f02b0f1287. The held list is now built from the list the owner
already holds, which is the one written element by element from each source. It
is built from the flat key only when the owner holds no list.
"An indexed key over a list that a profile replaced keeps the rows below it"
in `PropertySourcesConfigSpec` uses your three sources. It asserts `[[name:
'dev', port: '9'], [name: 'second']]` from `navigate` and
`getProperty('app.rows', List)`, and `second` from
`getProperty('app.rows[1].name')`, and the same after `refresh()`. It fails at
6ed67380ea. `config['app.rows']` stays `[[name:dev, port:9]]`, as on 7.0.x.
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -171,6 +177,158 @@ 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. Each element that is a map is held as a
nested map, which the owner also holds
+ * under the indexed name, {@code rows[0]}, and which holds the element as
application.yml gives it. 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.
+ */
+ 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)
+ String written = indexedKey.substring(subscriptStart + 1, subscriptEnd)
+ String writtenField = indexedKey.substring(subscriptEnd + 2)
+ if (container instanceof List) {
+ List flatList = (List) container
+ List held = new HeldList()
+ for (int i = 0; i < flatList.size(); i++) {
Review Comment:
Done in f02b0f1287. Once the owner holds the held list, an indexed key
updates only the element it writes. The element is usually the nested map the
merge has just written the field into, and then it is kept as it is. It is
rebuilt only when it is a plain map, such as an element of a list merged whole,
or when the owner's nested map for it belongs to an element the list held
before it was replaced.
With your setup, n rows of `name`, `db.url`, `db.user` and `port` merged
into a new `NavigableMap`, averaged over 3 runs after 2 warm-ups:
| n | 7.0.x | 6ed67380ea | f02b0f1287 |
|---|---|---|---|
| 400 | 13 ms | 117 ms | 33 ms |
| 1,600 | 16.5 ms | 453 ms | 24 ms |
| 6,400 | 18 ms | 6,364 ms | 35 ms |
It is linear again. The remaining constant factor over 7.0.x comes from
keeping the nested element maps up to date. The 400 row is the first in its
JVM, so it carries some warm-up.
--
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]
