Copilot commented on code in PR #16818:
URL: https://github.com/apache/grails-core/pull/16818#discussion_r4434029966
##########
grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/hibernatequery/PredicateGeneratorSpec.groovy:
##########
@@ -632,6 +632,53 @@ class PredicateGeneratorSpec extends
HibernateGormDatastoreSpec {
then:
thrown(UnsupportedOperationException)
}
+
+ def "subquery translation does not mutate source criteria projection list
in-place"() {
+ given:
+ def subCriteria = new
DetachedCriteria(PredicateGeneratorSpecPerson).eq("lastName", "Smith")
+ List criteria = [
+ new Query.In("id", subCriteria),
+ new Query.EqualsAll("age", subCriteria)
+ ]
+
+ when:
+ def predicates = predicateGenerator.getPredicates(query, root,
criteria, fromProvider, personEntity)
+
+ then:
+ predicates.length == 2
+ subCriteria.projections.isEmpty()
+ }
+
+ def "property comparison and property name criteria throw
ConfigurationException on non-existent property"() {
+ when:
+ predicateGenerator.getPredicates(query, root, [criterion],
fromProvider, personEntity)
Review Comment:
These are the only new tests for the fail-fast behavior, but they invoke
`PredicateGenerator` directly instead of exercising the public query API.
Please cover these four invalid-path cases through `DetachedCriteria.list()` or
`createCriteria().list()` and assert `ConfigurationException` identifies the
offending property. That verifies the exception survives the normal
query-translation path and follows the requirement to test through end-user
APIs.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/FunctionCallingCriterionHandler.java:
##########
@@ -77,7 +77,12 @@ public Predicate handle(CriterionContext context,
FunctionCallingCriterion crite
} else if (wrapped instanceof Query.In in) {
Collection<Object> values = new ArrayList<>();
for (Object v : in.getValues()) {
-
values.add(convertFunctionValue(context.getConversionService(), functionName,
v));
+ if (v != null) {
+
values.add(convertFunctionValue(context.getConversionService(), functionName,
v));
Review Comment:
The function-wrapped `In` path is not exercised by the new null-handling
tests: those use bare `inList`/`Query.In`, while `WhereQueryFunctionsSpec` only
tests a non-null `upper(name) in [...]` list. Please extend that public-query
spec with mixed-null, all-null, and empty lists and assert the returned rows,
including a numeric-returning function to cover value conversion. Otherwise
this separate handler's null filtering and false-predicate branch have no
regression coverage.
--
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]
