borinquenkid commented on PR #15939:
URL: https://github.com/apache/grails-core/pull/15939#issuecomment-4920388023
*(Analysis assisted by Claude Code — I reviewed and am posting the findings
below, but flagging the tooling for transparency.)*
Nice feature — the method-based `doWithSpring(BeanBuilder)` hook and its
dispatch through `DefaultGrailsPlugin` are correctly implemented and well
tested for `Plugin`-based plugin descriptors.
However, the PR description claims broader coverage than the diff actually
delivers, and I think that gap is worth closing (or the description narrowing)
before merge.
## The hook isn't wired into `GrailsApplicationLifeCycle` at all
The description says the new hook is "added as a `default` no-op on
`GrailsApplicationLifeCycle`" and "overridden as a no-op on ...
`GrailsApplicationLifeCycleAdapter`", and that **both** dispatch sites —
`DefaultGrailsPlugin` and `GrailsApplicationPostProcessor` — invoke both hooks.
Checking the diff against `21a937de06c` (this PR's head):
- `GrailsApplicationLifeCycle.groovy` is untouched — the interface still
only declares `Closure doWithSpring()`:
https://github.com/apache/grails-core/blob/21a937de06ccd318036e1d7cb1b0370c243225dd/grails-core/src/main/groovy/grails/core/GrailsApplicationLifeCycle.groovy#L29-L36
- `GrailsApplicationLifeCycleAdapter.groovy` is untouched — same, only the
closure-returning override:
https://github.com/apache/grails-core/blob/21a937de06ccd318036e1d7cb1b0370c243225dd/grails-core/src/main/groovy/grails/core/GrailsApplicationLifeCycleAdapter.groovy#L34
- `GrailsApplicationPostProcessor.groovy` still only calls the closure form:
```groovy
if (lifeCycle) {
def withSpring = lifeCycle.doWithSpring()
if (withSpring) {
def bb = new BeanBuilder(null, springConfig,
application.classLoader)
bb.beans(withSpring)
}
}
```
https://github.com/apache/grails-core/blob/21a937de06ccd318036e1d7cb1b0370c243225dd/grails-core/src/main/groovy/grails/boot/config/GrailsApplicationPostProcessor.groovy#L227-L231
- The new method exists only on `Plugin`, and (correctly, given the above)
carries no `@Override` — it isn't implementing anything from the interface:
https://github.com/apache/grails-core/blob/21a937de06ccd318036e1d7cb1b0370c243225dd/grails-core/src/main/groovy/grails/plugins/Plugin.groovy#L109-L121
## Why this matters in practice
`GrailsApplicationPostProcessor`'s `lifeCycle` field is typed
`GrailsApplicationLifeCycle`, and a standard Grails app's `Application` class
reaches it via `GrailsAutoConfiguration implements GrailsApplicationClass` →
`GrailsApplicationClass implements GrailsApplicationLifeCycle` (not `Plugin`):
-
https://github.com/apache/grails-core/blob/21a937de06ccd318036e1d7cb1b0370c243225dd/grails-core/src/main/groovy/grails/core/GrailsApplicationClass.groovy#L27
-
https://github.com/apache/grails-core/blob/21a937de06ccd318036e1d7cb1b0370c243225dd/grails-core/src/main/groovy/grails/boot/config/GrailsAutoConfiguration.groovy#L49
So as written, only actual `grails.plugins.Plugin` subclasses dispatched
through `DefaultGrailsPlugin` (i.e. plugin descriptors) can use the new hook.
An app's own `Application` class — or any other `GrailsApplicationLifeCycle`
implementor that isn't a `Plugin` — has no `doWithSpring(BeanBuilder)` method
to override, and nothing would call it if it did.
## Test coverage mirrors the gap
Every fixture class in `PluginDoWithSpringMethodSpec` extends `Plugin`
(`MethodHookPlugin`, `NoOpPlugin`, `MethodHookGrailsPlugin`,
`BothHooksGrailsPlugin`, `LegacyClosureGrailsPlugin`). Nothing exercises
`GrailsApplicationLifeCycleAdapter` or `GrailsApplicationPostProcessor`, which
is consistent with the interface/adapter/post-processor never actually being
touched:
https://github.com/apache/grails-core/blob/21a937de06ccd318036e1d7cb1b0370c243225dd/grails-core/src/test/groovy/grails/plugins/PluginDoWithSpringMethodSpec.groovy
## Suggestion
Either:
1. Add `doWithSpring(BeanBuilder)` as a default no-op on
`GrailsApplicationLifeCycle`, override it on
`GrailsApplicationLifeCycleAdapter`, and dispatch it from
`GrailsApplicationPostProcessor` alongside the closure form (with a test
covering that path) — so the feature actually matches the description, or
2. Narrow the PR description/docs to state this hook applies to plugin
descriptors specifically (via `Plugin`/`DefaultGrailsPlugin`), not to the
general `GrailsApplicationLifeCycle` contract or the application 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]
