Copilot commented on code in PR #15433: URL: https://github.com/apache/grails-core/pull/15433#discussion_r2836801710
########## grails-data-hibernate5/core/src/test/groovy/org/grails/orm/hibernate/connections/DataServiceDatasourceInheritanceSpec.groovy: ########## @@ -0,0 +1,215 @@ +/* + * 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 org.hibernate.dialect.H2Dialect +import spock.lang.AutoCleanup +import spock.lang.Shared +import spock.lang.Specification + +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.mapping.core.DatastoreUtils +import org.grails.orm.hibernate.HibernateDatastore + +class DataServiceDatasourceInheritanceSpec extends Specification { + + @Shared Map config = [ + '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.warehouse':[url:"jdbc:h2:mem:warehouseDB;LOCK_TIMEOUT=10000"], + ] + + @Shared @AutoCleanup HibernateDatastore datastore = new HibernateDatastore( + DatastoreUtils.createPropertyResolver(config), Inventory + ) + + @Shared InventoryService inventoryService + @Shared InventoryDataService inventoryDataService + @Shared ExplicitInventoryService explicitInventoryService + + void setupSpec() { + inventoryService = datastore + .getDatastoreForConnection('warehouse') + .getService(InventoryService) + inventoryDataService = datastore + .getDatastoreForConnection('warehouse') + .getService(InventoryDataService) + explicitInventoryService = datastore + .getDatastoreForConnection('warehouse') + .getService(ExplicitInventoryService) + } Review Comment: setupSpec() fetches services from datastore.getDatastoreForConnection('warehouse'), which doesn’t exercise the new runtime behavior added in DatastoreServiceMethodInvokingFactoryBean (choosing the correct connection-specific datastore when creating the service from the *default* datastore). To cover the regression this PR targets, instantiate at least one service via datastore.getService(InventoryService) and assert it still operates on the 'warehouse' datasource. ########## grails-test-examples/hibernate5/grails-data-service-multi-datasource/src/integration-test/groovy/functionaltests/DataServiceDatasourceInheritanceSpec.groovy: ########## @@ -0,0 +1,121 @@ +/* + * 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 functionaltests + +import example.Product +import example.InheritedProductService + +import org.springframework.beans.factory.annotation.Autowired + +import grails.testing.mixin.integration.Integration +import org.grails.orm.hibernate.HibernateDatastore +import spock.lang.Specification + +@Integration +class DataServiceDatasourceInheritanceSpec extends Specification { + + @Autowired + HibernateDatastore hibernateDatastore + + InheritedProductService inheritedProductService + + void setup() { + inheritedProductService = hibernateDatastore + .getDatastoreForConnection('secondary') + .getService(InheritedProductService) + } Review Comment: This spec overwrites the (auto-injected) InheritedProductService with hibernateDatastore.getDatastoreForConnection('secondary').getService(...), which bypasses the behavior this PR is trying to validate (automatic datasource inheritance for the Spring-managed service bean). To test inheritance end-to-end, use the injected service instance and assert it persists/queries against Product.secondary (and not the default datasource). ########## grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/DatastoreServiceMethodInvokingFactoryBean.groovy: ########## @@ -56,14 +62,53 @@ class DatastoreServiceMethodInvokingFactoryBean extends MethodInvokingFactoryBea protected Object invokeWithTargetException() throws Exception { Object object = super.invokeWithTargetException() if (object) { - ((Service) object).setDatastore((Datastore) targetObject) + Datastore effectiveDatastore = resolveEffectiveDatastore((Datastore) targetObject) + ((Service) object).setDatastore(effectiveDatastore) if (beanFactory instanceof AutowireCapableBeanFactory) { ((AutowireCapableBeanFactory) beanFactory).autowireBeanProperties(object, AutowireCapableBeanFactory.AUTOWIRE_BY_TYPE, false) } } object } + private Datastore resolveEffectiveDatastore(Datastore defaultDatastore) { + if (!(defaultDatastore instanceof MultipleConnectionSourceCapableDatastore)) { + return defaultDatastore + } + + Class<?> domainClass = getServiceDomainClass() + if (domainClass == null || domainClass == Object) { + return defaultDatastore + } + + PersistentEntity entity = defaultDatastore.getMappingContext()?.getPersistentEntity(domainClass.getName()) + if (entity == null) { + return defaultDatastore + } + + String domainConnection = ConnectionSourcesSupport.getDefaultConnectionSourceName(entity) + if (domainConnection != null + && !ConnectionSource.DEFAULT.equals(domainConnection) + && !ConnectionSource.ALL.equals(domainConnection)) { + return ((MultipleConnectionSourceCapableDatastore) defaultDatastore).getDatastoreForConnection(domainConnection) + } Review Comment: resolveEffectiveDatastore() always selects the datastore based on the *domain mapping* and ignores an explicit @Transactional(connection=...) declared on the data service. This breaks the documented precedence rule (explicit service connection should win) and can also cause a mismatch between the transaction manager/connection used by the generated code and the datastore injected into the service. Consider resolving the effective connection from the service class’s @Transactional(connection) first (non-empty, not DEFAULT/ALL), and only fall back to domain mapping when the service doesn’t specify one. ########## grails-doc/src/en/guide/conf/dataSource/multipleDatasources.adoc: ########## @@ -260,6 +260,50 @@ Note that the datasource specified in a service has no bearing on which datasour If you have a `Foo` domain class in `dataSource1` and a `Bar` domain class in `dataSource2`, if `WahooService` uses `dataSource1`, a service method that saves a new `Foo` and a new `Bar` will only be transactional for `Foo` since they share the same datasource. The transaction won't affect the `Bar` instance. If you want both to be transactional you'd need to use two services and XA datasources for two-phase commit, e.g. with the Atomikos plugin. +===== GORM Data Service Datasource Inheritance + +When using GORM Data Services (the `@Service` annotation), the service automatically inherits the datasource from its domain class's `mapping` block. This means you do not need to explicitly specify `@Transactional(connection = '...')` if the domain class already declares its datasource. + +For example, given a domain class mapped to the `'lookup'` datasource: + +[source,groovy] +---- +class ZipCode { + + String code + + static mapping = { + datasource 'lookup' + } +} +---- + +A GORM Data Service targeting this domain class will automatically route all auto-implemented operations (save, get, delete, find, count) to the `'lookup'` datasource: + +[source,groovy] +---- +@Service(ZipCode) +abstract class ZipCodeService { + + abstract ZipCode get(Serializable id) + abstract ZipCode save(ZipCode zipCode) + abstract ZipCode delete(Serializable id) + abstract List<ZipCode> findByCode(String code) Review Comment: The example method signature is inconsistent with GORM finder semantics: findByCode typically returns a single ZipCode, while a List return type should use findAllByCode. As written, this snippet may mislead users and/or fail to compile depending on the data service implementers. ```suggestion abstract List<ZipCode> findAllByCode(String code) ``` ########## grails-data-hibernate5/core/src/test/groovy/org/grails/orm/hibernate/connections/DataServiceDatasourceInheritanceSpec.groovy: ########## @@ -0,0 +1,215 @@ +/* + * 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 org.hibernate.dialect.H2Dialect +import spock.lang.AutoCleanup +import spock.lang.Shared +import spock.lang.Specification + +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.mapping.core.DatastoreUtils +import org.grails.orm.hibernate.HibernateDatastore + +class DataServiceDatasourceInheritanceSpec extends Specification { + + @Shared Map config = [ + '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.warehouse':[url:"jdbc:h2:mem:warehouseDB;LOCK_TIMEOUT=10000"], + ] + + @Shared @AutoCleanup HibernateDatastore datastore = new HibernateDatastore( + DatastoreUtils.createPropertyResolver(config), Inventory + ) + + @Shared InventoryService inventoryService + @Shared InventoryDataService inventoryDataService + @Shared ExplicitInventoryService explicitInventoryService + + void setupSpec() { + inventoryService = datastore + .getDatastoreForConnection('warehouse') + .getService(InventoryService) + inventoryDataService = datastore + .getDatastoreForConnection('warehouse') + .getService(InventoryDataService) + explicitInventoryService = datastore + .getDatastoreForConnection('warehouse') + .getService(ExplicitInventoryService) + } + + void setup() { + def api = GormEnhancer.findStaticApi(Inventory, 'warehouse') + api.withNewTransaction { + api.executeUpdate('delete from Inventory') + } + } + + void "abstract service without @Transactional(connection) inherits from domain"() { + when: "saving through a service that has no @Transactional(connection)" + def saved = inventoryService.save(new Inventory(sku: 'ABC-001', quantity: 50)) + + then: "the entity is persisted on the warehouse datasource" + saved != null + saved.id != null + saved.sku == 'ABC-001' + + and: "it exists on the warehouse datasource" + GormEnhancer.findStaticApi(Inventory, 'warehouse').withNewTransaction { + GormEnhancer.findStaticApi(Inventory, 'warehouse').count() + } == 1 + } + + void "get by ID routes to inherited datasource"() { + given: "an inventory item saved on warehouse" + def saved = inventoryService.save(new Inventory(sku: 'GET-001', quantity: 10)) + + when: "retrieving by ID" + def found = inventoryService.get(saved.id) + + then: "the correct entity is returned" + found != null + found.id == saved.id + found.sku == 'GET-001' + } + + void "delete routes to inherited datasource"() { + given: "an inventory item saved on warehouse" + def saved = inventoryService.save(new Inventory(sku: 'DEL-001', quantity: 5)) + + when: "deleting by ID" + def deleted = inventoryService.delete(saved.id) + + then: "the entity is deleted" + deleted != null + deleted.sku == 'DEL-001' + inventoryService.get(saved.id) == null + } + + void "count routes to inherited datasource"() { + given: "items saved on warehouse" + inventoryService.save(new Inventory(sku: 'CNT-001', quantity: 1)) + inventoryService.save(new Inventory(sku: 'CNT-002', quantity: 2)) + + expect: "count returns 2" + inventoryService.count() == 2 + } + + void "findBySku routes to inherited datasource"() { + given: "items saved on warehouse" + inventoryService.save(new Inventory(sku: 'FIND-001', quantity: 100)) + + when: "finding by sku" + def found = inventoryService.findBySku('FIND-001') + + then: "the correct entity is returned" + found != null + found.sku == 'FIND-001' + found.quantity == 100 + } + + void "interface service inherits datasource from domain"() { + when: "saving through an interface service with no @Transactional(connection)" + def saved = inventoryDataService.save(new Inventory(sku: 'IFACE-001', quantity: 25)) + + then: "the entity is persisted on warehouse" + saved != null + saved.id != null + + and: "retrievable through the same service" + inventoryDataService.get(saved.id) != null + } + + void "explicit @Transactional(connection) wins over domain datasource"() { + when: "saving through a service with explicit @Transactional(connection='warehouse')" + def saved = explicitInventoryService.save(new Inventory(sku: 'EXPL-001', quantity: 75)) + + then: "the entity is persisted correctly" + saved != null + saved.id != null + saved.sku == 'EXPL-001' + } Review Comment: The “explicit @Transactional(connection) wins over domain datasource” test case currently uses the same connection ('warehouse') for both the domain mapping and the explicit annotation, so it can’t detect precedence bugs (e.g., the factory bean picking the domain connection even when the service overrides it). Consider changing the explicit service to use a different configured connection and asserting the effective datastore/operations follow the service’s connection. -- 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]
