matrei commented on PR #16292:
URL: https://github.com/apache/grails-core/pull/16292#issuecomment-5570803357
AI review:
# Review Findings
This PR bundles four themes on `@GrailsBeans`: diagnostics for previously
silent failures, new declaration forms (`bean(name, Interface, Impl)`,
`group(...)`), the remaining `@Conditional*` qualifiers, and a
`-Dgrails.beans.dsl.dumpdir` build-time introspection hook. The implementation
is defensive, the diagnostic messages are actionable, and the spec grew ~4400
lines with matched happy-path and error-path coverage for every new form. The
following items should be addressed before merge.
## [P1] `OnGrailsEnvCondition` reflection catch misses `NoClassDefFoundError`
**File:**
`grails-beans-dsl/src/main/java/org/grails/compiler/beans/OnGrailsEnvCondition.java:76-93`
The reflective lookup into `grails.util.Environment` is guarded by:
```java
catch (ReflectiveOperationException | RuntimeException ignored) {
// Not a Grails application, or an Environment that cannot answer - fall
back to the
// property rather than fail a condition the rest of the context depends
on.
return null;
}
```
The comment describes the "Grails not on the classpath" case, but
`Class.forName("grails.util.Environment", ...)` raises `ClassNotFoundException`
(caught) only when the class is missing from the loader that resolves it. When
the class is *referenced* from bytecode that cannot be linked, or when a
transitive class it needs is absent, the JVM raises `NoClassDefFoundError` — a
`LinkageError`, which is neither a `ReflectiveOperationException` nor a
`RuntimeException`. In that case the condition throws out of `matches(...)` and
Spring fails the enclosing configuration.
Please broaden the catch to also cover `LinkageError` (or `Throwable`, with
a targeted rethrow of `Error` subclasses you do want to surface such as
`VirtualMachineError`), so the fallback matches the documented intent. Add a
direct unit test for `OnGrailsEnvCondition` — the class is currently exercised
only end-to-end through generated bytecode in the transform spec, and the
"Grails absent" branch is not covered.
## [P2] `ClassLoader` fallback in `OnGrailsEnvCondition` should also try the
TCCL
**File:**
`grails-beans-dsl/src/main/java/org/grails/compiler/beans/OnGrailsEnvCondition.java:78`
```java
ClassLoader loader = classLoader != null ? classLoader :
getClass().getClassLoader();
```
`ConditionContext.getClassLoader()` can legitimately return `null` early in
Spring's bootstrapping. The current fallback to `getClass().getClassLoader()`
is fine for the common case where `grails-beans-dsl` and `grails-core` share a
loader, but it can miss a Grails `Environment` class supplied by a parent
context loader in a multi-loader setup (e.g. Boot's `LaunchedClassLoader`,
application server deployments, native-image agents). Adding
`Thread.currentThread().getContextClassLoader()` as a third rung matches
Spring's own class-presence checks and costs nothing here.
## [P2] Stray-statement diagnostic changes plugin-descriptor compile behavior
**File:**
`grails-core/src/main/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformation.groovy:412-478`
The rule "any `beans` closure with at least one `bean/field/method`
top-level statement must be entirely DSL" is correct for the DSL's own
semantics, and the message is thorough about both the "typo/`if` around beans"
case and the "not the DSL — rename it" case. However, extending this to
`grails.plugins.Plugin` subclasses (commit `6509f29929`) turns a formerly
silent quirk into a compile error: any pre-8.0 plugin descriptor with a stray
`bean(...)` call sitting inside a `beans` property that Grails never read now
fails compilation.
The justification given in the doc-comment is sound — an undiscovered
miswiring in a plugin surfaces far away in downstream applications, so loudness
matters more, not less. But this is a behavior change even though it is a bug
fix. Please flag it in the release notes for 8.0 and, if there is a
`grails-doc` migration section, add a note there. See CLAUDE.md rule #7
(user-facing changes require doc coverage).
## [P3] Direct unit test missing for `OnGrailsEnvCondition`
**File:**
`grails-beans-dsl/src/test/groovy/org/grails/compiler/beans/GrailsBeansASTTransformationSpec.groovy`
`OnGrailsEnvCondition` is the runtime side of `.conditionalOnGrailsEnv(...)`
and has no dedicated spec. The transform tests confirm the annotation is
attached correctly, but they do not exercise:
- `matches(...)` returning `false` when
`metadata.getAnnotationAttributes(...)` is `null`.
- Fallback to the `grails.env` property when `grails.util.Environment` is
not on the classpath.
- Case-insensitive matching.
A small standalone Spock spec wiring a mock `ConditionContext` and
`AnnotatedTypeMetadata` locks the contract in, is cheap, and would catch a
regression in the [P1] catch fix above.
## [P3] Documentation coverage for the new DSL surface
**Files:** `grails-doc/**` (no changes in this PR)
The Javadoc on `@GrailsBeans`, `@ConditionalOnGrailsEnv` and the new
qualifiers is thorough and self-contained. If the beans-DSL reference in the
user guide is generated from the annotation Javadoc, this is already covered.
If the guide has hand-written sections that enumerate the qualifiers or
declaration forms, they need entries for `bean(name, Interface, Impl)`,
`group(...)`, the five new `.conditionalOn*` forms, `.aliases(...)`,
`.typeArguments(...)`, and the `-Dgrails.beans.dsl.dumpdir` build property.
## What I Verified
- Read the full diff of all seven changed files against `origin/8.0.x`
(+6499/-1929, 40 commits).
- Traced every `addError(...)` and `addErrorAndContinue(...)` site in
`GrailsBeansASTTransformation.java` for accurate source positioning at the
offending node.
- Confirmed `ClassNode` comparisons use `.equals(...)` (no `==` slip-ups),
null-guards on AST accessors, and closure-boundary handling in the
`CodeVisitorSupport` used by the sibling-bean rejection.
- Confirmed the `dumpdir` writer explicitly uses `StandardCharsets.UTF_8`
and reports failures via `addError(...)` rather than a runtime exception.
- Confirmed both new Java files (`ConditionalOnGrailsEnv.java`,
`OnGrailsEnvCondition.java`) carry the Apache license header, use `jakarta.*`
where applicable, avoid wildcard imports, and use 4-space indentation.
- Spot-checked the spec for happy-path and error-path coverage of every new
declaration form, qualifier, and diagnostic.
--
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]
