jdaugherty commented on code in PR #16771:
URL: https://github.com/apache/grails-core/pull/16771#discussion_r4417366010
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -171,6 +177,205 @@ 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
+ * in the nested map that owns it, so that {@code navigate('a', 'rows')}
and {@code config.a.rows} read it. The list
+ * the owner already holds is the one held, as written element by element
from application.yml or by a profile's
+ * list over it; only when it holds none is it built from the list under
the flat key. An 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. Only the element just written is
updated, so each indexed key costs the same
+ * however long the list is. The flat keys are left as they are, so the
environment presents the same names and
+ * values as before.
+ */
+ 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
+ }
Review Comment:
This returns when the container has no dot, on the grounds that the owner
already holds it. What the owner holds there is the plain list the subscript
wrote, an `ArrayList` of `LinkedHashMap`s. At the config root that is what
7.0.x held too. Inside an element it is new: `heldElement` merges the dotted
field into the element's nested map, and that inner merge lands here, so
`rows[0]` holds `cols` as the plain list beside the `cols[0]` nested map. On
7.0.x the element had no `cols` entry at all, so every read below it was a
null-safe navigator. Now a miss two levels in throws, from application.yml as
well:
```yaml
app:
rows:
- cols:
- name: c0
db:
url: x
- name: c1
- name: r1
```
```groovy
config.app.rows[0].cols[1].missing.deeper ?: 'default' // 7.0.x: 'default'
here: NullPointerException
config.app.rows[0].cols[0].name // 7.0.x:
null-safe navigator here: 'c0'
```
With the same list from application.properties (`app.rows[0].cols[0].name`,
`app.rows[0].cols[0].db.url`, `app.rows[0].cols[1].name`, `app.rows[1].name`),
the plain element holds `db.url` as one entry, so a read two levels into it
throws too:
```groovy
config.app.rows[0].cols[0].db.url ?: 'default' // 7.0.x: 'default' here:
NullPointerException
```
For the same reason `getProperty('app.rows', List)[0]` differs between the
two sources here. Could the containers of an element be held the same way, when
the map the key is merged into is not the config root (its path is not empty),
with a test for a nested list from both sources and a miss two levels in?
##########
grails-core/src/main/groovy/org/grails/config/NavigableMapConfig.java:
##########
@@ -330,6 +339,14 @@ private <T> T convertValueIfNecessary(Object
originalValue, Class<T> targetType,
if (originalValue instanceof NavigableMap &&
targetType.equals(Map.class)) {
final IdentityHashMap<NavigableMap, Map<Object, Object>>
cache = new IdentityHashMap<>();
return (T) convertToMap((NavigableMap) originalValue,
cache);
+ } else if (originalValue instanceof List &&
Collection.class.isAssignableFrom(targetType) && holdsNavigableMap((List<?>)
originalValue)) {
+ // the maps of a list are given as plain maps, as a map is
+ final IdentityHashMap<NavigableMap, Map<Object, Object>>
cache = new IdentityHashMap<>();
+ final List<Object> converted = new ArrayList<>();
+ for (Object o: (List<?>) originalValue) {
+ converted.add(o instanceof NavigableMap ?
convertToMap((NavigableMap) o, cache) : o);
+ }
+ return (T) converted;
Review Comment:
This runs only inside `targetType.isInstance(originalValue)`, so `List` and
`Collection` get plain maps while `Set`, `LinkedList`, `Iterable` and `Object`
get the `NavigableMap` elements, through the conversion service or as they are.
For a list written by indexed keys all of these gave the plain maps of the flat
key on 7.0.x:
```groovy
config.merge(['app.rows[0].name': 'first', 'app.rows[1].name': 'second'])
config.getProperty('app.rows', List)[0].getClass() // LinkedHashMap on
both
config.getProperty('app.rows', Set)[0].getClass() // 7.0.x:
LinkedHashMap here: NavigableMap
config.getProperty('app.rows', Object)[0].getClass() // 7.0.x:
LinkedHashMap here: NavigableMap
```
Smaller than the two above: a line in the release note would do, or the
conversion could run before the type check.
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -171,6 +177,205 @@ 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
+ * in the nested map that owns it, so that {@code navigate('a', 'rows')}
and {@code config.a.rows} read it. The list
+ * the owner already holds is the one held, as written element by element
from application.yml or by a profile's
+ * list over it; only when it holds none is it built from the list under
the flat key. An 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. Only the element just written is
updated, so each indexed key costs the same
+ * however long the list is. The flat keys are left as they are, so the
environment presents the same names and
+ * values as before.
+ */
+ 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)
+ if (owner == null) {
+ return
+ }
+ String name = containerKey.substring(lastDot + 1)
+ String written = indexedKey.substring(subscriptStart + 1, subscriptEnd)
+ String writtenField = indexedKey.substring(subscriptEnd + 2)
+ Object container = targetMap.get(containerKey)
+ Object current = owner.get(name)
+ if (container instanceof List) {
+ if (!written.isInteger()) {
+ return
+ }
+ HeldList held
+ if (current instanceof HeldList) {
+ held = (HeldList) current
+ } else {
+ boolean fromFlatKey = !(current instanceof List)
+ List source = fromFlatKey ? (List) container : (List) current
+ held = new HeldList()
+ for (int i = 0; i < source.size(); i++) {
+ String index = String.valueOf(i)
+ Object element = fromFlatKey ? nestedOrElement(owner,
"${name}[${index}]".toString(), source.get(i)) : source.get(i)
+ held.add(index == written ? element :
heldElement(targetMap, owner, containerKey, name, index, element, null))
+ }
+ }
+ int index = written.toInteger()
+ Object element = index < held.size() ? held.get(index) : null
+ if (element == null && index < ((List) container).size()) {
+ element = ((List) container).get(index)
+ }
+ fill(held, index, null)
+ held.set(index, heldElement(targetMap, owner, containerKey, name,
written, element, writtenField))
+ owner.put(name, held)
+ } else if (container instanceof Map) {
+ NavigableMap held
+ if (current instanceof NavigableMap) {
+ held = (NavigableMap) current
+ } else {
+ boolean fromFlatKey = !(current instanceof Map)
+ Map source = fromFlatKey ? (Map) container : (Map) current
+ held = new NavigableMap(owner.getRootConfig(),
(owner.getPath() + [name]).asImmutable())
+ for (Map.Entry entry in ((Map) source).entrySet()) {
+ String key = String.valueOf(entry.key)
+ Object element = fromFlatKey ? nestedOrElement(owner,
"${name}[${key}]".toString(), entry.value) : entry.value
+ held.put(key, key == written ? element :
heldElement(targetMap, owner, containerKey, name, key, element, null))
+ }
+ }
+ Object element = held.containsKey(written) ? held.get(written) :
((Map) container).get(written)
+ held.put(written, heldElement(targetMap, owner, containerKey,
name, written, element, writtenField))
+ owner.put(name, held)
+ }
+ }
+
+ /**
+ * The element to hold, which the owner also holds under its indexed name,
{@code rows[0]}. An element that is a
+ * nested map is kept, and so is the nested map the owner holds for the
element just written when it is that same
+ * map, since the merge has written the field into it, or when the element
is a value that is not a map, which the
+ * map the indexed key wrote replaces, as {@code rows[1].name} replaces
{@code 'two'}. Otherwise an element that is a map is held as a nested map
+ * built from it, so that it holds every field the element holds, such as
{@code db.url} beside the {@code db.port}
+ * just written below it, and none of an element the list held before it
was replaced. The field just written then
+ * takes the value the owner's nested map holds for it, which the merge
stored through
+ * {@link #mergeMapEntry(NavigableMap, String, Object)}, so a placeholder
in it is resolved as in every other entry,
+ * and the keys of the map the indexed key was merged into that held the
element, or a map inside it, such as
+ * {@code a.rows[0]} and {@code a.rows[0].db}, then hold the rebuilt
element and its maps.
+ */
+ private static Object heldElement(NavigableMap targetMap, NavigableMap
owner, String containerKey, String name, String index,
+ Object element, String writtenField) {
+ String indexedName = "${name}[${index}]".toString()
+ Object written = writtenField != null ? owner.get(indexedName) : null
+ if (written instanceof NavigableMap && (element == null ||
element.is(written) || !(element instanceof Map))) {
+ Object stored = valueAt(written, writtenField)
+ if (stored != null && writtenField.indexOf('.') > -1) {
+ ((NavigableMap) written).merge([(writtenField): stored], true)
+ }
+ return written
+ }
Review Comment:
When the subscript contains a dot, this branch keeps the held element
without the field just written. `mergeMaps` splits the flat key on every dot,
so `navigateSubMap` writes the field into `mappings[/api/v1` and `0/**]`, while
the element is held under `mappings[/api/v1.0/**]`. The first field rebuilds
the element from the flat element, which holds that field. For every later
field `valueAt(written, writtenField)` is null, so the element stays as it was.
`getProperty(key, Map)` and dot notation now find the held map before the flat
key, so they give a partial map where 7.0.x gave the whole one. With a
Boot-style source:
```properties
grails.cors.mappings[/api/v1.0/**].allowedOrigins[0]=http://a
grails.cors.mappings[/api/v1.0/**].allowCredentials=true
grails.cors.mappings[/api/v1.0/**].maxAge=10
```
| | `getProperty('grails.cors.mappings', Map)['/api/v1.0/**'].keySet()` |
|---|---|
| 7.0.x | `[allowedOrigins[0], allowCredentials, maxAge]` |
| here | `[allowedOrigins, allowedOrigins[0]]` |
| here, keys in reverse order | `[maxAge]` |
`config.grails.cors.mappings['/api/v1.0/**']` reads the same partial map,
while `config['grails.cors.mappings']` still holds all three. The same keys
with `[/api/**]` hold all three. From application.yml the map comes out
complete only because the config's merge of the flat key fills the same nested
map afterwards. When the owner's nested map has no value for the field, could
it be taken from the flat element, with a test for a bracketed key with a dot
and two fields under it?
--
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]
