jdaugherty commented on code in PR #16344: URL: https://github.com/apache/grails-core/pull/16344#discussion_r4056122212
########## 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: I would rather not take the runtime exception here. Every other GORM map argument is permissive - `save(flush: ...)`, `merge(params)`, `list(max: ...)` - and callers routinely hand a map built elsewhere straight through, so rejecting unknown keys in this one API would be the odd one out and would break that idiom for a map the caller did not compose literally. The typo risk is real, so `c4b22d54a0` addresses it at compile time instead, which catches it without constraining what a map may contain at runtime. `refresh(Map)` and `lock(Map, Serializable)` now declare their argument names as `@NamedParam` metadata, so a statically compiled `book.refresh(lcok: true)` fails with `unexpected named arg: lcok`, and `Book.lock(id, refesh: true)` with `unexpected named arg: refesh`. A caller passing a `Map` variable still compiles, so the pass-through case is untouched, and dynamic callers behave as before. Details in the reply on the `GormEntity` thread. The permissive runtime behaviour is therefore deliberate, and stays as the existing tests describe it. ########## 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: Done in `c4b22d54a0`, close to your sketch: `@NamedParams` on `refresh(Map)` naming `lock`, and on `lock(Map, Serializable)` naming `refresh` and `type`, with the same metadata on the `GormEntityApi` and `GormStaticOperations` declarations. The values are typed `Object` for the reason you give, and the javadoc now says why, so the next reader is not tempted to narrow them. No `@NamedVariant`, and the explicit overloads are unchanged. I left `GormInstanceOperations.refresh(D instance, Map args)` alone: its map is the second parameter, and Groovy only hoists named arguments into a leading map, so the metadata could never be exercised there. Verified in `GormEntityTraitSpec` through real domain-trait calls, for a class woven by `@Entity` and by `@Artefact('Domain')`: - the metadata survives trait weaving onto the domain class itself, asserted by reflection on `NamedParamBook.refresh(Map)` and `lock(Map, Serializable)` - statically compiled `book.refresh(lock: true)`, `Book.lock(id, refresh: true)`, `lock(id, type: 'pessimistic_read')` and `lock(id, refresh: true, type: null)` compile - statically compiled `refresh(lcok: true)`, `lock(id, refesh: true)` and `lock(id, refresh: true, unknown: 1)` are rejected as `unexpected named arg` - a `Map` variable passed to either method still compiles, and the same calls under dynamic compilation are unaffected ########## 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: The routing fix stays in this change set. Pulling it out now would mean re-sequencing work this release has already absorbed several rounds of review on, and the fix has no caller outside the locking work that exposed it, so a prerequisite PR would land with nothing exercising it. On the coverage, the concern is fair and I would rather be plain than imply coverage I do not have: I have not run the MongoDB or Neo4j suites this round. What the change can reach is narrow. `GormStaticApi` passed `null` where it now passes its own `qualifier`, and `GormInstanceApiRegistry.findInstanceApi` treats `null` and `ConnectionSource.DEFAULT` identically - both return the base api without calling `forQualifier`. So only a handle obtained for a named connection resolves to a different instance than before, which is exactly the bug. Locked refresh is unaffected for either store: neither reports `supportsLockedRefresh()`, so `refresh(lock: ...)` and `lock(id, refresh: true)` still throw `UnsupportedOperationException` from the default implementations rather than falling back to a read or write, and `mutex` keeps its `session.lock` path. I will run both module suites before this merges and post the numbers here. ########## 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: Agreed, and fixed in `c4b22d54a0` on both implementations. The early return now dispatches on whether the caller named a mode rather than on the mode it resolves to: ```groovy if (!refresh && !RefreshLockArguments.typeRequested(args)) { return lock(id) } ``` I left out the `lockMode == LockModeType.PESSIMISTIC_WRITE` half of your condition because it can never be false on that branch: with no `type` given, `lockTypeFrom` returns `PESSIMISTIC_WRITE` by definition. `lock(id)` and `lock(id, refresh: false)` keep their exact behaviour, and every call naming a mode - the default included - now reaches the transaction-checked path. On the two cases you asked me to decide: `typeRequested` reads the value rather than the key, so `type: null` counts as absent, the way `lockTypeFrom` already reads it - the call takes the default lock and behaves exactly like `lock(id)`, transaction requirement included. And the transaction check now precedes `convertIdentifier`, so `lock(null, type: ...)` is rejected rather than returning `null` first. Both are documented in `lock.adoc` and in the `GormEntity` and `GormStaticOperations` javadocs. Tests: both lock specs gained *"static lock(id, type: #description) rejects a missing transaction even when it names the default lock"*, over `LockModeType.PESSIMISTIC_WRITE` and the string form `'pessimistic_write'`. Hibernate 5 also pins that `lock(id)`, `lock(id, refresh: false)` and `lock(id, type: null)` still take the legacy route without a transaction; the Hibernate 7 equivalent compares those forms against `lock(id)`'s own outcome rather than pinning a version-specific one. `RefreshLockArgumentsSpec` covers `typeRequested` directly, including that it distinguishes the default mode from one the caller named. -- 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]
