[codeconsole]

A parsed merge of a map that holds a dotted key inside a nested map wrote that key a second time at the root under the dotted key alone, where it can overwrite another property or create one nobody wrote:

def config = new PropertySourcesConfig([a: [b: 'root'], x: ['a.b': 'nested']])
config['a.b']                              // was 'nested', now 'root'

def other = new PropertySourcesConfig([a: [c: '1'], x: ['a.b': 'nested']])
other.getProperty('a.b')                   // was 'nested', now null
other.navigate('a', 'b')                   // was 'nested', now null

def logging = new PropertySourcesConfig([logging: [level: ['org.springframework': 'DEBUG']]])
logging.getProperty('org.springframework') // was 'DEBUG', now null

A map value under such a key wrote root keys below the dotted key as well: merge([x: ['a.b': ['c.d': 'v']]], true) also wrote a.b, a.b.c.d and c.d. The second write of the key now names the flat key, and the path it recurses with, by the full path, x.a.b and logging.level.org.springframework, and the flat key of a map under it is the map the nested maps hold. NavigableMapSpec asserts the root key set for a value, a map and a map with a dot under such a key; PropertySourcesConfigSpec covers both configs above through the subscript, getProperty, navigate and toProperties().

Found while working on #16732; the bug is the same on every branch from 7.0.x to 9.0.x.

[jdaugherty]

The fix is right, and the root-level case is unchanged: with an empty path, nestedPath is the same pathParts.join('.') as before. The grails.config and org.grails.config specs of grails-bootstrap and grails-core all pass at f54b426. With the NavigableMap change reverted, both new NavigableMapSpec features and the new PropertySourcesConfigSpec feature fail.

The description and the commit message both say config.toProperties().getProperty('a.b') used to answer nested. For that example it already answered root on 7.0.x; what answered nested was config['a.b'] and the flat a.b entry of the map itself. Could you correct both? The commit message is what lands.

[jdaugherty]

With the NavigableMap change reverted, this feature still passes this line and line 128: getProperty('a.b') and toProperties().getProperty('a.b') both answered root on 7.0.x as well. getProperty walks the nested maps first, and toProperties() visits the a map after the flat a.b entry, so the nested value wins there too. The feature only fails at line 133, on the logging half. The read the overwrite did break is config['a.b'], which answered nested. Could you add config['a.b'] == 'root' here, so the overwrite half fails without the fix as well?

The overwrite does reach getProperty when a has no b of its own. On 7.0.x, new PropertySourcesConfig([a: [c: '1'], x: ['a.b': 'nested']]) answered nested for getProperty('a.b'), toProperties().getProperty('a.b') and navigate('a', 'b'); with this PR all three answer null. That would make a good second case.

[codeconsole]

Added in 20ca350. This feature now also asserts config['a.b'] == 'root', so its overwrite half fails without the fix as well. A second feature, "a dotted key inside a nested map writes no property that the nested maps do not hold", builds [a: [c: '1'], x: ['a.b': 'nested']] and asserts that getProperty('a.b'), toProperties().getProperty('a.b') and navigate('a', 'b') are all null.

The description is corrected too. I didn't rewrite f54b426 on a branch already under review, so 20ca350's message says what the first one got wrong: that getProperty('a.b') and toProperties() already answered root for that example, and that config['a.b'] is the read the overwrite broke.

[jdaugherty]

nestedPath is also the path that the sourceValue instanceof Map branch of mergeMapEntry recurses with, so this changes more than the scalar case the specs cover. On 7.0.x:

• merge([x: ['a.b': [c: 'v']]], true) also wrote root keys a.b and a.b.c
• merge([x: ['a.b': ['c.d': 'v']]], true) also wrote a.b, a.b.c.d and c.d
• merge([x: ['a.b.c': 'v']], true) also wrote a.b.c

With this change none of those keys are written, and root x.a.b is now the same map instance that navigate('x', 'a', 'b') returns (before, it was a different one). Could you cover these too, for example as a where: table in NavigableMapSpec that asserts the root key set?

[codeconsole]

Covered in 20ca350. "A dotted key inside a nested map holding #description writes the flat keys of its full path only" is a where: table in NavigableMapSpec that asserts the root key set for each shape:

merged	root keys
[x: ['a.b': [c: 'v']]]	x, x.a.b, x.a.b.c
[x: ['a.b': ['c.d': 'v']]]	x, x.a.b, x.a.b.c.d
[x: ['a.b.c': 'v']]	x, x.a.b.c
[a: [c: '1'], x: ['a.b': 'v']]	a, a.c, x, x.a.b

A separate feature asserts that root x.a.b is the instance navigate('x', 'a', 'b') returns.

[jamesfredley]

Approving. A dotted key merged into a nested map lands under that map, and the root keys stay. 7.0.x is the right line for this fix.

[jdaugherty]

Let's fix the merge and then merge.

[codeconsole]

The merge is fixed. a0f5c30 merges 7.0.x in; the conflict was only in PropertySourcesConfigSpec, where this PR and #16770 both added features at the end, and both sets are kept.

With #16770 in, d74daf3 adds the case that needs both fixes, from @jdaugherty's review of #16770: merge([grails: ['gorm.url': 'http://${host}']]). The subscript, navigate, getProperty and the export of grails.gorm.url now read one resolved value, and no gorm.url key is written. The new feature fails on 7.0.x. Both modules' suites pass locally.

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: d74daf3
▶️ Tests: 57013 executed
⚪️ Checks: 58/58 completed

---

Learn more about TestLens at testlens.app/docs.

[jamesfredley]

Approving again. The new commit covers a placeholder in a dotted key inside a nested map. It does not undo the nested-path fix.