sbglasius commented on code in PR #16344:
URL: https://github.com/apache/grails-core/pull/16344#discussion_r4046327780
##########
grails-doc/src/en/ref/Domain Classes/lock.adoc:
##########
@@ -25,24 +25,100 @@ under the License.
=== Purpose
-The `lock` method obtains a pessimistic lock using an SQL `select ... for
update`.
+The `lock` method obtains a pessimistic lock on a database row, typically
using SQL `select ... for update`. The instance method `entity.lock()` locks an
already-loaded instance. The static method `DomainClass.lock(id)` loads and
locks by identifier, and `DomainClass.lock(id, refresh: true)` additionally
reloads the state and version of an instance that is already managed in the
current session. To reload an already-loaded instance under a pessimistic write
lock, use link:{domainClassesRefFromRef}refresh.html[refresh(lock: true)].
=== Examples
+Locking an already-loaded instance:
+
[source,groovy]
----
-def book = Book.get(1)
-book.lock()
+Book.withTransaction {
+ def book = Book.get(1)
+ book.lock() // unchanged: lock without refreshing
+ book.title = 'Updated title'
+ book.save(failOnError: true)
+}
----
+Loading and locking by identifier:
+
+[source,groovy]
+----
+Book.withTransaction {
+ def book = Book.lock(1)
+ book.title = 'Updated title'
+ book.save(failOnError: true)
+}
+----
+
+Locking by identifier and reloading an already-managed instance under the lock:
+
+[source,groovy]
+----
+Book.withTransaction {
+ def book = Book.get(1)
+ // ... work that may leave book stale relative to the database ...
+ book = Book.lock(1, refresh: true) // same managed instance, reloaded
under the lock
+ if (book.title == 'Draft') {
+ book.title = 'Ready for review'
+ book.save(failOnError: true)
+ }
+}
+----
=== Description
-The `lock` method obtains a pessimistic lock on an instance, locking the row
in the database with `select ... for update`. The `lock` method is equivalent
to using Hibernate's
https://javadoc.io/doc/org.hibernate/hibernate-core/{hibernate5Version}/org/hibernate/LockMode.html#UPGRADE[LockMode.UPGRADE]
in combination with the
https://javadoc.io/doc/org.hibernate/hibernate-core/{hibernate5Version}/org/hibernate/Session.html#lock(java/lang/Object,
org/hibernate/LockMode)[lock] method.
+Pessimistic locking requires an active transaction. Use `withTransaction` as
above or a GORM `@Transactional` method, and keep the lock and the work it
protects in the same transaction. The lock is held until that transaction
commits or rolls back, not simply until a controller action returns.
+
+The instance method `entity.lock()` is unchanged: it acquires a lock without
reloading the entity's state. For an already-loaded, versioned entity,
Hibernate still checks its version against the database; a concurrent update
between loading and locking can therefore cause an optimistic locking failure.
+
+The static `DomainClass.lock(id)` method is also unchanged, as is
`DomainClass.lock(id, [:])`: both lock without refreshing. When the entity is
not yet in the session it is loaded under the lock, avoiding a separate
unlocked `get()`. When the entity is already managed in the current session,
its already-loaded state is locked and version-checked rather than reloaded.
Review Comment:
`DomainClass.lock(id, [:])` is documented here (and in the PR description)
as a supported, unchanged form, but I don't think that call binds to anything.
The new overload is `lock(Map, Serializable)` — map first. Groovy only
hoists inline `k: v` named arguments into a leading `Map`; an explicit map
literal in second position stays positional. So `Book.lock(1, [:])` arrives as
`(Integer, LinkedHashMap)`: `lock(Serializable)` takes one argument, `lock(Map,
Serializable)` requires a `Map` first, and dispatch falls through to
`methodMissing` → `MissingMethodException`.
The forms that actually work are `Book.lock([:], 1)` and `Book.lock(1,
refresh: true)`. `Hibernate7RefreshLockSpec` only exercises `lock(args,
book.id)` (explicit map-first) and the named-argument form, so the documented
`lock(id, [:])` spelling isn't covered by a test either.
Suggest either dropping the `lock(id, [:])` claim from the docs and the PR
description, or changing it to `lock([:], id)` and adding a case for it.
##########
grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/AbstractHibernateGormInstanceApi.groovy:
##########
@@ -252,6 +266,87 @@ abstract class AbstractHibernateGormInstanceApi<D> extends
GormInstanceApi<D> {
return instance
}
+ @Override
+ D refresh(D instance, Map args) {
+ LockModeType lockMode = RefreshLockArguments.lockModeFrom(args)
+ if (lockMode == null) {
+ return refresh(instance)
+ }
+ hibernateTemplate.execute { Session session ->
+ if (!session.getTransaction().isActive()) {
+ throw new
TransactionRequiredException(REFRESH_LOCK_REQUIRES_TRANSACTION)
+ }
+ // Hibernate 5 would silently re-associate a detached instance
where Hibernate 7 rejects it,
+ // so the attachment contract is enforced here, before a detached
proxy could be initialized.
+ if (!session.contains(instance)) {
+ throw new
IllegalArgumentException(REFRESH_LOCK_REQUIRES_ATTACHED)
+ }
+ // Hibernate skips the locked refresh for an uninitialized proxy.
+ Object target = proxyHandler.unwrap(instance)
+ if (RefreshLockArguments.pessimistic(lockMode)) {
+ session.refresh(target, lockMode)
+ } else {
+ // Hibernate 5's refresh reloads the state but does not
register an optimistic mode's version
+ // check or increment; lock() does, without touching the
database until the transaction ends.
+ session.refresh(target)
+ session.lock(target, lockMode)
+ }
+ // Hibernate 5 leaves GORM dirty flags behind on everything a
native refresh reloads.
+ resetDirtyAfterRefresh(session.unwrap(SessionImplementor), target,
+ Collections.newSetFromMap(new IdentityHashMap<Object,
Boolean>()))
+ }
+ return instance
+ }
+
+ /**
+ * Resets the dirty state of a refreshed entity, its embedded components,
and every initialized association
+ * that Hibernate's refresh cascade reloaded along with it.
+ */
+ private void resetDirtyAfterRefresh(SessionImplementor session, Object
entity, Set<Object> visited) {
+ if (!(entity instanceof DirtyCheckable) || !visited.add(entity)) {
+ return
+ }
+ EntityPersister persister = session.getEntityPersister(null, entity)
+ session.factory.customEntityDirtinessStrategy.resetDirty(entity,
persister, session)
+ CascadeStyle[] cascadeStyles = persister.propertyCascadeStyles
+ Type[] types = persister.propertyTypes
+ Object[] values = persister.getPropertyValues(entity)
+ for (int i = 0; i < cascadeStyles.length; i++) {
+ if (cascadeStyles[i].doCascade(CascadingActions.REFRESH)) {
+ resetCascadedDirty(session, types[i], values[i], visited)
+ }
+ }
+ }
+
+ /**
+ * Follows a refresh cascade through the given value to the entities
Hibernate reloaded. A component
+ * cascades whenever one of its own properties does, so it is descended
into rather than treated as an
+ * entity; collections are visited element by element.
+ */
+ private void resetCascadedDirty(SessionImplementor session, Type type,
Object value, Set<Object> visited) {
+ if (value == null || !Hibernate.isInitialized(value)) {
+ return
+ }
+ if (type.isComponentType()) {
+ CompositeType compositeType = (CompositeType) type
+ Type[] subtypes = compositeType.subtypes
+ Object[] subvalues = compositeType.getPropertyValues(value,
session)
+ for (int i = 0; i < subtypes.length; i++) {
+ if
(compositeType.getCascadeStyle(i).doCascade(CascadingActions.REFRESH)) {
+ resetCascadedDirty(session, subtypes[i], subvalues[i],
visited)
+ }
+ }
+ } else if (type.isCollectionType()) {
+ Type elementType = ((CollectionType)
type).getElementType(session.factory)
+ Collection<Object> elements = value instanceof Map ? ((Map)
value).values() : (Collection<Object>) value
Review Comment:
This assumes every `Type.isCollectionType()` value is either a `Map` or a
`Collection`, but Hibernate's `ArrayType` is a `CollectionType` whose property
value is a plain Java array.
For a domain class with an array-mapped persistent collection that has
cascade REFRESH, `entity.refresh(lock: true)` reaches this branch:
`Hibernate.isInitialized(Object[])` returns `true`, `type.isCollectionType()`
is `true`, the value is not a `Map`, and `(Collection<Object>) value` throws a
`ClassCastException` — after the lock has been taken and the state already
reloaded, so the failure surfaces at the worst point.
Reachability is admittedly low (GORM's `hasMany` produces `Set`s, so this
needs an explicitly array-mapped property), but the cast is unchecked.
Something like handling `value.getClass().isArray()` via `Arrays.asList(...)`
before the `Collection` cast would close it.
--
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]
