jamesfredley commented on PR #15730:
URL: https://github.com/apache/grails-core/pull/15730#issuecomment-4731153755
Thanks for the thorough review @codeconsole. Pushed `791565333e` addressing
the hardening notes:
**1 - `versionProperties` precedence (the `create-app` path).** Added a
`GrailsDependencyVersionsSpec` feature that wires a Grails BOM declaring
`groovy.version` and importing a Spring Boot BOM that declares a *conflicting*
`groovy.version`, then asserts `versionProperties['groovy.version']` resolves
to the Grails value (first-writer-wins). This is exactly the invariant
`CreateAppCommand` relies on when stamping a new app, and it now has direct
coverage so a regression would fail the build instead of silently emitting the
wrong Groovy.
**2 - Silent-failure blast radius in `resolveImportedBom`.** Expanded the
`catch` comment to call out that the swallowed-failure impact is now much wider
than when only Grails BOMs were followed - Spring Boot's managed versions exist
*only* behind the recursed import, so one silent failure drops them all. Kept
it as a comment since logging isn't idiomatic in this module, as you noted.
**3 - Precedence-ordering assumption.** Added a comment on the resolution
loop noting that ordering among third-party imports is just POM declaration
order and otherwise undefined, and that conflict-prone artifacts (Groovy,
Spock) stay correct only because they're direct constraints that
first-writer-wins pre-empts - with a warning not to move such a pin into a BOM
import without restoring an equivalent direct constraint.
**5 - Near-dead `startsWith('${')` guard.** Clarified the comment to say the
`null` check is the meaningful guard (`versionLookup` returns `null` for an
unresolved `${...}` reference); `startsWith('${')` only catches a malformed
reference with no closing brace.
**4 - Single-arg `find(String)` first-writer flip.** Left as-is - as
flagged, the two-arg `find` used internally is unaffected and the impact is
near-zero, so no behavior change seemed warranted.
The change is comments plus the one new spec (no production behavior
change). `./gradlew :grails-shell-cli:test --tests
"org.grails.cli.boot.GrailsDependencyVersionsSpec"` is green (8/8, including
the new feature).
The `selenium` pin remains deferred as a separate follow-up, per the open
item already noted on this PR.
--
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]
