codeconsole commented on code in PR #16732:
URL: https://github.com/apache/grails-core/pull/16732#discussion_r4373769110
##########
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:
Fixed in 6d2cdedad9. A map subscript written into a block went through the
block's `put`, which reads a dotted key as a path, so `app.routes[/v1.0/**]`
over a map from `application.groovy` wrote `/v1` -> `0/**` beside the entry and
left the entry on `old`. The index is now written as the key of the block as
written, in both representations, as it is into any other map.
`PropertySourcesConfigSpec` runs your sequence through a
`NavigableMapPropertySource` and asserts the property becomes `new`;
`NavigableMapSpec` asserts it for merge, put, subscript assignment, dot
notation and a write through the nested map.
987884c616 covers the other way a block can hold that key: as the path `/v1`
-> `0/**`, which is how a dotted key of `application.yml` is stored. There the
bracketed write updates the entry the bracketed read finds, not a second entry
under the same flat key.
The converse, a dotted key of `application.yml` over a key with a dot that a
map in `application.groovy` holds, leaves the two as separate entries in Grails
8 and on the base of this PR alike. A fix needs a rule for which form wins
across all three merge paths, so it is #16766 rather than part of this PR.
##########
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:
Fixed in 9552c45ed2, e88bf72988 and d5e63b1050. `toProperties()` and
`toFlatConfig()` name the entries of a map a subscript wrote by their bracketed
keys, `app.routes[/api/**]`, and write a map entry through its values only, as
`NavigableMapPropertySource` presents the map to Spring. e88bf72988 covers
`application.yml`: `PropertySourcesConfig` turned such a map into a block when
it merged the file's source, so the YAML form of `grails.cors.mappings` still
exported `./api/**`. d5e63b1050 drops the `{allowedOrigins[0]=...}` String that
an entry holding a map was also exported as, which Spring Boot bound in place
of the map. `YamlPropertySourceLoaderSpec` publishes the export to an
environment, as a `doWithConfig` block does, and asserts it binds to the same
map as the file.
Checking the export and the property source against Spring's `Binder` turned
up more of the same kind, each with a failing-first test:
- b472e32b5a: a key in brackets after another subscript, `app.rows[0][/a.b]`
or `app.grid[/x][/y.z]`, was presented to Spring as `app.rows[0]./a.b` and
bound as `a: [b: ...]`. It is now presented and exported in brackets, and a
name after a subscript, `app.rows[0].name`, stays dotted.
- 56cc9b19aa: two property sources that each write an entry into one element
of such a map both land in it, as Spring Boot binds them, and a write into the
config no longer reaches the property source's maps.
- c457d71af9: a dotted key, a map, or a profile's block written over a map a
subscript wrote, such as `app.routes.extra` from a profile over
`app.routes['[/api/**]']`, replaced the map and dropped its entries. It now
writes into the map, which stays bracketed. A name is a key of the map, or of a
map the map holds under a key the name starts with, as Spring Boot binds a map
of strings or of maps. So `grails.mail.props.mail.smtp.auth` writes the entry
`'[mail.smtp.auth]'` wrote, and `...mail.smtp.auth.mechanisms` writes a second
one beside it.
- 2466252e39 and e56f324f7e: the environment answers each key of such a map
by the name the file wrote it under, dotted or bracketed. A whole-number key,
or one holding `]`, is named in dot notation, since in brackets it would name a
list index or end early.
--
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]
