Copilot commented on code in PR #15448: URL: https://github.com/apache/grails-core/pull/15448#discussion_r2849025052
########## grails-test-examples/gorm/grails-app/services/gorm/WhereQueryVariableScopeService.groovy: ########## @@ -0,0 +1,86 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package gorm + +import grails.gorm.transactions.Transactional + +/** + * Service for testing where queries with local variable names that + * match domain property names inside @Transactional methods. + * + * This exercises the fix for https://github.com/apache/grails-core/issues/11464 + * where VariableScopeVisitor rewrites delegate method calls generated by + * DetachedCriteriaTransformer when implicitThis is true and a local variable + * with the same name exists in scope. + */ +@Transactional +class WhereQueryVariableScopeService { + + /** + * Where query using a local variable named 'author' which matches + * the Book.author association property. + */ + List<Book> findBooksByAuthor(Author author) { + Book.where { + author == author + }.list() + } + + /** + * Where query using a local variable named 'title' which matches + * the Book.title property. + */ + List<Book> findBooksByTitle(String title) { + Book.where { + title == title + }.list() + } + + /** + * Where query using association property traversal where the + * association variable name 'author' matches a local variable. + */ + List<Book> findBooksByAuthorName(String name) { + def author = Author.findByName(name) + Book.where { + author.name == name + }.list() + } + + /** + * Where query with multiple variable name collisions. + */ + List<Book> findBooksWithMultipleCollisions(String title, Boolean inStock) { + Book.where { + title == title && inStock == inStock + }.list() + } + + /** + * Non-transactional baseline - same query pattern works without + * @Transactional because VariableScopeVisitor is not re-run. + * Uses @Transactional(readOnly = true) to verify read-only variant. + */ + @Transactional(readOnly = true) + List<Book> findBooksByTitleReadOnly(String title) { + Book.where { Review Comment: The comment above this method says it's a "Non-transactional baseline", but the method is explicitly `@Transactional(readOnly = true)` (and the class is `@Transactional`). This is misleading for future readers—please update the comment to reflect that this is exercising the read-only transactional variant of the same scenario. ########## grails-datamapping-core-test/src/test/groovy/grails/gorm/tests/WhereMethodSpec.groovy: ########## @@ -1620,6 +1620,90 @@ class Project { results.every { it.age > 5 && it.age < 42 && it.lastName == 'Simpson' } } + + @Issue('https://github.com/apache/grails-core/issues/11464') + def "Test where query with overlapping variable names compiles in @Transactional context"() { + when: "A @Transactional class with where queries using variable names that match domain properties is compiled" + def gcl = new GroovyClassLoader(getClass().classLoader) + def clazz = gcl.parseClass(''' +import org.apache.grails.data.testing.tck.domains.* +import grails.gorm.annotation.* +import grails.gorm.transactions.Transactional +import org.grails.datastore.gorm.query.transform.ApplyDetachedCriteriaTransform + +@ApplyDetachedCriteriaTransform +@Entity +@Transactional +class TransactionalQueryService { + + def findPeopleByAge(int age) { + Person.where { + age > age + }.list() + } + + def findPetsOlderThan(int age) { + Person.where { + pets { age > age } + }.list() + } + + def findByFirstName(String firstName) { + Person.where { + firstName == firstName + }.list() + } + + def findByLastNameWithAssociation(String lastName) { + def pets = [] + Person.where { + lastName == lastName && pets { age > 5 } + }.list() + } +} +''') + + then: "The class compiles and can be instantiated without error" + clazz != null + clazz.newInstance() != null Review Comment: This test only asserts that the class compiles and can be instantiated, but the reported regression (#11464) manifests when the transactional method is executed (runtime `MissingMethodException` after VariableScopeVisitor rewriting). As written, this unit test would still pass even if the bug regressed. Consider extending the test to actually invoke one of the compiled methods and assert it executes without throwing (or inspect the transformed code) so it meaningfully guards against the regression. ```suggestion then: "The class compiles, can be instantiated, and its methods execute without error" clazz != null def service = clazz.newInstance() service != null service.findPeopleByAge(1) service.findPetsOlderThan(1) service.findByFirstName('Bob') service.findByLastNameWithAssociation('Smith') ``` ########## grails-datamapping-core-test/src/test/groovy/grails/gorm/tests/WhereMethodSpec.groovy: ########## @@ -1620,6 +1620,90 @@ class Project { results.every { it.age > 5 && it.age < 42 && it.lastName == 'Simpson' } } + + @Issue('https://github.com/apache/grails-core/issues/11464') + def "Test where query with overlapping variable names compiles in @Transactional context"() { + when: "A @Transactional class with where queries using variable names that match domain properties is compiled" + def gcl = new GroovyClassLoader(getClass().classLoader) + def clazz = gcl.parseClass(''' +import org.apache.grails.data.testing.tck.domains.* +import grails.gorm.annotation.* +import grails.gorm.transactions.Transactional +import org.grails.datastore.gorm.query.transform.ApplyDetachedCriteriaTransform + +@ApplyDetachedCriteriaTransform +@Entity +@Transactional +class TransactionalQueryService { + + def findPeopleByAge(int age) { + Person.where { + age > age + }.list() + } + + def findPetsOlderThan(int age) { + Person.where { + pets { age > age } + }.list() + } + + def findByFirstName(String firstName) { + Person.where { + firstName == firstName + }.list() + } + + def findByLastNameWithAssociation(String lastName) { + def pets = [] + Person.where { + lastName == lastName && pets { age > 5 } + }.list() + } +} +''') + + then: "The class compiles and can be instantiated without error" + clazz != null + clazz.newInstance() != null + } + + @Issue('https://github.com/apache/grails-core/issues/11464') + def "Test where query with overlapping association variable name compiles in @Transactional context"() { + when: "A @Transactional class uses a where query with an association name matching a local variable" + def gcl = new GroovyClassLoader(getClass().classLoader) + def clazz = gcl.parseClass(''' +import org.apache.grails.data.testing.tck.domains.* +import grails.gorm.annotation.* +import grails.gorm.transactions.Transactional +import org.grails.datastore.gorm.query.transform.ApplyDetachedCriteriaTransform + +@ApplyDetachedCriteriaTransform +@Entity +class TransactionalAssocQueryService { + + @Transactional + def findPeopleByPetAge(int age) { + def pets = [] + Person.where { + pets.age == age + }.list() + } + + @Transactional + def findPetsWithOwnerName(String firstName) { + Pet.where { + owner.firstName == firstName + }.list() + } +} +''') + + then: "The class compiles and can be instantiated without error" + clazz != null + clazz.newInstance() != null Review Comment: Same as the previous test: compiling + instantiating doesn’t exercise the runtime failure mode from #11464 (the rewritten `variable.call(closure)` only happens when the transactional method runs). To make this a regression test, consider invoking one of these methods and asserting it runs without throwing (or otherwise asserting on the transformed output). ```suggestion then: "The class compiles, can be instantiated, and transactional methods run without error" clazz != null def service = clazz.newInstance() service != null service.findPeopleByPetAge(10) service.findPetsWithOwnerName("Homer") ``` ########## grails-test-examples/gorm/src/integration-test/groovy/gorm/TransactionalWhereQueryVariableScopeSpec.groovy: ########## @@ -0,0 +1,109 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package gorm + +import spock.lang.Issue +import spock.lang.Specification + +import grails.gorm.transactions.Rollback +import grails.testing.mixin.integration.Integration + +import org.springframework.beans.factory.annotation.Autowired + +/** + * Functional tests for where queries with local variable names that match + * domain property names inside @Transactional methods. + * + * Verifies the fix for https://github.com/apache/grails-core/issues/11464 + * where the interaction between DetachedCriteriaTransformer and + * TransactionalTransform caused VariableScopeVisitor to rewrite + * delegate.property(closure) calls to variable.call(closure) when + * a local variable with the same name existed in scope. + */ +@Integration Review Comment: All other integration specs in this example module specify the application class (e.g. `@Integration(applicationClass = Application)`). Without it, the test harness may fail to bootstrap the correct Grails application context for this spec. Please align this annotation with the rest of `grails-test-examples/gorm` integration tests. ```suggestion @Integration(applicationClass = Application) ``` -- 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]
