codeconsole commented on code in PR #16732:
URL: https://github.com/apache/grails-core/pull/16732#discussion_r4381922753
##########
grails-core/src/main/groovy/org/grails/config/NavigableMapPropertySource.groovy:
##########
@@ -42,62 +44,97 @@ import org.springframework.util.StringUtils
@CompileStatic
class NavigableMapPropertySource extends MapPropertySource {
- final String[] propertyNames
- final String[] navigablePropertyNames
-
- private final Set<String> indexedLists
- private final Map<String, Object> elementProperties
+ private final Map<String, Object> wrappedElements
NavigableMapPropertySource(String name, NavigableMap source) {
super(name, source)
- Set<String> lists = new LinkedHashSet<>()
- Map<String, Object> elements = new LinkedHashMap<>()
+ Map<String, Object> wrapped = new LinkedHashMap<>()
for (String key : source.keySet()) {
- Object value = unwrap(source.get(key))
- if (value instanceof List && !((List) value).isEmpty()) {
- lists << key
- flatten(elements, key, value)
+ Object held = source.get(key)
+ if (held instanceof OriginTrackedValue && isList(unwrap(held))) {
+ // a list held wrapped whole cannot be stepped into or changed
through the configuration, so its
+ // elements are presented as they were when this source was
made
+ flatten(wrapped, key, unwrap(held))
}
}
- this.indexedLists = lists
- this.elementProperties = elements
+ this.wrappedElements = wrapped
+ }
+ /**
+ * The names of the values the source holds and of the elements of its
lists, read from the source on each
+ * call: a block is presented through its entries, which the source holds
under their own flat keys, a list
+ * through its elements and a map written by a map subscript, such as
{@code grails.cors.mappings[/api/**]},
+ * through its entries under their bracketed keys, so none of the three is
a name.
+ */
+ @Override
+ String[] getPropertyNames() {
Review Comment:
Gone with the split: `NavigableMapPropertySource` is 9.0.x's again, which
builds `propertyNames` once in its constructor and returns the same array on
every call, so a miss returns at the identity check in `Cache.tryUpdate` as
before.
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -190,41 +537,325 @@ 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)
}
}
}
+ /**
+ * An entry of a map a subscript wrote merged over the one it meets. A map
is merged into the map a subscript
+ * wrote that it meets, in place, so every holder of it sees the merge, or
into a copy of any other map, entry
+ * by entry, so that a property source above another adds to an element as
Spring Boot binds both and the
+ * configuration holds no map of a property source. A block is merged into
it by the dotted names of its
+ * entries, as dotted keys are. Anything else is stored as a merge stores
it; a {@code null} written in dot
+ * notation removes the entry.
+ */
+ private Object mergedElement(Object current, Object value, boolean
isNestedSet) {
+ boolean currentIsMap = current instanceof Map && !(current instanceof
NavigableMap)
+ if (!(value instanceof Map)) {
+ return merged(value)
+ }
+ Map held
+ if (currentIsMap && current instanceof HashMap && (current instanceof
SubscriptMap || !(value instanceof SubscriptMap))) {
+ held = (Map) current
+ } else {
+ held = value instanceof SubscriptMap || current instanceof
SubscriptMap ? new SubscriptMap() : new LinkedHashMap()
+ if (currentIsMap) {
+ held.putAll((Map) current)
+ if (current instanceof SubscriptMap) {
+ ((SubscriptMap)
held).getMarkedKeys().addAll(((SubscriptMap) current).getMarkedKeys())
+ }
+ }
+ }
+ if (value instanceof NavigableMap) {
+ mergeBlockInto(held, '', (NavigableMap) value, isNestedSet)
+ return held
+ }
+ for (Map.Entry entry in ((Map) value).entrySet()) {
+ String key = String.valueOf(entry.key)
+ if (entry.value == null && isNestedSet) {
+ held.remove(key)
+ } else {
+ held.put(key, mergedElement(held.get(key), entry.value,
isNestedSet))
+ }
+ if (held instanceof SubscriptMap && value instanceof SubscriptMap)
{
+ if (((SubscriptMap) value).marks(entry.key)) {
+ ((SubscriptMap) held).mark(key)
+ } else {
+ ((SubscriptMap) held).unmark(key)
+ }
+ }
+ }
+ held
+ }
+
+ /**
+ * Merges the entries of a block into a map a subscript wrote under the
names their dotted paths give them, as
+ * dotted keys are written there.
+ */
+ private void mergeBlockInto(Map held, String prefix, NavigableMap block,
boolean isNestedSet) {
+ for (Map.Entry<String, Object> entry in
block.getDelegateMap().entrySet()) {
+ String name = prefix + entry.key
+ if (entry.value instanceof NavigableMap) {
+ mergeBlockInto(held, name + '.', (NavigableMap) entry.value,
isNestedSet)
+ } else {
+ writeBelowSubscriptMap(held, name, entry.value, isNestedSet)
+ }
+ }
+ }
+
+ /** 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('.')) {
+ String[] containerParts = containerName.split(/\./)
+ Object[] below = subscriptMapOnPath(targetMap, containerParts,
containerParts.length - 1)
+ if (below != null) {
+ // a container below a map a subscript wrote is an entry of
that map, written in place
+ Object[] slot = slotBelow((Map) below[0], (String) below[1],
true)
+ Map holder = (Map) slot[0]
+ int end = subscripts.indexOf(']')
+ holder.put(slot[1], setSubscriptValue(holder.get(slot[1]),
subscripts.substring(1, end), subscripts.substring(end + 1),
+ merged(sourceValue)))
+ return
+ }
+ 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)
+ if (rootMap != null && !containerParent.is(rootMap)) {
+ mergeMapEntry(rootMap, containerPath, container)
+ }
+ }
+
+ /** Marks an indexed key that names no element. */
+ private static final Object ABSENT = new Object()
+
+ /**
+ * The element an indexed key names, such as {@code a.rows[0].name}: read
through the container this map holds
+ * under the name before the first subscript, then the element each
subscript and each name after it steps
+ * through, so that the key answers from the list as it is now, however
the list was written and whatever
+ * was set, added or removed in it since. A name after a subscript is
matched against the longest key the
+ * element holds, since a merge stores what follows the first subscript of
an indexed key, {@code tags[0]} or
+ * {@code meta.old}, as one key of the element. {@link #ABSENT} when the
key has no subscript or names nothing.
+ */
+ private Object elementAt(String key) {
+ Object[] slot = elementSlot(key)
+ if (slot == null) {
+ return ABSENT
+ }
+ Object container = slot[0]
+ String step = (String) slot[1]
+ container instanceof List ? ((List) container).get(step.toInteger()) :
((Map) container).get(step)
+ }
+
+ /**
+ * The container the last step of an indexed key reads from and that step,
as {@code [list, index]} or
+ * {@code [map, key]}, reached by stepping through the containers and
elements before it; null when the key
+ * has no subscript or steps through nothing.
+ */
+ private Object[] elementSlot(String key) {
+ int subscript = subscriptStart(key)
+ if (subscript < 0) {
+ return null
+ }
+ String containerName = key.substring(0, subscript)
+ Object current = delegateMap.get(containerName)
+ if (current == null) {
+ Object atPath = atPath(containerName)
+ current = ABSENT.is(atPath) ? null : atPath
+ }
+ String remaining = key.substring(subscript)
+ while (current != null) {
+ Object next
+ String step
+ String rest
+ if (remaining.startsWith('[')) {
+ int end = remaining.indexOf(']')
+ if (end < 0) {
+ return null
+ }
+ step = remaining.substring(1, end)
+ rest = remaining.substring(end + 1)
+ next = subscriptOf(current, step)
+ } else if (remaining.startsWith('.') && current instanceof Map) {
+ String name = remaining.substring(1)
+ int split = longestKeyHeld((Map) current, name)
+ if (split < 0) {
+ return null
+ }
+ step = name.substring(0, split)
+ rest = name.substring(split)
+ next = ((Map) current).get(step)
+ } else {
+ return null
+ }
+ if (ABSENT.is(next)) {
+ return null
+ }
+ if (!rest) {
+ return [current, step] as Object[]
+ }
+ current = next
+ remaining = rest
+ }
+ null
+ }
+
+ /** The element at a subscript, of a list for a numeric index and of a map
otherwise; {@link #ABSENT} when there is none. */
+ private static Object subscriptOf(Object container, String index) {
+ if (index.isInteger() && !(container instanceof Map)) {
+ if (!(container instanceof List)) {
+ return ABSENT
+ }
+ List list = (List) container
+ int i = index.toInteger()
+ return i >= 0 && i < list.size() ? list.get(i) : ABSENT
+ }
+ container instanceof Map && ((Map) container).containsKey(index) ?
((Map) container).get(index) : ABSENT
+ }
+
+ /**
+ * The length of the longest prefix of {@code rest} that the map holds as
a key, ending where the rest ends or
+ * before one of its separators, a dot or a subscript; -1 when it holds
none.
+ */
+ private static int longestKeyHeld(Map map, String rest) {
+ int split = rest.length()
+ while (split > 0) {
+ if (map.containsKey(rest.substring(0, split))) {
+ return split
+ }
+ split = Math.max(rest.lastIndexOf('.', split - 1),
rest.lastIndexOf('[', split - 1))
+ }
+ -1
+ }
+
+ /**
+ * The value a merge stores for a scalar, in the nested entry and in the
flat key alike, and for each element
+ * of a list and each entry of a plain map it stores. A config resolves
placeholders here, so that a value
+ * such as {@code ${random.uuid}} is resolved once and both
representations hold the same result. A nested map
+ * stores a value as the root of its configuration does, so a value is
stored alike whichever map it is
+ * written through.
+ */
+ protected Object mergedValue(Object value) {
+ value
+ }
+
+ /**
+ * The value a merge stores: a scalar as {@link #mergedValue} returns it,
and a list or a plain map with each
+ * element or entry stored the same way, in place, so that a placeholder
inside a list element is resolved as
+ * one in an entry of a block is. A container that refuses the write, as a
list built with
+ * {@code asImmutable()} does, is replaced by a copy that holds the
results; one whose elements are stored as
+ * they are is left as it is.
+ */
+ private Object merged(Object value) {
Review Comment:
Gone with the split: a merge stores values as 9.0.x does, without the hook
that resolved them in place. The separate PR for the flat key that keeps an
unresolved placeholder will resolve into a copy, and its spec will assert that
the caller's list and map and the property source's are unchanged.
##########
grails-skills/upgrade-guide-9/skills/grails-9-upgrade/SKILL.md:
##########
@@ -438,6 +451,7 @@ A datastore implementation of RxGORM compiled for Grails 8
or earlier, such as t
- [ ] No build file declares `grails-async-gpars` or `grails-events-gpars`,
and no source file imports `groovyx.gpars`
- [ ] No source file calls `GParsPool.withPool` or `GParsConfig`
- [ ] No build file sets `grails { indy = true }`, and build logic reads
`grails.indy` with `getOrElse` or `present`
+- [ ] No code reads a child of a string, list or closure configuration value
expecting the parent, and none writes into a nested configuration map with
`put` and reads the value back through `getProperty`
Review Comment:
Rewritten with the split: the skill now covers only the key past a value
that is not a map, as the guide does, with one action and one checklist item.
##########
grails-doc/src/en/guide/upgrading/upgrading90x.adoc:
##########
@@ -1229,3 +1229,26 @@ See <<asyncPromises,Promises>> and <<events,Events>>.
`grails-async-rxjava3` carries a `null` value as the RxJava 1 factory and
every other promise factory do: `createPromise()` and `createPromise(Class)`
give a promise whose `get()` returns `null`, a task that returns `null`
resolves to `null`, and a `null` value keeps its place in the list that
`onComplete`, `onError` and `waitAll` deliver.
`grails-async-rxjava2` does not: there `createPromise()` and `onComplete` over
a list holding a bound promise of `null` fail with a `NullPointerException`,
and a task that returns `null` resolves to the `java.lang.Void` class.
+
+==== 20. Configuration Lookups Stop at a Value That Is Not a Map and Read the
Flat Key First
Review Comment:
Done with the split. Section 20 is now "A Configuration Key Below a Value
That Is Not a Map Is Absent": the observable change, with examples of a string,
a list and a closure, and its required action. The PR no longer adds a what's
new sentence.
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -262,27 +957,61 @@ 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.
The index of a map is its key as
+ * written, a dot in it included, in a block as in any other map.
*/
private Object setSubscriptValue(Object currentValue, String index, String
rest, Object sourceValue) {
- if (index.isNumber()) {
+ if (index ==~ /\d+/) {
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)
+ }
+ if (currentValue instanceof NavigableMap) {
+ // the entry a read of the key finds: the one held under the key
as written, or else the one at the
+ // path it names, as a dotted key of application.yml is merged; a
new entry is held as written
+ NavigableMap block = (NavigableMap) currentValue
+ if (!block.delegateMap.containsKey(index) &&
!ABSENT.is(block.atPath(index))) {
Review Comment:
The representation this comment was about is gone. Following @jdaugherty's
review, this PR keeps the nested walk first and holds only the change that
makes a key past a value that is not a map absent, so `NavigableMap` and
`NavigableMapPropertySource` are 9.0.x's again. There, `app.routes[/api/**]`
written after `app.routes.extra` is presented to the environment by its
bracketed name, and Spring Boot binds the key `/api/**`. `toProperties()`
exports that name too, beside the dotted `app.routes./api/**` that 9.0.x and
Grails 8 also export.
--
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]
