Add test coverage for AbstractDetachedCriteria and rx DetachedCriteria

[borinquenkid]

Summary

• Adds mock-based unit specs for AbstractDetachedCriteria (via grails.gorm.DetachedCriteria) and for the reactive grails.gorm.rx.DetachedCriteria, taking both from ~0% to full line/method coverage without needing a real datastore.
• Fixes two real bugs surfaced while writing the rx specs:
  • buildQueryableCriteria() cast the built DetachedCriteria to QueryableCriteria, but the rx class never implemented that interface, so every closure-based subquery (in, inList, notIn, eqAll/gtAll/ltAll/geAll/leAll, gtSome/geSome/ltSome/leSome) threw a ClassCastException at runtime. Fixed with a small SubqueryAdapter that extends the shared AbstractDetachedCriteria base directly, since the reactive class's own find()/list() return Observable and can't coexist with QueryableCriteria's T/List<T> signatures on the same type.
  • prepareQuery() applied fetch strategies (join/select) twice: once via DynamicFinder.applyDetachedCriteria(), then again via a redundant hand-rolled loop that also ignored custom JoinTypes. Removed the dead duplicate.
• Fixes a handful of definite-assignment/shadowing/raw-getAt warnings in AbstractDetachedCriteria (uninitialized prop in createAlias, a local variable named criteria shadowing the instance field of the same name in clone(), and negative-index List access replaced with getLast()).

Test plan

• :grails-datamapping-core:test — 71 new tests in AbstractDetachedCriteriaSpec, all pass
• :grails-datamapping-rx:test — 45 new tests across DetachedCriteriaSpec and DetachedCriteriaQuerySpec, all pass
• :grails-datamapping-core:codeStyle and :grails-datamapping-rx:codeStyle clean
• Verified via jacocoTestReport that AbstractDetachedCriteria and grails.gorm.rx.DetachedCriteria (plus its new SubqueryAdapter) are at ~100% line/method coverage

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This pull request significantly expands unit test coverage around detached criteria usage in both the core (AbstractDetachedCriteria via grails.gorm.DetachedCriteria) and reactive (grails.gorm.rx.DetachedCriteria) stacks, while also fixing reactive subquery handling and simplifying query preparation behavior.

Changes:

• Add extensive mock-based Spock specs to drive high line/method coverage for AbstractDetachedCriteria and reactive DetachedCriteria.
• Fix reactive closure-based subqueries by introducing a QueryableCriteria-compatible SubqueryAdapter (avoids runtime ClassCastException).
• Simplify reactive prepareQuery() by removing redundant fetch-strategy application and relying on DynamicFinder.applyDetachedCriteria().

Reviewed changes

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

Show a summary per file

File	Description
grails-datamapping-rx/src/test/groovy/org/grails/gorm/rx/api/DetachedCriteriaQuerySpec.groovy	New spec covering reactive query execution paths that go through prepareQuery() and static API lookup.
grails-datamapping-rx/src/test/groovy/grails/gorm/rx/DetachedCriteriaSpec.groovy	New spec verifying reactive DetachedCriteria override methods delegate correctly and return the narrowed type.
grails-datamapping-rx/src/main/groovy/grails/gorm/rx/DetachedCriteria.groovy	Fix reactive subquery construction and remove redundant fetch-strategy application during query preparation.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/criteria/AbstractDetachedCriteriaSpec.groovy	New comprehensive spec driving coverage of AbstractDetachedCriteria behavior via the concrete DetachedCriteria.
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/criteria/AbstractDetachedCriteria.groovy	Small correctness/cleanup tweaks (definite assignment, safer list access, variable shadowing cleanup).

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[bito-code-review]

The suggestion to add a JoinType assertion to the test is appropriate. Verifying that DynamicFinder.applyDetachedCriteria correctly calls Query#join(property, JoinType) ensures the bugfix is functional and prevents future regressions in the query generation logic.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

grails-datamapping-rx/src/test/groovy/org/grails/gorm/rx/api/DetachedCriteriaQuerySpec.groovy:42

• The class is public, so the Javadoc claim that it's "Package-local" is inaccurate. Either adjust the wording, or make the spec package-scoped (e.g., via @PackageScope) if that’s the intent.

 * Package-local so the test can call the {@code protected static}
 * {@code RxGormEnhancer.registerEntityWithConnectionSource} directly rather than driving the
 * full {@code registerEntity} multi-tenancy/connection-source resolution machinery.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 52.8609%. Comparing base (0613772) to head (7de0bc6).

Additional details and impacted files

@@                Coverage Diff                 @@
##                8.1.x     #16143        +/-   ##
==================================================
+ Coverage     52.8038%   52.8609%   +0.0571%     
- Complexity      18863      18903        +40     
==================================================
  Files            2079       2079                
  Lines           97207      97208         +1     
  Branches        16873      16873                
==================================================
+ Hits            51329      51385        +56     
+ Misses          38468      38418        -50     
+ Partials         7410       7405         -5     

Files with missing lines	Coverage Δ
...orm/query/criteria/AbstractDetachedCriteria.groovy	97.3054% <100.0000%> (+6.6147%)	⬆️
...main/groovy/grails/gorm/rx/DetachedCriteria.groovy	99.3151% <100.0000%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jdaugherty]

Let's just combine this PR with the other one? It looks like the tests that are commented were never uncommented and we can do all of this in one PR. @borinquenkid

[borinquenkid]

Let's just combine this PR with the other one? It looks like the tests that are commented were never uncommented and we can do all of this in one PR. @borinquenkid

@jdaugherty I would prefer to keep them separate so they are as focused as possible

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: 7de0bc6
▶️ Tests: 61200 executed
⚪️ Checks: 61/61 completed

---

Learn more about TestLens at testlens.app.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

grails-datamapping-rx/src/main/groovy/grails/gorm/rx/DetachedCriteria.groovy:653

• The extra cast chain (Class<Observable<T>>) (Class) targetClass is redundant (since targetClass is already a Class), and it makes the intent harder to read. A single cast is enough here.

        if (additionalCriteria != null) {
            def additionalDetached = new DetachedCriteria((Class<Observable<T>>) (Class) targetClass).build(additionalCriteria)
            DynamicFinder.applyDetachedCriteria(query, additionalDetached)

grails-datamapping-rx/src/test/groovy/org/grails/gorm/rx/api/DetachedCriteriaQuerySpec.groovy:119

• This spec name says get(Closure) but the test actually calls criteria.get() with no closure. Renaming the feature text avoids confusion about which overload is being exercised.

    void "get(Closure) applies a max of 1 and delegates to the no-arg RxQuery#singleResult"() {