codeconsole commented on code in PR #16732:
URL: https://github.com/apache/grails-core/pull/16732#discussion_r4361738756
##########
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 at the write rather than at the read, in 1dee0b59a5: `navigateSubMap`
now replaces the root flat key whenever the value it holds is not a map,
instead of only when it is absent. After `merge(['server.url': 'x'])` then
`merge(['server.url.port': 'y'])` both representations hold the block, so
`getProperty('server.url', Map)` is `[port: y]`, `getProperty('server.url')` is
`null`, `navigate('server', 'url')` agrees and `getProperty('server.url.port')`
is `y`: the answers of 9.0.x, and `config['server.url']` is right as well. That
keeps the section 17 sentence true without a type-dependent fall-through in the
lookup. Pinned for this order in `PropertySourcesConfigSpec` (`a block merged
over a scalar is read as the block`) and at the map level in
`NavigableMapSpec`. The reverse order stays as already pinned: the descendant
flat key that a scalar leaves behind is still read, since removing it is the
replace semantics of #10188.
##########
grails-doc/src/en/guide/upgrading/upgrading90x.adoc:
##########
@@ -1151,3 +1151,18 @@ When upgrading an affected application:
----
See xref:theWebLayer#commandObjects[Command Objects] for a trait-based
alternative to sharing a non-domain superclass with a domain class.
+
+==== 17. Configuration Lookups Stop at a Value That Is Not a Map and Read the
Flat Key First
+
+`getProperty` on `grailsApplication.config` resolves a dotted key by walking
the nested maps of the configuration. In Grails 8 a key that ran past a value
that is not a map, such as `server.url.port` when `server.url` is a string, was
answered with that value, and `containsProperty` of such a key was `true`. The
same held for a key that ran past a list, or past a closure from
`application.groovy` such as `grails.gorm.default.constraints.nullable`. In
Grails 9 such a key is absent: `getProperty` returns `null` or the default,
`containsProperty` returns `false`, and `navigate` on the configuration answers
the same way.
+
+Grails 9 also reads the flat key that `merge` writes for every nested value
before it walks the nested maps. Every documented way of writing configuration,
that is the configuration files, `merge` and dot-notation assignment such as
`config.a.b = value`, writes both the flat key and the nested entry, so nothing
changes for them. What changes is which value `getProperty` answers with once
the two have been made to disagree:
+
+- A dotted key written directly, with `config.put('a.b', value)` or
`config['a.b'] = value`, is now read by `getProperty('a.b')`, as it always was
by `config['a.b']`.
+- A value written directly into a nested map, with `config.a.put('b', value)`
or `config.a['b'] = value`, is no longer read by `getProperty('a.b')`; dot
navigation, `config.a.b`, still reads it.
Review Comment:
Added in 6fefb65c94: `putAt(String, Object)` on `NavigableMap` routes
through `setProperty`, so `config.a['b'] = value` writes the flat key and the
nested entry and `getProperty('a.b')` reads it back. Same result as your probe,
the instance method wins over the DGM one. Specs: `subscript assignment on a
nested map writes both representations` in `NavigableMapSpec`, and a subscript
step in `the flat key is read before the nested maps are walked`. Section 17 no
longer lists it as a direct write and says it writes both, as `config.a.b =
value` does.
##########
grails-doc/src/en/guide/upgrading/upgrading90x.adoc:
##########
@@ -1151,3 +1151,18 @@ When upgrading an affected application:
----
See xref:theWebLayer#commandObjects[Command Objects] for a trait-based
alternative to sharing a non-domain superclass with a domain class.
+
+==== 17. Configuration Lookups Stop at a Value That Is Not a Map and Read the
Flat Key First
+
+`getProperty` on `grailsApplication.config` resolves a dotted key by walking
the nested maps of the configuration. In Grails 8 a key that ran past a value
that is not a map, such as `server.url.port` when `server.url` is a string, was
answered with that value, and `containsProperty` of such a key was `true`. The
same held for a key that ran past a list, or past a closure from
`application.groovy` such as `grails.gorm.default.constraints.nullable`. In
Grails 9 such a key is absent: `getProperty` returns `null` or the default,
`containsProperty` returns `false`, and `navigate` on the configuration answers
the same way.
+
+Grails 9 also reads the flat key that `merge` writes for every nested value
before it walks the nested maps. Every documented way of writing configuration,
that is the configuration files, `merge` and dot-notation assignment such as
`config.a.b = value`, writes both the flat key and the nested entry, so nothing
changes for them. What changes is which value `getProperty` answers with once
the two have been made to disagree:
+
+- A dotted key written directly, with `config.put('a.b', value)` or
`config['a.b'] = value`, is now read by `getProperty('a.b')`, as it always was
by `config['a.b']`.
+- A value written directly into a nested map, with `config.a.put('b', value)`
or `config.a['b'] = value`, is no longer read by `getProperty('a.b')`; dot
navigation, `config.a.b`, still reads it.
+
+A dotted key inside a nested block, such as `org.springframework` under
`logging.level`, used to be written a second time as a key at the root, where
it could overwrite a real `org.springframework` property; it is now written
only under its parent, as `logging.level.org.springframework`.
Review Comment:
Right, the files never took that branch; I had the same wrong example in the
reply to James and corrected it there. 2ac37e0142 names `config.merge([logging:
[level: ['org.springframework': 'DEBUG']]])` as the case and says the
configuration files are flattened before they are merged.
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -218,7 +217,7 @@ class NavigableMap implements Map<String, Object>,
Cloneable {
targetMap.remove(sourceKey)
} else {
if (path) {
- rootMap.put("${path}.${sourceKey}".toString(), newValue)
+ mergeMapEntry(rootMap, "${path}.${sourceKey}".toString(),
newValue)
Review Comment:
Fixed in 043e2e0fea. The value is now resolved once: `NavigableMap` asks a
`mergedValue` hook for the value it stores, and `NavigableMapConfig` resolves
placeholders there instead of in the put hook, so the flat key and the nested
entry get the same string. Pinned in `PropertySourcesConfigSpec` with a
property source whose value changes on every read:
`getProperty('auth.secret')`, `getProperty('auth', Map).secret` and
`config['auth.secret']` are all the first value, and the source is read once.
##########
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
Review Comment:
Fixed in fad0a60050. The subscript branch wrote the list into the target map
only, so the root alias stayed on the list the first merge created. It now
writes the parent-path alias on the root with the same list it stores in the
nested map, so after `merge(['app.items[0]': 'old'])` then `merge([app:
['items[0]': 'new']])` both `getProperty('app.items', List)` and
`navigate('app', 'items')` are `['new']`, and the two representations hold one
list rather than two copies. Pinned in `PropertySourcesConfigSpec` and at the
map level in `NavigableMapSpec`.
--
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]
