Clean up AbstractCriteriaBuilder: remove dead code, add test coverage, fix warnings - #16140
Open
borinquenkid wants to merge 5 commits into
Open
Clean up AbstractCriteriaBuilder: remove dead code, add test coverage, fix warnings#16140borinquenkid wants to merge 5 commits into
borinquenkid wants to merge 5 commits into
Conversation
AbstractCriteriaBuilder had zero direct unit tests and, since it's abstract, can only be exercised through its concrete subclasses. Adds CriteriaBuilderSpec in grails-datamapping-core (via grails.gorm.CriteriaBuilder) and a matching spec in grails-datamapping-rx (via grails.gorm.rx.CriteriaBuilder), using mocked Query/QueryCreator/MappingContext collaborators since the class only builds and delegates Query.Criterion objects rather than persisting anything itself. Coverage: AbstractCriteriaBuilder 0% -> 99.7% lines / 100% methods; CriteriaBuilder (both sync and rx) -> 100% lines / 100% methods. Along the way: - grails.gorm.CriteriaBuilder's cache/readOnly/join(String)/select overrides were byte-for-byte duplicates of AbstractCriteriaBuilder's own bodies, existing only to narrow the return type from Criteria to BuildableCriteria for fluent chaining. Replaced each with a cast-and-delegate to super, matching the pattern already used by grails.gorm.rx.DetachedCriteria. - Fixed a real, previously-undetected bug in grails.gorm.rx.CriteriaBuilder.count(Map, Closure): it assigned the void return of prepareQuery(...) to a local `query` variable, which Groovy evaluates as null, shadowing the real query field for the rest of the method. Guaranteed NPE on every real call; never caught because the module had zero test coverage before being re-enabled. - Cleaned up AbstractCriteriaBuilder per PMD (run as a local, temporary diagnostic only - not applied to the build): added 47 missing @OverRide annotations, removed a dead initializer, and reordered 4 string comparisons to put the known constant first (avoids NPE if the compared value is null). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Removed 2 truly-unused public constants (ORDER_DESCENDING/ORDER_ASCENDING) after confirming zero references anywhere in the repo; the class's own order(String, String) already used grails.gorm.CriteriaBuilder's identical constants instead. - Parameterized raw uses of Class, PersistentProperty, Association, Closure, QueryableCriteria, and DetachedCriteria. - Converted 3 instanceof-plus-cast blocks to Java pattern variables. - Replaced a redundant `associationQuery instanceof AssociationQuery` check with a null check, since Query.createQuery(String) is declared to return AssociationQuery directly, making the instanceof always true. - Replaced manual list.get(size()-1)/remove(size()-1) with getLast()/removeLast(). - Extracted the duplicated MetaMethod lookup-and-invoke block in invokeMethod into a private helper, using a NOT_FOUND sentinel to preserve the exact "found but returned null" vs "not found" distinction the original two copies each handled inline. - Left 2 items as accepted/suppressed rather than "fixed" incorrectly: in(String, Collection) and inList(String, Collection) keep a raw Collection parameter with @SuppressWarnings("rawtypes"), since Criteria's own declaration is raw and a parameterized override would not actually override it (name clash, not an override, due to erasure); addToCriteria keeps @SuppressWarnings("UnusedReturnValue") since most callers discard its return value by design, even though a few legitimately use it. - Also drops an unused Criteria import from grails.gorm.rx.CriteriaBuilder. AbstractCriteriaBuilder coverage held at 99.7% lines / 100% methods / 90% branches throughout; all changes verified semantic-preserving. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request refactors and hardens the GORM criteria builder implementation by cleaning up AbstractCriteriaBuilder, fixing a concrete RxGORM bug, and adding direct unit test coverage for the criteria builder DSL and reactive terminal operations.
Changes:
- Fixes
grails.gorm.rx.CriteriaBuilder.count(Map, Closure)by removing an accidental shadowing assignment that would always yieldnulland NPE at runtime. - Refactors
org.grails.datastore.gorm.query.criteria.AbstractCriteriaBuilderto remove dead/duplicated logic and address static analysis warnings (generics,@Override, safer string comparisons, pattern variables, small helper extraction). - Adds new Spock specs to directly cover both
grails.gorm.CriteriaBuilderandgrails.gorm.rx.CriteriaBuilderbehavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| grails-datamapping-rx/src/main/groovy/grails/gorm/rx/CriteriaBuilder.groovy | Fixes count(Map, Closure) logic so the prepared query isn’t replaced with null. |
| grails-datamapping-rx/src/test/groovy/grails/gorm/rx/CriteriaBuilderSpec.groovy | Adds reactive terminal-operation coverage for Rx criteria builder. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/criteria/AbstractCriteriaBuilder.java | Refactors core criteria DSL dispatch and validation; addresses warnings and improves maintainability. |
| grails-datamapping-core/src/main/groovy/grails/gorm/CriteriaBuilder.java | Removes duplicate override bodies by delegating to super and casting for fluent chaining. |
| grails-datamapping-core/src/test/groovy/grails/gorm/CriteriaBuilderSpec.groovy | Adds extensive coverage for criteria DSL and builder behaviors via mocked datastore/query collaborators. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+62
to
+66
| CriteriaBuilder<CriteriaBuilderTestPerson> newBuilder() { | ||
| def criteria = new CriteriaBuilder<CriteriaBuilderTestPerson>(CriteriaBuilderTestPerson, queryCreator, mappingContext) | ||
| criteria.@query = query | ||
| criteria | ||
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/enable-datamapping-rx #16140 +/- ##
====================================================================
+ Coverage 52.8175% 52.9580% +0.1405%
- Complexity 18864 18947 +83
====================================================================
Files 2079 2079
Lines 97197 97194 -3
Branches 16870 16871 +1
====================================================================
+ Hits 51337 51472 +135
+ Misses 38446 38333 -113
+ Partials 7414 7389 -25
🚀 New features to boost your workflow:
|
Copilot review: both CriteriaBuilderSpec.groovy files set the builder's internal `query` field directly (`criteria.@query = query`) to seed a mocked query, bypassing the public construction/init path. Removed it from the core spec's newBuilder() and three individually-constructed tests, since queryCreator.createQuery(...) was already stubbed to return the same mock and ensureQueryIsInitialized() wires it up lazily through the real public path. For cases that need the query pre-set before any DSL call runs (the projection accessor tests, which don't call ensureQueryIsInitialized() themselves), switched newBuilder() to the public CriteriaBuilder(Class, Session, Query) constructor instead. Removed the same field write in the rx spec, where every test already goes through a real entry point that initializes the query itself. Codecov: patch coverage on AbstractCriteriaBuilder.java was 84% with 1 missing + 3 partial lines. Added targeted tests for: an association subquery whose query creator returns null, a nested association closure whose associated entity is unresolvable (exercises the persistentEntity == null early return in validatePropertyName), the SCROLL_CALL construction check's non-matching argument-count/type branches, invokeClosureNode with a non-closure argument (call with a non-closure arg), handleJunction with a null callable (and(null)), and ensureQueryIsInitialized's query-meta-class caching on a second call. Branches missed on AbstractCriteriaBuilder dropped from 9 to 2; the 2 remaining (getMetaClass() meta-method fast path in invokeMethod) were confirmed empirically unreachable via a dynamic-metaClass test that passed without covering them, matching a prior assessment that this path needs metaclass pollution for near-zero real value. Also discovered (but did not fix, as it's pre-existing and untouched by this PR): calling CriteriaBuilder.scroll() with zero args, or any argument Groovy's meta-method lookup null-coerces to match scroll(Closure), causes infinite recursion between invokeMethod and scroll() and a StackOverflowError. Worth a follow-up issue. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
scroll(Closure c) unconditionally re-entered invokeMethod(SCROLL_CALL, new Object[]{c})
to reuse the shared criteria-construction logic. When scroll() is called with the wrong
arg count/type (e.g. bare scroll(), or a call whose argument gets null-coerced by Groovy's
meta-method matching), isCriteriaConstructionMethod() correctly rejects it, but invokeMethod
then falls through to its getMetaClass() meta-method lookup, which is willing to match
scroll(Closure) with a null argument and invoke it - re-entering scroll() with the same null
argument forever, and StackOverflowing.
Extracted the shared criteria-construction logic (evaluate the closure, run the query,
reset state) into a new executeCriteriaConstruction() method, and changed scroll() to call
it directly instead of bouncing back through the dynamic dispatch in invokeMethod. This
closes the recursion path entirely: a malformed scroll() call now just runs the query with
no criteria applied, matching how an empty/null closure already behaves everywhere else in
this class, instead of crashing.
Found and root-caused while extending the SCROLL_CALL branch's test coverage for PR #16140;
CriteriaBuilder.java is already one of that PR's changed files even though this particular
method wasn't touched by its diff.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…CriteriaBuilder Resolves an add/add conflict on grails-datamapping-rx CriteriaBuilderSpec.groovy: both branches independently added a spec for this class. Keeps the Book/newCriteria() style already established on feat/enable-datamapping-rx and folds in the one complementary test case from this branch that exercises a distinct code path -- findAll() applying pre-populated orderEntries via prepareQuery(), as opposed to the closure-driven order() call covered by the existing test.
✅ All tests passed ✅🏷️ Commit: db5a42e Learn more about TestLens at testlens.app. |
3 tasks
Member
Author
|
@jdaugherty Did you intend to review this? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on #16135 (needs that merged first). Cleans up
AbstractCriteriaBuilderand its two concrete subclasses (grails.gorm.CriteriaBuilder,grails.gorm.rx.CriteriaBuilder), which had zero direct test coverage and hadn't been reviewed for a while.grails.gorm.CriteriaBuilder'scache/readOnly/join(String)/selectoverrides were byte-for-byte duplicates ofAbstractCriteriaBuilder's own bodies, existing only to narrow the return type fromCriteriatoBuildableCriteriafor fluent chaining. Replaced each with a cast-and-delegate tosuper, matching the pattern already used bygrails.gorm.rx.DetachedCriteria.grails.gorm.rx.CriteriaBuilder.count(Map, Closure)assigned thevoidreturn ofprepareQuery(...)to a localqueryvariable, which Groovy evaluates asnull, shadowing the realqueryfield for the rest of the method — a guaranteed NPE on every real call, never caught because the module had zero tests.Query/QueryCreator/MappingContextcollaborators, since the class only builds/delegatesQuery.Criterionobjects rather than persisting anything).AbstractCriteriaBuilder0% → 99.7% lines / 100% methods / 90% branches; bothCriteriaBuilders → 100% lines / 100% methods.AbstractCriteriaBuilder: added missing@Overrideannotations, removed a dead initializer, reordered string comparisons to put the known constant first (avoids NPE if compared value is null), parameterized raw generic types, convertedinstanceof+cast to pattern variables, replaced a redundantinstanceof AssociationQuerycheck with a null check (confirmedQuery.createQuery(String)'s declared return type isAssociationQueryitself), replaced manuallist.get(size()-1)/.remove(size()-1)withgetLast()/removeLast(), and extracted a duplicatedMetaMethodlookup block into a helper (using a sentinel to preserve exact null-vs-not-found semantics). Two items left as accepted/suppressed with comments explaining why:in/inList(String, Collection)keep a rawCollectionparameter to matchCriteria's own raw interface declaration (can't be fixed without touching the shared interface), andaddToCriteriakeeps its non-void return since a few callers legitimately use it.Test plan
:grails-datamapping-core:test— all passing:grails-datamapping-rx:test— all passing:grails-datamapping-core:codeStyle/:grails-datamapping-rx:codeStyle— cleanjacocoTestReportat each step🤖 Generated with Claude Code