codeconsole commented on code in PR #16344:
URL: https://github.com/apache/grails-core/pull/16344#discussion_r4053949389
##########
grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormStaticApi.groovy:
##########
@@ -151,7 +180,90 @@ class HibernateGormStaticApi<D> extends
AbstractHibernateGormStaticApi<D> {
@Override
D lock(Serializable id) {
- (D) hibernateTemplate.lock((Class)persistentClass,
convertIdentifier(id), LockMode.PESSIMISTIC_WRITE)
+ if (!persistentEntity.isMultiTenant()) {
+ return (D) hibernateTemplate.lock((Class) persistentClass,
convertIdentifier(id), LockMode.PESSIMISTIC_WRITE)
+ }
+ // Hibernate's tenant filter does not apply to a load by identifier,
so a multi-tenant row is loaded
+ // through a query the way get(id) does, rather than handed to
whichever tenant asks for the id.
+ Serializable identifier = convertIdentifier(id)
+ if (identifier == null) {
+ return null
+ }
+ (D) hibernateTemplate.execute { Session session ->
+ lockedLoad(session, identifier, LockModeType.PESSIMISTIC_WRITE)
+ }
+ }
+
+ @Override
+ D lock(Map args, Serializable id) {
+ LockModeType lockMode = RefreshLockArguments.lockTypeFrom(args)
+ boolean refresh = RefreshLockArguments.refreshRequested(args)
+ if (!refresh && lockMode == LockModeType.PESSIMISTIC_WRITE) {
Review Comment:
**Medium: distinguish an omitted type from an explicitly supplied
PESSIMISTIC_WRITE.** `Book.lock(id, type: LockModeType.PESSIMISTIC_WRITE)`
takes this early return and bypasses the new transaction check. Hibernate 5's
legacy path permits the operation without an active transaction, so an
auto-commit connection can release the lock immediately, contradicting the
documented requirement for calls passing `type`. Restrict legacy delegation to
calls without an explicit type (while preserving `lock(id, refresh: false)`
compatibility), and route explicitly typed calls through transaction
validation. Apply the same dispatch rule to Hibernate 7 for consistency. Please
add a no-transaction test for this exact call, including the string form of the
mode.
For example, change the early-return condition in both implementations to:
```groovy
boolean explicitType = args?.containsKey(RefreshLockArguments.TYPE) == true
if (!refresh && !explicitType && lockMode == LockModeType.PESSIMISTIC_WRITE)
{
return lock(id)
}
```
Explicitly typed calls then proceed to the transaction-checked path below.
Separately decide/document how an explicitly supplied null type and a null
identifier should behave; the existing null-identifier return still precedes
transaction validation.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormInstanceApi.groovy:
##########
@@ -136,7 +137,14 @@ class GormInstanceApi<D> extends AbstractGormApi<D>
implements GormInstanceOpera
@Override
def <T> T mutex(D instance, Closure<T> callable) {
execute({ Session session ->
- session.lock(instance)
+ if (supportsLockedRefresh()) {
+ // Reload the row under the lock instead of version-checking
the state already loaded, so that a
+ // competing writer is waited for and the closure runs on the
committed state. Always an
+ // exclusive lock: a shared or optimistic one would not give
the closure mutual exclusion.
+ refresh(instance, [(RefreshLockArguments.LOCK): true])
Review Comment:
**High: keep refreshing opt-in for existing mutex calls.** This branch
changes `book.title = 'Updated'; book.mutex { book.save() }` from preserving
the pending edit to silently discarding it on Hibernate. It also changes
stale-version conflict handling and attachment requirements. Upgrade
documentation acknowledges this, but supporting a new capability should not
automatically change another existing API's semantics. Please retain the
previous `mutex(Closure)` locking behavior and expose refreshing through an
explicit option or separate operation. Add a public-API regression test proving
the existing form preserves pending edits.
For example, preserve the existing implementation:
```groovy
@Override
def <T> T mutex(D instance, Closure<T> callable) {
execute({ Session session ->
session.lock(instance)
callable?.call()
} as SessionCallback)
}
```
Callers that want the new behavior can already opt in explicitly within
their transaction:
```groovy
Book.withTransaction {
def book = Book.get(id)
book.refresh(lock: true)
book.title = 'Updated'
book.save(failOnError: true)
}
```
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormStaticApi.groovy:
##########
@@ -226,92 +229,97 @@ class GormStaticApi<D> extends AbstractGormApi<D>
implements GormAllOperations<D
// GormInstanceOperations delegation
@Override
def propertyMissing(D instance, String name) {
- registry.findInstanceApi(persistentClass,
null).propertyMissing(instance, name)
+ registry.findInstanceApi(persistentClass,
qualifier).propertyMissing(instance, name)
}
@Override
boolean instanceOf(D instance, Class cls) {
- registry.findInstanceApi(persistentClass, null).instanceOf(instance,
cls)
+ registry.findInstanceApi(persistentClass,
qualifier).instanceOf(instance, cls)
}
@Override
D lock(D instance) {
- registry.findInstanceApi(persistentClass, null).lock(instance)
+ registry.findInstanceApi(persistentClass, qualifier).lock(instance)
}
@Override
def <T1> T1 mutex(D instance, Closure<T1> callable) {
- registry.findInstanceApi(persistentClass, null).mutex(instance,
callable)
+ registry.findInstanceApi(persistentClass, qualifier).mutex(instance,
callable)
}
@Override
D refresh(D instance) {
- registry.findInstanceApi(persistentClass, null).refresh(instance)
+ registry.findInstanceApi(persistentClass, qualifier).refresh(instance)
+ }
+
+ @Override
+ D refresh(D instance, Map args) {
+ registry.findInstanceApi(persistentClass, qualifier).refresh(instance,
args)
}
@Override
D save(D instance) {
- registry.findInstanceApi(persistentClass, null).save(instance)
+ registry.findInstanceApi(persistentClass, qualifier).save(instance)
Review Comment:
**Cross-datastore regression coverage:** changing delegation from `null` to
`qualifier` affects existing operations such as `Book.secondary.save(book)` for
MongoDB and other implementations inheriting this API, not just Hibernate
locked refresh. The correction makes sense, but please add public-API
named-connection tests for MongoDB and Neo4j and run their module/TCK suites.
Verify that writes reach only the selected connection and that unsupported
locked refresh still fails without a fallback read/write. I recommend
separating this shared routing fix into a prerequisite change.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/internal/RefreshLockArguments.groovy:
##########
@@ -0,0 +1,174 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.grails.datastore.gorm.internal
+
+import groovy.transform.CompileStatic
+
+import jakarta.persistence.LockModeType
+
+/**
+ * Resolves the named arguments shared by {@code refresh(Map)} and {@code
lock(Map, Serializable)}.
+ *
+ * @since 8.1
+ */
+@CompileStatic
+class RefreshLockArguments {
+
+ /**
+ * The {@code refresh(Map)} argument that requests a lock: {@code true}
for a pessimistic write lock, or a
+ * {@link LockModeType} naming the lock to acquire.
+ */
+ static final String LOCK = 'lock'
+
+ /**
+ * The {@code lock(Map, Serializable)} argument that requests a reload of
the instance's state and version
+ * under the lock.
+ */
+ static final String REFRESH = 'refresh'
+
+ /**
+ * The {@code lock(Map, Serializable)} argument that selects the {@link
LockModeType} to acquire; defaults to
+ * {@link LockModeType#PESSIMISTIC_WRITE}.
+ */
+ static final String TYPE = 'type'
+
+ /**
+ * The message reported when a datastore does not support refreshing an
instance under a lock.
+ */
+ static final String UNSUPPORTED = 'Datastore implementation does not
support refreshing under a lock'
+
+ /**
+ * The message reported when a datastore does not support lock modes other
than a pessimistic write lock.
+ */
+ static final String UNSUPPORTED_TYPE = 'Datastore implementation does not
support lock types other than PESSIMISTIC_WRITE'
+
+ /**
+ * The message reported when a lock or a locked refresh is requested
outside of an active transaction.
+ */
+ static final String TRANSACTION_REQUIRED = 'An active transaction is
required.'
+
+ /**
+ * Resolves the lock requested by the {@code lock} argument.
+ *
+ * @param args The named arguments, may be {@code null}
+ * @return The requested lock mode, or {@code null} when no lock was
requested
+ * @throws IllegalArgumentException if the argument is neither a boolean,
a {@link LockModeType}, nor the name of one
+ */
+ static LockModeType lockModeFrom(Map args) {
+ Object value = args?.get(LOCK)
Review Comment:
**Design recommendation: reject unknown option names.** This parser only
reads `lock`, so `book.refresh(lcok: true)` silently performs an unlocked
refresh. Likewise a misspelled `refresh` option can select legacy lock
behavior. For a concurrency API, consider explicit allowed-key validation at
the public map entry points, with `IllegalArgumentException` for unknown keys
and tests demonstrating that invalid options perform no datastore operation.
This would intentionally revise the current permissive unknown-key behavior, so
document that choice.
One possible validation helper (illustrative):
```groovy
private static void validateKeys(Map args, Set<String> allowed) {
if (args == null) {
return
}
for (Object key : args.keySet()) {
if (!allowed.contains(key)) {
throw new IllegalArgumentException("Unknown locking option:
${key}")
}
}
}
```
Validate once at the respective parsing entry points:
```groovy
// refresh options: before reading the lock value
validateKeys(args, Collections.singleton(LOCK))
// static lock options: before reading type/refresh
validateKeys(args, new HashSet<String>([TYPE, REFRESH]))
```
Update the tests that currently deliberately accept unrelated keys if this
stricter contract is adopted.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormEntity.groovy:
##########
@@ -711,9 +749,48 @@ trait GormEntity<D> implements GormValidateable,
DirtyCheckable, GormEntityApi<D
*/
@Generated
static D lock(Serializable id) {
+ if (id instanceof Map) {
+ // Groovy resolves entity.lock(refresh: true) to this static
method with the options map as the id.
+ throw new IllegalArgumentException('lock was called with named
arguments but no identifier. ' +
+ 'Use DomainClass.lock(id, refresh: true) to lock by
identifier, ' +
+ 'or instance.refresh(lock: true) to reload an instance
under a lock')
+ }
currentGormStaticApi().lock(id)
}
+ /**
+ * Locks an instance for an update, with options.
+ *
+ * <p>Supported arguments:</p>
+ * <ul>
+ * <li>{@code type} - the {@link jakarta.persistence.LockModeType} to
acquire, or its name. Defaults to
+ * {@link jakarta.persistence.LockModeType#PESSIMISTIC_WRITE}; {@code
NONE} is rejected.</li>
+ * <li>{@code refresh} - when {@code true}, reloads the database state
and version of an instance that is
+ * already managed in the current session under the lock instead of
locking the version already loaded.
+ * Unflushed changes to the instance are discarded. Requires an active
transaction.</li>
+ * </ul>
+ *
+ * <pre>
+ * Book.withTransaction {
+ * def book = Book.lock(id, refresh: true)
+ * def shared = Book.lock(otherId, type: LockModeType.PESSIMISTIC_READ)
+ * }
+ * </pre>
+ *
+ * @param args The named arguments
+ * @param id The identifier
+ * @return The instance, or {@code null} if no instance exists for the
identifier
+ * @throws RuntimeException an implementation-specific exception if {@code
refresh: true} is requested without
+ * an active transaction, such as {@code
jakarta.persistence.TransactionRequiredException} for Hibernate
+ * @throws IllegalArgumentException if {@code type} is neither a lock mode
nor the name of one, or is {@code NONE}
+ * @throws UnsupportedOperationException if {@code refresh: true} or a
non-default {@code type} is requested
+ * and the datastore does not support it
+ */
+ @Generated
+ static D lock(Map args, Serializable id) {
Review Comment:
**Optional Groovy API improvement:** consider `@NamedParam`/`@NamedParams`
metadata on the public map parameters, with corresponding metadata on public
operation interfaces where useful. This can improve static-call checking and
IDE discovery of `refresh`, `type`, and `lock`. Preserve the deliberately
accepted boolean/enum/string forms rather than annotating with overly narrow
types, and verify the annotations through real domain-trait calls under dynamic
and static compilation. I would retain these explicit overloads rather than
introduce `@NamedVariant` solely for modernization.
For example, metadata on the existing static method could look like this
(verify propagation through the trait):
```groovy
import groovy.transform.NamedParam
import groovy.transform.NamedParams
@Generated
static D lock(
@NamedParams([
@NamedParam(value = 'refresh', type = Object),
@NamedParam(value = 'type', type = Object)
]) Map args,
Serializable id) {
currentGormStaticApi().lock(args, id)
}
```
`Object` is intentional here because the runtime parser accepts multiple
value types. This describes allowed names without promising strict value-type
checking; retain runtime validation. The analogous refresh parameter would
declare the `lock` name.
--
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]
