borinquenkid opened a new pull request, #16140: URL: https://github.com/apache/grails-core/pull/16140
## Summary Stacked on #16135 (needs that merged first). Cleans up `AbstractCriteriaBuilder` and 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. - **Dead code removal**: `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`. - **Real bug fix**: `grails.gorm.rx.CriteriaBuilder.count(Map, Closure)` 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 — a guaranteed NPE on every real call, never caught because the module had zero tests. - **Test coverage added**: new Specs for both concrete subclasses (mocked `Query`/`QueryCreator`/`MappingContext` collaborators, since the class only builds/delegates `Query.Criterion` objects rather than persisting anything). `AbstractCriteriaBuilder` 0% → 99.7% lines / 100% methods / 90% branches; both `CriteriaBuilder`s → 100% lines / 100% methods. - **PMD + IntelliJ inspection cleanup** on `AbstractCriteriaBuilder`: added missing `@Override` annotations, removed a dead initializer, reordered string comparisons to put the known constant first (avoids NPE if compared value is null), parameterized raw generic types, converted `instanceof`+cast to pattern variables, replaced a redundant `instanceof AssociationQuery` check with a null check (confirmed `Query.createQuery(String)`'s declared return type is `AssociationQuery` itself), replaced manual `list.get(size()-1)`/`.remove(size()-1)` with `getLast()`/`removeLast()`, and extracted a duplicated `MetaMethod` lookup 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 raw `Collection` parameter to match `Criteria`'s own raw interface declaration (can't be fixed without touching the shared interface), and `addToCriteria` keeps its non-void return since a few callers legitimately use it. ## Test plan - [x] `:grails-datamapping-core:test` — all passing - [x] `:grails-datamapping-rx:test` — all passing - [x] `:grails-datamapping-core:codeStyle` / `:grails-datamapping-rx:codeStyle` — clean - [x] Coverage verified via `jacocoTestReport` at each step 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
