jdaugherty commented on code in PR #16772:
URL: https://github.com/apache/grails-core/pull/16772#discussion_r4382481659
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -416,6 +416,17 @@ class NavigableMap implements Map<String, Object>,
Cloneable {
}
}
+ /**
+ * Whether the entry is one of a map written by map subscripts that the
root holds under its bracketed flat key, as
+ * {@code app.routes[/api/**]} holds the entry {@code /api/**} of the map
{@code app.routes}: that key exports it, and
+ * its dotted name, {@code app.routes./api/**}, would export it a second
time under a name Spring Boot binds as
+ * {@code api}.
+ */
+ private boolean isEntryOfBracketedKey(Map currentMap, List<String> path,
String key) {
+ path && !(currentMap instanceof NavigableMap) && rootConfig.is(this) &&
Review Comment:
The `NavigableMap` exemption leaves the duplicate in place for a common
shape of this config: a map with a plain key as well as bracketed ones, where
the plain key comes first. Spring Boot's YAML loader keeps the file's order, so
with
```yaml
app:
routes:
default: z
'[/api/**]': x
```
`app.routes.default` is merged first, `navigateSubMap` makes `app.routes` a
`NavigableMap`, and `app.routes[/api/**]` is then written into that same map.
This guard skips it, so `toProperties()` still exports `app.routes./api/**`,
and binding the export with `Binder` still gives `[api:x, default:z,
/api/**:x]`. With the plain key after the bracketed one, `app.routes` stays a
plain map and the entry is exported once. So the export depends on the order of
the keys in the file.
Both names come from the same entry, so it looks safe to skip it in a
`NavigableMap` as well. With the exemption removed and the lookup change
suggested on the next line, the existing tests still pass locally.
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -416,6 +416,17 @@ class NavigableMap implements Map<String, Object>,
Cloneable {
}
}
+ /**
+ * Whether the entry is one of a map written by map subscripts that the
root holds under its bracketed flat key, as
+ * {@code app.routes[/api/**]} holds the entry {@code /api/**} of the map
{@code app.routes}: that key exports it, and
+ * its dotted name, {@code app.routes./api/**}, would export it a second
time under a name Spring Boot binds as
+ * {@code api}.
+ */
+ private boolean isEntryOfBracketedKey(Map currentMap, List<String> path,
String key) {
+ path && !(currentMap instanceof NavigableMap) && rootConfig.is(this) &&
+ delegateMap.containsKey("${path.join('.')}[${key}]".toString())
Review Comment:
Looking the bracketed key up only in the root, by its full path, misses
three more shapes. Each one still exports the dotted duplicate at this head:
- **A subscript after a subscript**: `app.tenants[acme].routes[/api/**]`
also exports `app.tenants[acme].routes./api/**`. The merge stores the flat key
`routes[/api/**]` in the nested map at `app.tenants[acme]`, not in the root, so
the root lookup never finds it. Boot binds `[acme:[routes:[api:x, /api/**:x]]]`.
- **A nested map** (`config.navigate('app').toProperties()`):
`rootConfig.is(this)` is false, so the check is off and `routes./api/**` is
exported.
- **A clone of the root**: `clone()` keeps the original's `rootConfig`, so
the check is off there too. `root.clone().toProperties()` exports
`app.routes./api/**` and `root.toProperties()` does not.
Every map the merge writes holds the bracketed flat keys of its own entries,
relative to itself: `routes[/api/**]` in `app`, `app.routes[/api/**]` in the
root. Checking an entry against the nearest enclosing `NavigableMap`, with the
path relative to that map, covers these three and the shape on the previous
line, and both guards can go. I tried this shape locally:
```groovy
private void flattenKeys(Map<? extends Object, Object> flatConfig, Map
currentMap, List<String> path, boolean forceStrings) {
flattenKeys(flatConfig, currentMap, path, forceStrings, this, 0)
}
private void flattenKeys(Map<? extends Object, Object> flatConfig, Map
currentMap, List<String> path, boolean forceStrings, NavigableMap owner, int
ownerDepth) {
NavigableMap childOwner = currentMap instanceof NavigableMap ?
(NavigableMap) currentMap : owner
int childOwnerDepth = currentMap instanceof NavigableMap ? path.size() :
ownerDepth
currentMap.each { key, value ->
String stringKey = String.valueOf(key)
List<String> relative = path.subList(ownerDepth, path.size())
if (value != null && !(relative &&
owner.containsKey("${relative.join('.')}[${stringKey}]".toString()))) {
// ... recurse with childOwner, childOwnerDepth
```
With it, all four shapes export only the bracketed key, and your new
features still pass. The full `:grails-bootstrap:test` and the
`org.grails.config`, `grails.config`, `grails.spring` and `org.grails.core`
specs in grails-core also pass.
##########
grails-core/src/test/groovy/org/grails/config/PropertySourcesConfigSpec.groovy:
##########
@@ -116,4 +119,34 @@ class PropertySourcesConfigSpec extends Specification {
then: 'the second value is not merged with the first value'
cfg.foo.bar == ['two': '2']
}
+
+ void 'a map written by map subscripts is exported by its bracketed keys
only, which Spring Boot binds as written'() {
Review Comment:
Could the tests cover the shapes from the two comments on
`isEntryOfBracketedKey` too? That is a plain key before a bracketed one in the
same map (loaded through `YamlPropertySourceLoader`, so the order is the
file's), a subscript after a subscript, and `toProperties()` on a nested map
and on a clone.
The description and the commit also say `toFlatConfig()` is fixed, but only
`toProperties()` is asserted. `ConfigMapSpec` in grails-bootstrap already holds
the `toFlatConfig()`/`toProperties()` flattening tests for `NavigableMap`, so a
clause there through `merge(map, true)` would keep coverage in the module that
owns the class.
--
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]
