matrei commented on PR #16047:
URL: https://github.com/apache/grails-core/pull/16047#issuecomment-5728165501
## Review
Thanks for this, generating the configuration metadata instead of
hand-maintaining it is a good direction.
I built `generateConfigurationMetadata` for `grails-databinding`,
`grails-views-gson`, `grails-views-markup` and `grails-web-url-mappings` and
inspected the generated JSON (I did not build `grails-cache`). The metadata
shipped for the converted modules looks correct today, because the curated
overlays cover every generated entry. The points below matter as soon as other
modules adopt the plugin without a complete overlay, so I think the first three
should be fixed before merge.
### `ConfigurationMetadataPlugin.groovy` (bytecode scan path)
**1. Constructor parameter types are misaligned (around line 243)**
The parameter index counts synthetic and mandated parameters, but
`argumentTypeNames` comes from the generic `Signature` attribute, which omits
them. For a Java non-static inner class with a generic constructor parameter,
e.g. `Inner(List<String> names, int size)`, `MethodParameters` reports `[this$0
(mandated), names, size]` while the signature list is `[List<String>, int]`.
`names` gets type `int` and `size` gets `null`. The null then reaches
`PropertyModel.rawType()` (`type.replaceFirst`) and fails the task with an NPE.
When no NPE occurs, wrong types are published silently.
**2. The bytecode fallback publishes a bogus `<prefix>.metaClass` property
(around line 256)**
When a Groovy class uses a non-literal prefix, e.g.
`@ConfigurationProperties(PREFIX)` as in `grails-mail`'s
`MailConfigurationProperties`, the AST transform returns without a payload and
the task falls back to scanning bytecode. That path does not exclude
`getMetaClass`/`setMetaClass` (`javap` confirms they are plain `ACC_PUBLIC`,
not synthetic), nor `setGrailsApplication`. The AST path filters both. The
result would be `grails.mail.metaClass` of type `groovy.lang.MetaClass` in the
published metadata.
**3. Getter-only nested objects are dropped (around line 298)**
The per-class filter keeps only writable, collection/map or
constructor-bound properties, so this standard Spring Boot idiom is removed
before nested-group resolution in `addProperties` can see it:
```java
private final Pool pool = new Pool();
public Pool getPool() { return pool; }
```
Neither the `prefix.pool` group nor any `prefix.pool.*` property is
generated, and there is no warning. Spring binds these at runtime, and the
Groovy AST path does handle getter-only nested properties, so the two paths
disagree.
**4. The reported property type depends on method order (around line 307)**
`addAccessor` overwrites the property type whenever a later accessor has a
generic signature. With `setPackageImports(String[])` and
`setPackageImports(List<String>)`, or a getter returning `Collection<String>`
with a setter taking `List<String>`, the last generic-bearing method visited
wins. The published type may not match the binding setter and can change when
methods are reordered. A non-generic overload visited later never corrects a
generic type set earlier.
### `ConfigurationMetadataTransformation.groovy` (AST path)
**5. Primitive types are emitted unboxed (around line 268)**
`boolean enabled = true` with no overlay entry produces `"type": "boolean"`.
Spring Boot's own processor and every curated overlay entry in this repo use
the wrapper (`java.lang.Boolean`). The bytecode path's `fieldType` does the
same. This is currently hidden only because the overlays override the type for
every shipped property.
**6. The JSON payload ships in the runtime class (around line 92)**
The payload is stored as a `static final String` constant, so every
`@ConfigurationProperties` class in the released jars carries the
`__grailsConfigurationMetadata` constant (confirmed with `javap`). It is
build-time-only data living permanently in the runtime constant pool. It is
also limited to the 65535-byte class-file constant size, so a large config tree
with deep `@NestedConfigurationProperty` expansion inlined into the root
payload would fail compilation with a cryptic "UTF8 string too long" error.
A side-car file per class, or stripping the field after aggregation, would
avoid both problems. This is a design decision that is much cheaper to settle
now than after a release.
**7. The `@Delegate` warning cannot be suppressed (around line 161)**
It fires on every compilation, even when the overlay already documents the
delegated properties. `GrailsCorsConfiguration` has `@Delegate
GrailsDefaultCorsConfiguration` and its overlay already supplies
`allowedOrigins`, `maxAge` and the rest, yet every build of
`grails-web-url-mappings` would emit a warning asking for metadata that already
exists. That is permanent noise, and it could fail a build that treats warnings
as errors. The aggregation task can see the overlay and could check coverage
there; the transform cannot.
### Tests
**8. `JsonViewGrailsPluginSpec.groovy:73` reads a private synthetic field
reflectively**
`JsonViewConfiguration.getDeclaredField('__grailsConfigurationMetadata')`
with `accessible = true` ties the test to an internal compiler artefact, which
goes against the "test via public APIs" rule in the agent guide. The
user-visible contract is the jar's
`META-INF/spring-configuration-metadata.json`, and that file is not checked.
The test also breaks if the payload mechanism changes (see point 6).
**9. `ConfigurationMetadataPluginSpec.groovy:46` hardcodes fixture versions**
The TestKit fixtures pull `org.apache.groovy:groovy:5.0.7` and
`spring-boot:4.1.0` from Maven Central instead of the versions in
`dependencies.gradle`. The transform is therefore tested against Groovy 5.0.x
rather than the 5.1.x it ships with, so a difference in AST or phase behaviour
(timing of `@Delegate`, trait composition) would go unnoticed. The tests also
need network access and will drift as the BOM is bumped. Separately, the file
is ~600 lines of inline fixture strings repeating the same build script, which
could be factored out.
### Summary
- **Should fix before merge:** 1, 2, 3
- **Worth deciding now:** 6
- **Can follow up:** 4, 5, 7, 8, 9
Points 2, 5 and 6 were confirmed against compiled output or the generated
JSON. The rest come from reading the code, so please push back if I have
misread something.
--
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]
