jdaugherty commented on code in PR #16066:
URL: https://github.com/apache/grails-core/pull/16066#discussion_r3785085335
##########
grails-datamapping-core/src/main/groovy/grails/gorm/multitenancy/Tenants.groovy:
##########
@@ -193,14 +211,15 @@ class Tenants {
throw new UnsupportedOperationException('Datastore implementation
does not support multi-tenancy')
}
}
+
/**
* Execute the given closure with given tenant id
* @param tenantId The tenant id
* @param callable The closure
* @return The result of the closure
*/
- static <T> T withId(Class<? extends Datastore> datastoreClass,
Serializable tenantId, Closure<T> callable) {
- Datastore datastore = GormEnhancer.findDatastoreByType(datastoreClass)
+ static <T> T withId(Class domainClass, Serializable tenantId, Closure<T>
callable) {
Review Comment:
Thanks for tracing the RX module — you're right that
`grails.gorm.rx.multitenancy.Tenants` is independent and my ~40-call-site count
was wrong. The `TenantDelegatingGormOperations` migration to the
`MultiTenantCapableDatastore` overload is a good change on its own merits.
What I still want addressed isn't in-tree callers, though — it's that
`withId(Class<? extends Datastore>, Serializable, Closure)` is gone from a
public, documented entry point and nothing anywhere says so. I traced what an
existing out-of-tree caller does now, and it's quieter than I assumed when I
first wrote this:
`Tenants.withId(SomeDatastore, tenantId) { }`
→ `Tenants:229` `withId(Class domainClass, ..)` — same erasure, still
compiles
→ `datastoreLocator.getDatastoreForDomain(SomeDatastore)` →
`GormApiResolver.findDatastore(SomeDatastore, null)`
→ no `entityDatastores` entry under that class name, so the preferred /
qualified / active-session selectors all miss
→ `DefaultDatastoreSelector.select` → `getDatastoreByString(className,
DEFAULT)` → `datastoresByQualifier.get(DEFAULT)`
That returns the application's DEFAULT datastore. It's non-null, so the
`instanceof MultiTenantCapableDatastore` check passes and the closure runs —
against the wrong datastore, with no exception and no log. On a
single-datastore app it's indistinguishable from correct; on a multi-datastore
app it's a silent cross-database read or write that surfaces only as bad data.
The loud `stateException` at `GormApiResolver:89` fires only when there is no
DEFAULT datastore at all, which isn't the case in any app that would hit this.
So either:
1. restore the datastore-class overload and give the domain-class variant a
distinct name — still my preference, it costs one method; or
2. keep it as-is and document the removal in `upgrading80x.adoc`. Section 45
covers `GormEnhancer`'s removed members and `CurrentTenantHolder.get()`
thoroughly, but says nothing about `Tenants` — this is exactly the class of
change that section exists for.
Either works for me. What I can't sign off on is it landing undocumented.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateSession.java:
##########
@@ -389,30 +392,71 @@ public List retrieveAll(final Class type, final Iterable
keys) {
final String entityName = persistentEntity.getName();
final String idName = persistentEntity.getIdentity().getName();
final String hql = "from " + entityName + " as e where e." + idName +
" in (:keys)";
+ final Class idType = persistentEntity.getIdentity().getType();
+ final ConversionService conversionService =
getMappingContext().getConversionService();
+
+ // Convert each requested id to the entity's identifier type,
preserving order and
+ // duplicates. getAll() must return entities in the supplied id order
with a null slot
+ // for any id that does not resolve to a row, so order is driven by
the request rather
+ // than the database.
+ final List<Serializable> requestedIds = new ArrayList<>();
Review Comment:
The `getAll()` trace holds up — I checked, and `GormEnhancerSpec` has a
zero-byte diff against 8.0.x, so "preserves the supplied id order" and "returns
a null slot for a missing id" genuinely do pre-date this PR. That makes it a
pre-existing contract violation being fixed rather than new undocumented
behaviour, and I'm happy for it to stay. Thanks also for the `String.valueOf`
fix on both sides of `entitiesById` — confirmed in H5 (`:212`/`:219`) and H7
(`:442`/`:449`).
On `disjunction()`/`conjunction()`/`negation()`: yes please, add the test —
that's what unblocks keeping it here rather than splitting it out. What I'm
after is one that actually fails against core's base implementations, so it
needs to reach the junction factory (the path you describe as `countByXOrY`
losing its disjunction) rather than only exercising a builder that never calls
it, and it should assert the result set rather than the shape of the criteria
object:
- a `countByXOrY` over two rows where each matches exactly one side — expect
2; with the junction dropped onto the unused base `criteria` field this returns
the full table count
- the negation and conjunction equivalents, since all three overrides have
the same root cause and only one of them is covered by the finder above
H7 only — H5's `HibernateQuery` extends `AbstractHibernateQuery` and never
had this bug — so worth a line in the commit message noting the asymmetry,
otherwise it reads like a missed backport.
`grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/hibernatequery/`
looks like the right home.
With that in place I'm fine with all of these staying in this 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]
