jamesfredley commented on code in PR #15448: URL: https://github.com/apache/grails-core/pull/15448#discussion_r2849056056
########## 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: This is not accurate - of the 15 integration specs in the gorm module, 9 use plain `@Integration` (including `GormWhereQueryAdvancedSpec` which is the most similar spec to this one) and only 6 use `@Integration(applicationClass = Application)`. Both forms work; plain `@Integration` is consistent with the majority of specs in this module. No change needed. ########## 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: Good catch - the comment was indeed misleading. Fixed in 8d50b75: updated the Javadoc to accurately describe this as a "Read-only transactional variant" that verifies the fix works with `@Transactional(readOnly = true)`. ########## 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: These unit tests use `gcl.parseClass(...)` which compiles the class in isolation - no GORM datastore, no `PlatformTransactionManager`, no Spring context. Invoking `service.findPeopleByAge(1)` on such an instance would fail with a missing transaction manager error, not because of the bug being tested. The purpose of these tests is to verify the AST transformation doesn't corrupt the code (the bug manifests as a compilation/class-loading failure when `VariableScopeVisitor` rewrites `delegate.zone(closure)` to `zone.call(closure)`). Compile + instantiate is the correct assertion level for this unit test context. Runtime behavior is fully covered by the 5 functional integration tests in `TransactionalWhereQueryVariableScopeSpec` which run inside a real Grails application context with GORM and transaction infrastructure. No change needed. ########## 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 reasoning as above - these `gcl.parseClass` compiled classes have no GORM/Spring infrastructure backing them, so invoking the methods would fail for unrelated infrastructure reasons. The compile + instantiate assertion correctly validates the AST fix, and the functional integration tests in `TransactionalWhereQueryVariableScopeSpec` cover the full runtime path. No change needed. -- 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]
