You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@grails.apache.org by "jamesfredley (via GitHub)" <gi...@apache.org> on 2026/02/28 23:36:45 UTC
[PR] fix: add null safety for getDatastoreForConnection in data service factory bean [grails-core]
jamesfredley opened a new pull request, #15474:
URL: https://github.com/apache/grails-core/pull/15474
## Summary
- Add null guards after `getDatastoreForConnection()` calls in `DatastoreServiceMethodInvokingFactoryBean.resolveEffectiveDatastore()`, throwing a clear `ConfigurationException` with the connection name, domain class, and service class rather than silently setting a null datastore.
- Covers both the explicit `@Transactional(connection=...)` path and the domain class mapping inheritance path.
The `MultipleConnectionSourceCapableDatastore` interface doesn't guarantee non-null returns, and some implementations (e.g. Neo4j, SimpleMapDatastore) do a raw map lookup without validation. Hibernate already throws on missing connections, but this makes the factory bean defensive regardless of the underlying datastore implementation.
Flagged by Copilot review on #15472.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: add null safety for getDatastoreForConnection in data service factory bean [grails-core]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: add null safety for getDatastoreForConnection in data service factory bean [grails-core]
Posted by "jamesfredley (via GitHub)" <gi...@apache.org>.
jamesfredley merged PR #15474:
URL: https://github.com/apache/grails-core/pull/15474
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: add null safety for getDatastoreForConnection in data service factory bean [grails-core]
Posted by "jamesfredley (via GitHub)" <gi...@apache.org>.
jamesfredley commented on PR #15474:
URL: https://github.com/apache/grails-core/pull/15474#issuecomment-3987438419
Added unit tests in `DatastoreServiceMethodInvokingFactoryBeanSpec` - 7 Spock tests covering all `resolveEffectiveDatastore()` code paths, including both null guard branches.
Regarding functional tests in `grails-test-examples`: these aren't feasible for this change. The null guard paths throw `ConfigurationException` during bean initialization (before the app is fully started), so there's no way to exercise them from a running functional test. The unit tests cover this thoroughly by mocking `MultipleConnectionSourceCapableDatastore` and verifying both the happy path and the error paths directly.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: add null safety for getDatastoreForConnection in data service factory bean [grails-core]
Posted by "jamesfredley (via GitHub)" <gi...@apache.org>.
jamesfredley commented on code in PR #15474:
URL: https://github.com/apache/grails-core/pull/15474#discussion_r2868039889
##########
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 existing `HibernateDatastore.getDatastoreForConnection()` already uses the same "DataSource not found" wording in its own `ConfigurationException`. Keeping our messages consistent with that convention. A terminology change would be a broader codebase concern, not something to address in this fix.
##########
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 domain class is intentionally omitted here. In this code path the connection comes from the explicit `@Transactional(connection="...")` annotation on the service - the domain class mapping is irrelevant. Including it would mislead users into thinking the domain mapping caused the problem. The second path (domain mapping inheritance) does include the domain class because there it is the source of the connection name.
##########
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 - consistent with the existing `HibernateDatastore` error messages.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org