jamesfredley commented on PR #15625:
URL: https://github.com/apache/grails-core/pull/15625#issuecomment-4364588452
### Self-review pass: applied targeted feedback
I ran a multi-agent self-review against codebase patterns, prior reviewer
history (PR #15614 / jdaugherty feedback), upstream `cyclonedx-gradle-plugin`
v3.0.0 source, and architectural correctness. Force-pushed three small
refinements:
1. **Defensive file-existence guard** in `rewriteSbomFile`. The aggregate
task's `task.jsonOutput.isPresent()` check confirms the property is set, but
doesn't guarantee the file exists on disk. Added `if (!f.exists() || f.length()
== 0) { logger.warn(...); return }` to fail soft if the upstream task produced
no output.
2. **Method-level Javadoc on `configureAggregateSbomReproducibility`**
explaining when this hook fires (Spring Boot 4's `CycloneDxPluginAction`
registers `cyclonedxBom` and packages output at
`META-INF/sbom/application.cdx.json`) and why the strip set is broader than the
direct task's (no explicit externalReferences fallback).
3. **Method-level Javadoc on `FieldDefinition.builder()` and
`PropertyDefinition.builder()`** explaining that the qualified
`FieldDefinition.Builder` / `PropertyDefinition.Builder` return type is
intentional for groovydoc reproducibility. Without this, a future "simplify
return type" refactor would silently re-introduce the bug.
### Empirical groovydoc verification
I noticed I'd never actually generated groovydoc to confirm the
qualified-Builder fix produces correct output. Did so now (build twice with
`SOURCE_DATE_EPOCH` set):
```
✅ FieldDefinition.html builder() correctly links to
.../FieldDefinition.Builder.html
✅ PropertyDefinition.html builder() correctly links to
.../PropertyDefinition.Builder.html
✅ Both HTMLs byte-identical across two consecutive runs
```
Compared to v8.0.0-M1's published javadoc (where every Builder reference
linked to PropertyDefinition.Builder regardless of which class was being
documented), this is the correct, deterministic output.
### Review findings I considered but rejected
- **Reorganize all imports in SbomPlugin.groovy**: scope creep. My only
addition (`CyclonedxAggregateTask`) was alphabetically correct in its position
relative to existing `org.cyclonedx.*` imports. The pre-existing `java.*`
ordering quirk (with `AtomicBoolean` separated from other `java.*` imports) is
the project's existing convention.
- **Revert qualified return types in FieldDefinition/PropertyDefinition**:
that was the bug. Empirically verified via the groovydoc rebuild above.
- **Add SbomPluginSpec unit tests**: no existing test infrastructure for
build-logic plugins. Behavioral verification via the integration build (run
twice, byte-identical) is the established pattern for this code per
CONTRIBUTING.md.
### Pre-existing tech debt noted (NOT addressed in this PR)
- Both `configureSbomTask` (existing) and
`configureAggregateSbomReproducibility` (new) capture `project` in `Provider`
closures, which would break Gradle's configuration cache if it were enabled.
The new method matches the existing pattern. Should be a follow-up issue
covering both call sites - happy to file one if desired.
--
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]
