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 53.4558%. Comparing base (0613772) to head (8a544b4).
⚠️ Report is 277 commits behind head on 8.1.x.

Additional details and impacted files

@@                Coverage Diff                 @@
##                8.1.x     #16143        +/-   ##
==================================================
+ Coverage     52.8038%   53.4558%   +0.6519%     
- Complexity      18863      19493       +630     
==================================================
  Files            2079       2081         +2     
  Lines           97207      98994      +1787     
  Branches        16873      17361       +488     
==================================================
+ Hits            51329      52918      +1589     
- Misses          38468      38531        +63     
- Partials         7410       7545       +135     

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 96 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

[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"() {

[jdaugherty]

-1 is an accepted way to get the last when the array is defined, why change it?

[jdaugherty]

Almost all of this spec duplicates coverage that grails.gorm.rx.DetachedCriteriaSpec already provides on 8.1.x (find/findAll/get/toList/list/getCount/updateAll/deleteAll/toQuery/toObservable/subscribe, the prepareQuery max/offset and fetch-strategy paths, and additional-criteria merging). The only new behavior covered here is the custom JoinType passthrough.

That existing spec also shows this setup works through the public RxGormEnhancer.registerEntity(entity, datastoreClient) with a stubbed ConnectionSources, so the package-local placement to reach the protected registerEntityWithConnectionSource is not needed.

Suggest moving the "prepareQuery passes a custom JoinType through to the query" feature method into the existing DetachedCriteriaSpec and dropping this file.

This is the reason I think this should have been merged into the other PR

[jdaugherty]

TestEntity here resolves to the class declared at the bottom of DetachedCriteriaCloneSpec.groovy. Depending on a fixture defined inside another spec's file is fragile - refactoring that spec would silently break this one. Suggest either moving TestEntity into its own file in this package or declaring a dedicated fixture class in this file.

[testlens-app]

🚨 TestLens detected 1 failed test 🚨

Here is what you can do:

1. Inspect the test failures carefully.
2. If you are convinced that some of the tests are flaky, you can mute them below.
3. Finally, trigger a rerun by checking the rerun checkbox.

Test Summary

CI / Functional Tests (Java 25, indy=false, shard 1) > :grails-test-examples-gsp-sitemesh3:integrationTest

Test	Runs	Flakiness
EndToEndSpec > async multiple levels of layouts	❌	1% 🟡

🏷️ Commit: 8a544b4
▶️ Tests: 61996 executed
⚪️ Checks: 77/77 completed

Test Failures

EndToEndSpec > async multiple levels of layouts (:grails-test-examples-gsp-sitemesh3:integrationTest in CI / Functional Tests (Java 25, indy=false, shard 1))

Condition not satisfied:

pageSource == """<html><head><title>Decorated Base - Dialog - This is the title</title><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"></head> <body><h1>Hello</h1><div id="base"><div id="dialog">body text</div></div> </body></html>"""
|          |
|          false
|          Strings too large to calculate edit distance.
<html lang="en"><head><title>HTTP Status 500 – Internal Server Error</title><style type="text/css">body {font-family:Tahoma,Arial,sans-serif;} h1, h2, h3, b {color:white;background-color:#525D76;} h1 {font-size:22px;} h2 {font-size:16px;} h3 {font-size:14px;} p {font-size:12px;} a {color:black;} .line {height:1px;background-color:#525D76;border:none;}</style></head><body><h1>HTTP Status 500 – Internal Server Error</h1></body></html>

	at EndToEndSpec.async multiple levels of layouts(EndToEndSpec.groovy:107)

expected	actual
<html><head><title>Decorated Base - Dialog - This is the title</title><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"></head>	<html lang="en"><head><title>HTTP Status 500 – Internal Server Error</title><style type="text/css">body {font-family:Tahoma,Arial,sans-serif;} h1, h2, h3, b {color:white;background-color:#525D76;} h1 {font-size:22px;} h2 {font-size:16px;} h3 {font-size:14px;} p {font-size:12px;} a {color:black;} .line {height:1px;background-color:#525D76;border:none;}</style></head><body><h1>HTTP Status 500 – Internal Server Error</h1></body></html>
<body><h1>Hello</h1><div id="base"><div id="dialog">body text</div></div>
</body></html>

Rerun Controls

Select tests to mute in this pull request:

• EndToEndSpec > async multiple levels of layouts

Reuse successful test results:

• ♻️ Only rerun the tests that failed or were muted before

Click the checkbox to trigger a rerun:

• Rerun jobs

---

Learn more about TestLens at testlens.app/docs.