jamesfredley commented on PR #15561:
URL: https://github.com/apache/grails-core/pull/15561#issuecomment-4262379221
## Review feedback addressed in dc7c4a079c
### 1. Validate `-PhibernateVersion` and fail fast (Copilot)
> `hibernateVersion` is accepted as an arbitrary string and silently falls
back to "Hibernate 5 behavior" for any value other than `'7'`.
Added validation in `gradle/functional-test-config.gradle` that throws a
`GradleException` when the value is not `'5'` or `'7'`:
```
./gradlew :grails-test-examples-gorm:help -PhibernateVersion=6
...
> Unsupported hibernateVersion '6'. Expected '5' or '7'.
```
### 2. Rename counter-intuitive booleans (Copilot, @sanjana2505006)
> `isHibernate5 == false` actually means "this *is* an hibernate5-labeled
project".
Renamed to positive-meaning names and collapsed the nested `if` statements
in the `onlyIf` block so the reader no longer has to negate mentally:
```groovy
// before
boolean isHibernate5 =
!project.name.startsWith('grails-test-examples-hibernate5')
if (project.hasProperty('skipHibernate5Tests')) {
if (!isHibernate5) { return false }
}
// after
boolean isHibernate5Project =
project.name.startsWith('grails-test-examples-hibernate5')
if (project.hasProperty('skipHibernate5Tests') && isHibernate5Project) {
return false
}
```
Same pattern for `isHibernate7Project` and `isMongoTaskProject`. Behavior is
unchanged.
### 3. Fix blocking checkstyle failure in `PredicateGenerator.java`
The Core Projects CI check was failing with `SingleSpaceSeparator` on the
aligned switch-arm arrows introduced in the "fix 3 H7 GORM bugs" commit.
Removed the extra spaces.
```java
// before
case ADD -> cb.sum(propertyPath, pa.operand());
case DIVIDE -> cb.quot(propertyPath, pa.operand());
// after
case ADD -> cb.sum(propertyPath, pa.operand());
case DIVIDE -> cb.quot(propertyPath, pa.operand());
```
Verified locally with `./gradlew
:grails-data-hibernate7-core:checkstyleMain` - now passes.
---
### Open strategy questions (not addressed here)
@jdaugherty - your two strategic points are acknowledged and deserve their
own discussion:
1. **Duplicate vs. substitute test projects.** Module substitution does
surface the ehcache-style conflict you flagged (addressed in this PR with
explicit `exclude` rules for `hibernate-ehcache` and
`jboss-transaction-api_1.3_spec`). If we want stronger guarantees against other
silent conflicts, the alternative is duplicating the general test projects
under `grails-test-examples/hibernate7/` the same way the h5-labeled projects
are mirrored. Happy to split that into a follow-up PR once we decide.
2. **Merge path for just the fixes.** The H7 bug fixes in
`HibernateQueryExecutor`, `HqlQueryContext`, `PredicateGenerator`,
`PropertyReference`, `PropertyArithmetic`, `HibernateDetachedCriteria`, and
`HibernateGormStaticApi` (commits 707c33d6, a458c4a6) are independent of the CI
matrix. I can cherry-pick them into a separate PR against `8.0.x-hibernate7` if
that unblocks the bug-fix merge while this matrix discussion continues.
### Remaining CI item
The latest run had 1 remaining failure: `UserControllerSpec > User list` in
`grails-test-examples-scaffolding` - the page loaded "Please sign in" instead
of "User List", i.e. a Spring Security redirect, not a GORM/H7 issue. Looks
like test setup rather than a product bug, but I'll re-verify on the next CI
run after this push.
Assisted-by: claude-code:claude-opus-4
--
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]
