Copilot commented on code in PR #15472:
URL: https://github.com/apache/grails-core/pull/15472#discussion_r2868002387
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/DatastoreServiceMethodInvokingFactoryBean.groovy:
##########
@@ -54,16 +58,86 @@ class DatastoreServiceMethodInvokingFactoryBean extends
MethodInvokingFactoryBea
@Override
protected Object invokeWithTargetException() throws Exception {
- Object object = super.invokeWithTargetException()
+ def object = super.invokeWithTargetException()
if (object) {
- ((Service) object).setDatastore((Datastore) targetObject)
+ def effectiveDatastore = resolveEffectiveDatastore((Datastore)
targetObject)
+ ((Service) object).datastore = effectiveDatastore
if (beanFactory instanceof AutowireCapableBeanFactory) {
- ((AutowireCapableBeanFactory)
beanFactory).autowireBeanProperties(object,
AutowireCapableBeanFactory.AUTOWIRE_BY_TYPE, false)
+ ((AutowireCapableBeanFactory)
beanFactory).autowireBeanProperties(
+ object,
+ AutowireCapableBeanFactory.AUTOWIRE_BY_TYPE,
+ false
+ )
}
}
object
}
+ private Datastore resolveEffectiveDatastore(Datastore defaultDatastore) {
+ if (!(defaultDatastore instanceof
MultipleConnectionSourceCapableDatastore)) {
+ return defaultDatastore
+ }
+
+ // Check for explicit @Transactional(connection=...) on the service
first - it takes precedence
+ String serviceConnection = getServiceTransactionalConnection()
+ if (serviceConnection != null
+ && ConnectionSource.DEFAULT != serviceConnection
+ && ConnectionSource.ALL != serviceConnection) {
+ return ((MultipleConnectionSourceCapableDatastore)
defaultDatastore)
+ .getDatastoreForConnection(serviceConnection)
+ }
+
+ // Fall back to domain class mapping datasource
+ def domainClass = serviceDomainClass
+ if (domainClass == null || domainClass == Object) {
+ return defaultDatastore
+ }
+
+ def entity =
defaultDatastore.mappingContext?.getPersistentEntity(domainClass.name)
+ if (entity == null) {
+ return defaultDatastore
+ }
+
+ def domainConnection =
ConnectionSourcesSupport.getDefaultConnectionSourceName(entity)
+ if (domainConnection != null
+ && ConnectionSource.DEFAULT != domainConnection
+ && ConnectionSource.ALL != domainConnection) {
+ return ((MultipleConnectionSourceCapableDatastore)
defaultDatastore)
+ .getDatastoreForConnection(domainConnection)
+ }
Review Comment:
Same concern for the domain-mapping path:
`getDatastoreForConnection(domainConnection)` can be null depending on
datastore implementation. It would be safer to explicitly handle null (throwing
a configuration error that includes the missing connection name and service
type) instead of silently setting a null datastore.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/DatastoreServiceMethodInvokingFactoryBean.groovy:
##########
@@ -29,10 +28,15 @@ import
org.springframework.beans.factory.config.MethodInvokingFactoryBean
import org.springframework.lang.Nullable
import org.grails.datastore.mapping.core.Datastore
+import org.grails.datastore.mapping.core.connections.ConnectionSource
+import org.grails.datastore.mapping.core.connections.ConnectionSourcesSupport
+import
org.grails.datastore.mapping.core.connections.MultipleConnectionSourceCapableDatastore
import org.grails.datastore.mapping.services.Service
/**
- * Variant of {#link MethodInvokingFactoryBean} which returns the correct data
service type instead of {@code java.lang.Object} so the Autowire with type
works correctly.
+ * Variant of {#link MethodInvokingFactoryBean} which returns the correct
Review Comment:
Groovydoc/Javadoc link tag is malformed: `{#link MethodInvokingFactoryBean}`
should be `{@link MethodInvokingFactoryBean}` (and similarly keep the tag on a
single token) so the generated docs render correctly.
```suggestion
* Variant of {@link MethodInvokingFactoryBean} which returns the correct
```
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/DatastoreServiceMethodInvokingFactoryBean.groovy:
##########
@@ -54,16 +58,86 @@ class DatastoreServiceMethodInvokingFactoryBean extends
MethodInvokingFactoryBea
@Override
protected Object invokeWithTargetException() throws Exception {
- Object object = super.invokeWithTargetException()
+ def object = super.invokeWithTargetException()
if (object) {
- ((Service) object).setDatastore((Datastore) targetObject)
+ def effectiveDatastore = resolveEffectiveDatastore((Datastore)
targetObject)
+ ((Service) object).datastore = effectiveDatastore
if (beanFactory instanceof AutowireCapableBeanFactory) {
- ((AutowireCapableBeanFactory)
beanFactory).autowireBeanProperties(object,
AutowireCapableBeanFactory.AUTOWIRE_BY_TYPE, false)
+ ((AutowireCapableBeanFactory)
beanFactory).autowireBeanProperties(
+ object,
+ AutowireCapableBeanFactory.AUTOWIRE_BY_TYPE,
+ false
+ )
}
}
object
}
+ private Datastore resolveEffectiveDatastore(Datastore defaultDatastore) {
+ if (!(defaultDatastore instanceof
MultipleConnectionSourceCapableDatastore)) {
+ return defaultDatastore
+ }
+
+ // Check for explicit @Transactional(connection=...) on the service
first - it takes precedence
+ String serviceConnection = getServiceTransactionalConnection()
+ if (serviceConnection != null
+ && ConnectionSource.DEFAULT != serviceConnection
+ && ConnectionSource.ALL != serviceConnection) {
+ return ((MultipleConnectionSourceCapableDatastore)
defaultDatastore)
+ .getDatastoreForConnection(serviceConnection)
+ }
Review Comment:
`getDatastoreForConnection(serviceConnection)` may return null for some
`MultipleConnectionSourceCapableDatastore` implementations (e.g. Neo4j returns
`datastoresByConnectionSource.get(...)` without validation). Consider guarding
against null and throwing a clear configuration exception (or falling back to
`defaultDatastore`) so the service doesn’t end up with a null `datastore`
reference.
--
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]
