Repository navigation
Treat a dotted configuration key past a value that is not a map as absent - #16732
codeconsole wants to merge 13 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 9.0.x #16732 +/- ##
=============================================
Coverage 47.0212% 47.0212%
Complexity 913 913
=============================================
Files 94 94
Lines 5153 5153
Branches 901 901
=============================================
Hits 2423 2423
Misses 2399 2399
Partials 331 331 🚀 New features to boost your workflow:
|
jamesfredley
left a comment
There was a problem hiding this comment.
Not approving. Flat-key lookup and stopping at a non-map are right. A nested dotted merge can still overwrite an unrelated flat key.
|
The three failures on 2dbb3b9:
The push reruns all three. |
A dotted key inside a nested map, such as 'a.b' under x, was written
a second time with only its own path, so merge([a: [b: 'root'],
x: ['a.b': 'nested']]) wrote the root key a.b as nested and overwrote
the real property, while navigate('a', 'b') kept root. The second pass
now carries the parent path, so the key is flattened as x.a.b only.
Found by James Fredley on apache#16732, where reading the flat key first
turned the stale alias into the answer of getProperty('a.b').
jdaugherty
left a comment
There was a problem hiding this comment.
Checked at 5f1c39a against 9.0.x with the same probes. A key past a scalar, a list or a closure is absent, a raw flat put is read, the flat key left behind when a scalar replaces its parent block is read, and the nested dotted key fix holds. The org.grails.config.* specs in grails-core and the grails.config.* specs in grails-bootstrap pass locally.
Three things below: a Map read that changes for a config written only through merge, a subscript write that can keep working, and the root-key note in section 17.
| 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); |
There was a problem hiding this 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:
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 bothnavigateSubMap 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.
There was a problem hiding this comment.
Fixed at the write rather than at the read, in 1dee0b5: 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.
There was a problem hiding this comment.
Confirmed at 92ea847. After merge(['server.url': 'x']) then merge(['server.url.port': 'y']), getProperty('server.url', Map) is [port:y], getProperty('server.url') is null, and navigate('server', 'url') agrees.
| 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. |
There was a problem hiding this 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
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.
There was a problem hiding this comment.
Added in 6fefb65: 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.
There was a problem hiding this comment.
Confirmed at 92ea847. config.a['b'] = 'x' is read back as 'x' by both getProperty('a.b') and config.a.b.
| - 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`. |
There was a problem hiding this 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']]]).
There was a problem hiding this comment.
Right, the files never took that branch; I had the same wrong example in the reply to James and corrected it there. 2ac37e0 names config.merge([logging: [level: ['org.springframework': 'DEBUG']]]) as the case and says the configuration files are flattened before they are merged.
There was a problem hiding this comment.
Confirmed at 92ea847. Section 20 now names config.merge(...) with a nested map as the case, and says the configuration files are flattened before they are merged.
jamesfredley
left a comment
There was a problem hiding this comment.
The earlier merge case is fixed. config.merge([a: [b: 'root'], x: ['a.b': 'nested']]) leaves getProperty('a.b') as root. That thread is resolved. A real flat key still wins. Not approving. Two other lookups can now disagree.
A merged value was resolved separately for the flat key and for the
nested entry, each write going through the mergeMapEntry hook, so a
value such as ${random.uuid} differed between getProperty('auth.secret')
and getProperty('auth', Map).secret. NavigableMap now asks for the value
it stores once, through mergedValue, and NavigableMapConfig resolves
placeholders there; the put hook is a put again. Found by James Fredley
on apache#16732.
The subscript branch of mergeMapEntry wrote the list into the target map only, so merge(['app.items[0]': 'old']) followed by merge([app: ['items[0]': 'new']]) updated the nested list and left the root flat app.items on the list the first merge had created. The branch now writes the parent-path alias on the root as well, with the same list, so both representations hold one list. Found by James Fredley on apache#16732.
jamesfredley
left a comment
There was a problem hiding this comment.
The two earlier comments are fixed. A placeholder is resolved once for both representations, and getProperty('app.items', List) follows the nested list. Those threads are resolved. Not approving. An intermediate flat alias can still point at the old scalar.
jamesfredley
left a comment
There was a problem hiding this comment.
The earlier comment is fixed. merge(['x.a': 'old']) then merge([x: ['a.b': 'new']]) leaves getProperty('x.a', Map) as [b: 'new']. That thread is resolved. Not approving. A later merge can still leave a stale block alias, and a clone can write into its original.
| 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); |
There was a problem hiding this comment.
merge(['a.b.c': 1]), then merge([a: 'scalar']), then merge(['a.b.d': 2]) leaves the nested a.b as [d: 2] while the flat alias stays [c: 1]. getProperty('a.b', Map) returns the old block.
Replace an existing map alias when the branch is recreated, before flat lookup treats it as current.
There was a problem hiding this comment.
Fixed in 850c97b. navigateSubMap kept the root alias of a block it created whenever the alias already held a map, which is right only while that map is the nested block; after merge([a: 'scalar']) destroyed the nested branch the alias a.b still held the old block, and the recreation left it there. A block merged as a map value already replaced its alias unconditionally, and the dotted key path now does the same: whatever the alias holds when the nested block has to be created is stale. After merge(['a.b.c': 1]), merge([a: 'scalar']), merge(['a.b.d': 2]), getProperty('a.b', Map) and navigate('a', 'b') are both [d: 2]. The flat a.b.c of the destroyed block stays readable as 1, as the description says a lower-precedence key does when a higher-precedence scalar replaces its parent; the recreated block does not pick it up. Pinned in PropertySourcesConfigSpec and NavigableMapSpec.
| rootMap.put(fullPath, newMap) | ||
| // the flat alias of a new block lives at the configuration root under the block's full path, | ||
| // whichever map the walk started on; a scalar an earlier merge left there is replaced | ||
| NavigableMap root = currentMap.getRootConfig() |
There was a problem hiding this comment.
Cloning an empty map and merging ['new.block.value': 1] into the copy inserts new and new.block into the original, because the alias write uses getRootConfig() and the clone still points at the source.
Write those aliases on the copy. The original should stay empty.
There was a problem hiding this comment.
Fixed in f4f6485. clone() copied the entries into a map whose rootConfig was still the original, and shared the nested maps with it, so once the alias write followed getRootConfig() a merge into the copy wrote the original's root, and a dot-notation write below a shared block, copy.a.b.d = 2, had always written the original's nested map and its root alias. The copy of a root is now the root of its own configuration and its nested maps are copies under it, with a map the original holds twice, as the nested entry and the flat alias of a block, copied once so the copy keeps both representations on one map; the copy of a nested map stays under the root of the original, as before. Merging ['new.block.value': 1] into the clone of an empty map leaves the original empty, and copy.a.b.d = 2, copy.merge(['a.e.f': 3]) and copy.put('g', 4) leave the original's nested maps and flat keys untouched while the copy reads each through both representations. Pinned in NavigableMapSpec; CodeGenConfig.clone() builds its copy by merging and is unchanged.
|
Following @jdaugherty's review, this PR is split. It now holds only the change that makes a dotted key past a value that is not a map absent, with Dropped with the flat-first read are reading the flat key before the walk and the commits that kept the flat keys and the nested maps in step for it, including the bracketed-key and subscript-map handling of the last review rounds. Apart from the two lines of the change, The merge bugs the probes found that 9.0.x has as well will follow as separate PRs against the oldest branch each affects:
|
jamesfredley
left a comment
There was a problem hiding this comment.
The two earlier comments are fixed. The three-merge case leaves a.b as [d: 2] in both lookups, and merging into a clone no longer writes into the original. Those threads are resolved. Not approving. A nested indexed property is no longer built into a list.
jamesfredley
left a comment
There was a problem hiding this comment.
The earlier indexed-list comment is fixed. merge([a: ['rows[0].name': 'row']], true) builds [[name: 'row']] for both lookups. That thread is resolved. Not approving. An immutable list from application.groovy can now fail startup, and replacing one indexed element leaves a stale child alias.
jamesfredley
left a comment
There was a problem hiding this comment.
The two earlier comments are fixed. An immutable list no longer fails startup, and replacing an indexed element drops the old child aliases. Those threads are resolved. Not approving. Changing a list element in place leaves a stale alias that a later merge writes back.
jamesfredley
left a comment
There was a problem hiding this comment.
The in-place assignment is fixed. names[0] = 'two' stays 'two' through a later merge. That thread is resolved. Not approving. Removing an indexed key that is not there creates configuration.
jdaugherty
left a comment
There was a problem hiding this comment.
Checked at fa56ec6, with a probe against 9549579, this branch's merge-base with 9.0.x. The split is in, and the change itself is right. With only server.url set, Spring Boot's Environment has no server.url.port: getProperty is null, containsProperty is false, and the Binder leaves it unbound, so the config now agrees with the environment beside it. The old answer was never designed either. It arrived in 3.3.11 with #11042, whose walk skipped the remaining segments once it reached a value that is not a map. Up to 3.3.10, getProperty read only the flat key, and such a key was absent.
Below are two notes on the upgrade guide, and a [DISCUSS] thread on the list case, where Boot does something neither 9.0.x nor this PR does.
Optional, and outside this PR: a warning on 8.1.x whenever the walk answers a key with the value above it would let an application find a read that depends on it before it moves to 9.
|
|
||
| ==== 20. A Configuration Key Below a Value That Is Not a Map Is Absent | ||
|
|
||
| In Grails 8, `getProperty` on `grailsApplication.config` answered a dotted key that runs past a value that is not a map with that value: `server.url.port` when `server.url` is a string, `server.ports.0` when `server.ports` is a list, or `grails.gorm.default.constraints.nullable` when `grails.gorm.default.constraints` is a closure from `application.groovy`. `containsProperty` of such a key was `true`. In Grails 9 the key is absent: `getProperty` returns `null` or the default, `containsProperty` returns `false`, and `navigate` answers `null`. |
There was a problem hiding this comment.
This holds while nothing else sets the deeper key. When a lower-precedence property source does, getProperty now answers with that key, where 9.0.x answered with the value above it:
def sources = new MutablePropertySources()
sources.addLast(new MapPropertySource('higher', ['a.b': 'x']))
sources.addLast(new MapPropertySource('lower', ['a.b.c': 'y']))
def config = new PropertySourcesConfig(sources)| 9.0.x | this PR | |
|---|---|---|
config.getProperty('a.b.c') |
x |
y |
config.containsProperty('a.b.c') |
true |
true |
config.navigate('a', 'b', 'c') |
x |
null |
y is the right answer, and it is what a Spring Environment over the same two sources answers. But it is a second change an application can observe, and this sentence says the key is absent. Please describe it here and in the skill, and pin it in PropertySourcesConfigSpec with two property sources like the ones above.
There was a problem hiding this comment.
Done in 07a31f2. Section 20 and the skill now say that a property source of lower precedence that sets the deeper key now answers it. With a.b: x above a.b.c: y, getProperty('a.b.c') answers y where Grails 8 answered x, as a Spring Environment over the same sources does, containsProperty stays true, and navigate('a', 'b', 'c') answers null. PropertySourcesConfigSpec builds the config over your two MapPropertySources and a StandardEnvironment over the same two, and asserts all four. It fails on 9.0.x.
|
|
||
| ===== Required Actions | ||
|
|
||
| Code that reads a child of a string, a list or a closure and relied on being answered with the parent reads the parent instead. |
There was a problem hiding this comment.
The read that changes with no null or exception to point at it is a typed one with a default, where the application set the parent to a boolean as a shorthand for the block:
// x.y: false
config.getProperty('x.y.enabled', Boolean, true) // 9.0.x: false this PR: truePlease add that as an example here and in the skill. The default takes over silently, so nothing else points at the change.
There was a problem hiding this comment.
Done in 07a31f2. Section 20 has the x.y: false example with getProperty('x.y.enabled', Boolean, true) (Grails 8: false, Grails 9: true), saying that the default takes over with nothing to point at the change, plus a required action for it. The skill has the same example and, as sbglasius asked, a checklist entry: no typed read with a default names a key below a value that is not a map. PropertySourcesConfigSpec pins the read, and it fails on 9.0.x.
| and: "a key that names a child of any of them is absent rather than the value" | ||
| config.getProperty('server.url.port') == null | ||
| config.getProperty('server.url.port', String, 'none') == 'none' | ||
| config.getProperty('server.ports.0') == null |
There was a problem hiding this comment.
[DISCUSS] Read a numeric segment after a list as an index, as Spring Boot does
Spring Boot treats server.ports.0 and server.ports[0] as the same property once ConfigurationPropertySources is attached to the environment, which SpringApplication always does. With server.ports: [8080, 8443] loaded through Grails' YamlPropertySourceLoader into an attached StandardEnvironment, and a PropertySourcesConfig built over the same sources:
| 9.0.x | this PR | |
|---|---|---|
environment.getProperty('server.ports.0') |
8080 |
8080 |
config.getProperty('server.ports.0') |
the whole list, '8080,8443' |
null |
config.getProperty('server.ports[0]') |
8080 |
8080 |
Neither branch gives the environment's answer. Should Grails 9 read a numeric segment after a list as an index, so that config.getProperty('server.ports.0') is 8080, rather than making the key absent?
- For: the config and the environment of one application agree on the same key, and an application that reads a list element this way keeps working, now with the element instead of the list.
- Against:
server.ports[0]already works, andnavigate('server', 'ports', '0')would have to index too, or the two would disagree.
The walk would index only when it reaches a list, so a map with a '0' key reads as it does now. The answer decides what section 20 and these assertions say about a list, so settling it before this merges would avoid documenting one answer and then changing it.
There was a problem hiding this comment.
@matrei @sbglasius @jamesfredley I'd love your thoughts here
There was a problem hiding this comment.
On reflection I think Grails 9 should do what Spring Boot does, in this PR.
- It is the reason for the PR. The case for making the key absent is that the config then agrees with the
Environmentbeside it. For a list, absent is the one answer where they disagree: per your table, the environment answersserver.ports.0with8080, and this PR answersnull. - The element is what such a read meant. Code that reads
server.ports.0almost certainly wants the first port. Grails 8 handed it the whole list, which was never designed, as you found for the walk in NavigableMapConfig.getProperty does not resolve environmental variables #11042.nullbreaks that code as much as the element would, and only the element does what it asked for. - There is only one change if it goes in here. sbglasius's point stands for a split: going from
nullto the element later is compatible, and going back is not. If this PR goes from the list straight to the element, Grails 9 never answersnullfor an index, and section 20 documents one answer. server.ports[0]isn't the fallback it looks like. It names the element only where the source wrote indexed keys, asapplication.ymland.propertiesfiles do. For a list fromapplication.groovyor a merged map,config.getProperty('server.ports[0]')isnullon 9.0.x and here.
What I'd do:
getPropertyandnavigatestep into a list when the next segment is a whole number within the list,server.ports.0, and readports[0]the same way, so both spellings answer for every list.- A segment that is not an index, or is out of range, leaves the key absent, as for any other value that is not a map.
- The walk indexes only when it reaches a list, so a map with a
'0'key reads as it does now. containsPropertyfollowsgetProperty.- A list of maps reads through:
app.rows.0.name. - Section 20 and the skill would say that
server.ports.0is the first element, as in Spring Boot, where Grails 8 answered with the list.
One difference from Boot: Boot resolves both spellings against the indexed keys a YAML or .properties source holds, while Grails would also index a list held whole, as application.groovy gives it. That seems the right side to err on, since the config already answers with the whole list there.
If you agree, I'll add it here before this merges.
There was a problem hiding this comment.
Yes, I'm happy with this solution. Will re-review once this change is made.
There was a problem hiding this comment.
Done in 08c7f05, with the guide and skill in 1d08fa3. e536f6c merges 9.0.x in first, since the branch had fallen behind.
getPropertyandnavigatestep into a list when the next segment is a whole number within it.server.ports.0is8080andnavigate('server', 'ports', '0')agrees.ports[0]names the element of the list underportswhen the map holds no key of that name, soserver.ports[0]now answers for a list fromapplication.groovytoo.- A list of maps reads into its elements:
app.rows.0.name. - An index past the end, a negative or signed number, or any other segment after a list stays absent. A map whose keys are numbers reads as before.
Tests:
PropertySourcesConfigSpecchecks a list held whole and a list fromapplication.yml.- It compares with a
StandardEnvironmentwithConfigurationPropertySourcesattached, which gives8080forserver.ports.0andnullpast the end, as the config now does. - It also covers a map with numeric keys.
NavigableMapSpeccoversnavigate.
The new features fail at 07a31f2. grails-bootstrap (308) and grails-core (1087) pass.
Section 20 now describes the list case with an example and a required action for code that read the whole list through server.ports.0. It also drops the earlier advice to read the element from the whole list, which the index makes unnecessary.
jamesfredley
left a comment
There was a problem hiding this comment.
Approving. A dotted key past a value that is not a map is absent, and a bracketed key written into an existing block keeps its name. The earlier comments are resolved.
sbglasius
left a comment
There was a problem hiding this comment.
This an AI assisted review
Reviewed at fa56ec6. The code change is right and small: NavigableMap.navigateMap and NavigableMapConfig.getValueWithDotNotatedKeySupport now stop at a value that is not a map instead of returning it. containsProperty goes through getProperty, so it follows. The specs cover a scalar, a list and a closure through getProperty, containsProperty and navigate, and the #16130 clause now asserts absence. I have no concerns about the code.
I agree with @jdaugherty's two open doc comments; they are still unaddressed at this commit and I would like them in before merge.
1. A lower-precedence source can now answer the key. getProperty falls through to configMap.get(key) once the walk returns null, so with a.b: x in a higher source and a.b.c: y in a lower one, getProperty('a.b.c') is y (9.0.x: x) while navigate('a', 'b', 'c') is null. y is the right answer and matches Spring's Environment, but the guide and skill say the key is absent. Please describe it and pin it in PropertySourcesConfigSpec with two property sources.
2. The silent typed read. x.y: false with getProperty('x.y.enabled', Boolean, true) was false and is now true. Nothing throws, so it is worth a line in section 20 and in the skill's checklist.
On the [DISCUSS] thread (server.ports.0 as an index): I would keep it absent in this PR. Going from null to the element later is a compatible change; going from the element back to null is not. It would also have to be done in navigate as well, or the two lookups would disagree, which is a bigger change than this PR is scoped for. If we want it, a follow-up on 9.0.x seems the right place. The upgrade guide can say that server.ports[0] is the spelling that works.
Not blocking beyond the two doc items above.
A key below a value that is not a map changes two reads besides the
ones that now give null. A typed read with a default gets the default,
with nothing to point at the change: with x.y: false as a shorthand for
its block, getProperty('x.y.enabled', Boolean, true) answered false and
answers true. And a property source of lower precedence that sets the
deeper key now answers it: with a.b: x above a.b.c: y,
getProperty('a.b.c') answered x and answers y, as a Spring Environment
over the same sources does, while navigate('a', 'b', 'c') is null.
Section 20 and the grails-9-upgrade skill now describe both, and say
how to read a list element: from the list, as
getProperty('server.ports', List)[0], since server.ports[0] names the
element only for a list from application.yml. PropertySourcesConfigSpec
pins both reads; each fails on 9.0.x.
jamesfredley
left a comment
There was a problem hiding this comment.
Approved again. A dotted key past a value that is not a map stays absent, and the upgrade text matches that. 9.0.x is the right line.
…-9.0.x # Conflicts: # grails-bootstrap/src/test/groovy/grails/config/NavigableMapSpec.groovy # grails-core/src/test/groovy/org/grails/config/PropertySourcesConfigSpec.groovy
Spring Boot reads server.ports.0 and server.ports[0] as the same
property, the first element of the list. With a key past a value that
is not a map now absent, server.ports.0 answered null, so the config and
the environment beside it disagreed on exactly the key that Grails 8
answered with the whole list.
The walk of getProperty and navigate now steps into a list when the
next segment is a whole number within it, so server.ports.0 is 8080 and
navigate('server', 'ports', '0') agrees. A segment with an index after a
name, ports[0], names the element of the list under ports when the map
holds no key of that name itself, so server.ports[0] answers for a list
held whole, as application.groovy gives it, as well as for one from
application.yml. A list of maps reads into its elements, app.rows.0.name.
An index past the end, a negative or signed number, or any other segment
after a list stays absent, and a map whose keys are numbers reads as
before.
Section 20 and the grails-9-upgrade skill now say that a whole number after a list names an element, as Spring Boot reads it, where Grails 8 answered with the whole list, with the required action for code that read the list that way, and drop the advice to read an element from the whole list, which the index now makes unnecessary.
…-9.0.x # Conflicts: # grails-doc/src/en/guide/upgrading/upgrading90x.adoc # grails-skills/upgrade-guide-9/skills/grails-9-upgrade/SKILL.md
|
@sbglasius, thanks for the review. Your points, at the current head: 1. A lower-precedence source can now answer the key. Described in the upgrade guide and the 2. The silent typed read. Also in 07a31f2. The section and the skill show On
On the guide recommending |
jdaugherty
left a comment
There was a problem hiding this comment.
Checked at e1c1d0b against 9.0.x. The index read is in as agreed: server.ports.0 and server.ports[1] answer the element for a list from application.yml and for a list held whole, navigate agrees, a list of maps and a nested list read through, and a map with numeric keys reads as before. NavigableMapSpec and the org.grails.config specs pass locally.
Two notes below, neither blocking: a list element holding a placeholder is returned as written where the Environment resolves it, which is pre-existing and worth a sentence in the guide, and a leading zero reads as an index.
|
|
||
| In Grails 8, `getProperty` on `grailsApplication.config` answered a dotted key that runs past a value that is not a map with that value: `server.url.port` when `server.url` is a string, `server.ports.0` when `server.ports` is a list, or `grails.gorm.default.constraints.nullable` when `grails.gorm.default.constraints` is a closure from `application.groovy`. `containsProperty` of such a key was `true`. In Grails 9 the key is absent: `getProperty` returns `null` or the default, `containsProperty` returns `false`, and `navigate` answers `null`. | ||
|
|
||
| Below a list, a whole-number segment names an element instead, as Spring Boot reads it, and so does an index after the name of the list, whether the list comes from `application.yml` or from `application.groovy`: |
There was a problem hiding this comment.
This holds for the element's value, but not for a placeholder in it. The config stores a list whole and its merge hook resolves placeholders only in string values, so an element written as ${EXAMPLE_ORIGIN:https://default.example.com} is returned as written, where the Environment resolves it:
| 9.0.x | this PR | Environment |
|
|---|---|---|---|
config.getProperty('app.origins.1') |
the whole list | ${EXAMPLE_ORIGIN:…} |
https://default.example.com |
config.getProperty('app.origins[1]') |
${EXAMPLE_ORIGIN:…} |
${EXAMPLE_ORIGIN:…} |
https://default.example.com |
Pre-existing: the whole-list read, getProperty('app.origins', List), has the same raw elements on 9.0.x, and both loaders take this path. Not a blocker here. Please add a sentence here and in the skill that a placeholder in a list element is not resolved by config, and resolve list elements in the hook as a separate PR against the oldest branch it affects, like the other merge follow-ups.
There was a problem hiding this comment.
Added in dc9924d, in §21 and in the grails-9-upgrade skill. A placeholder in a list element is not resolved by config, by index or in the whole list, as in Grails 8. The Environment resolves it, so the text says to read such an element there or to bind the list with @ConfigurationProperties.
I reproduced it with Grails' YAML loader at the new head. app.origins.1, app.origins[1] and the elements of getProperty('app.origins', List) all come back as ${EXAMPLE_ORIGIN:…}. So does a value in a map inside a list, app.rows.0.url. The Environment answers https://default.example.com for each. Spring Boot's YAML loader is not affected, because PropertySourcesConfig resolves each element of a list it reads from a source that is not a NavigableMapPropertySource.
The hook fix is #16798, against 7.0.x, where the hook and the loader are the same as on 9.0.x. It resolves the strings in a list, through the maps and lists inside it, and a value or map written under an indexed key, so the config answers each of the reads above as the Environment does. Its description says to drop this sentence when it reaches 9.0.x.
| return null; | ||
| } | ||
|
|
||
| private static int listIndex(String segment) { |
There was a problem hiding this comment.
Nit: a leading zero reads as an index here, so app.ports.01 is the second element, where the Environment answers null. Rejecting a segment that starts with 0 unless it is exactly 0 would keep the two in step. Optional.
There was a problem hiding this comment.
Done in 60414c1. A whole-number segment indexes a list only when it is 0 or does not start with 0. That applies in getProperty and in navigate, so server.ports.01 and server.ports[01] are absent.
PropertySourcesConfigSpecadds the case to the comparison with aStandardEnvironmentwithConfigurationPropertySourcesattached, which answersnulltoo.NavigableMapSpeccoversnavigate.- The guide and the skill now list a leading zero among the segments that stay absent.
- The new assertions failed at e1c1d0b.
grails-bootstrap(308) andgrails-core(1087) pass.
jamesfredley
left a comment
There was a problem hiding this comment.
The merge of 9.0.x leaves the list-index lookup as it was. A dotted key past a value that is not a map stays absent, a list index reads that element, and a numeric map key stays a map lookup. 9.0.x is the right line.
server.ports.01 named the second element, where the Spring Environment answers null. A whole-number segment now indexes a list only when it is 0 or does not start with 0, in getProperty and in navigate.
The merge hook resolves placeholders only in string values, and the Grails loaders hold a list whole, so an element written as a placeholder is returned as written by index and in the whole list, where the Environment resolves it. Also name a leading zero among the segments that are not an index.
jamesfredley
left a comment
There was a problem hiding this comment.
Not approving. A bracket lookup returns the list element before an explicit full-key override is seen.
config.put('server.ports[1]', 9000) stores the key as written. The lookup
found the element of server.ports first and answered 8443, so put, putAll
and setAt on such a key were ignored. The stored key is now read before an
element is taken from the list.
jamesfredley
left a comment
There was a problem hiding this comment.
A bracket lookup returns the list element only when that name is not also stored as a full key. Indexed writes, scalar boundaries, and that override stay consistent.
🚨 TestLens detected 19 failed tests 🚨Failed Jobs without Test Failures❌ Build Health / Forge Projects Test SummaryTip Inspect all details with the TestLens CLI: CI / Build Grails-Core (Ubuntu JDK 21) > :grails-openapi:test
CI / Build Grails-Core (Ubuntu JDK 21) > :grails-web-url-mappings:test
CI / Build Grails-Core (Ubuntu JDK 25) > :grails-openapi:test
CI / Build Grails-Core (Ubuntu JDK 25) > :grails-web-url-mappings:test
CI / Build Grails-Core (Windows JDK 25 shard 1) > :grails-openapi:test
CI / Build Grails-Core (Windows JDK 25 shard 2) > :grails-web-url-mappings:test
CI / Build Grails-Core (macOS JDK 21) > :grails-openapi:test
CI / Build Grails-Core (macOS JDK 21) > :grails-web-url-mappings:test
CI / Build Grails-Core Rerunning all Tasks (Ubuntu JDK 21 shard 0) > :grails-async-groovy-concurrent:test
CI / Build Grails-Core Rerunning all Tasks (Ubuntu JDK 21 shard 1) > :grails-openapi:test
CI / Build Grails-Core Rerunning all Tasks (Ubuntu JDK 21 shard 2) > :grails-web-url-mappings:test
Groovy Snapshot Canary Build / Build Grails (shard 1) > :grails-openapi:test
Groovy Snapshot Canary Build / Build Grails (shard 2) > :grails-web-url-mappings:test
🏷️ Commit: 8c9c77b Test Failures (first 10 of 19)PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context (:grails-openapi:test in Groovy Snapshot Canary Build / Build Grails (shard 1))LinkGeneratorSpec > cache key should use identity of resource value (:grails-web-url-mappings:test in Groovy Snapshot Canary Build / Build Grails (shard 2))
LinkGeneratorSpec > resource links should use ident and allow controller override (:grails-web-url-mappings:test in Groovy Snapshot Canary Build / Build Grails (shard 2))
LinkGeneratorSpec > cache key should use identity of resource value (:grails-web-url-mappings:test in CI / Build Grails-Core (Ubuntu JDK 21))
LinkGeneratorSpec > resource links should use ident and allow controller override (:grails-web-url-mappings:test in CI / Build Grails-Core (Ubuntu JDK 21))
PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context (:grails-openapi:test in CI / Build Grails-Core (Ubuntu JDK 21))LinkGeneratorSpec > cache key should use identity of resource value (:grails-web-url-mappings:test in CI / Build Grails-Core (Ubuntu JDK 25))
LinkGeneratorSpec > resource links should use ident and allow controller override (:grails-web-url-mappings:test in CI / Build Grails-Core (Ubuntu JDK 25))
PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context (:grails-openapi:test in CI / Build Grails-Core (Ubuntu JDK 25))PersistentEntitySchemaSpec > describes the resource as a plain type when the application has no mapping context (:grails-openapi:test in CI / Build Grails-Core (Windows JDK 25 shard 1))Rerun ControlsSelect tests to mute in this pull request:
Reuse successful test results:
Click the checkbox to trigger a rerun:
Learn more about TestLens at testlens.app/docs. |
In Grails 8,
grailsApplication.config.getPropertyanswered a dotted key that runs past a value that is not a map with that value, andcontainsPropertyof the key wastrue. In Grails 9 such a key is absent:The same holds below a closure from
application.groovy, such asgrails.gorm.default.constraints.nullable.Two reads change without a
nullto show it. A typed read with a default gets the default, and a property source of lower precedence that sets the deeper key now answers it, as a SpringEnvironmentover the same sources does:Below a list, a whole-number segment names an element, as Spring Boot reads it, and so does an index after the name of the list, for a list from
application.ymlor fromapplication.groovy:A list of maps reads into its elements,
app.rows.0.name; an index past the end, a number with a leading zero such as01, or any other segment after a list, is absent; a map whose keys are numbers reads as before. A placeholder in a list element is not resolved byconfig, as in Grails 8; theEnvironmentresolves it.Section 21 of the Grails 9 upgrade guide and the
grails-9-upgradeskill describe the change and what an application that relied on the old answer does instead.PropertySourcesConfigSpecandNavigableMapSpeccover a scalar, a list and a closure throughgetProperty,containsPropertyandnavigate, the typed read with a default, the lower property source beside a SpringEnvironmentover the same sources, and list indexes from a list held whole and fromapplication.yml, compared with an environment withConfigurationPropertySourcesattached; the clause #16130 added on 8.1.x to pin Grails 8's answer now asserts that the key is absent.