codeconsole commented on code in PR #16732:
URL: https://github.com/apache/grails-core/pull/16732#discussion_r4372547100
##########
grails-core/src/main/groovy/org/grails/config/NavigableMapConfig.java:
##########
@@ -217,67 +222,29 @@ public <T> T getProperty(String key, Class<T> targetType,
T defaultValue, List<T
return value;
}
+ /**
+ * Reads the process environment again, so that a variable installed after
this config was
+ * created, as a test can do reflectively, is seen by the lookups that
follow.
+ */
+ protected void refreshSystemEnvironment() {
+ systemEnvironment = new SystemEnvironmentIndex(System.getenv());
+ }
+
@Override
public <T> T getProperty(String key, Class<T> targetType, T defaultValue) {
- Object value = findInSystemEnvironment(key);
+ Object value = systemEnvironment.get(key);
if (value == null) {
- value = getValueWithDotNotatedKeySupport(configMap, key);
+ // merge and dot-notation assignment write the flat key as well as
the nested entry, so the flat
+ // key answers nearly every lookup without a walk; where the two
disagree the flat key wins.
+ value = configMap.get(key);
Review Comment:
Fixed in 7299775d7d. `merge` on a nested map merged with that map as its own
root and an empty path, so it wrote the nested entry only. A new block or list
it merged got no flat key, and each dotted key was written a second time as a
key of the nested map. It now merges under the configuration root at the map's
path, as the same entries merged at the root do and as dot-notation assignment
already did.
Pinned in `NavigableMapSpec` with your sequence and with a new block, a
block over a block, a parsed dotted key, an indexed key and a merge two levels
down. Each is read through the flat key and `navigate`, with a check that every
flat key agrees with the nested maps. `PropertySourcesConfigSpec` reads your
sequence through `getProperty`.
So that this is the last of these found one write path at a time, I then ran
every public write, at the root and two levels down, against two oracles: the
representations agree, and the nested maps equal a plain `LinkedHashMap` given
the same writes. That, and a review of the fixes, found seven more, each fixed
in its own commit with its tests:
- e1cd4af5b1: a scalar written over a block by `put`, assignment or a view,
as `config.a.x = 'scalar'` does, left `getProperty('a.x.y')` reading the old
entry. Only `merge` keeps those flat keys now, which is the layering rule the
description states.
- cc29ae09bf: a map put over a block was merged into it, since `put` went
through dot-notation assignment. It now replaces the block, as `put` did before
this PR.
- b64ae763a0: a write through the `clone()` of a nested map, or through a
block that a write had removed or replaced, still wrote the root's flat keys.
It now changes that map only.
- 8654d8bf9f: a map assigned in dot notation, `config.a.x = [v: 5]`, merged
its entries, so `v` replaced the block `a.x.v` and kept its flat keys. Each
entry is now written as dot notation writes it.
- 8000475162: a list written by an indexed key over a block,
`config.a['x[0]'] = 'v'`, kept the block's flat keys.
- fbad39f862: a value written through a nested map skipped the root's
placeholder hook, so `config.a.merge([b: '${name}'])` stored the literal, now
in the flat key too.
- 8c49c0094f: a dotted key written through a nested map,
`config.a.put('x.y', 5)`, was a key of `a` whose flat key also named the path
`a.x.y`. It is now that path, as on the root.
All 232 scenarios agree. The probe comment and the description describe the
matrix, and section 20 of the upgrade guide has the rules.
--
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]
