jdaugherty commented on code in PR #16559:
URL: https://github.com/apache/grails-core/pull/16559#discussion_r4256550230
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/PredicateGenerator.java:
##########
@@ -305,20 +305,27 @@ private Predicate handleAssociationCriteria(
JpaQueryContext fromsByProvider,
DetachedAssociationCriteria<?> associationCriteria) {
String associationName = associationCriteria.getAssociationPath();
- From<?, ?> associationRoot = fromsByProvider.getFrom(associationName);
+ // The block joins from the entity its context queries, with the join
type recorded for it there. A join that
+ // an outer block or an outer query made for an association of the
same name, such as face in
+ // face { ... } owner { face { ... } } or owner in a subquery, is
another table, so only the joins of this
+ // context are reused.
+ From<?, ?> associationRoot =
fromsByProvider.getLocalFrom(associationName);
if (associationRoot == null) {
- // Check if we already have it in our parent or alias map
- Expression<?> expr =
fromsByProvider.getFullyQualifiedExpression(associationName);
- if (expr instanceof From<?, ?> from) {
- associationRoot = from;
- } else {
- associationRoot =
fromsByProvider.getRoot().join(associationName);
- fromsByProvider.addFrom(associationName, associationRoot);
- }
+ HibernateAlias definition =
fromsByProvider.getLocalAliasDefinition(associationName);
+ associationRoot = definition != null ?
+ fromsByProvider.getRoot().join(definition.path(),
definition.joinType()) :
+ fromsByProvider.getRoot().join(associationName,
fromsByProvider.getLocalJoinType(associationName));
+ fromsByProvider.addFrom(associationName, associationRoot);
+ fromsByProvider.registerAlias(associationName, associationRoot);
}
// Create a nested context for this association
JpaQueryContext nestedContext = new JpaQueryContext(fromsByProvider,
null, associationRoot);
+ // The join type of an association block nested in this one, such as
the LEFT of face in
+ // owner(LEFT) { face(LEFT) { ... } }, is recorded on this block's
criteria. It is registered as a join type,
+ // not as an alias, so a criterion on the property of the same name,
such as isNotEmpty('pets') next to
+ // pets { ... }, still resolves to the property rather than to the
join.
+
associationCriteria.getJoinTypes().forEach(nestedContext::registerJoinType);
Review Comment:
Not something for this PR to fix, but the claim here holds only when the
property criterion comes before the block. With `pets { eq('name', 'Rex') }`
followed by `isNotEmpty('pets')` inside the `owner` block, `ExpressionResolver`
returns the join the block already made (step 5, the join tracker) and
`CriteriaBuilder.isNotEmpty` throws `ClassCastException: SqmSetJoin cannot be
cast to SqmPluralValuedSimplePath`. The same thing happens at the top level in
either order, because a block of the queried class always records a join type
and `registerDetachedJoins` turns that into an alias definition. All of this
behaves identically on `8.0.x` without this PR, so it is pre-existing, and the
spec's criterion-first ordering is the right guard for what this change must
not break. Could you narrow the wording to say the criterion still resolves to
the property when it precedes the block? The resolver returning a join for a
collection-valued path is worth a follow-up issue of its own.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/PredicateGenerator.java:
##########
@@ -305,20 +305,27 @@ private Predicate handleAssociationCriteria(
JpaQueryContext fromsByProvider,
DetachedAssociationCriteria<?> associationCriteria) {
String associationName = associationCriteria.getAssociationPath();
- From<?, ?> associationRoot = fromsByProvider.getFrom(associationName);
+ // The block joins from the entity its context queries, with the join
type recorded for it there. A join that
+ // an outer block or an outer query made for an association of the
same name, such as face in
+ // face { ... } owner { face { ... } } or owner in a subquery, is
another table, so only the joins of this
+ // context are reused.
+ From<?, ?> associationRoot =
fromsByProvider.getLocalFrom(associationName);
if (associationRoot == null) {
- // Check if we already have it in our parent or alias map
- Expression<?> expr =
fromsByProvider.getFullyQualifiedExpression(associationName);
- if (expr instanceof From<?, ?> from) {
- associationRoot = from;
- } else {
- associationRoot =
fromsByProvider.getRoot().join(associationName);
- fromsByProvider.addFrom(associationName, associationRoot);
- }
+ HibernateAlias definition =
fromsByProvider.getLocalAliasDefinition(associationName);
+ associationRoot = definition != null ?
+ fromsByProvider.getRoot().join(definition.path(),
definition.joinType()) :
+ fromsByProvider.getRoot().join(associationName,
fromsByProvider.getLocalJoinType(associationName));
+ fromsByProvider.addFrom(associationName, associationRoot);
+ fromsByProvider.registerAlias(associationName, associationRoot);
Review Comment:
Is this line needed? With it removed,
`HibernateCriteriaBuilderNestedJoinTypeSpec` (8), `PredicateGeneratorSpec` (44)
and `JpaCriteriaQueryCreatorSpec` (26) all still pass. The old code only
realized the association name as an alias when an alias definition existed for
it; this realizes it for every block, so after the block the association name
resolves through the alias registry of the enclosing context rather than
through the join tracker. The join tracker already finds the join for a bare
path (`ExpressionResolver` step 5), so I don't see what this adds. If it is
there for a case I'm missing, a short comment saying which would help;
otherwise I'd drop it to keep the alias registry to the explicit aliases, which
is the same reasoning the comment below gives for registering join types rather
than aliases.
--
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]
