matrei commented on PR #16344:
URL: https://github.com/apache/grails-core/pull/16344#issuecomment-5719967264
## Review round 5
**Head:** `d60b7a74fc` (7 commits) · **Base:** `8.1.x` · Merge-base
`0980623`, `git diff --check` clean.
**What I ran locally** (`cleanTest` + `--no-build-cache`, XML report
timestamps verified):
| Run | Result |
|---|---|
| `grails-data-hibernate5-core:test` (full, incl.
`Hibernate5RefreshLockSpec` 77/77) | 915 tests, 0 failures, 39 skipped |
| `grails-data-hibernate7-core:test` (full, incl.
`Hibernate7RefreshLockSpec` 79/79) | 3158 tests, 0 failures, 29 skipped |
| `codeStyle` on both modules | clean |
| GitHub checks | 76 passed, 1 failed (see below), 1 pending |
Both round-4 items are in: `lockRow` picks the concrete entity for
union-mapped hierarchies and the root otherwise, both specs gain the
table-per-concrete-class competing-commit pair, and the optional round-3 item
is done too (`recordLockMode` sets the entry directly for
`PESSIMISTIC_READ`/`WRITE`, with a statement-count feature pinning 2 vs 3). The
`loaded.await || refreshing.get` diagnostic nit is applied in all three
competing-commit features. Good.
**The failing check is unrelated.** *Functional Tests (Java 25, indy=false)*
fails in `grails-test-examples-scaffolding:integrationTest`,
`UserControllerSpec > User list`, with a Geb `WaitTimeoutException` on the
*Please sign in* page (`LoginPage.login`). The same job passed on the previous
head `d2f76c9e3b`, the Java 21 and shard-1 Java 25 variants of the job pass on
this head, and nothing in the two new commits touches that module. A rerun
should clear it.
As before I ran throwaway specs against both versions for what the new tests
do not reach: proxies in the session, repeated lock calls in one transaction,
and the plain lock forms on the union hierarchy. One of them found a new
problem in the union branch.
### Findings
#### 1. Hibernate 7: the union branch fails with `HibernateSystemException`
when the managed instance is a root-typed proxy (medium)
`lockRow` now names the instance's concrete entity in the HQL but still
binds `instance` itself. When the static form finds a *root-typed* proxy in the
session (`findManagedInstance` deliberately returns the proxy) for a row whose
concrete class is a subclass, the query is `select 1 from Sub e where e =
:instance` with a `Root$HibernateProxy` bound to a `Sub`-typed parameter.
Hibernate cannot coerce it and fails before anything is locked:
```
org.grails.orm.hibernate.support.hibernate7.HibernateSystemException:
Could not convert 'grails.gorm.tests.ProbeR5bUnionRoot$HibernateProxy' to
'java.lang.Long'
using 'org.hibernate.type.descriptor.java.LongJavaType' to wrap
```
Reproducer (`Root` mapped `tablePerConcreteClass true`, `id generator:
'increment'`; `Sub extends Root`; row is a `Sub`):
```groovy
Sub.withTransaction {
def proxy = Root.load(id)
Hibernate.initialize(proxy) // or any access that initializes it,
e.g. proxy.title
Root.lock(id, refresh: true) // throws on Hibernate 7
}
```
An initialized root-typed proxy is what a lazy to-one association declared
with the root type holds after first use, so this is reachable from ordinary
code. Joined and single-table hierarchies are unaffected because their query
names the root and the proxy is root-typed. Hibernate 5 handles the same shape
(`session.refresh(proxy, lockMode)` unproxies internally): all my proxy
variants pass there. The instance form on Hibernate 7 is also fine because
GORM's proxy forwards `refresh` to the target, so `lockRow` receives the real
`Sub`.
Probe matrix on Hibernate 7 (`Root.load(id)` first, then the static call;
contender is a raw JDBC `FOR UPDATE` on the concrete table):
| Proxy state | Call | `d60b7a74fc` | with the fix |
|---|---|---|---|
| uninitialized | `Root.lock(id, refresh: true)` | not-loaded branch, `find`
through the union, **no lock** (pre-existing `lock(id)` limit, see #2) | same |
| **initialized** | `Root.lock(id, refresh: true)` |
**`HibernateSystemException`** | locked, `PESSIMISTIC_WRITE` |
| uninitialized | `Sub.lock(id, refresh: true)` | locked (find on the
concrete table) | same |
| uninitialized | `Sub.load(id)` then `Sub.lock(id, refresh: true)` |
locked, returns the proxy | same |
**Fix (verified locally):** bind the unproxied target.
`session.getEntityName(instance)` has already initialized the proxy in this
branch, so it costs nothing there:
```groovy
.setParameter('instance', Hibernate.unproxy(instance))
```
With that one-line change all 11 probe features pass and
`Hibernate7RefreshLockSpec` still passes 79/79. If you would rather keep an
uninitialized proxy uninitialized on the joined/single-table branches (where
binding the proxy works today), make it `descriptor instanceof
UnionSubclassEntityPersister ? Hibernate.unproxy(instance) : instance`. Please
add the reproducer above as a feature next to the union competing-commit one.
#### 2. Docs: the union-subclass NOTE describes the limit too narrowly (low)
The new NOTE in the Hibernate 7 guide says the union rendering applies to
"an already-loaded instance of a class that has subclasses". On Hibernate 7 the
entity's `session.lock` always goes through the *root* persister, so it applies
to every class in the hierarchy, leaf included. Measured on H2 with `Sub` a
leaf under a union root (contender = raw JDBC `FOR UPDATE` on the concrete
table after the call):
| Form on a union hierarchy | Row locked? |
|---|---|
| `Sub.get(id).lock()` | **no** (`select ... from (select ... from
probe_union_sub) ... for update`) |
| `Sub.lock(id)` with the instance managed | **no** |
| `Root.lock(id)` / `Root.lock(id, refresh: true)` not loaded | **no**
(`find` through the union) |
| `Sub.lock(id)` not loaded | yes (plain concrete table) |
| `refresh(lock: true)` / managed `lock(id, refresh: true)` on a leaf
instance, loaded via `Sub` or `Root` | yes (this PR) |
Suggested wording: *"In a hierarchy mapped with `tablePerConcreteClass
true`, Hibernate 7 locks through a union of the concrete tables whenever it
goes through the hierarchy root: `lock()` and `lock(id)` on a managed instance
of any class in the hierarchy, and `lock(id)` or `lock(id, refresh: true)`
through the root class for a row not yet loaded. Some databases, H2 among them,
do not lock rows through such a query. `refresh(lock: true)` and `lock(id,
refresh: true)` on a managed instance of a class without subclasses, and
`lock(id)` through that class for a row not yet loaded, lock its concrete table
directly."* The `lockRow` javadoc's last sentence ("the same limit `lock()`
has") could say `lock()` has it for every class in the hierarchy. The plain
`lock()` behaviour itself is pre-existing Hibernate 7 and deserves a separate
issue, not this PR.
#### 3. `recordLockMode` can downgrade the recorded lock (low)
Setting the entry unconditionally records a weaker mode than the transaction
holds, which Hibernate's own `upgradeLock` never does
(`requestedLockMode.greaterThan(entry.getLockMode())`). Measured on Hibernate
7, same transaction:
| Sequence | Recorded after (`getCurrentLockMode`) | Before `2842f51fe0` |
|---|---|---|
| `lock()` then `refresh(lock: PESSIMISTIC_READ)` | `PESSIMISTIC_READ` |
`PESSIMISTIC_WRITE` |
| `refresh(lock: true)` then `refresh(lock: PESSIMISTIC_READ)` |
`PESSIMISTIC_READ` | `PESSIMISTIC_WRITE` |
| `refresh(lock: PESSIMISTIC_FORCE_INCREMENT)` then `refresh(lock: true)` |
`PESSIMISTIC_WRITE` | `PESSIMISTIC_FORCE_INCREMENT` |
The database lock is unaffected (the exclusive lock is still held), so the
consequence is only what `getCurrentLockMode` reports and a redundant
version-checked re-lock statement on a later `lock()`. Guarding with `if
(requested.greaterThan(entry.lockMode)) entry.setLockMode(requested)` restores
Hibernate's semantics. Optional.
### Verified as correct
- **Union branch (real instances):** `select 1 from <Sub> e ... for update`
on the concrete table for `Sub.get`, `Root.get` (concrete class `Sub`) and the
static forms; all four competing-commit rows pass on both versions in the fresh
full runs. The `#16349` reference for the insert-then-update version start is
accurate (I saw the same with the `table` generator in round 4).
- **`recordLockMode`:** the statement-count feature holds (2 for
`PESSIMISTIC_READ`/`WRITE`, 3 for `FORCE_INCREMENT`). Follow-ups in the same
transaction behave: `refresh(lock: true)` then `lock()` issues 0 statements and
keeps `PESSIMISTIC_WRITE`; then plain `refresh()` issues 1 and keeps it
(7.4.7's HHH-19937 restore); `refresh(lock: PESSIMISTIC_READ)` then
`refresh(lock: true)` upgrades to `PESSIMISTIC_WRITE`; a second `refresh(lock:
true)` costs 2 again. `EntityEntryImpl.setLockMode` throws
`UnsupportedLockAttemptException` for a non-mutable persister, which is exactly
what `session.lock` threw before, and GORM's mapping DSL has no immutable
keyword, so no behaviour change there.
- **Proxies on the instance form:** `Sub.load(id).refresh(lock: true)` locks
and returns the proxy on both union and joined hierarchies.
`Root.load(id).refresh(lock: true)` returns the target rather than the proxy,
but so do plain `refresh()`, `lock()` and `save()` on a root-typed proxy (GORM
proxy forwarding, pre-existing), so nothing new.
- **Hibernate 5:** every static and instance form with a root or leaf proxy
in the session works on union and joined hierarchies, and the union hierarchy
locks the concrete table for every form, so no Hibernate 5 change is needed.
- **Tests and helpers:** the H5 union entities use `id generator:
'increment'`, the H7 spec asserts versions relative to the saved instance with
an accurate comment, and the shared helpers are unchanged.
**Verdict:** request changes for finding 1 (deterministic exception on a
supported mapping, one-line fix verified). Findings 2 and 3 at your discretion,
though the NOTE wording is worth fixing while you are in the file.
--
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]
