Copilot commented on code in PR #15393: URL: https://github.com/apache/grails-core/pull/15393#discussion_r2818640551
########## grails-data-hibernate5/core/src/test/groovy/org/grails/orm/hibernate/connections/DataServiceMultiTenantMultiDataSourceSpec.groovy: ########## @@ -0,0 +1,274 @@ +/* + * 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.orm.hibernate.connections + +import grails.gorm.MultiTenant +import grails.gorm.annotation.Entity +import grails.gorm.services.Service +import grails.gorm.transactions.Transactional +import org.grails.datastore.gorm.GormEnhancer +import org.grails.datastore.gorm.GormEntity +import org.grails.datastore.gorm.GormStaticApi +import org.grails.datastore.mapping.core.DatastoreUtils +import org.grails.datastore.mapping.multitenancy.MultiTenancySettings +import org.grails.datastore.mapping.multitenancy.resolvers.SystemPropertyTenantResolver +import org.grails.orm.hibernate.HibernateDatastore +import org.hibernate.Session +import org.hibernate.dialect.H2Dialect +import spock.lang.AutoCleanup +import spock.lang.Shared +import spock.lang.Specification + +/** + * Tests GORM Data Service auto-implemented CRUD methods when both DISCRIMINATOR + * multi-tenancy and a non-default datasource are configured on the same domain. + * + * This combination triggers the allQualifiers() bug: when MultiTenant is present, + * allQualifiers() returns tenant IDs instead of datasource names, causing schema + * creation and query routing to go to the wrong database. + * + * Covers: + * - Schema creation on the correct (analytics) datasource for MultiTenant domains + * - save(), get(), delete(), count() with tenant isolation on secondary datasource + * - findBy* dynamic finders with tenant isolation on secondary datasource + * - GormEnhancer escape-hatch for aggregate HQL on secondary datasource + * - Tenant isolation: same-named data under different tenants stays separate + * + * @see PartitionedMultiTenancySpec for basic DISCRIMINATOR multi-tenancy + * @see DataServiceMultiDataSourceSpec for Data Services on secondary datasource without multi-tenancy + */ +class DataServiceMultiTenantMultiDataSourceSpec extends Specification { + + @Shared @AutoCleanup HibernateDatastore datastore + + void setupSpec() { + Map config = [ + "grails.gorm.multiTenancy.mode": MultiTenancySettings.MultiTenancyMode.DISCRIMINATOR, + "grails.gorm.multiTenancy.tenantResolverClass": SystemPropertyTenantResolver, + 'dataSource.url': "jdbc:h2:mem:grailsDB;LOCK_TIMEOUT=10000", + 'dataSource.dbCreate': 'create-drop', + 'dataSource.dialect': H2Dialect.name, + 'dataSource.formatSql': 'true', + 'hibernate.flush.mode': 'COMMIT', + 'hibernate.cache.queries': 'true', + 'hibernate.hbm2ddl.auto': 'create-drop', + 'dataSources.analytics': [url: "jdbc:h2:mem:analyticsDB;LOCK_TIMEOUT=10000"], + ] + + datastore = new HibernateDatastore( + DatastoreUtils.createPropertyResolver(config), Metric) + } + + MetricService metricService + + void setup() { + System.setProperty(SystemPropertyTenantResolver.PROPERTY_NAME, "tenant1") + metricService = datastore.getDatastoreForConnection("analytics").getService(MetricService) + metricService.deleteAll() Review Comment: This spec’s setup and service definition always explicitly target the `analytics` connection (`getDatastoreForConnection("analytics")` and `@Transactional(connection = "analytics")`). The regression described in the PR occurs when the qualifier is inferred (eg calling unqualified GORM/Data Service methods that resolve through `GormEnhancer.findStaticApi(Metric)` / `Metric.save()` in DISCRIMINATOR mode). Add at least one assertion that exercises the unqualified path and proves it still routes to the explicitly mapped `analytics` datasource. ########## grails-data-hibernate5/core/src/test/groovy/org/grails/orm/hibernate/connections/DataServiceMultiTenantMultiDataSourceSpec.groovy: ########## @@ -0,0 +1,274 @@ +/* + * 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.orm.hibernate.connections + +import grails.gorm.MultiTenant +import grails.gorm.annotation.Entity +import grails.gorm.services.Service +import grails.gorm.transactions.Transactional +import org.grails.datastore.gorm.GormEnhancer +import org.grails.datastore.gorm.GormEntity +import org.grails.datastore.gorm.GormStaticApi +import org.grails.datastore.mapping.core.DatastoreUtils +import org.grails.datastore.mapping.multitenancy.MultiTenancySettings +import org.grails.datastore.mapping.multitenancy.resolvers.SystemPropertyTenantResolver +import org.grails.orm.hibernate.HibernateDatastore +import org.hibernate.Session +import org.hibernate.dialect.H2Dialect +import spock.lang.AutoCleanup +import spock.lang.Shared +import spock.lang.Specification + +/** + * Tests GORM Data Service auto-implemented CRUD methods when both DISCRIMINATOR + * multi-tenancy and a non-default datasource are configured on the same domain. + * + * This combination triggers the allQualifiers() bug: when MultiTenant is present, + * allQualifiers() returns tenant IDs instead of datasource names, causing schema + * creation and query routing to go to the wrong database. + * + * Covers: + * - Schema creation on the correct (analytics) datasource for MultiTenant domains + * - save(), get(), delete(), count() with tenant isolation on secondary datasource + * - findBy* dynamic finders with tenant isolation on secondary datasource + * - GormEnhancer escape-hatch for aggregate HQL on secondary datasource + * - Tenant isolation: same-named data under different tenants stays separate + * + * @see PartitionedMultiTenancySpec for basic DISCRIMINATOR multi-tenancy + * @see DataServiceMultiDataSourceSpec for Data Services on secondary datasource without multi-tenancy + */ +class DataServiceMultiTenantMultiDataSourceSpec extends Specification { + + @Shared @AutoCleanup HibernateDatastore datastore + + void setupSpec() { + Map config = [ + "grails.gorm.multiTenancy.mode": MultiTenancySettings.MultiTenancyMode.DISCRIMINATOR, + "grails.gorm.multiTenancy.tenantResolverClass": SystemPropertyTenantResolver, + 'dataSource.url': "jdbc:h2:mem:grailsDB;LOCK_TIMEOUT=10000", + 'dataSource.dbCreate': 'create-drop', + 'dataSource.dialect': H2Dialect.name, + 'dataSource.formatSql': 'true', + 'hibernate.flush.mode': 'COMMIT', + 'hibernate.cache.queries': 'true', + 'hibernate.hbm2ddl.auto': 'create-drop', + 'dataSources.analytics': [url: "jdbc:h2:mem:analyticsDB;LOCK_TIMEOUT=10000"], + ] + + datastore = new HibernateDatastore( + DatastoreUtils.createPropertyResolver(config), Metric) + } + + MetricService metricService + + void setup() { + System.setProperty(SystemPropertyTenantResolver.PROPERTY_NAME, "tenant1") + metricService = datastore.getDatastoreForConnection("analytics").getService(MetricService) + metricService.deleteAll() + // Also clean tenant2 data + System.setProperty(SystemPropertyTenantResolver.PROPERTY_NAME, "tenant2") + metricService.deleteAll() + // Reset to tenant1 for tests + System.setProperty(SystemPropertyTenantResolver.PROPERTY_NAME, "tenant1") + } + + void cleanup() { + System.setProperty(SystemPropertyTenantResolver.PROPERTY_NAME, "") + } + + void "schema is created on analytics datasource"() { + expect: "The analytics datasource connects to the analyticsDB H2 database" + Metric.analytics.withNewSession { Session s -> + assert s.connection().metaData.getURL() == "jdbc:h2:mem:analyticsDB" + return true + } + + and: "The default datasource connects to a different database" + datastore.withNewSession { Session s -> + assert s.connection().metaData.getURL() == "jdbc:h2:mem:grailsDB" + return true + } + } + + void "save routes to analytics datasource with tenant isolation"() { + when: "A metric is saved under tenant1" + Metric saved = metricService.save(new Metric(name: "page_views", amount: 100)) + + then: "The metric is persisted with an ID" + saved != null + saved.id != null + saved.name == "page_views" + saved.amount == 100 Review Comment: Test name/description says the save routes to the analytics datasource, but the assertions only check that an ID was generated. This would still pass if the row was persisted to the default datasource. Add an assertion that distinguishes the target DB (for example, verify via a default-datasource Session that the Metric table/row is absent there, or check the JDBC URL associated with the Session used during the operation). ```suggestion saved.amount == 100 and: "The metric is stored only in the analytics datasource, not the default datasource" Metric.analytics.get(saved.id) != null Metric.get(saved.id) == null ``` ########## grails-data-hibernate5/core/src/test/groovy/org/grails/orm/hibernate/connections/DataServiceMultiTenantMultiDataSourceSpec.groovy: ########## @@ -0,0 +1,274 @@ +/* + * 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.orm.hibernate.connections + +import grails.gorm.MultiTenant +import grails.gorm.annotation.Entity +import grails.gorm.services.Service +import grails.gorm.transactions.Transactional +import org.grails.datastore.gorm.GormEnhancer +import org.grails.datastore.gorm.GormEntity +import org.grails.datastore.gorm.GormStaticApi +import org.grails.datastore.mapping.core.DatastoreUtils +import org.grails.datastore.mapping.multitenancy.MultiTenancySettings +import org.grails.datastore.mapping.multitenancy.resolvers.SystemPropertyTenantResolver +import org.grails.orm.hibernate.HibernateDatastore +import org.hibernate.Session +import org.hibernate.dialect.H2Dialect +import spock.lang.AutoCleanup +import spock.lang.Shared +import spock.lang.Specification + +/** + * Tests GORM Data Service auto-implemented CRUD methods when both DISCRIMINATOR + * multi-tenancy and a non-default datasource are configured on the same domain. + * + * This combination triggers the allQualifiers() bug: when MultiTenant is present, + * allQualifiers() returns tenant IDs instead of datasource names, causing schema + * creation and query routing to go to the wrong database. + * + * Covers: + * - Schema creation on the correct (analytics) datasource for MultiTenant domains + * - save(), get(), delete(), count() with tenant isolation on secondary datasource + * - findBy* dynamic finders with tenant isolation on secondary datasource + * - GormEnhancer escape-hatch for aggregate HQL on secondary datasource + * - Tenant isolation: same-named data under different tenants stays separate + * + * @see PartitionedMultiTenancySpec for basic DISCRIMINATOR multi-tenancy + * @see DataServiceMultiDataSourceSpec for Data Services on secondary datasource without multi-tenancy Review Comment: The class-level Javadoc references `@see DataServiceMultiDataSourceSpec`, but that spec/class doesn’t exist in this repository (a search under `grails-data-hibernate5/core/src/test/groovy` finds no such file). Update the reference to an existing spec (or remove it) so the documentation remains accurate. ```suggestion ``` -- 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]
