Copilot commented on code in PR #15474:
URL: https://github.com/apache/grails-core/pull/15474#discussion_r2868024828
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/DatastoreServiceMethodInvokingFactoryBean.groovy:
##########
@@ -102,8 +111,17 @@ class DatastoreServiceMethodInvokingFactoryBean extends
MethodInvokingFactoryBea
if (domainConnection != null
&& ConnectionSource.DEFAULT != domainConnection
&& ConnectionSource.ALL != domainConnection) {
- return ((MultipleConnectionSourceCapableDatastore)
defaultDatastore)
+ Datastore resolved = ((MultipleConnectionSourceCapableDatastore)
defaultDatastore)
.getDatastoreForConnection(domainConnection)
+ if (resolved == null) {
+ throw new ConfigurationException(
+ "DataSource not found for connection name
[${domainConnection}] " +
+ "mapped on domain class [${domainClass.name}] " +
+ "used by service [${serviceClass.name}]. " +
+ 'Please check your multiple data sources configuration
and try again.'
Review Comment:
Same as above: the exception message says "DataSource" but this factory bean
is datastore-agnostic and is resolving a datastore/connection source by name.
Consider using "connection source" or "datastore" here to avoid JDBC-specific
terminology leaking into Mongo/Neo4j/SimpleMap setups.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/DatastoreServiceMethodInvokingFactoryBean.groovy:
##########
@@ -83,8 +84,16 @@ class DatastoreServiceMethodInvokingFactoryBean extends
MethodInvokingFactoryBea
if (serviceConnection != null
&& ConnectionSource.DEFAULT != serviceConnection
&& ConnectionSource.ALL != serviceConnection) {
- return ((MultipleConnectionSourceCapableDatastore)
defaultDatastore)
+ Datastore resolved = ((MultipleConnectionSourceCapableDatastore)
defaultDatastore)
.getDatastoreForConnection(serviceConnection)
+ if (resolved == null) {
+ throw new ConfigurationException(
+ "DataSource not found for connection name
[${serviceConnection}] " +
+ "specified via @Transactional on service
[${serviceClass.name}]. " +
+ 'Please check your multiple data sources configuration
and try again.'
+ )
Review Comment:
The PR description says the thrown ConfigurationException should include the
connection name, domain class, and service class. In the
@Transactional(connection=...) branch, the exception message currently omits
the domain class (even though `serviceDomainClass` is available later in this
method). Consider including the domain class name when it can be resolved (or
explicitly stating that it’s unknown) to match the intended diagnostics.
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/config/DatastoreServiceMethodInvokingFactoryBean.groovy:
##########
@@ -83,8 +84,16 @@ class DatastoreServiceMethodInvokingFactoryBean extends
MethodInvokingFactoryBea
if (serviceConnection != null
&& ConnectionSource.DEFAULT != serviceConnection
&& ConnectionSource.ALL != serviceConnection) {
- return ((MultipleConnectionSourceCapableDatastore)
defaultDatastore)
+ Datastore resolved = ((MultipleConnectionSourceCapableDatastore)
defaultDatastore)
.getDatastoreForConnection(serviceConnection)
+ if (resolved == null) {
+ throw new ConfigurationException(
+ "DataSource not found for connection name
[${serviceConnection}] " +
+ "specified via @Transactional on service
[${serviceClass.name}]. " +
+ 'Please check your multiple data sources configuration
and try again.'
Review Comment:
This is in datastore-core and applies to non-JDBC implementations too
(Mongo/Neo4j/SimpleMap). The error message refers to a "DataSource" even though
the lookup is `getDatastoreForConnection(...)`/connection sources; this can be
misleading outside JDBC. Consider changing the wording to "connection source"
or "datastore" to keep the message datastore-agnostic.
--
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]
