matrei commented on PR #16344:
URL: https://github.com/apache/grails-core/pull/16344#issuecomment-5694779374
## Review of #16344 — `refresh(lock: true)` / `lock(id, refresh: true)`
**Head:** `0bf4554fcc` (`feat/gorm-lock-latest`, 2 commits) · **Base:**
`8.1.x` · Merge-base `0980623`, `git diff --check` clean.
**What I ran locally** (`--no-build-cache` + `cleanTest`, XML mtimes
verified):
| Run | Result |
|---|---|
| `grails-datamapping-core:test` (full) | 961 pass, 1 skipped |
| `grails-datamapping-support:test` (full) | 36 pass |
| `grails-data-hibernate5-core:test` (full, incl.
`Hibernate5RefreshLockSpec` 23/23) | 861 pass, 39 skipped |
| `grails-data-hibernate7-core:test` (full, incl.
`Hibernate7RefreshLockSpec` 23/23) | 3102 pass, 29 skipped |
| `codeStyle` on the four modules | clean |
I also read the Hibernate 5.6.15 and 7.4.7 sources
(`DefaultRefreshEventListener`, `SessionImpl`, `EntityEntryImpl`) to check the
claims in the two instance-API comments. Both hold: Hibernate 5 returns early
from a refresh on an uninitialized proxy, Hibernate 7 refreshes it; and
Hibernate 7's `EntityEntryImpl.postLoad` calls
`CustomEntityDirtinessStrategy.resetDirty`, which is why only the Hibernate 5
implementation resets GORM dirty state by hand. Finally I ran a throwaway spec
on both Hibernate versions to probe a few behaviours the new tests don't cover;
the results are quoted where relevant.
The design is good. `book.refresh(lock: true)` says what it does,
`Book.lock(id, refresh: true)` is Map-first so the named-argument form binds
under both dynamic and static compilation, the `default` methods keep
third-party datastores compiling, and the instance-form coverage in the two
specs is thorough. The findings below are one real bug in the static form, two
doc claims that don't match the code, and a few smaller items.
### Findings
#### 1. `Book.secondary.lock(id, refresh: true)` runs the locked refresh on
the **default** connection (high)
`GormStaticApi.groovy:501-513` implements the static form generically:
`get(id)` on this API's connection, then `refresh(instance, [lock: true])`.
That second call goes through `GormStaticApi.refresh(D, Map)` at line 253,
which resolves the instance API with `registry.findInstanceApi(persistentClass,
null)`. The `null` qualifier always yields the **default** connection's
instance API, so for a named-connection static API the entity is fetched from
`secondary` and then refreshed on `default`.
Probe (entity saved and loaded on `secondary`, then
`RoutedBook.secondary.lock(id, refresh: true)` inside
`secondary.withTransaction`):
| | Outcome |
|---|---|
| Hibernate 5 | `HibernateObjectRetrievalFailureException: No row with the
given identifier exists` — the default database has no such row; entity left
unrefreshed with its pending edit |
| Hibernate 7 | `IllegalArgumentException` wrapping
`DetachedObjectException: Given entity is not associated with the persistence
context` — the default session doesn't hold the instance |
The instance form is fine, because `book.secondary.refresh(lock: true)` goes
through `DelegatingGormEntityApi`, which was built from the correct instance
API. Only the static form is affected, and it has no named-connection test (the
two `secondary` features in each spec exercise `book.secondary.refresh(lock:
true)` only).
Two ways to fix:
- **Self-contained (recommended):** override `lock(Map, Serializable)` in
both `HibernateGormStaticApi` classes and do the refresh-with-lock on the API's
own `hibernateTemplate`, which is always bound to the right session factory.
This also lets you look the id up in the persistence context
(`SessionImplementor.getPersistenceContextInternal().getEntity(generateEntityKey(id,
persister))`) instead of calling `get(id)`, which removes the extra unlocked
SELECT for an entity that isn't loaded yet (see finding 4).
- **Registry-based:** replace `null` with `qualifier` in the new `refresh(D,
Map)` call. Note this only works on Hibernate 7: on Hibernate 5,
`HibernateGormEnhancer.getStaticApi` and
`HibernateGormApiFactory.createStaticApi` construct the static API through the
datastore constructor and never pass the qualifier through, so a `secondary`
static API reports `qualifier == 'default'` (verified by probe). That's a
pre-existing bug behind the other `findInstanceApi(persistentClass, null)`
calls in `GormStaticApi` too, but fixing it is a separate change.
Either way, add `RoutedBook.secondary.lock(id, refresh: true)` to both
specs. On Hibernate 5 the named-connection test must nest
`secondary.withTransaction { secondary.withSession { … } }` (as the existing
`secondary` features already do); the other nesting gives you a session that
isn't the transactional one.
#### 2. Detached instances behave differently on Hibernate 5 and 7, and the
difference is neither documented nor tested (medium)
`refresh.adoc:69` and both `locking.adoc` files say *"The instance must be
attached to the current session; a detached instance can be re-attached with
`attach` first"*, which implies a single behaviour when it isn't attached.
Probe (evict, edit, `book.refresh(lock: true)` inside a transaction):
| | Outcome |
|---|---|
| Hibernate 5 | **Succeeds**: silently reattached, reloaded to
`title=original`, `PESSIMISTIC_WRITE` held |
| Hibernate 7 | `IllegalArgumentException` wrapping
`DetachedObjectException` |
Hibernate 5's `DefaultRefreshEventListener` treats an entity without an
`EntityEntry` as transient and refreshes it anyway, reassociating it; Hibernate
7's throws. So the same documented contract yields a silent reattach on one
implementation and a Hibernate-internal exception on the other, and that
exception is neither of the two the Javadoc lists.
Suggested fix: add `if (!session.contains(instance)) throw new
IllegalArgumentException(...)` after the transaction check in both instance
APIs (before `proxyHandler.unwrap` on Hibernate 5, so a detached proxy isn't
initialized), document the exception in the `@throws` lists, and add a "rejects
a detached instance" feature to both specs. If you'd rather keep Hibernate 5's
reattach, the docs need to say the two versions differ; I think consistency is
the better outcome for a new API.
#### 3. The named-connection doc example cannot work (medium)
`grails-doc/src/en/ref/Domain Classes/refresh.adoc:78-83`:
```groovy
Book.withTransaction {
def book = Book.get(1)
book.secondary.refresh(lock: true) // <1>
}
```
`Book.withTransaction` is a transaction on the default datasource, so the
`secondary` session has none and this throws `TransactionRequiredException`.
Your own feature *"refresh(lock: true) requires a transaction on the named
connection even when the default transaction is active"* proves it.
`Book.get(1)` also loads the instance into the default session, so on Hibernate
7 it would be detached from `secondary` even with the right transaction. Should
be:
```groovy
Book.secondary.withTransaction {
def book = Book.secondary.get(1)
book.secondary.refresh(lock: true)
}
```
#### 4. Static form does work before the transaction check, and costs an
extra SELECT (low)
Because `GormStaticApi.lock(Map, Serializable)` calls `get(id)` first and
the transaction check lives inside the instance API's `refresh(lock: true)`:
- With no transaction, an entity that isn't loaded is SELECTed (unlocked)
and an uninitialized proxy is initialized before `TransactionRequiredException`
is thrown. Probe on both versions: `proxyInitializedBefore=false
proxyInitializedAfter=true`. The instance form has a feature asserting the
opposite (*"rejects a missing transaction before initializing a proxy"*); the
static form silently doesn't hold that property.
- For an entity that isn't loaded, the static form issues two statements
(plain `get`, then `SELECT … FOR UPDATE` via refresh) where `lock(id)` issues
one. `lock.adoc:79` says the not-loaded case is "loaded and locked", which
reads as one operation.
The persistence-context lookup from finding 1 fixes both: check the
transaction first, refresh-with-lock if the entity is managed, otherwise fall
through to `lock(id)`.
#### 5. `book.lock(refresh: true)` fails with a misleading message on
Hibernate 5 (low)
The docs correctly say there is no `entity.lock(refresh: true)` and that
Groovy binds it to the static `lock(Serializable)` with the map as the id. What
the user actually sees:
| | Outcome |
|---|---|
| Hibernate 5 | `IllegalArgumentException: id to load is required for
loading` |
| Hibernate 7 | `IllegalArgumentException: Argument '{refresh=true}' could
not be converted to the identifier type …` |
Hibernate 7's message is usable; Hibernate 5's says the id was missing,
which sends people looking in the wrong place. Now that `refresh: true` is a
documented option, this call will be attempted. A two-line guard in
`GormEntity.lock(Serializable id)` (`if (id instanceof Map) throw new
IllegalArgumentException("… use ${name}.lock(id, refresh: true) or
book.refresh(lock: true)")`) gives the same helpful message on every datastore,
and a one-feature test covers it.
#### 6. Hibernate 5: dirty reset does not follow refresh cascades (low, not
verified by a test)
`AbstractHibernateGormInstanceApi.groovy` resets dirty state on the root
entity and its embedded components after the locked refresh. `session.refresh`
also cascades to associations mapped with `cascade: 'refresh'`/`'all'`, and
those refreshed children keep their GORM dirty flags on Hibernate 5 (there is
no `resetDirty` in Hibernate 5's post-load path, which is why the manual reset
exists for the root). At the next flush `GrailsEntityDirtinessStrategy.isDirty`
would report such a child as dirty and Hibernate would issue an UPDATE with the
freshly refreshed values: a no-op write plus a spurious version bump on the
child.
Plain `refresh()` on Hibernate 5 has the same behaviour today, so not
blocking. But the docs explicitly say *"Configured refresh cascades can also
discard unflushed changes to associated entities"*, so users will hit this
path. A sentence in the Hibernate 5 guide noting the version-bump caveat would
do, ideally with a test.
#### 7. Minor / nits
- `ARGUMENT_LOCK`, `ARGUMENT_REFRESH` and `REFRESH_LOCK_UNSUPPORTED` are
public constants on the public `GormInstanceOperations` /
`GormStaticOperations` interfaces. `REFRESH_LOCK_UNSUPPORTED` in particular is
an implementation message leaking into API; a `private static final` in the
implementing classes would keep the surface unchanged.
- `GormInstanceOperations`, `GormStaticOperations`, `GormEntityApi` and
`GormEntity` Javadoc declare `@throws
jakarta.persistence.TransactionRequiredException`. That is a JPA type on
datastore-agnostic interfaces. Fine while only the Hibernate datastores
implement the option, but if MongoDB/Neo4j ever do they'll need a different
exception; "an implementation-specific exception if no transaction is active"
would age better.
- `GormEntityApi.refresh(Map)` is added as an `abstract` trait method.
Consistent with the rest of that trait, but it is the one place the change is
not strictly source-compatible for anything implementing `GormEntityApi`
directly rather than through `GormEntity`. A trait body throwing
`UnsupportedOperationException` would make the whole PR strictly additive. Your
call.
- PR title and description still describe `lockLatest()`; please update them
before merge.
- The checklist ticks "verified that all existing tests pass by running
`./gradlew build --rerun-tasks`" while the description says that command was
**not** run. Please untick or run it.
### Verified as correct
- `Book.lock(id, refresh: true)` and `Book.lock(refresh: true, id)` both
bind to the two-argument overload under dynamic and static compilation, and no
existing one-argument `lock` call changes meaning (checked with a standalone
dispatch script against a trait shaped like `GormEntity`, before and after
adding the overload).
- `ClassUtils.getBooleanFromMap` handles a null map, a missing key, `null`,
`Boolean` and string values, so `refresh([:])`, `refresh(lock: false)` and
`lock([:], id)` all take the plain path. Tested in both specs and the core
specs.
- `default` methods on `GormInstanceOperations` / `GormStaticOperations`
keep third-party implementations compiling; `TenantDelegatingGormOperations`
overrides both and restores the tenant on success and failure (tested).
- The transaction check runs before `proxyHandler.unwrap` in the instance
form, so a missing transaction never initializes the proxy (tested in both
specs). Hibernate 7's own `checkTransactionNeededForLock` only fires when
`hibernate.allow_update_outside_transaction` is false, so the explicit check is
needed to make the contract unconditional.
- `refresh(Object, LockOptions)` is the non-deprecated Hibernate 5.6
overload; `refresh(Object, LockModeType)` maps to `refresh(entity,
LockOptions)` in 7.4.7.
- The Hibernate 5 `resetDirty` call goes through the session factory's
`CustomEntityDirtinessStrategy`, which defaults to a no-op if a user has
replaced GORM's strategy, so no NPE risk.
- Instance-form coverage is complete: stale reload and save, flush modes,
embedded dirty tracking, proxies (initialized and not), non-versioned entities,
transaction requirement with an open session, named connections and their
transaction requirement, and lock contention until commit. The contention test
is a real end-to-end check of `FOR UPDATE` semantics on H2, not just a
`getCurrentLockMode` assertion.
- What's New entry, `lock` and `refresh` reference pages, both Hibernate
guide sections and the `ormdsl` cross-reference are present; the `lockLatest`
reference page and its includes are cleanly removed.
**Verdict:** request changes for #1 (a real bug on named connections, no
test), #2 and #3. #4 and #5 fall out naturally if #1 is fixed the
self-contained way. #6 and #7 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]
