Copilot commented on code in PR #16773:
URL: https://github.com/apache/grails-core/pull/16773#discussion_r4384849588
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/ExpressionResolver.java:
##########
@@ -108,8 +114,14 @@ private Expression<?> resolveFromAlias(String alias,
String subPath) {
return subPath != null ? getPath((From<?, ?>) aliased, subPath) :
aliased;
}
- // 3. If defined but not joined, materialize it on the CURRENT root
+ // 3. If defined but not joined, materialize it on the CURRENT root or
PARENT context
if (aliasRegistry.isDefined(alias)) {
+ if (!aliasRegistry.hasLocalDefinition(alias) && context != null &&
context.getParent() != null) {
+ Expression<?> parentExpr =
context.getParent().getFullyQualifiedExpression(alias);
Review Comment:
This delegates unrealized aliases from ordinary association blocks as well
as subqueries. Because `discoverAliases()` registers nested aliases on the
query-root context, a detached query such as `owner { createAlias('address',
'a'); eq('a.city', 'X') }` now joins `address` on Pet rather than its owner,
throwing if Pet has no such association or filtering the wrong association if
it does. Preserve each nested alias's declaration scope when deciding which
context should materialize it.
##########
grails-data-hibernate7/core/src/test/groovy/grails/orm/SubqueryAssociationCorrelationSpec.groovy:
##########
@@ -0,0 +1,143 @@
+/*
+ * 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 grails.orm
+
+import grails.gorm.DetachedCriteria
+import grails.gorm.annotation.Entity
+import grails.gorm.tests.HibernateGormDatastoreSpec
+
+class SubqueryAssociationCorrelationSpec extends HibernateGormDatastoreSpec {
+
+ def setupSpec() {
+ manager.registerDomainClasses(SubqueryCorrPerson, SubqueryCorrPet)
+ }
+
+ def setup() {
+ def homer = new SubqueryCorrPerson(firstName:
"Homer").save(failOnError: true)
+ def ned = new SubqueryCorrPerson(firstName: "Ned").save(failOnError:
true)
+
+ // Homer owns "Santa's Little Helper"
+ new SubqueryCorrPet(name: "Santa's Little Helper", owner:
homer).save(failOnError: true)
+ // Ned owns a pet named "Homer"
+ new SubqueryCorrPet(name: "Homer", owner: ned).save(failOnError: true,
flush: true)
+ }
+
+ void "subquery projecting association property should not correlate with
outer association block"() {
+ when:
+ // Ned owns a pet named "Homer".
+ // The subquery projects all pets' owner.firstName, which contains
["Homer", "Ned"].
+ // Ned's pet's name is "Homer", which is in ["Homer", "Ned"].
+ // If the subquery incorrectly correlates with the outer `owner` block
(where owner is Ned),
+ // the subquery would project Ned's firstName ("Ned") instead of
joining owner from its own root,
+ // resulting in 0 matches because "Homer" != "Ned".
+ def results = SubqueryCorrPet.createCriteria().list {
+ owner {
+ eq('firstName', 'Ned')
+ }
+ 'in'('name', new DetachedCriteria(SubqueryCorrPet).build {
+ projections {
+ property('owner.firstName')
+ }
+ })
+ }
+
+ then:
+ results.size() == 1
+ results[0].name == "Homer"
+ results[0].owner.firstName == "Ned"
+ }
+
+ void "subquery filtering on association property should not correlate with
outer association block"() {
+ when:
+ def results = SubqueryCorrPet.createCriteria().list {
+ owner {
+ eq('firstName', 'Ned')
+ }
+ 'in'('name', new DetachedCriteria(SubqueryCorrPet).build {
+ eq('owner.firstName', 'Ned')
+ projections {
+ property('name')
+ }
+ })
+ }
+
+ then:
+ results.size() == 1
+ results[0].name == "Homer"
+ }
+
+ void "subquery with its own association block should not correlate with
outer association block"() {
+ when:
+ def results = SubqueryCorrPet.createCriteria().list {
+ owner {
+ eq('firstName', 'Ned')
+ }
+ 'in'('name', new DetachedCriteria(SubqueryCorrPet).build {
+ owner {
+ eq('firstName', 'Ned')
+ }
Review Comment:
The association-block regression also passes when the inner block
incorrectly reuses the outer join: its owner predicate is already true for the
outer Ned row, and all pet names still include `Homer`. Use different inner and
outer owner predicates with a shared pet name so this test fails when
association-block joins leak into the subquery.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/JpaQueryContext.java:
##########
@@ -162,7 +173,7 @@ public void registerAliasFromPath(String path) {
}
public boolean hasAlias(String alias) {
- return aliasRegistry.isDefined(alias) ||
aliasRegistry.hasRealized(alias) || (parent != null && parent.hasAlias(alias));
+ return aliasRegistry.isDefined(alias) ||
aliasRegistry.hasRealized(alias);
Review Comment:
The explicit-only visibility rule is still bypassed by
`getAliasedExpression()`, which unconditionally retries the parent after the
registry rejects an implicit alias. `JpaProjectionTranslator` uses that method
for count/countDistinct projections, so a subquery projecting
`countDistinct('owner')` after an outer `owner { ... }` block still counts the
outer owner instead of the subquery's owners. Remove that fallback and let
AliasRegistry handle explicit parent aliases.
##########
grails-data-hibernate7/core/src/test/groovy/grails/orm/SubqueryAssociationCorrelationSpec.groovy:
##########
@@ -0,0 +1,143 @@
+/*
+ * 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 grails.orm
+
+import grails.gorm.DetachedCriteria
+import grails.gorm.annotation.Entity
+import grails.gorm.tests.HibernateGormDatastoreSpec
+
+class SubqueryAssociationCorrelationSpec extends HibernateGormDatastoreSpec {
+
+ def setupSpec() {
+ manager.registerDomainClasses(SubqueryCorrPerson, SubqueryCorrPet)
+ }
+
+ def setup() {
+ def homer = new SubqueryCorrPerson(firstName:
"Homer").save(failOnError: true)
+ def ned = new SubqueryCorrPerson(firstName: "Ned").save(failOnError:
true)
+
+ // Homer owns "Santa's Little Helper"
+ new SubqueryCorrPet(name: "Santa's Little Helper", owner:
homer).save(failOnError: true)
+ // Ned owns a pet named "Homer"
+ new SubqueryCorrPet(name: "Homer", owner: ned).save(failOnError: true,
flush: true)
+ }
+
+ void "subquery projecting association property should not correlate with
outer association block"() {
+ when:
+ // Ned owns a pet named "Homer".
+ // The subquery projects all pets' owner.firstName, which contains
["Homer", "Ned"].
+ // Ned's pet's name is "Homer", which is in ["Homer", "Ned"].
+ // If the subquery incorrectly correlates with the outer `owner` block
(where owner is Ned),
+ // the subquery would project Ned's firstName ("Ned") instead of
joining owner from its own root,
+ // resulting in 0 matches because "Homer" != "Ned".
+ def results = SubqueryCorrPet.createCriteria().list {
+ owner {
+ eq('firstName', 'Ned')
+ }
+ 'in'('name', new DetachedCriteria(SubqueryCorrPet).build {
+ projections {
+ property('owner.firstName')
+ }
+ })
+ }
+
+ then:
+ results.size() == 1
+ results[0].name == "Homer"
+ results[0].owner.firstName == "Ned"
+ }
+
+ void "subquery filtering on association property should not correlate with
outer association block"() {
+ when:
+ def results = SubqueryCorrPet.createCriteria().list {
+ owner {
+ eq('firstName', 'Ned')
+ }
+ 'in'('name', new DetachedCriteria(SubqueryCorrPet).build {
+ eq('owner.firstName', 'Ned')
+ projections {
+ property('name')
+ }
+ })
+ }
+
+ then:
+ results.size() == 1
+ results[0].name == "Homer"
+ }
+
+ void "subquery with its own association block should not correlate with
outer association block"() {
+ when:
+ def results = SubqueryCorrPet.createCriteria().list {
+ owner {
+ eq('firstName', 'Ned')
+ }
+ 'in'('name', new DetachedCriteria(SubqueryCorrPet).build {
+ owner {
+ eq('firstName', 'Ned')
+ }
+ projections {
+ property('name')
+ }
+ })
+ }
+
+ then:
+ results.size() == 1
+ results[0].name == "Homer"
+ }
+
+ void "correlated subquery using explicit outer alias works"() {
+ when:
+ def results = SubqueryCorrPet.createCriteria().list {
+ createAlias('owner', 'o')
+ 'in'('name', new DetachedCriteria(SubqueryCorrPet).build {
+ eqProperty('owner.id', 'o.id')
+ projections {
+ property('name')
+ }
+ })
+ }
+
+ then:
+ results.size() == 2
+ results*.name.sort() == ["Homer", "Santa's Little Helper"]
Review Comment:
This test passes even if `o` is incorrectly materialized on the subquery
root: each outer pet's name appears in the subquery either way. With this
fixture, project `owner.firstName` and expect no results; a subquery-local `o`
would instead return the pet named Homer.
##########
grails-data-hibernate7/core/src/test/groovy/grails/orm/SubqueryAssociationCorrelationSpec.groovy:
##########
@@ -0,0 +1,143 @@
+/*
+ * 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 grails.orm
+
+import grails.gorm.DetachedCriteria
+import grails.gorm.annotation.Entity
+import grails.gorm.tests.HibernateGormDatastoreSpec
+
+class SubqueryAssociationCorrelationSpec extends HibernateGormDatastoreSpec {
+
+ def setupSpec() {
+ manager.registerDomainClasses(SubqueryCorrPerson, SubqueryCorrPet)
+ }
+
+ def setup() {
+ def homer = new SubqueryCorrPerson(firstName:
"Homer").save(failOnError: true)
+ def ned = new SubqueryCorrPerson(firstName: "Ned").save(failOnError:
true)
+
+ // Homer owns "Santa's Little Helper"
+ new SubqueryCorrPet(name: "Santa's Little Helper", owner:
homer).save(failOnError: true)
+ // Ned owns a pet named "Homer"
+ new SubqueryCorrPet(name: "Homer", owner: ned).save(failOnError: true,
flush: true)
+ }
+
+ void "subquery projecting association property should not correlate with
outer association block"() {
+ when:
+ // Ned owns a pet named "Homer".
+ // The subquery projects all pets' owner.firstName, which contains
["Homer", "Ned"].
+ // Ned's pet's name is "Homer", which is in ["Homer", "Ned"].
+ // If the subquery incorrectly correlates with the outer `owner` block
(where owner is Ned),
+ // the subquery would project Ned's firstName ("Ned") instead of
joining owner from its own root,
+ // resulting in 0 matches because "Homer" != "Ned".
+ def results = SubqueryCorrPet.createCriteria().list {
+ owner {
+ eq('firstName', 'Ned')
+ }
+ 'in'('name', new DetachedCriteria(SubqueryCorrPet).build {
+ projections {
+ property('owner.firstName')
+ }
+ })
+ }
+
+ then:
+ results.size() == 1
+ results[0].name == "Homer"
+ results[0].owner.firstName == "Ned"
+ }
+
+ void "subquery filtering on association property should not correlate with
outer association block"() {
+ when:
+ def results = SubqueryCorrPet.createCriteria().list {
+ owner {
+ eq('firstName', 'Ned')
+ }
+ 'in'('name', new DetachedCriteria(SubqueryCorrPet).build {
+ eq('owner.firstName', 'Ned')
Review Comment:
This fixture does not detect accidental correlation: both queries filter the
owner to Ned, and Ned's pet already supplies its own name to the subquery.
Reusing the outer owner join therefore still returns `Homer`, satisfying these
assertions. Please make the inner owner differ and add a matching name under
that owner so the correct and incorrectly correlated queries return different
results.
--
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]
