jamesfredley commented on code in PR #16515:
URL: https://github.com/apache/grails-core/pull/16515#discussion_r4210305843
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormStaticApi.groovy:
##########
@@ -389,11 +397,127 @@ class GormStaticApi<D> extends AbstractGormApi<D>
implements GormAllOperations<D
* @return A list of identifiers
*/
List<D> getAll(Serializable... ids) {
+ if (isRetrievedByQuery()) {
+ return retrieveAllByQuery((Collection<Serializable>)
ids.flatten(), true)
+ }
(List<D>) execute({ Session session ->
session.retrieveAll(persistentClass, ids.flatten())
} as SessionCallback)
}
+ /**
+ * Whether instances are retrieved by id through a query. In {@link
MultiTenancyMode#DISCRIMINATOR} mode the
+ * multi-tenant event listener restricts a query to the current tenant,
but a lookup by key bypasses it, so the
+ * instances of a multi-tenant entity are retrieved through a query
instead. Inside
+ * {@link Tenants#withoutId(groovy.lang.Closure)}, where the current id is
the default connection source, a lookup
+ * by id is not restricted to a tenant and stays a lookup by key.
+ *
+ * @return Whether instances are retrieved by id through a query
+ * @throws
org.grails.datastore.mapping.multitenancy.exceptions.TenantNotFoundException if
there is no current tenant
+ */
+ private boolean isRetrievedByQuery() {
+ if (multiTenancyMode != MultiTenancyMode.DISCRIMINATOR
+ || persistentEntity?.isMultiTenant() != true
+ || persistentEntity.identity == null) {
+ return false
+ }
+ return !isWithoutTenant()
+ }
+
+ /**
+ * Whether the current id is the default connection source, as it is inside
+ * {@link Tenants#withoutId(groovy.lang.Closure)}.
+ *
+ * @return Whether the current id is the default connection source
+ * @throws
org.grails.datastore.mapping.multitenancy.exceptions.TenantNotFoundException if
there is no current tenant
+ */
+ private boolean isWithoutTenant() {
+ Serializable currentId = datastore instanceof
MultiTenantCapableDatastore
+ ? Tenants.currentId((MultiTenantCapableDatastore) datastore)
+ : Tenants.currentId(datastore.getClass())
+ return ConnectionSource.DEFAULT.equals(currentId)
+ }
+
+ /**
+ * Retrieves the instances for the given identifiers through a query,
which the multi-tenant event listener
+ * restricts to the current tenant.
+ *
+ * @param ids The identifiers
+ * @param strict Whether an identifier that cannot be converted to the
type of the identity throws
+ * @return The instances in the order of the identifiers, with {@code
null} for an identifier no instance was found for
+ */
+ private List<D> retrieveAllByQuery(Collection<Serializable> ids, boolean
strict) {
Review Comment:
The `strict` flag should stay. `get`, `read`, and `exists` pass `false`, so
an id that cannot be converted is treated as missing. `getAll` passes `true`,
and `load` uses the same strict conversion, so those still throw
`ConversionFailedException`. That matches the key lookup this replaced, and it
is what the earlier review comments asked to restore.
A private boolean with those two call sites is enough. Splitting the query
method would duplicate the lookup without a clearer contract. Making every
method throw, or making `getAll` and `load` return null, would be a separate
behavior change.
--
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]
