matrei commented on PR #16344:
URL: https://github.com/apache/grails-core/pull/16344#issuecomment-5703596659
## Review round 2
**Head:** `293fb7768c` (3 commits) · **Base:** `8.1.x` · Merge-base
`0980623`, `git diff --check` clean.
**What I ran locally** (`cleanTest` + `--no-build-cache`, XML reports
verified):
| Run | Result |
|---|---|
| `grails-datamapping-core:test` (full) | 983 pass, 1 skipped |
| `grails-data-hibernate5-core:test` (full, incl.
`Hibernate5RefreshLockSpec` 43/43) | 881 pass, 39 skipped |
| `grails-data-hibernate7-core:test` (full, incl.
`Hibernate7RefreshLockSpec` 43/43) | 3122 pass, 29 skipped |
| `codeStyle` on the three modules | clean |
Thanks for the thorough round. Everything from round 1 is addressed and I
re-verified each item (list at the end). The Hibernate 5 dirty reset now
follows the refresh cascade, the detached-instance contract is enforced
identically on both versions, the doc example is fixed, the static form no
longer does the unlocked `get`, `book.lock(refresh: true)` gets a useful
message, the constants moved off the public interfaces, and
`GormEntityApi.refresh(Map)` has a body so the trait stays source-compatible.
The `type` argument is a sensible generalisation and its parsing
(`RefreshLockArguments`) is well covered.
I again ran throwaway specs against both Hibernate versions to probe the new
code paths. Two of them found real problems, one of them serious.
### Findings
#### 1. Hibernate 7: `refresh(lock: true)` acquires **no database lock at
all** when a refresh-cascaded association is join-fetched (high)
`HibernateGormInstanceApi.groovy:266-271` sets `Locking.FollowOn.IGNORE` on
every locked refresh. In Hibernate 7.4.7 that option does not "skip follow-on
locking and keep the root row locked"; it makes the SQL translator drop the
lock clause entirely. `AbstractSqlAstTranslator.determineLockingStrategy`
returns `LockStrategy.NONE` for `IGNORE` whenever the select contains an outer
join and the dialect cannot lock outer-joined rows
(`H2LockingSupport.getOuterJoinLockingType() == IGNORED`, same for PostgreSQL).
The `Locking.FollowOn.IGNORE` Javadoc says as much: *"This can lead to rows not
being locked when they are expected to be."*
The new cascade test doesn't catch this because it only asserts
`session.getCurrentLockMode(parent) == PESSIMISTIC_WRITE`, which is the lock
mode Hibernate *recorded* on the entity entry, not evidence that a lock was
taken. Probe (H2, `Hibernate7RefreshLockCascadeParent` from your spec, `child
cascade: 'all', lazy: false`): transaction A does `parent.refresh(lock: true)`
and holds; transaction B runs a raw `select id from
hibernate7_refresh_lock_cascade_parent where id = ? for update` over JDBC.
| Scenario | SQL issued by the refresh | Contender acquired the row while A
held it? |
|---|---|---|
| `Hibernate7RefreshLockBook` (no association), `refresh(lock: true)` | `...
where id=? for update` | no (correct) |
| `Hibernate7RefreshLockCascadeParent`, `refresh(lock: true)` | `select ...
from ..._parent left join ..._child ... where id=?` — **no `for update`** |
**yes** |
| `Hibernate7RefreshLockCascadeParent.lock(id, refresh: true)`, entity not
in session (goes through `session.find`, default follow-on) | locked via
follow-on select | no (correct) |
So on H2 and PostgreSQL, any entity with an eagerly fetched refresh-cascaded
association silently gets an unlocked refresh while `getCurrentLockMode`
reports `PESSIMISTIC_WRITE`. For a feature whose whole purpose is "reload under
the lock", that is worse than the NullPointerException it replaces. Hibernate 5
is fine: its refresh issues the parent select `for update` and refreshes the
child in a separate unlocked select (verified in the same probe).
I did verify the NullPointerException you worked around: with the default
follow-on strategy, `session.refresh(parent, new
LockOptions(PESSIMISTIC_WRITE))` on H2 with the child already in the
persistence context fails with `Cannot invoke
"EntityInitializer.getNavigablePath()" because the return value of
"EntityHolder.getEntityInitializer()" is null`. I couldn't find a matching
upstream ticket in a quick search. The static form (`session.find` with the
parent not yet loaded, child loaded or not) does not hit it.
Suggested fix: don't let Hibernate decide whether the row is locked. One
option that works on every dialect and keeps the "reload the version under the
lock" semantics is to take the row lock first, by identifier and **without** a
version check, and then run a plain (cascading) refresh under it, e.g. an
HQL/criteria query for the id with `LockModeType.PESSIMISTIC_WRITE` (no join
fetch, so no outer join, no follow-on) followed by `session.refresh(instance)`.
Two statements, but no window between them in which a stale version can fail
the lock, because the lock is already held when the refresh reads. If you would
rather keep the single-statement path, use `Locking.FollowOn.DISALLOW` so the
unsupported case fails loudly instead of silently unlocking, and document the
limitation. Either way, please add a contention test for the cascade parent
modelled on your existing *"waits for a competing commit and holds the write
lock"* feature (a raw JDBC `for update` contender is the most direc
t check), because `getCurrentLockMode` cannot detect this class of bug.
Related: `session.refresh(Object, LockOptions)` is `@Deprecated(since =
"7.0", forRemoval = true)` in Hibernate 7. The previous revision used the JPA
`refresh(Object, LockModeType)` overload, which is not deprecated. If a
`LockOptions` is still needed after the fix, `session.refresh(instance,
(RefreshOption) lockOptions)` reaches the supported varargs overload.
#### 2. Hibernate 5: locked refresh throws `Unknown entity` for an embedded
component that holds a cascading association (medium)
`AbstractHibernateGormInstanceApi.resetDirtyAfterRefresh` walks
`persister.propertyCascadeStyles` and recurses into every value whose style
cascades `REFRESH`. Hibernate computes a component property's cascade style as
`ALL` as soon as any sub-property cascades
(`Property.getCompositeCascadeStyle`), so an embedded component containing a
cascading association is passed to `resetDirtyAfterRefresh` as if it were an
entity. It is `DirtyCheckable` (embedded classes carry `@DirtyCheck`), so the
method calls `session.getEntityPersister(null, component)`, and
`MetamodelImpl.entityPersister` throws for a class that is not an entity.
Probe (H5): `@Entity class Owner { Details details; static embedded =
['details'] }`, `@DirtyCheck class Details { String note; CascadeChild child;
static mapping = { child cascade: 'all' } }`:
```
propertyCascadeStyles = [STYLE_NONE, STYLE_NONE, STYLE_ALL] // version,
title, details
owner.refresh(lock: true)
-> HibernateSystemException: Unknown entity:
grails.gorm.tests.Hibernate5ProbeDetails
```
Plain `refresh()` on the same entity works, and the row is already locked
when this throws, so the caller gets an exception after the refresh has
succeeded. Fix: in the loop, treat a component value as a container rather than
an entity. `persister.propertyTypes[i].isComponentType()` tells you; for
components, reset via the existing `resetDirty(root, ...)` (which already
handles embedded objects) and recurse into the component's own cascading
sub-properties, or simply skip values that are not entities
(`session.factory.metamodel.entityPersisters.containsKey(...)` /
`Hibernate.getClass`). Please add an embedded-with-association case to
`Hibernate5RefreshLockSpec`.
#### 3. `Book.lock(id, refresh: true)` returns the proxy's target instead of
the proxy the caller holds (low)
`findManagedInstance` uses `persistenceContext.getEntity(key)`, which
returns the underlying entity for an initialized proxy, so the static form
refreshes and returns the implementation object. `Book.lock(id)` (both
versions) and `Book.get(id)` (Hibernate 7) return the proxy itself. Probe,
initialized proxy in session:
| | H5 | H7 |
|---|---|---|
| `Book.get(id).is(proxy)` | false | true |
| `Book.lock(id).is(proxy)` | true | true |
| `Book.lock(id, refresh: true).is(proxy)` | **false** (returns unproxied
target) | **false** (returns unproxied target) |
The docs say the call "returns that same managed instance", and a caller
comparing with `is()` or relying on the returned object being the one they hold
in a collection will be surprised. The uninitialized-proxy case goes through
`session.find`, which returns the narrowed proxy (verified: `Book.lock(id,
refresh: true).is(uninitializedProxy)` is true on both versions), so the two
proxy states behave differently. Cheapest fix: after `getEntity`, also check
`persistenceContextInternal.getProxy(key)` and return the proxy when one exists
(refresh the unwrapped target, return the proxy). One feature per spec with an
initialized proxy would pin it.
#### 4. Minor
- `GormStaticApi.lock(Map, Serializable)` (core) still resolves the instance
API with a `null` qualifier for the `refresh: true` path. Both Hibernate static
APIs now override it, so nothing shipped is affected, but any future datastore
that implements `refresh(D, Map)` without overriding `lock(Map, Serializable)`
inherits the round-1 named-connection bug. Passing `qualifier` there costs
nothing.
- `Hibernate7RefreshLockSpec` / `Hibernate5RefreshLockSpec` cascade feature:
`session.getCurrentLockMode(...)` assertions should be complemented by a
contention check (see finding 1); as written they would pass with no lock taken.
- PR title and description still describe `lockLatest()`; the checklist
still ticks `./gradlew build --rerun-tasks` while the text says it was not run.
### Round-1 items, re-verified
- **Round-1 item 1, named connection:** `HibernateGormStaticApi.lock(Map,
Serializable)` on both versions now does the whole operation on its own
`hibernateTemplate`; the new *"uses the named connection rather than the
default database"* feature checks the JDBC URL and that the default database is
untouched. Good.
- **Round-1 item 2, detached instances:** `session.contains` check before
`proxyHandler.unwrap` on both versions, `IllegalArgumentException` with the
same message, tested with and without a proxy, proxy not initialized. Good.
- **Round-1 item 3, doc example:** now `Book.secondary.withTransaction {
Book.secondary.get(1) ... }` with the explanatory callout. Good.
- **Round-1 item 4, static form:** transaction check first,
persistence-context lookup, single locked `find` when not loaded
(statement-count assertion added). Good.
- **Round-1 item 5, `book.lock(refresh: true)`:** guard in
`GormEntity.lock(Serializable)`, tested in core and both Hibernate specs. Good.
- **Round-1 item 6, cascade dirty reset (H5):** implemented and tested
(`discards cascaded child edits without a spurious child update at flush`),
documented in the H5 guide. Good, modulo finding 2 above.
- **Round-1 item 7, nits:** constants moved to `RefreshLockArguments`,
Javadoc no longer promises a JPA exception type on the datastore-agnostic
interfaces, `GormEntityApi.refresh(Map)` has a default body with a
direct-implementation test. Good.
- `grails-doc/build.gradle` `hibernate7Guide` attribute:
`https://grails.apache.org/docs/latest/grails-data/hibernate7/manual/` resolves
(200), so the new links work.
**Verdict:** request changes for finding 1 (silent no-lock on H2/PostgreSQL
with an eager refresh-cascaded association, and the test cannot see it) and
finding 2 (crash). Findings 3 and 4 at your discretion.
--
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]
