jdaugherty commented on code in PR #16771:
URL: https://github.com/apache/grails-core/pull/16771#discussion_r4394906425
##########
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:
This takes the container from the flat key. After a profile's
application.yml, the flat key and the nested list can hold different lists, and
rebuilding the nested list from the flat key drops elements.
`PropertySourcesConfig` merges a `NavigableMapPropertySource` with
`parseFlatKeys` false. The flat key `app.rows` takes the profile's list whole,
while the nested list is written element by element through `rows[0]` and on.
An indexed key from a source above them then rebuilds the nested list from the
shorter list:
```groovy
def sources = new MutablePropertySources()
sources.addFirst(yaml('application.yml', "app:\n rows:\n - name: first\n
- name: second\n"))
sources.addFirst(yaml('application-dev.yml', "app:\n rows:\n - name:
dev\n"))
sources.addFirst(new MapPropertySource('commandLineArgs',
['app.rows[0].port': '9']))
def config = new PropertySourcesConfig(sources)
config.navigate('app', 'rows') // 7.0.x: [[name:dev, port:9],
[name:second]] here: [[name:dev, port:9]]
config.getProperty('app.rows', List) // 7.0.x: [[name:dev, port:9],
[name:second]] here: [[name:dev, port:9]]
config.getProperty('app.rows[1].name') // 'second' on both
```
`yaml(...)` loads the text with `YamlPropertySourceLoader`. The result is
the same after `refresh()`.
Without the override, `config['app.rows']` is `[[name:dev]]` while
`navigate` gives both rows, on 7.0.x and here. So after a profile's list, the
flat key is not a reliable source for the container.
When the owner already holds a list, could the held list be built from that
list? Please add a test with these three sources.
##########
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:
This makes the flat key of a map written by subscripts a `NavigableMap`.
`NavigableMapPropertySource` leaves such a key out of `getPropertyNames()` and
returns null for it from `getProperty()`, so in a Grails `application.yml`
Spring Boot no longer sees the documented CORS mapping. The example from
`cors.adoc`:
```yaml
grails:
cors:
enabled: true
allowedHeaders:
- Content-Type
mappings:
'[/api/**]':
allowedOrigins:
- http://localhost:5000
```
I loaded it with the Grails `YamlPropertySourceLoader`, which a Grails app
uses for application.yml, and bound it with
`Binder.get(environment).bind('grails.cors',
Bindable.of(GrailsCorsConfiguration))`:
| | `mappings` | `corsConfigurations` |
|---|---|---|
| 7.0.x | `[/api/**:[allowedOrigins[0]:http://localhost:5000]]` | `/api/**`
→ `[http://localhost:5000]` |
| here | `[:]` | `/**` → `[*]` |
With no mappings, `getCorsConfigurations()` falls back to `/**` with the
global defaults. CORS goes from one path and one origin to every path and any
origin.
The YAML is flattened to `grails.cors.mappings[/api/**].allowedOrigins[0]`,
a subscript after a subscript, and the merge writes no flat key for that. So on
7.0.x the only property Binder could bind the mapping from was
`grails.cors.mappings`, whose value was the plain map. The
`NavigableMapPropertySourceSpec` feature passes because its leaf, `name`, is a
plain key and does get a flat key. With a scalar, `allowedOrigins:
http://localhost:5000`, both branches bind `/api/**`.
Could the mapping stay visible to Binder, with the spec also binding this
YAML (a list inside a bracketed map entry)?
##########
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:
Each indexed key builds the whole held list again: a new `HeldList`, plus an
indexed name and an `owner.put` for every element. So a merge costs keys ×
elements. I merged a Boot-style source with n rows of 4 fields each (`name`,
`db.url`, `db.user`, `port`) into a new `NavigableMap`, averaging 3 runs after
2 warm-ups:
| n | 7.0.x | here |
|---|---|---|
| 400 | 6.5 ms | 54 ms |
| 1,600 | 16.5 ms | 530 ms |
The Grails `YamlPropertySourceLoader` goes through the same merge, so
loading an application.yml with 400 such rows went from about 11–15 ms to 49
ms. When the owner already holds the held list, could the merge update only the
element just written?
--
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]
