borinquenkid commented on PR #16573:
URL: https://github.com/apache/grails-core/pull/16573#issuecomment-6102630575
Thanks for the fix. Reading `JpaCriteriaQueryCreator` as a whole, the alias
handling is the part that is hard to test, and I think it should move into a
class of its own. Three things in the current shape:
* `createQuery()` and `populateSubquery()` repeat the same thirteen lines
(lines 105-117 and 167-179): gather the explicit aliases from the
`HibernateQuery` and from the `HibernateAlias` criteria, build the
`JpaQueryContext`, `registerDetachedJoins`, `discoverAliases`.
* `discoverAliases` is private and recursive, and its behavior (which join
type an alias gets, when a dotted property name implies an alias) can only be
observed by building a whole query against a database. The new
`CreateAliasJoinSpec` has to do exactly that.
* `assignCriteria` calls `discoverAliases` a second time on the same
criteria. It is harmless because of the `hasAlias` guards, but the
`DetachedAssociationCriteria` branch has no guard, and a separate class makes
that visible.
Proposal, behavior unchanged: a package-private class that owns the alias
registration for one detached criteria.
```java
package org.grails.orm.hibernate.query;
/**
* Collects the aliases of a criteria query and registers them in its {@link
JpaQueryContext}: the aliases made with
* {@code createAlias}, the joins requested on the detached criteria, the
aliases of association blocks and the
* aliases implied by a dotted property name.
*/
final class QueryAliasRegistrar {
private final DetachedCriteria<?> detachedCriteria;
private final HibernateQuery hibernateQuery; // null for a detached
criteria without a query
QueryAliasRegistrar(DetachedCriteria<?> detachedCriteria, HibernateQuery
hibernateQuery) {
this.detachedCriteria = detachedCriteria;
this.hibernateQuery = hibernateQuery;
}
/** The context of the query or subquery over {@code root}, with every
alias of this criteria registered. */
JpaQueryContext createContext(JpaQueryContext parent, Root<?> root) {
JpaQueryContext context = JpaQueryContext.forSubquery(parent,
explicitAliases(), root);
registerDetachedJoins(context);
discover(detachedCriteria.getCriteria(), context);
return context;
}
List<HibernateAlias> explicitAliases() {
List<HibernateAlias> aliases = new ArrayList<>();
if (hibernateQuery != null) {
aliases.addAll(hibernateQuery.getAliases());
}
for (Query.Criterion criterion : detachedCriteria.getCriteria()) {
if (criterion instanceof HibernateAlias ha) {
aliases.add(ha);
}
}
return aliases;
}
void registerDetachedJoins(JpaQueryContext context) {
detachedCriteria.getJoinTypes().forEach((path, joinType) ->
context.registerAlias(path, new HibernateAlias(path, path,
joinType)));
}
void discover(List<Query.Criterion> criteria, JpaQueryContext context) {
// the body of discoverAliases as it is in this PR, with
detachedCriteria taken from the field
}
}
```
`JpaCriteriaQueryCreator` then keeps one field, `private final
QueryAliasRegistrar aliases;` built in the constructor, and:
```java
Root<?> root = cq.from(javaClass);
context = aliases.createContext(parentContext, root); //
createQuery()
...
context = aliases.createContext(parentContext, root); //
populateSubquery()
...
aliases.discover(criteriaList, context); //
assignCriteria()
```
That removes the duplicated block and both private methods from the creator,
and the alias rules become unit-testable with a `Mock(JpaQueryContext)`, no
database, for example:
```groovy
def "a join requested on the detached criteria keeps its join type"() {
given:
def detached = new DetachedCriteria(Pet).join('owner', JoinType.LEFT)
def context = Mock(JpaQueryContext)
when:
new QueryAliasRegistrar(detached, null).registerDetachedJoins(context)
then:
1 * context.registerAlias('owner', new HibernateAlias('owner', 'owner',
JoinType.LEFT))
}
def "a dotted property name registers an inner alias for its first
segment"() {
given:
def context = Mock(JpaQueryContext) { hasAlias('owner') >> false }
when:
new QueryAliasRegistrar(new DetachedCriteria(Pet), null)
.discover([new Query.Equals('owner.name', 'x')], context)
then:
1 * context.registerAlias('owner', new HibernateAlias('owner', 'owner'))
}
def "an alias the context already knows is not registered again"() {
given:
def context = Mock(JpaQueryContext) { hasAlias('owner') >> true }
when:
new QueryAliasRegistrar(new DetachedCriteria(Pet), null)
.discover([new Query.Equals('owner.name', 'x')], context)
then:
0 * context.registerAlias(_, _)
}
```
`HibernateAlias` is a record, so the interaction matchers compare it by
value. The association-block case from this PR (`DetachedAssociationCriteria`
with `LEFT` registered for its path) fits the same spec, and
`CreateAliasJoinSpec` can then stay for the query-level behavior (one join, the
right rows) instead of carrying the alias rules too.
One smaller point from the same read: the two `createAlias` overloads in
`HibernateCriteriaBuilder` repeat each other (the two-argument one is the
three-argument one with `INNER`), so the two-argument one can delegate.
Happy to send this as a patch on top of your branch, or you can take it as
it is, whichever you prefer.
--
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]
