Fix @Where and DetachedCriteria query methods ignoring @Transactional(connection)

[jamesfredley]

Summary

Fixes #15416

Data Service methods using @Where annotations or DetachedCriteria-based queries (count(), list(), findBy*()) were ignoring the class-level @Transactional(connection) annotation, causing queries to execute against the default datasource instead of the specified connection.

Reproducer: https://github.com/jamesfredley/grails-15416-where-connection-routing

Root Cause

Two issues were found:

1. Wrong method node passed to findConnectionId()

Both AbstractWhereImplementer and AbstractDetachedCriteriaServiceImplementor called findConnectionId(newMethodNode) instead of findConnectionId(abstractMethodNode).

• newMethodNode belongs to the generated $ServiceImplementation class, which does not carry the @Transactional annotation
• abstractMethodNode belongs to the original interface/abstract class where @Transactional(connection) is declared
• findConnectionId() resolves the connection by walking up to methodNode.getDeclaringClass() to find @Transactional - so passing the wrong node meant it could never find the annotation

This is the same class of bug fixed in #15395 for SaveImplementer, DeleteImplementer, and FindAndDeleteImplementer.

2. build() after withConnection() in AbstractWhereImplementer

In the @Where code path, the original order was:

1. query = new DetachedCriteria(Foo)
2. query = query.withConnection('secondary') - sets connectionName
3. query = query.build(closure) - internally calls clone(), which does not copy connectionName

The clone() call inside build() creates a new DetachedCriteria instance without the connection setting, so the connection was silently lost.

Fixed by reordering to: new DetachedCriteria -> build(closure) -> withConnection(name).

Note: AbstractDetachedCriteria.clone() not copying connectionName is arguably a separate bug. The non-@Where path in AbstractDetachedCriteriaServiceImplementor is not affected because it does not call build() after withConnection().

Changes

File	Change
AbstractWhereImplementer.groovy	findConnectionId(newMethodNode) -> findConnectionId(abstractMethodNode) + reorder build()/withConnection()
AbstractDetachedCriteriaServiceImplementor.groovy	findConnectionId(newMethodNode) -> findConnectionId(abstractMethodNode)

Tests

Unit Tests (WhereConnectionRoutingSpec) - 5 tests

• Verifies @Where method generates withConnection() call when @Transactional(connection) is present
• Verifies @Where method does NOT generate withConnection() when no connection specified
• Verifies DetachedCriteria count() generates withConnection() call
• Verifies DetachedCriteria list() generates withConnection() call
• Verifies DetachedCriteria findByName() generates withConnection() call

Integration Tests (WhereQueryMultiDataSourceSpec) - 5 tests

• Uses two real H2 in-memory databases (default + secondary)
• Domain class mapped with datasource 'ALL'
• Inserts data only into secondary, verifies queries route correctly:
  • @Where-annotated method returns data from secondary
  • @Where-annotated method returns empty from default (proving isolation)
  • count() routes to secondary
  • list() routes to secondary
  • findByName() routes to secondary

All existing ServiceTransformSpec tests (30) continue to pass with no regressions.

[Copilot]

Pull request overview

Fixes datasource connection routing for GORM Data Service query implementations so that class/interface-level @Transactional(connection = ...) is respected when generating @Where-based and DetachedCriteria-based query methods (e.g., count(), list(), dynamic findBy*() methods).

Changes:

• Resolve the connection id from the original service method (abstractMethodNode) instead of the generated implementation method (newMethodNode) in @Where and DetachedCriteria implementers.
• Reorder DetachedCriteria.build() vs withConnection() in the @Where path to avoid losing the connection due to build() cloning.
• Add unit + integration regression tests covering connection routing for @Where and common DetachedCriteria-backed query methods in multi-datasource scenarios.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/AbstractWhereImplementer.groovy	Fix connection resolution node + ensure build() happens before withConnection() so connection isn’t dropped during cloning.
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/services/implementers/AbstractDetachedCriteriaServiceImplementor.groovy	Fix connection resolution node for DetachedCriteria query implementers so class/interface @Transactional(connection) is honored.
grails-datamapping-core/src/test/groovy/grails/gorm/services/WhereConnectionRoutingSpec.groovy	Adds unit-level compilation/transform assertions for @Where and DetachedCriteria-implemented methods under @Transactional(connection).
grails-data-hibernate5/core/src/test/groovy/org/grails/orm/hibernate/connections/WhereQueryMultiDataSourceSpec.groovy	Adds integration coverage verifying runtime query routing against separate default + secondary H2 datasources.

---

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

[bito-code-review]

The suggestion is valid. The unit spec tests @where method routing but lacks a findByName test, while the integration spec covers it. Adjust the PR description to clarify that integration tests verify DetachedCriteria findByName routing.

[jdaugherty]

The change is great, but what do you think about adding a test to the tck?

[jdaugherty]

Is this from test pollution? Why isn't this in cleanup?

[jdaugherty]

These changes are in core, but there are additional tests in hibernate. That strongly tells me the test should be part of the TCK.