codeconsole commented on PR #15730:
URL: https://github.com/apache/grails-core/pull/15730#issuecomment-4724678972
## Review
Solid, well-reasoned PR. The premise is correct: a `grails-bom` pin equal to
Spring Boot's managed version is dead weight that only blocks free
patch/security updates. The `dependencies.gradle` removals are clean, the
`graphql-java` tripwire and `spring-retry` retention are good calls, and the
trickiest part — making the legacy CLI's `GrailsDependencyVersions` follow
third-party BOMs — is handled carefully with deferral, first-writer-wins
precedence, and cycle protection. Reworking the spec to use placeholder
versions instead of hard-coded releases is the right move. Mergeable as-is; the
notes below are mostly hardening.
Thanks for the spec rework in `635df22` — that resolves my earlier question
about hard-coded BOM versions.
### Recommendations (prioritized)
**1 — Highest value: missing test for `versionProperties` precedence (the
`create-app` path).**
`CreateAppCommand` reads
`profileDependencyVersions.versionProperties.get('groovy.version')` (and
`gorm.version`, `grails-gradle-plugins.version`) to stamp a **newly created
app's** build. This PR changes `getVersionProperties()` from "replaced per POM"
to an accumulated `putIfAbsent` merge that now also recurses into
`spring-boot-dependencies`, which declares a conflicting `groovy.version`
(5.0.x). It works today only because the root `grails-bom` declares
`groovy.version` (4.0.32) in its own `<properties>` and is merged before the
Spring Boot recursion runs — first-writer-wins protects it. But that invariant
has **no test coverage**; every new spec asserts on `find(...)`, none on the
`versionProperties` map. If it ever broke, `grails create-app` would silently
stamp Grails 8 apps with Groovy 5. Suggest a spec where both the Grails BOM and
a recursed third-party BOM declare the same `*.version` property, asserting the
Grails value wins in `versions.versionProperties`.
**2 — Silent-failure blast radius in `resolveImportedBom`.**
The empty `catch (Exception e) { }` ("skip it") predates this PR, but its
impact is now much larger: Spring Boot's versions exist **only** behind the
recursed import, so a silently-swallowed resolution failure drops *all* Spring
Boot-managed versions at once, with no signal. Logging isn't idiomatic in this
module, so this is a judgment call — at minimum worth a comment acknowledging
the wider blast radius, or a debug log if a logger is acceptable here.
**3 — Document the precedence-ordering assumption.**
"Grails imports before third-party imports" only separates
`org.apache.grails` from everything else. Among third-party imports
(`groovy-bom`, `selenium-bom`, `spock-bom`, `spring-boot-dependencies`),
precedence is just POM declaration order. It's safe *today* for the
conflict-prone artifacts (Groovy, Spock) only because those are **direct**
constraints in `dependencies.gradle`, so first-writer-wins pre-empts every
import. A one-line comment on the resolution loop noting "direct constraints
above protect us; ordering among third-party imports is otherwise undefined"
would stop a future maintainer from moving a Groovy pin into a BOM import and
silently regressing it.
**4 — Minor: `find(String artifactId)` single-arg now resolves first-writer
for ambiguous artifactIds.**
`io.reactivex.rxjava2:rxjava` and `io.reactivex.rxjava3:rxjava` share
artifactId `rxjava`; the new first-writer guard flips single-arg
`find('rxjava')` from last- to first-writer. The two-arg `find` (used
everywhere internally) is unaffected, so impact is near-zero — just flagging it.
**5 — Nit: the `!version.startsWith('${')` guard is nearly dead.**
After `versionLookup`, `version` is either resolved, a literal, or `null`.
The meaningful check is the `null` one (skip unresolved-property deps);
`startsWith('${')` only triggers on a malformed `${foo` with no closing brace.
Harmless; the comment could just say "skip deps whose version property didn't
resolve."
### Verified and fine
- Cycle/duplicate protection (`resolvedBoms`, keyed
`group:artifact:version`) correctly handles `spring-boot-dependencies` being
imported by both `grails-bom` and `grails-base-bom` — covered by the "resolved
exactly once" spec.
- The per-POM `localProperties` change genuinely fixes the recursion
property-clobbering bug and is more Maven-accurate (properties resolve in their
declaring POM).
- Removing the `jakarta.servlet` / `jakarta.validation` excludes is
consistent with removing the matching direct pins — both now inherit `6.1.0` /
`3.1.1` from Spring Boot.
- Groovy `4.0.32` precedence holds because all groovy artifacts are direct
constraints.
### Open item the PR already flags
`selenium` is left pinned at `4.38.0`, now *behind* Spring Boot's `4.43.0`.
Dropping it would be a version bump with Geb/functional impact rather than pure
inheritance, so deferring to a separate follow-up is reasonable.
</content>
</invoke>
--
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]
