Copilot commented on code in PR #16723:
URL: https://github.com/apache/grails-core/pull/16723#discussion_r4343857850
##########
grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/AbstractHibernateGormStaticApi.groovy:
##########
@@ -178,9 +179,9 @@ abstract class AbstractHibernateGormStaticApi<D> extends
GormStaticApi<D> {
@Override
D load(Serializable id) {
- if (persistentEntity.isMultiTenant()) {
- // Hibernate's tenant filter does not apply to a load by
identifier, so a multi-tenant entity is loaded
- // through a query, as get(id) does, rather than handed to
whichever tenant asks for the id
+ if (persistentEntity.isMultiTenant() && multiTenancyMode ==
MultiTenancyMode.DISCRIMINATOR) {
+ // Hibernate's discriminator tenant filter does not apply to a
load by identifier, so use a query,
+ // as get(id) does, rather than handing out another tenant's row.
return get(id)
}
Review Comment:
The PR description says both `load` and `proxy` called `get` for every
multi-tenant entity, and that the fix applies to both. In the hibernate5
changes shown here, only `load` is updated. If hibernate5 has an overridden
`proxy(Serializable id)` with the same issue, it should be updated to match the
discriminator-only behavior as well (or, if it’s handled elsewhere, it would
help to include that change in this PR for completeness).
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormStaticApi.groovy:
##########
@@ -311,9 +312,9 @@ class HibernateGormStaticApi<D> extends GormStaticApi<D> {
@Override
D load(Serializable id) {
- if (persistentEntity.isMultiTenant()) {
- // Hibernate's tenant filter does not apply to a load by
identifier, so a multi-tenant entity is loaded
- // through a query, as get(id) does, rather than handed to
whichever tenant asks for the id
+ if (persistentEntity.isMultiTenant() && multiTenancyMode ==
MultiTenancyMode.DISCRIMINATOR) {
+ // Hibernate's discriminator tenant filter does not apply to a
load by identifier, so use a query,
+ // as get(id) does, rather than handing out another tenant's row.
return get(id)
}
Review Comment:
This PR introduces/strengthens a discriminator-only branch for `load` and
`proxy`, but the added tests only assert DATABASE/SCHEMA laziness. Please add a
test that exercises the `DISCRIMINATOR` branch (e.g., confirm `load`/`proxy` do
not return an uninitialized proxy for a missing id, and that tenant isolation
is respected), so future refactors don’t regress the original safety behavior.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormStaticApi.groovy:
##########
@@ -326,7 +327,7 @@ class HibernateGormStaticApi<D> extends GormStaticApi<D> {
@Override
D proxy(Serializable id) {
- if (persistentEntity.isMultiTenant()) {
+ if (persistentEntity.isMultiTenant() && multiTenancyMode ==
MultiTenancyMode.DISCRIMINATOR) {
return get(id)
}
Review Comment:
This PR introduces/strengthens a discriminator-only branch for `load` and
`proxy`, but the added tests only assert DATABASE/SCHEMA laziness. Please add a
test that exercises the `DISCRIMINATOR` branch (e.g., confirm `load`/`proxy` do
not return an uninitialized proxy for a missing id, and that tenant isolation
is respected), so future refactors don’t regress the original safety behavior.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormStaticApi.groovy:
##########
@@ -311,9 +312,9 @@ class HibernateGormStaticApi<D> extends GormStaticApi<D> {
@Override
D load(Serializable id) {
- if (persistentEntity.isMultiTenant()) {
- // Hibernate's tenant filter does not apply to a load by
identifier, so a multi-tenant entity is loaded
- // through a query, as get(id) does, rather than handed to
whichever tenant asks for the id
+ if (persistentEntity.isMultiTenant() && multiTenancyMode ==
MultiTenancyMode.DISCRIMINATOR) {
+ // Hibernate's discriminator tenant filter does not apply to a
load by identifier, so use a query,
+ // as get(id) does, rather than handing out another tenant's row.
return get(id)
}
Review Comment:
The same discriminator-mode guard is duplicated in both `load` and `proxy`.
Consider extracting a small private helper (e.g.,
`shouldQueryByIdForMultiTenancy()`), so any future changes to the
condition/comment only have to be made in one place.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormStaticApi.groovy:
##########
@@ -326,7 +327,7 @@ class HibernateGormStaticApi<D> extends GormStaticApi<D> {
@Override
D proxy(Serializable id) {
- if (persistentEntity.isMultiTenant()) {
+ if (persistentEntity.isMultiTenant() && multiTenancyMode ==
MultiTenancyMode.DISCRIMINATOR) {
return get(id)
}
Review Comment:
The same discriminator-mode guard is duplicated in both `load` and `proxy`.
Consider extracting a small private helper (e.g.,
`shouldQueryByIdForMultiTenancy()`), so any future changes to the
condition/comment only have to be made in one place.
##########
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/connections/SchemaMultiTenantSpec.groovy:
##########
@@ -84,6 +84,18 @@ class SchemaMultiTenantSpec extends Specification {
}
}
+ and: 'load and proxy stay lazy for missing identifiers in SCHEMA mode'
+ SingleTenantAuthor.withNewSession {
+ def handler = datastore.mappingContext.proxyHandler
+ def loaded = SingleTenantAuthor.load(Long.MAX_VALUE)
+ def proxied = SingleTenantAuthor.proxy(Long.MAX_VALUE - 1)
+ assert handler.isProxy(loaded)
+ assert !handler.isInitialized(loaded)
+ assert handler.isProxy(proxied)
+ assert !handler.isInitialized(proxied)
+ true
+ }
Review Comment:
This exact assertion block is duplicated across multiple specs (hibernate5/7
+ DATABASE/SCHEMA). To reduce repetition and keep future behavior changes
consistent, consider extracting this into a shared helper method/trait used by
the specs (optional, but it will make later updates less error-prone).
--
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]
