jdaugherty commented on code in PR #16140:
URL: https://github.com/apache/grails-core/pull/16140#discussion_r4146441751
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/criteria/AbstractCriteriaBuilder.java:
##########
@@ -58,11 +56,14 @@ public abstract class AbstractCriteriaBuilder extends
GroovyObjectSupport implem
public static final String ORDER_DESCENDING = "desc";
Review Comment:
Restored these. They're public on an abstract class that
`grails.gorm.rx.CriteriaBuilder` inherits, so dropping them is an API break for
a minor release even though nothing in-repo references them. `order(String,
String)` and `isCriteriaConstructionMethod` now use the class's own constants
instead of reaching into the `grails.gorm.CriteriaBuilder` subclass, which also
removes that import.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/criteria/AbstractCriteriaBuilder.java:
##########
@@ -810,9 +858,10 @@ public Criteria ilike(String propertyName, Object
propertyValue) {
*
* @return A Criterion instance
*/
+ @Override
public Criteria rlike(String propertyName, Object propertyValue) {
- validatePropertyName(propertyName, "like");
- Assert.notNull(propertyValue, "Cannot use like expression with null
value");
+ validatePropertyName(propertyName, "rlike");
Review Comment:
`rlike` validated and reported its property under the `like` label; both now
say `rlike`.
##########
grails-datamapping-core/src/test/groovy/grails/gorm/CriteriaBuilderSpec.groovy:
##########
@@ -18,117 +18,1176 @@
*/
package grails.gorm
-import grails.gorm.annotation.Entity
-import org.grails.datastore.mapping.simple.SimpleMapDatastore
-import spock.lang.AutoCleanup
+import org.grails.datastore.mapping.core.Session
+import org.grails.datastore.mapping.model.MappingContext
+import org.grails.datastore.mapping.model.PersistentEntity
+import org.grails.datastore.mapping.model.PersistentProperty
+import org.grails.datastore.mapping.model.types.Association
+import org.grails.datastore.mapping.query.AssociationQuery
+import org.grails.datastore.mapping.query.Query
+import org.grails.datastore.mapping.query.QueryCreator
+import org.grails.datastore.mapping.query.api.BuildableCriteria
+import org.grails.datastore.mapping.query.api.QueryableCriteria
import spock.lang.Specification
+import spock.lang.Unroll
/**
- * {@code CriteriaBuilder} is the concrete class real callers get from
- * {@code GormStaticApi#createCriteria()} (see {@code GormStaticApi.groovy}'s
- * {@code new CriteriaBuilder(persistentClass, session)}) - and both it and
its abstract superclass
- * {@code AbstractCriteriaBuilder} had 0% coverage from this module's own
JaCoCo perspective (real
- * usage is exercised only via adapter-specific subclasses/specs in other
modules, which don't count
- * toward this module's report - see item 14's note on cross-module coverage
attribution).
- *
- * This PR's diff added {@code ensureQueryIsInitialized()} guards to several
- * {@code AbstractCriteriaBuilder} methods (`cache`, `join`, `select`,
`order`, `invokeList`,
- * `projections`) plus a new `getPersistentEntity()` getter - fixing the exact
NPE
- * item 9 found and left as a known gap: {@code query} is null on a criteria
builder obtained
- * directly from `createCriteria()` until something inside a
`.list{}`/`.get{}` closure first
- * touches it.
+ * Exercises {@link CriteriaBuilder}, and through it its abstract superclass
+ * {@link org.grails.datastore.gorm.query.criteria.AbstractCriteriaBuilder},
which cannot be
+ * instantiated directly. Collaborators
(MappingContext/PersistentEntity/QueryCreator/Query) are
+ * mocked since this class's own responsibility is translating DSL calls into
Query.Criterion
+ * objects and delegating to a Query, not persistence itself.
*/
class CriteriaBuilderSpec extends Specification {
- @AutoCleanup
- SimpleMapDatastore datastore = new
SimpleMapDatastore(CriteriaBuilderSpecBook, CriteriaBuilderSpecAuthor)
+ PersistentProperty idProperty = Stub(PersistentProperty) {
+ getName() >> 'id'
+ }
+ PersistentProperty nameProperty = Stub(PersistentProperty) {
+ getName() >> 'name'
+ }
+ PersistentEntity persistentEntity = Stub(PersistentEntity) {
+ getIdentity() >> idProperty
+ getPropertyByName(_) >> nameProperty
+ }
+ MappingContext mappingContext = Stub(MappingContext) {
+ getPersistentEntity(CriteriaBuilderTestPerson.name) >> persistentEntity
+ }
+ Query query = Mock(Query)
+ QueryCreator queryCreator = Stub(QueryCreator) {
+ createQuery(CriteriaBuilderTestPerson) >> query
+ isSchemaless() >> false
+ }
+ Session session = Stub(Session) {
+ getMappingContext() >> mappingContext
+ }
+
+ CriteriaBuilder<CriteriaBuilderTestPerson> newBuilder() {
+ new
CriteriaBuilder<CriteriaBuilderTestPerson>(CriteriaBuilderTestPerson, session,
query)
+ }
+
+ void "constructor rejects a null target class"() {
+ when:
+ new CriteriaBuilder(null, queryCreator, mappingContext)
+
+ then:
+ thrown(IllegalArgumentException)
+ }
+
+ void "constructor rejects a null mapping context"() {
+ when:
+ new CriteriaBuilder(CriteriaBuilderTestPerson, queryCreator, null)
+
+ then:
+ thrown(IllegalArgumentException)
+ }
+
+ void "constructor rejects a class the mapping context does not recognise
as persistent"() {
+ given:
+ MappingContext unknownContext = Stub(MappingContext) {
+ getPersistentEntity(_) >> null
+ }
+
+ when:
+ new CriteriaBuilder(CriteriaBuilderTestPerson, queryCreator,
unknownContext)
- void "cache/select/order can be called directly on a bare createCriteria()
without a wrapping closure"() {
- given: "a criteria builder obtained directly, not via .list{}/.get{} -
previously NPE'd on a null query"
- def criteria = CriteriaBuilderSpecBook.createCriteria()
+ then:
+ IllegalArgumentException e = thrown()
+ e.message.contains(CriteriaBuilderTestPerson.name)
+ }
+ void "getTargetClass returns the class the criteria was built for"() {
expect:
- criteria.cache(true).is(criteria)
- criteria.select('title').is(criteria)
- criteria.order('title').is(criteria)
- criteria.order('title', 'desc').is(criteria)
- criteria.getPersistentEntity() != null
+ newBuilder().targetClass == CriteriaBuilderTestPerson
+ }
+
+ void "constructing from a Session resolves the mapping context and query
creator from it"() {
+ given:
+ Session session = Stub(Session) {
+ getMappingContext() >> mappingContext
+ }
+
+ when:
+ def criteria = new
CriteriaBuilder<CriteriaBuilderTestPerson>(CriteriaBuilderTestPerson, session)
+
+ then:
+ criteria.targetClass == CriteriaBuilderTestPerson
+ criteria.session.is(session)
+ }
+
+ void "constructing from a Session and an existing query reuses that
query"() {
+ given:
+ Session session = Stub(Session) {
+ getMappingContext() >> mappingContext
+ }
+
+ when:
+ def criteria = new
CriteriaBuilder<CriteriaBuilderTestPerson>(CriteriaBuilderTestPerson, session,
query)
+
+ then:
+ criteria.query.is(query)
+ criteria.session.is(session)
}
- void "join(String) can be called directly on a bare createCriteria()
without a wrapping closure"() {
- given: "join(String) had the same NPE as cache/select before this fix
- test with a real association"
- def criteria = CriteriaBuilderSpecAuthor.createCriteria()
+ void "setUniqueResult flags a single-result query"() {
+ given:
+ def criteria = newBuilder()
+
+ when:
+ criteria.setUniqueResult(true)
+
+ then:
+ criteria.uniqueResult
+ }
+ void "getQuery returns null before the query has been initialized"() {
expect:
- criteria.join('books').is(criteria)
+ new
CriteriaBuilder<CriteriaBuilderTestPerson>(CriteriaBuilderTestPerson,
queryCreator, mappingContext).query == null
+ }
+
+ void "cache delegates to the query and preserves BuildableCriteria for
chaining"() {
+ given:
+ def criteria = newBuilder()
+
+ when:
+ BuildableCriteria result = criteria.cache(true)
+
+ then:
+ 1 * query.cache(true)
+ result.is(criteria)
+ }
+
+ void "readOnly sets the readOnly flag and preserves BuildableCriteria for
chaining"() {
+ given:
+ def criteria = newBuilder()
+
+ when:
+ BuildableCriteria result = criteria.readOnly(true)
+
+ then:
+ result.is(criteria)
+ criteria.readOnly
+ }
+
+ void "join(String) delegates to the query and preserves BuildableCriteria
for chaining"() {
+ given:
+ def criteria = newBuilder()
+
+ when:
+ BuildableCriteria result = criteria.join('books')
+
+ then:
+ 1 * query.join('books')
+ result.is(criteria)
+ }
+
+ void "join(String, JoinType) delegates to the query with the join type"() {
+ given:
+ def criteria = newBuilder()
+
+ when:
+ BuildableCriteria result = criteria.join('books',
jakarta.persistence.criteria.JoinType.LEFT)
+
+ then:
+ 1 * query.join('books', jakarta.persistence.criteria.JoinType.LEFT)
+ result.is(criteria)
+ }
+
+ void "select delegates to the query and preserves BuildableCriteria for
chaining"() {
+ given:
+ def criteria = newBuilder()
+
+ when:
+ BuildableCriteria result = criteria.select('name')
+
+ then:
+ 1 * query.select('name')
+ result.is(criteria)
+ }
+
+ @Unroll
+ void "#method(propertyName, value) adds a #criterion.simpleName criterion
for the property and value"() {
+ given:
+ def criteria = newBuilder()
+
+ when:
+ def result = criteria."$method"('name', 'value')
+
+ then:
+ 1 * query.add({ it.getClass() == criterion && it.property == 'name' &&
it.value == 'value' })
Review Comment:
Tightened every criterion interaction from `1 * query.add(_)` to check the
concrete `Query.Criterion` class plus property/value (or `otherProperty`,
`from`/`to`, subquery identity, junction contents). A wildcard only proves
*something* was added, and it's what hid the `ltSome` mismatch.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/criteria/AbstractCriteriaBuilder.java:
##########
@@ -562,9 +599,9 @@ public Criteria geSome(String propertyName, Closure<?>
propertyValue) {
}
@Override
- public Criteria ltSome(String propertyName, QueryableCriteria
propertyValue) {
+ public Criteria ltSome(String propertyName, QueryableCriteria<?>
propertyValue) {
validatePropertyName(propertyName, "ltSome");
- addToCriteria(new Query.LessThanEqualsSome(propertyName,
propertyValue));
+ addToCriteria(new Query.LessThanSome(propertyName, propertyValue));
Review Comment:
Pre-existing bug that the wildcard `1 * query.add(_)` interactions in the
spec let through: `ltSome` built a `LessThanEqualsSome`.
`AbstractDetachedCriteria.ltSome` already uses `LessThanSome`, and the
Hibernate 5/7 adapters and `JpaQueryBuilder` all handle it, so this is purely a
wrong-criterion fix. Reverting it fails the spec.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/criteria/AbstractCriteriaBuilder.java:
##########
@@ -1113,16 +1184,14 @@ private void handleJunction(Query.Junction junction,
Closure callable) {
* LogicalExpression.
*/
protected Query.Criterion addToCriteria(Query.Criterion c) {
Review Comment:
Dropped `@SuppressWarnings("UnusedReturnValue")`: it's an IntelliJ
inspection key that javac, PMD and Checkstyle don't recognize, so it only adds
noise.
--
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]
