sbglasius commented on code in PR #16344:
URL: https://github.com/apache/grails-core/pull/16344#discussion_r4057733350
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormStaticApi.groovy:
##########
@@ -166,6 +178,96 @@ class HibernateGormStaticApi<D> extends GormStaticApi<D> {
((HibernateDatastore) datastore).withSession(callable)
}
+ @Override
+ D lock(Serializable id) {
+ if (!persistentEntity.isMultiTenant()) {
+ return super.lock(id)
+ }
+ // 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:
Same missing fix as the Hibernate 5 implementation — see the comment there
for the details and the `git` output showing `c4b22d54a0` is not reachable from
this branch.
Here the consequence is milder than on Hibernate 5, because `lock(id)` on
Hibernate 7 rejects a missing transaction on its own. But the dispatch is still
inconsistent: `lock(id, type: PESSIMISTIC_WRITE)` and `lock(id, type:
PESSIMISTIC_READ)` take different paths for no reason the caller can see, and
the two implementations should agree on the rule.
##########
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:
**The fix you describe as landed in `c4b22d54a0` is not in this branch.** At
head `e635ada7` the early return still dispatches on the *resolved* mode rather
than on whether the caller named one, and
`RefreshLockArguments.typeRequested(...)` does not exist anywhere in the tree:
```
$ git grep -n 'typeRequested' e635ada7 -- '*/src/main/*' '*/src/test/*'
(no matches)
$ git merge-base --is-ancestor c4b22d54a0 e635ada7
fatal: Not a valid object name c4b22d54a0
```
So the commit is not reachable from the PR head at all — it looks lost in
the `1ceec3a8` merge of `upstream/8.0.x`, or in a force-push. Worth checking
whether anything else went with it.
The original defect therefore stands: `Book.lock(id, type:
LockModeType.PESSIMISTIC_WRITE)` (and the string form `type:
'PESSIMISTIC_WRITE'`) resolves to `PESSIMISTIC_WRITE` with `refresh` false,
takes this early return, and never reaches the `TransactionRequiredException`
guard on line 210 — while `type: LockModeType.PESSIMISTIC_READ` does throw. On
Hibernate 5 the legacy path then runs on an auto-commit connection and the row
lock is released as soon as the statement completes, which is exactly the
hazard `grails-doc/src/en/ref/Domain Classes/lock.adoc:80-87` warns about while
promising that "any call passing `type`" throws `TransactionRequiredException`.
Please re-apply the `typeRequested` dispatch on both implementations, and
add the no-transaction test for this exact call including the string form of
the mode.
##########
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)
Review Comment:
Seconding @codeconsole here, with a specific worry about how `qualifier`
resolves.
This hunk changes 19 instance-operation delegations from
`findInstanceApi(persistentClass, null)` to `findInstanceApi(persistentClass,
qualifier)` — `save`, `insert`, `merge`, `delete`, `attach`, `discard`,
`ident`, `instanceOf`, `propertyMissing` and the rest — for **every** GORM
datastore, not only the locked-refresh path that needed it.
For the plain case it looks inert: `AbstractGormApi` sets `this.qualifier =
qualifier ?: ConnectionSource.DEFAULT`, and `resolveInstanceApi` normalizes
`null` to `DEFAULT`, so `null` and `"DEFAULT"` land in the same place,
including through the `MultiTenant` branch.
What I cannot settle by reading is which `qualifier` these reads bind to.
Both Hibernate `HibernateGormStaticApi` classes shadow the inherited field with
their own nullable `protected String qualifier` **and** override
`getQualifier()` with a fallback to
`persistentEntity.mapping.mappedForm.datasources[0]`. `GormStaticApi` is
`@CompileDynamic`, and its own line 140 (`qualifier != null ? ...`) only makes
sense against the nullable getter, since the inherited field is never null. If
these reads do go through the overridden getter, then for a domain class mapped
to a single named datasource the *default* static API now resolves instance
APIs under that connection name where it previously passed `null` — silently
moving where `Book.save(book)` writes.
Could you confirm which one it binds to, and pin it with a test? And I would
rather see this routing change split out as its own prerequisite PR with the
MongoDB/Neo4j named-connection coverage @codeconsole asked for; it is a
cross-datastore change riding along in a locking PR.
--
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]
