This is an automated email from the ASF dual-hosted git repository. borinquenkid pushed a commit to branch chore/cleanup-AbstractCriteriaBuilder in repository https://gitbox.apache.org/repos/asf/grails-core.git
commit a5a524aaf0aa68c9ca881eb784239d7b2f306a47 Author: Walter Duque de Estrada <[email protected]> AuthorDate: Wed Aug 12 16:39:01 2026 -0500 Address Copilot review and Codecov patch-coverage gaps on PR #16140 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 <[email protected]> --- .../groovy/grails/gorm/CriteriaBuilderSpec.groovy | 132 ++++++++++++++++++++- .../grails/gorm/rx/CriteriaBuilderSpec.groovy | 4 +- 2 files changed, 127 insertions(+), 9 deletions(-) diff --git a/grails-datamapping-core/src/test/groovy/grails/gorm/CriteriaBuilderSpec.groovy b/grails-datamapping-core/src/test/groovy/grails/gorm/CriteriaBuilderSpec.groovy index 58e2b7f99c..228aa6cafe 100644 --- a/grails-datamapping-core/src/test/groovy/grails/gorm/CriteriaBuilderSpec.groovy +++ b/grails-datamapping-core/src/test/groovy/grails/gorm/CriteriaBuilderSpec.groovy @@ -58,11 +58,12 @@ class CriteriaBuilderSpec extends Specification { createQuery(CriteriaBuilderTestPerson) >> query isSchemaless() >> false } + Session session = Stub(Session) { + getMappingContext() >> mappingContext + } CriteriaBuilder<CriteriaBuilderTestPerson> newBuilder() { - def criteria = new CriteriaBuilder<CriteriaBuilderTestPerson>(CriteriaBuilderTestPerson, queryCreator, mappingContext) - criteria.@query = query - criteria + new CriteriaBuilder<CriteriaBuilderTestPerson>(CriteriaBuilderTestPerson, session, query) } void "constructor rejects a null target class"() { @@ -755,6 +756,30 @@ class CriteriaBuilderSpec extends Specification { 1 * query.add(associationQuery) } + void "invoking an association name whose query does not yield an association query does not add a criterion"() { + given: + Association association = Stub(Association) { + getName() >> 'books' + getAssociatedEntity() >> persistentEntity + } + PersistentEntity ownerEntity = Stub(PersistentEntity) { + getIdentity() >> idProperty + getPropertyByName('books') >> association + } + MappingContext ownerMappingContext = Stub(MappingContext) { + getPersistentEntity(CriteriaBuilderTestPerson.name) >> ownerEntity + } + query.createQuery('books') >> null + def criteria = new CriteriaBuilder<CriteriaBuilderTestPerson>(CriteriaBuilderTestPerson, queryCreator, ownerMappingContext) + + when: + def result = criteria.books {} + + then: + result == null + 0 * query.add(_) + } + void "an unresolvable method call throws a MissingMethodException"() { given: def criteria = newBuilder() @@ -798,7 +823,6 @@ class CriteriaBuilderSpec extends Specification { getPersistentEntity(CriteriaBuilderTestPerson.name) >> entityWithoutNameLookup } def criteria = new CriteriaBuilder<CriteriaBuilderTestPerson>(CriteriaBuilderTestPerson, queryCreator, idOnlyMappingContext) - criteria.@query = query when: def result = criteria.eq('id', 1L) @@ -818,7 +842,6 @@ class CriteriaBuilderSpec extends Specification { getPersistentEntity(CriteriaBuilderTestPerson.name) >> entityWithNoProperties } def criteria = new CriteriaBuilder<CriteriaBuilderTestPerson>(CriteriaBuilderTestPerson, queryCreator, emptyMappingContext) - criteria.@query = query when: criteria.eq('missing', 1L) @@ -889,7 +912,6 @@ class CriteriaBuilderSpec extends Specification { isSchemaless() >> true } def criteria = new CriteriaBuilder<CriteriaBuilderTestPerson>(CriteriaBuilderTestPerson, schemalessQueryCreator, emptyMappingContext) - criteria.@query = query when: def result = criteria.eq('missing', 1L) @@ -986,6 +1008,104 @@ class CriteriaBuilderSpec extends Specification { result == ['a'] 1 * query.add(_) } + + void "scroll with more than one argument is not treated as a criteria construction call"() { + given: + def criteria = newBuilder() + + when: + // Deliberately not scroll(): CriteriaBuilder.scroll(Closure) unconditionally re-enters + // invokeMethod with its (possibly null) argument, and Groovy's meta-method lookup keeps + // matching a single-null-arg call back to that same method - an infinite recursion this + // test must not trip. Two non-null args can never match that one-arg method, so this + // safely misses it and exercises the "wrong arg count" branch without the loop. + criteria.scroll('not-a-closure', 'extra') + + then: + thrown(MissingMethodException) + } + + void "scroll with a non-closure argument is not treated as a criteria construction call"() { + given: + def criteria = newBuilder() + + when: + criteria.scroll('not-a-closure') + + then: + thrown(MissingMethodException) + } + + void "call with a non-closure argument executes the query without evaluating any closure"() { + given: + def criteria = newBuilder() + query.list() >> ['a'] + + when: + def result = criteria.call('not-a-closure') + + then: + result == ['a'] + 0 * query.add(_) + } + + void "and tolerates a null closure and still adds an empty conjunction"() { + given: + def criteria = newBuilder() + + when: + def result = criteria.and(null) + + then: + result.is(criteria) + 1 * query.add(_) + } + + void "invoking an association whose associated entity cannot be resolved still evaluates the nested closure"() { + given: + Association association = Stub(Association) { + getName() >> 'books' + getAssociatedEntity() >> null + } + PersistentEntity ownerEntity = Stub(PersistentEntity) { + getIdentity() >> idProperty + getPropertyByName('books') >> association + } + MappingContext ownerMappingContext = Stub(MappingContext) { + getPersistentEntity(CriteriaBuilderTestPerson.name) >> ownerEntity + } + AssociationQuery associationQuery = Mock(AssociationQuery) + query.createQuery('books') >> associationQuery + def criteria = new CriteriaBuilder<CriteriaBuilderTestPerson>(CriteriaBuilderTestPerson, queryCreator, ownerMappingContext) + boolean invoked = false + + when: + criteria.books { + invoked = true + eq('title', 'a') + } + + then: + invoked + noExceptionThrown() + } + + void "ensureQueryIsInitialized resolves the query meta class only once across multiple criteria construction calls"() { + given: + // Each "call" invocation resets the query to null afterwards (a fresh query per call), so + // this must construct via queryCreator (stubbed to always hand back the same query mock) + // rather than newBuilder()'s pre-set query, which has no way to be recreated a second time. + def criteria = new CriteriaBuilder<CriteriaBuilderTestPerson>(CriteriaBuilderTestPerson, queryCreator, mappingContext) + query.list() >> ['a'] + + when: + criteria.call { eq('name', 'a') } + criteria.call { eq('name', 'b') } + + then: + noExceptionThrown() + 2 * query.add(_) + } } class CriteriaBuilderTestPerson { diff --git a/grails-datamapping-rx/src/test/groovy/grails/gorm/rx/CriteriaBuilderSpec.groovy b/grails-datamapping-rx/src/test/groovy/grails/gorm/rx/CriteriaBuilderSpec.groovy index da66bf2322..e1e679825b 100644 --- a/grails-datamapping-rx/src/test/groovy/grails/gorm/rx/CriteriaBuilderSpec.groovy +++ b/grails-datamapping-rx/src/test/groovy/grails/gorm/rx/CriteriaBuilderSpec.groovy @@ -63,9 +63,7 @@ class CriteriaBuilderSpec extends Specification { } CriteriaBuilder<CriteriaBuilderTestPerson> newBuilder() { - def criteria = new CriteriaBuilder<CriteriaBuilderTestPerson>(CriteriaBuilderTestPerson, queryCreator, mappingContext) - criteria.@query = query - criteria + new CriteriaBuilder<CriteriaBuilderTestPerson>(CriteriaBuilderTestPerson, queryCreator, mappingContext) } void "get(Closure) evaluates the closure, flags a unique result and returns a single observable"() {
