matrei commented on PR #16048:
URL: https://github.com/apache/grails-core/pull/16048#issuecomment-5731031297
Deriving metadata from the DSL defaults instead of duplicating them by hand
is a nice idea. The parser handles the shapes in `DefaultSecurityConfig.groovy`
well, but it is quite permissive about what it treats as configuration and
quite silent about what it skips. The points below are about those two edges,
plus the state of the branch.
I ran the parser on small sample inputs for points 1, 2, 4, 5 and 7, and
checked the merge state for point 8. The rest come from reading the code.
### Wrong metadata (`GroovyDslConfigurationMetadataParser.groovy`)
**1. Local variable declarations become configuration properties (around
line 133)**
```groovy
security {
def tmp = 'x'
String other = 'y'
}
```
emits `<prefix>.tmp` and `<prefix>.other` with String types and default
values. `DeclarationExpression` extends `BinaryExpression` with an assignment
operation, so it passes the assignment check. Skipping `expression instanceof
DeclarationExpression` fixes it. Any helper local in a registered DSL file
currently ends up in the published `spring-configuration-metadata.json`.
**2. Any method call with a closure argument becomes a nested section
(around line 140)**
```groovy
security {
items.each { item -> visited = true }
}
```
emits `<prefix>.each.visited` (Boolean, default `true`). The receiver of the
call is never checked, so `with {}`, `tap {}` and `someList.each {}` all
fabricate property paths. Only calls with an implicit-this receiver
(`expression.implicitThis`) should be treated as sections.
**3. Type merging ignores branches whose type could not be inferred (around
line 80)**
```groovy
if (x) { timeout = 'PT30S' } else { timeout = computeTimeout() }
```
is published as `java.lang.String`, although the other branch may yield an
Integer or a Duration. The same happens for an unconditional `foo = 5` followed
by an environment override `foo = bar()`. Null literals and genuinely unknown
expressions are conflated here. I think only null constants should be ignored;
any other uninferable branch should drop the type.
**4. `System.getenv()` without arguments is typed `java.lang.String` (around
line 218)**
Every `System.getenv(...)` / `System.getProperty(...)` call is typed as
String without looking at the arguments, but `System.getenv()` returns a
`Map<String, String>`. Requiring at least one argument before inferring String
is enough.
**5. Map literals are emitted as `defaultValue` JSON objects (around line
256)**
`m = [k: [1, 2]]` produces `"defaultValue": {"k": [1, 2]}`, and the plugin
spec asserts `options.defaultValue == [maxSessions: 1, rememberMe: true]`. The
Spring Boot metadata format only allows a scalar or an array of scalars for
`defaultValue`, so IDEs and other consumers may reject or mis-render it. In
Spring's property model a map-valued setting is really the keys `prefix.m.k[0]`
and so on, not one property with an object default. I would omit the default
for maps (and for lists containing maps or lists).
### Silently missing metadata
**6. Missing DSL files and unmatched root names are ignored (around line
52)**
An explicitly registered file that does not exist is dropped by
`file.isFile()`, and a `dslRootPrefixes` key that matches no top-level call is
skipped without a diagnostic. If `DefaultSecurityConfig.groovy` is moved or
renamed, or the path in `dslConfigurationFiles.from(...)` has a typo, the task
still succeeds and all DSL-derived properties vanish from the published
metadata. The plugin spec currently asserts this silent behaviour when the DSL
source is deleted. Since these files are registered explicitly, I think a
missing file, or a root name that matches nothing, should fail the task (or at
least warn).
**7. Only top-level `root { ... }` calls are parsed (around line 99)**
```groovy
security.top = 5
environments {
production {
security { envOnly = 1 }
}
}
```
emits neither property, and nothing is reported. Both are idiomatic
ConfigSlurper forms. The `environments` and `if` handling added in
`parseStatements` is unreachable at the top level of the file. `parseFile`
could route top-level statements through the same statement walker, with the
root-prefix mapping applied.
### Branch state and design
**8. The branch no longer merges into its base**
It is 508 commits behind `feature/automated-configuration-metadata`, and
`git merge-tree` reports two conflicts:
- A content conflict in `ConfigurationMetadataPlugin.groovy`. The base now
reads per-class side-car payloads (`PAYLOAD_DIRECTORY` instead of
`PAYLOAD_FIELD`), reads the overlay once up front, and added
`addDelegatedProperties`.
- A modify/delete conflict on
`grails-core/src/main/resources/META-INF/spring-configuration-metadata.json`.
The base added `grails.gorm.defaultIdType` (String, default `long`) to that
file, and this PR deletes it. Resolving by keeping the deletion would drop that
property from the grails-core metadata, so it needs to be ported into
grails-core's `additional-spring-configuration-metadata.json`.
**9. The overlay silently overrides DSL-derived defaults
(`ConfigurationMetadataPlugin.groovy`, around line 405)**
I compared the parser output for `DefaultSecurityConfig` with the curated
overlay. All 143 DSL properties are already fully specified in the overlay,
including `type` and `defaultValue`, so for Spring Security the DSL parsing
currently contributes nothing to the published result. For `providerNames`,
`voterNames` and `logout.handlerNames` the two even disagree: the DSL default
is `[]`, while the overlay publishes non-empty lists such as
`[daoAuthenticationProvider, ...]`. The build neither warns nor fails on the
mismatch.
That means a future edit to a default in `DefaultSecurityConfig` will not
reach the metadata unless the overlay is edited by hand as well, which is the
duplication this PR set out to remove. Two options: drop the redundant `type`
and `defaultValue` fields from the overlay and keep only descriptions and hints
there, or keep them and warn when a DSL default and an overlay default differ.
The three list properties probably need a decision either way, since the
effective runtime default is not what the DSL literal says.
**10. `config.adoc` (around line 128) documents a build-internal mechanism**
The new paragraph says "Framework modules can also register legacy Groovy
configuration DSL sources for static metadata discovery". That describes
`org.apache.grails.buildsrc.configuration-metadata`, an unpublished convention
plugin in `build-logic`, so application and plugin authors cannot use it. It
also goes against the "no internal APIs in docs" rule in the agent guide. I
would drop the paragraph from the user guide, or reduce it to a statement of
what the published metadata covers.
### Summary
- **Should fix before merge:** 1, 2, 6, 8
- **Worth deciding now:** 9
- **Can follow up:** 3, 4, 5, 7, 10
--
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]
