jdaugherty commented on code in PR #16732:
URL: https://github.com/apache/grails-core/pull/16732#discussion_r4352538873
##########
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:
Reading the flat key first also changes a `Map` read on a config written
only through `merge`, here with a lower-precedence scalar followed by a
higher-precedence block under it:
```groovy
config.merge(['server.url': 'x'])
config.merge(['server.url.port': 'y'])
config.getProperty('server.url') // 9.0.x: null, now: 'x'
config.getProperty('server.url', Map) // 9.0.x: [port: y], now: null
config.navigate('server', 'url') // [port: y] on both
```
`navigateSubMap` replaces the nested scalar with a new map, but it writes
the root flat key only when that key is absent, so the root `server.url` still
holds the scalar. This line answers the `Map` read with that scalar, the
conversion fails, and the default comes back. The block was merged later, yet
the earlier source now wins the `Map` read, and `getProperty` and `navigate`
disagree although only `merge` was used. That contradicts the "nothing changes
for them" sentence in section 17.
Please either fall through to the walk when the flat value is not a `Map`
and the target type is one, or pin the `null` in `PropertySourcesConfigSpec`
and describe it in section 17. Either way this order (scalar first, block
second) needs a test: the new spec covers only the reverse.
##########
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:
The configuration files never had this leak. `application.yml` is flattened
before it is merged (`YamlPropertySourceLoader` merges `getFlattenedMap`), and
`application.groovy` is merged without parsing dotted keys, so neither reaches
the nested dotted key branch. On 9.0.x, an `application.yml` with
`org.springframework: DEBUG` under `logging.level` has no root
`org.springframework` (`containsProperty` is `false`), and a real
`org.springframework` next to it keeps its value.
Only a dotted key inside a nested map took that branch: passed to
`config.merge(...)`, to `new PropertySourcesConfig(Map)`, or as a `Map` value
of a property source other than a `NavigableMapPropertySource`. As written, the
note sends readers looking for a problem in their `application.yml`. Please
name `merge` with a nested map as the case, for example `config.merge([logging:
[level: ['org.springframework': 'DEBUG']]])`.
##########
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:
`config.a['b'] = value` does not have to become a raw write.
`NavigableMap.setAt` already routes a subscript write through `setProperty`,
which writes both the flat key and the nested entry. But Groovy compiles
`map[key] = value` to `putAt`, so `setAt` never runs and
`DefaultGroovyMethods.putAt(Map, ...)` calls `put`. With
```groovy
void putAt(String key, Object value) {
setProperty(key, value)
}
```
on `NavigableMap`, `config.a['b'] = 'x'` is read back as `'x'` by
`getProperty('a.b')`, and the `org.grails.config.*` specs in grails-core and
the `grails.config.*` specs in grails-bootstrap still pass. In the same probe,
a `putAt(Object, Object)` did not take over from the DGM method.
Subscript assignment is the usual way to write a key that is not a valid
identifier. Please add the `putAt` with a spec for it, and drop `config.a['b']
= value` from this list.
--
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]
