jamesfredley commented on code in PR #16732:
URL: https://github.com/apache/grails-core/pull/16732#discussion_r4372863841
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -264,27 +688,33 @@ class NavigableMap implements Map<String, Object>,
Cloneable {
if (submap instanceof Map) {
return navigateMap((Map<String, Object>) submap, path.tail())
}
- return submap
+ return null
}
}
/**
* Sets a value in a list (numeric index) or map (other index) at the
given subscript, creating
* the container when the current value is not one. The text following the
subscript is either
* empty, further subscripts such as {@code [1]} for a list nested in a
list, or a {@code .name}
- * remainder stored as a key of a map element.
+ * remainder stored as a key of a map element. A container that already
holds the object is left
+ * as it is, so a merge of the same entries again, as a config merges its
property sources again,
+ * writes nothing into it. A container that refuses the write, such as a
list {@code application.groovy}
+ * built with {@code asImmutable()}, is replaced by a copy that holds it.
*/
private Object setSubscriptValue(Object currentValue, String index, String
rest, Object sourceValue) {
if (index.isNumber()) {
List list = currentValue instanceof List ? (List) currentValue : []
int i = index.toInteger()
- fill(list, i, null)
- list.set(i, subscriptElementValue(list.get(i), rest, sourceValue))
- return list
+ boolean held = i < list.size()
+ Object element = held ? list.get(i) : null
+ Object newElement = subscriptElementValue(element, rest,
sourceValue)
+ if (held && element.is(newElement)) {
+ return list
+ }
+ return writeElement(list, i, newElement)
}
- Map nestedMap = currentValue instanceof Map ? (Map) currentValue : [:]
- nestedMap.put(index, subscriptElementValue(nestedMap.get(index), rest,
sourceValue))
- return nestedMap
+ Map map = currentValue instanceof Map ? (Map) currentValue : [:]
Review Comment:
After `app.routes[/v1.0/**]=old`, merging `['app.routes[/v1.0/**]': 'new']`
leaves `getProperty('app.routes[/v1.0/**]')` as `old`. The write treats the dot
in the bracket as a path separator.
Keep that index literal. Please assert the property becomes `new`.
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -192,41 +357,239 @@ class NavigableMap implements Map<String, Object>,
Cloneable {
else {
subMap = new NavigableMap(targetMap.getRootConfig(),
newPathList.asImmutable())
if (currentValue instanceof Map) {
- subMap.putAll((Map) currentValue)
+ subMap.delegateMap.putAll((Map) currentValue)
}
}
String newPath = path ? "${path}.${sourceKey}" : sourceKey
- mergeMaps(rootMap, newPath , subMap, (Map) sourceValue,
parseFlatKeys)
+ // the entries of a map written in dot notation are written as
dot notation writes them, so a value
+ // replaces a block of the map it is merged into and a null
removes it
+ mergeMaps(rootMap, newPath, subMap, (Map) sourceValue,
parseFlatKeys, isNestedSet)
newValue = subMap
} else {
- newValue = sourceValue
+ newValue = merged(sourceValue)
}
if (isNestedSet && newValue == null) {
- if (path) {
-
- def subMap = rootMap.get(path)
- if (subMap instanceof Map) {
- subMap.remove(sourceKey)
- }
- def keysToRemove = rootMap.keySet().findAll() { String key
->
- key.startsWith("${path}.")
- }
- for (key in keysToRemove) {
- rootMap.remove(key)
- }
+ // the flat key of the removed property and those below it go
with it; the flat keys of its siblings stay
+ String removedKey = path ? "${path}.${sourceKey}".toString() :
sourceKey
+ removeKeysBelow(rootMap, removedKey)
+ if (rootMap != null) {
+ rootMap.delegateMap.remove(removedKey)
}
- targetMap.remove(sourceKey)
+ targetMap.delegateMap.remove(sourceKey)
} else {
- if (path) {
- rootMap.put("${path}.${sourceKey}".toString(), newValue)
+ if (isNestedSet && !(newValue instanceof NavigableMap) &&
targetMap.delegateMap.get(sourceKey) instanceof NavigableMap) {
+ // a value written over a block replaces it, so no flat
key of its entries is read any more; a merge
+ // keeps them, as the environment reads a key a
lower-precedence property source sets
+ removeKeysBelow(rootMap, path ?
"${path}.${sourceKey}".toString() : sourceKey)
+ }
+ if (path && rootMap != null) {
+ mergeMapEntry(rootMap, "${path}.${sourceKey}".toString(),
newValue)
}
mergeMapEntry(targetMap, sourceKey, newValue)
}
}
}
+ /** Removes the flat keys below a flat key, those of the entries of the
block it named. */
+ private void removeKeysBelow(NavigableMap rootMap, String flatKey) {
+ if (rootMap == null) {
+ return
+ }
+ String prefix = flatKey + '.'
+ List<String> keysBelow = rootMap.keySet().findAll { String key ->
key.startsWith(prefix) } as List<String>
+ for (String key in keysBelow) {
+ rootMap.delegateMap.remove(key)
+ }
+ }
+
+ /** The index of the first subscript of a key such as {@code
rows[0].name}, or -1 when the key has none. */
+ private static int subscriptStart(String key) {
+ int start = key.indexOf('[')
+ start > -1 && key.indexOf(']', start) > start ? start : -1
+ }
+
+ /**
+ * Merges a key with a subscript, such as {@code rows[0].name}, {@code
groups[0][1]} or {@code mappings[/api/**]}.
+ * The container named before the first subscript, a list for a numeric
index and a map otherwise, is held by
+ * the nested map that owns it and, under its flat key, by the root, as
one object in both places. Nothing is
+ * held per element: {@code a.rows[0]} and {@code a.rows[0].name} are read
through the container, so they
+ * answer from the list as it is when they are read. The dots of the
container's name are a path only when
+ * the merge parses flat keys, as for any other key.
+ */
+ private void mergeIndexedEntry(NavigableMap rootMap, String path,
NavigableMap targetMap, String containerName,
+ String subscripts, Object sourceValue,
boolean parseFlatKeys, boolean isNestedSet) {
+ NavigableMap containerParent = targetMap
+ String containerKey = containerName
+ if (parseFlatKeys && containerName.contains('.')) {
+ List<String> nameParts = containerName.split(/\./) as List<String>
+ containerParent = targetMap.navigateSubMap(nameParts[0..-2], true,
rootMap)
+ containerKey = nameParts[-1]
+ }
+ String containerPath = path ? "${path}.${containerName}".toString() :
containerName
+ int subscriptEnd = subscripts.indexOf(']')
+ Object previousContainer = containerParent.get(containerKey)
+ Object value = merged(sourceValue)
+ Object container = setSubscriptValue(previousContainer,
subscripts.substring(1, subscriptEnd),
+ subscripts.substring(subscriptEnd + 1), value)
+ if (isNestedSet && previousContainer instanceof NavigableMap &&
!container.is(previousContainer)) {
+ // a list written over a block replaces it, as a value written
over a block does
+ removeKeysBelow(rootMap, containerPath)
+ }
+ mergeMapEntry(containerParent, containerKey, container)
Review Comment:
`toProperties()` no longer exports `app.routes[/api/**]`. It exports
`app.routes./api/**` instead, so Spring binds the key as `api`.
Export the bracketed key, as a list index is exported.
--
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]
