jdaugherty commented on code in PR #16344:
URL: https://github.com/apache/grails-core/pull/16344#discussion_r4047161011
##########
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:
Right, thanks. `Book.lock(1, [:])` arrives as `(Integer, LinkedHashMap)` and
matches neither overload. Fixed in `c4f3fee64f`: the reference page, the What's
New page and both Hibernate guides now name `lock(id, refresh: false)`, which
is the form both specs already exercise (the `'refresh: false'` row of *"static
lock(id, args) without a refresh request locks the managed instance"*), and the
PR description says the same.
##########
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:
Thanks, fixed in `c4f3fee64f`, and probing it turned up a bigger gap in that
branch. Hibernate 5 runs the refresh cascade over the graph as it stands
*before* the reload, and the reload then replaces the root's collections with
new, uninitialized wrappers. The post-refresh walk therefore bailed out at
`Hibernate.isInitialized` for every collection and never reset the elements the
cascade had reloaded: a `hasMany` child edited and then discarded by
`parent.refresh(lock: true)` still reported `title` in
`listDirtyPropertyNames()` and scheduled an update at the next flush. The
reloaded entities are now gathered from the pre-refresh graph and reset once
the refresh has run, and elements are iterated with
`CollectionType.getElementsIterator(value, session)`, Hibernate's own iterator,
which handles sets, lists, map values and arrays without the cast. Both specs
gain a `hasMany` cascade feature that checks the element dirty state and the
absence of a spurious update at flush. For the rec
ord, GORM's mapping factory treats arrays as basic values, so an `ArrayType`
collection cannot come out of the GORM binder, but the unchecked cast is gone
either way.
--
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]
