Skip to content

fix: add null safety for getDatastoreForConnection in data service factory bean#15474

Open
jamesfredley wants to merge 4 commits into7.1.xfrom
fix/dataservice-null-datastore-guard
Open

fix: add null safety for getDatastoreForConnection in data service factory bean#15474
jamesfredley wants to merge 4 commits into7.1.xfrom
fix/dataservice-null-datastore-guard

Conversation

@jamesfredley
Copy link
Contributor

@jamesfredley jamesfredley commented Feb 28, 2026

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.

Tests

Added DatastoreServiceMethodInvokingFactoryBeanSpec with 7 Spock tests covering all resolveEffectiveDatastore() code paths:

Test Path Covered
Non-multi-connection datastore passthrough Early return for simple datastores
Default connection resolution No annotation, no domain mapping
@Transactional(connection) resolution Annotation-driven connection lookup
@Transactional(connection) null guard ConfigurationException when connection missing
Domain class mapping resolution Domain datasource mapping lookup
Domain class mapping null guard ConfigurationException when connection missing
DEFAULT connection name passthrough Annotation with DEFAULT does not override

Functional tests in grails-test-examples were assessed but are not feasible for this change - the null guard paths trigger ConfigurationException during bean initialization, which prevents application startup and cannot be caught in a running functional test.

…ctory bean

Guard against datastore implementations that may return null from
getDatastoreForConnection() by throwing a clear ConfigurationException
with the connection name and service class, rather than silently
setting a null datastore.

Assisted-by: Claude Code <Claude@Claude.ai>
@github-actions github-actions bot added the bug label Feb 28, 2026
@jamesfredley jamesfredley self-assigned this Feb 28, 2026
@jamesfredley jamesfredley marked this pull request as ready for review February 28, 2026 23:39
Copilot AI review requested due to automatic review settings February 28, 2026 23:39
@jamesfredley jamesfredley added this to the grails:7.1.0 milestone Feb 28, 2026
@jamesfredley jamesfredley moved this to In Progress in Apache Grails Feb 28, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes DatastoreServiceMethodInvokingFactoryBean.resolveEffectiveDatastore() defensive against MultipleConnectionSourceCapableDatastore.getDatastoreForConnection(...) returning null, by throwing a ConfigurationException instead of allowing a null datastore to be set on a data service.

Changes:

  • Add null-guards after getDatastoreForConnection(...) for explicit @Transactional(connection=...) selection.
  • Add null-guards after getDatastoreForConnection(...) for domain-class mapping-derived connection selection.
  • Introduce ConfigurationException error messages intended to provide clearer misconfiguration diagnostics.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@jdaugherty jdaugherty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add tests?

…solveEffectiveDatastore()

Add comprehensive Spock tests covering all code paths in resolveEffectiveDatastore(),
including both null safety guard branches that throw ConfigurationException when
getDatastoreForConnection() returns null.

7 test cases cover:
- Non-multi-connection datastore passthrough
- Default connection resolution
- @transactional(connection) annotation resolution
- Domain class datasource mapping resolution
- Both null guard paths (ConfigurationException)
- DEFAULT connection name does not override

Assisted-by: Claude Code <Claude@Claude.ai>
@jamesfredley
Copy link
Contributor Author

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.

The @service AST transform registers annotated classes in
META-INF/services/. When DomainService and SampleDomain were
private static, ServiceLoader could not instantiate them, breaking
DefaultServiceRegistrySpec. Removing private allows ServiceLoader
to instantiate these classes without affecting test behavior.

Assisted-by: Claude Code <Claude@Claude.ai>
Move DomainService and SampleDomain from inner classes to top-level classes
so the @service AST transformation can register them in META-INF/services
and ServiceLoader can instantiate them. As inner classes, ServiceLoader
could not instantiate DomainService, causing ServiceConfigurationError
in DefaultServiceRegistrySpec.

Assisted-by: Claude Code <Claude@Claude.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants