diff --git a/ebean-core/src/main/java/io/ebeaninternal/api/SpiExpressionValidation.java b/ebean-core/src/main/java/io/ebeaninternal/api/SpiExpressionValidation.java index 4f28591bdd..2c62437bb3 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/api/SpiExpressionValidation.java +++ b/ebean-core/src/main/java/io/ebeaninternal/api/SpiExpressionValidation.java @@ -12,6 +12,7 @@ public final class SpiExpressionValidation { private final BeanType desc; private final LinkedHashSet unknown = new LinkedHashSet<>(); + private final LinkedHashSet all = new LinkedHashSet<>(); public SpiExpressionValidation(BeanType desc) { this.desc = desc; @@ -21,6 +22,7 @@ public SpiExpressionValidation(BeanType desc) { * Validate that the property expression (path) is valid. */ public void validate(String propertyName) { + all.add(propertyName); if (!desc.isValidExpression(propertyName)) { unknown.add(propertyName); } @@ -33,4 +35,14 @@ public Set unknownProperties() { return unknown; } + /** + * Return the set of all property names visited during this validation, regardless of + * whether they were considered valid against the bean type. Used to inspect the shape of + * an expression (for example, to check whether it references any associated/joined path) + * without needing a correctly-typed bean descriptor. + */ + public Set allProperties() { + return all; + } + } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/deploy/DbSqlContext.java b/ebean-core/src/main/java/io/ebeaninternal/server/deploy/DbSqlContext.java index cd5843abb2..ddf64a0d73 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/deploy/DbSqlContext.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/deploy/DbSqlContext.java @@ -181,4 +181,11 @@ default void addFormula2Join(String joinLiteral, String table, String a2, String * Include the filter many predicates if specified into the JOIN clause. */ void includeFilterMany(); + + /** + * Return true if the given fetch path (relative to the query root) is the exact join clause + * that the pending filterMany predicate must be attached to - i.e. the deepest path the + * filterMany expression itself references. + */ + boolean isFilterManyAttachPoint(String prefix); } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryPredicates.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryPredicates.java index cd5f17d70c..54769f9806 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryPredicates.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryPredicates.java @@ -78,6 +78,11 @@ public final class CQueryPredicates { */ private Set predicateIncludes; private Set orderByIncludes; + /** + * The fetch path (relative to the query root) of the many-root whose own join clause the + * filterMany-in-JOIN predicate is attached to + */ + private String filterManyAttachPath; CQueryPredicates(Binder binder, OrmQueryRequest request) { this.binder = binder; @@ -222,6 +227,8 @@ public void prepare(boolean buildSql) { filterMany = new DefaultExpressionRequest(request, deployParser, binder, filterManyExpr); if (buildSql) { dbFilterMany = filterMany.buildSql(); + // safe as filterManyJoin only holds when the expression is root-property only - + filterManyAttachPath = manyProperty.path(); } } } @@ -399,6 +406,14 @@ String dbFilterManyJoin() { return filterManyJoin ? dbFilterMany : null; } + /** + * Return the fetch path of the filterMany-in-JOIN predicate - the path whose own join clause + * the predicate must be appended to (or null if there is no filterMany-in-JOIN predicate at all). + */ + String filterManyAttachPath() { + return filterManyJoin ? filterManyAttachPath : null; + } + /** * Return the db column version of the order by clause. */ diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/DefaultDbSqlContext.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/DefaultDbSqlContext.java index 67398de30b..ba3f142a80 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/DefaultDbSqlContext.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/query/DefaultDbSqlContext.java @@ -23,6 +23,7 @@ final class DefaultDbSqlContext implements DbSqlContext { private final ArrayStack prefixStack = new ArrayStack<>(); private final String fromForUpdate; private final String dbFilterManyJoin; + private final String filterManyAttachPath; private boolean useColumnAlias; private int columnIndex; private int asOfTableCount; @@ -42,7 +43,8 @@ final class DefaultDbSqlContext implements DbSqlContext { private boolean joinSuppressed; DefaultDbSqlContext(SqlTreeAlias alias, String columnAliasPrefix, CQueryHistorySupport historySupport, - CQueryDraftSupport draftSupport, String fromForUpdate, String dbFilterManyJoin) { + CQueryDraftSupport draftSupport, String fromForUpdate, String dbFilterManyJoin, + String filterManyAttachPath) { this.alias = alias; this.columnAliasPrefix = columnAliasPrefix; this.useColumnAlias = columnAliasPrefix != null; @@ -51,6 +53,12 @@ final class DefaultDbSqlContext implements DbSqlContext { this.historyQuery = (historySupport != null); this.fromForUpdate = fromForUpdate; this.dbFilterManyJoin = dbFilterManyJoin; + this.filterManyAttachPath = filterManyAttachPath; + } + + @Override + public boolean isFilterManyAttachPoint(String prefix) { + return dbFilterManyJoin != null && filterManyAttachPath != null && filterManyAttachPath.equals(prefix); } @Override diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeBuilder.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeBuilder.java index db6a44cfe9..e2990149cb 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeBuilder.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeBuilder.java @@ -108,7 +108,7 @@ public final class SqlTreeBuilder { CQueryHistorySupport historySupport = builder.historySupport(query); CQueryDraftSupport draftSupport = builder.draftSupport(query); String colAlias = subQuery || rootNode.isSingleProperty() ? null : columnAliasPrefix; - this.ctx = new DefaultDbSqlContext(alias, colAlias, historySupport, draftSupport, fromForUpdate, predicates.dbFilterManyJoin()); + this.ctx = new DefaultDbSqlContext(alias, colAlias, historySupport, draftSupport, fromForUpdate, predicates.dbFilterManyJoin(), predicates.filterManyAttachPath()); } /** diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java index e3f8d6327e..fbecfcbe47 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java @@ -342,6 +342,10 @@ SqlJoinType appendFromBaseTable(DbSqlContext ctx, SqlJoinType joinType) { if (desc.isSoftDelete() && temporalMode != SpiQuery.TemporalMode.SOFT_DELETED) { ctx.append(" and ").append(desc.softDeletePredicate(ctx.tableAlias(prefix))); } + if (prefix != null && ctx.isFilterManyAttachPoint(prefix)) { + // this node is where we inline the filterMany predicate + ctx.includeFilterMany(); + } return sqlJoinType; } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyRoot.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyRoot.java index ec34d53953..ebfa9dd3f0 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyRoot.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyRoot.java @@ -45,6 +45,5 @@ protected void appendExtraWhere(DbSqlContext ctx) { @Override public void appendFrom(DbSqlContext ctx, SqlJoinType joinType) { super.appendFrom(ctx, joinType.autoToOuter()); - ctx.includeFilterMany(); } } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/querydefn/OrmQueryDetail.java b/ebean-core/src/main/java/io/ebeaninternal/server/querydefn/OrmQueryDetail.java index 7a46083190..9fa2411b0a 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/querydefn/OrmQueryDetail.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/querydefn/OrmQueryDetail.java @@ -384,14 +384,18 @@ SpiQueryManyJoin markQueryJoins(BeanDescriptor beanDescriptor, String lazyLoa OrmQueryProperties chunk = pair.getProperties(); if (isQueryJoinCandidate(lazyLoadManyPath, chunk)) { // this is a 'fetch join' (included in main query) - if (fetchJoinFirstMany) { + BeanDescriptor targetDescriptor = ((BeanPropertyAssoc) elProp.beanProperty()).targetDescriptor(); + if (fetchJoinFirstMany && !chunk.filterManyHasNestedProperty(targetDescriptor)) { // letting the first one remain a 'fetch join' fetchJoinFirstMany = false; manyFetchProperty = pair.getPath(); chunk.filterManyInline(); many = elProp; } else { - // convert this one over to a 'query join' + // convert this one over to a 'query join' - either because another many has already claimed the + // 'fetch join' slot, or because its filterMany references a property that requires crossing into + // an associated bean and can't safely be included as a JOIN predicate (see + // OrmQueryProperties.filterManyHasNestedProperty) chunk.markForQueryJoin(); } } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/querydefn/OrmQueryProperties.java b/ebean-core/src/main/java/io/ebeaninternal/server/querydefn/OrmQueryProperties.java index 1da618babd..97003ae38f 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/querydefn/OrmQueryProperties.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/querydefn/OrmQueryProperties.java @@ -9,7 +9,10 @@ import io.ebeaninternal.api.SpiExpression; import io.ebeaninternal.api.SpiExpressionFactory; import io.ebeaninternal.api.SpiExpressionList; +import io.ebeaninternal.api.SpiExpressionValidation; import io.ebeaninternal.api.SpiQuery; +import io.ebeaninternal.server.deploy.BeanDescriptor; +import io.ebeaninternal.server.el.ElPropertyValue; import io.ebeaninternal.server.expression.FilterExprPath; import io.ebeaninternal.server.expression.FilterExpressionList; @@ -234,6 +237,26 @@ public boolean isFilterManyJoin() { return filterMany != null && !markForQueryJoin; } + /** + * Return true if the filterMany expression (if any) references a property that requires + * crossing into an associated bean/join - e.g. {@code "group.name"} - rather than only + * plain/embedded properties resolving to columns on the many bean's own base table. + */ + boolean filterManyHasNestedProperty(BeanDescriptor targetDescriptor) { + if (filterMany == null) { + return false; + } + SpiExpressionValidation validation = new SpiExpressionValidation(targetDescriptor); + filterMany.validate(validation); + for (String property : validation.allProperties()) { + ElPropertyValue elProp = targetDescriptor.elGetValue(property); + if (elProp != null && elProp.isAssocProperty()) { + return true; + } + } + return false; + } + /** * Adjust filterMany expressions for inclusion in main query. */ diff --git a/ebean-test/src/test/java/org/tests/query/TestQueryFilterMany.java b/ebean-test/src/test/java/org/tests/query/TestQueryFilterMany.java index c0d75d099b..0b6667f6b7 100644 --- a/ebean-test/src/test/java/org/tests/query/TestQueryFilterMany.java +++ b/ebean-test/src/test/java/org/tests/query/TestQueryFilterMany.java @@ -174,7 +174,7 @@ public void filterManyRaw_singleQuery() { assertThat(customers).isNotEmpty(); List sqlList = LoggedSql.stop(); assertEquals(1, sqlList.size()); - assertThat(sqlList.get(0)).contains(" left join o_customer t2 on t2.id = t1.kcustomer_id and t1.status = ? where "); + assertThat(sqlList.get(0)).contains(" left join o_order t1 on t1.kcustomer_id = t0.id and t1.order_date is not null and t1.status = ? left join o_customer t2 on t2.id = t1.kcustomer_id where "); assertThat(sqlList.get(0)).contains(" where lower(t0.name) = ? order by t0.id"); } @@ -211,7 +211,7 @@ public void test_with_findOne_rawSameQuery() { assertThat(result).isNotEmpty(); List sql = LoggedSql.stop(); assertThat(sql).hasSize(1); - assertThat(sql.get(0)).contains("from o_customer t0 left join o_order t1 on t1.kcustomer_id = t0.id and t1.order_date is not null left join o_customer t2 on t2.id = t1.kcustomer_id and t1.order_date is not null order by t0.id"); + assertThat(sql.get(0)).contains("from o_customer t0 left join o_order t1 on t1.kcustomer_id = t0.id and t1.order_date is not null and t1.order_date is not null left join o_customer t2 on t2.id = t1.kcustomer_id order by t0.id"); } @Test @@ -340,9 +340,9 @@ public void test_filterMany_copy_findList() { List sqlList = LoggedSql.stop(); assertEquals(1, sqlList.size()); if (isPostgresCompatible()) { - assertThat(sqlList.get(0)).contains("left join o_customer t2 on t2.id = t1.kcustomer_id and t1.status = any(?) order by t0.id"); + assertThat(sqlList.get(0)).contains("left join o_order t1 on t1.kcustomer_id = t0.id and t1.order_date is not null and t1.status = any(?) left join o_customer t2 on t2.id = t1.kcustomer_id order by t0.id"); } else { - assertThat(sqlList.get(0)).contains("left join o_customer t2 on t2.id = t1.kcustomer_id and t1.status in (?) order by t0.id"); + assertThat(sqlList.get(0)).contains("left join o_order t1 on t1.kcustomer_id = t0.id and t1.order_date is not null and t1.status in (?) left join o_customer t2 on t2.id = t1.kcustomer_id order by t0.id"); } } @@ -386,7 +386,7 @@ public void testDisjunction() { List sql = LoggedSql.stop(); assertEquals(1, sql.size()); - assertSql(sql.get(0)).contains(" left join o_customer t2 on t2.id = t1.kcustomer_id and (t1.status = ? or t1.order_date = ?) order by t0.id"); + assertSql(sql.get(0)).contains(" left join o_order t1 on t1.kcustomer_id = t0.id and t1.order_date is not null and (t1.status = ? or t1.order_date = ?) left join o_customer t2 on t2.id = t1.kcustomer_id order by t0.id"); } @Test @@ -459,7 +459,10 @@ void testFilterManyWithNestedPathExpression() { List sql = LoggedSql.stop(); - assertThat(sql).hasSize(1); - assertSql(sql.get(0)).contains(" from o_customer t0 left join contact t1 on t1.customer_id = t0.id left join contact_group t2 on t2.id = t1.group_id and t2.name = ? and t1.cretime is not null order by t0.id"); + // nested "group.name" reference forces this to a query join so the filter is applied + // as a genuine WHERE clause (not misapplied to a LEFT JOIN's ON clause) + assertThat(sql).hasSize(2); + assertSql(sql.get(1)).contains(" from contact t0 left join contact_group t1 on t1.id = t0.group_id where (t0.customer_id) in ("); + assertSql(sql.get(1)).contains(" and t1.name = ? and t0.cretime is not null"); } } diff --git a/ebean-test/src/test/java/org/tests/query/TestQueryFilterManySimple.java b/ebean-test/src/test/java/org/tests/query/TestQueryFilterManySimple.java index 47103dbcc1..e5be0a7f42 100644 --- a/ebean-test/src/test/java/org/tests/query/TestQueryFilterManySimple.java +++ b/ebean-test/src/test/java/org/tests/query/TestQueryFilterManySimple.java @@ -35,7 +35,7 @@ public void test() { list.get(0).getOrders().size(); List sql = LoggedSql.stop(); assertThat(sql).hasSize(2); - assertThat(sql.get(0)).contains("left join o_order t1 on t1.kcustomer_id = t0.id and t1.order_date is not null left join o_customer t2 on t2.id = t1.kcustomer_id and t1.status = ? and t1.order_date > ?"); + assertThat(sql.get(0)).contains("left join o_order t1 on t1.kcustomer_id = t0.id and t1.order_date is not null and t1.status = ? and t1.order_date > ? left join o_customer t2 on t2.id = t1.kcustomer_id"); assertThat(sql.get(0)).contains("order by t0.id"); if (isPostgresCompatible()) { assertThat(sql.get(1)).contains("from contact t0 where (t0.customer_id) = any(?) and t0.first_name is not null;"); @@ -55,7 +55,10 @@ void testNestedFilterMany() { .findList(); List sql = LoggedSql.stop(); - assertThat(sql).hasSize(1); - assertThat(sql.get(0)).contains("from o_customer t0 left join o_order t1 on t1.kcustomer_id = t0.id and t1.order_date is not null left join o_customer t2 on t2.id = t1.kcustomer_id and t2.status = ? order by t0.id"); + // nested "customer.status" reference forces this to a query join so the filter is + // applied as a genuine WHERE clause (not misapplied to a LEFT JOIN's ON clause) + assertThat(sql).hasSize(2); + assertThat(sql.get(1)).contains("from o_order t0 join o_customer t1 on t1.id = t0.kcustomer_id where t0.order_date is not null and (t0.kcustomer_id) in ("); + assertThat(sql.get(1)).contains(" and t1.status = ?"); } } diff --git a/ebean-test/src/test/java/org/tests/query/TestQueryFilterManyWithDeeperNestedFetch.java b/ebean-test/src/test/java/org/tests/query/TestQueryFilterManyWithDeeperNestedFetch.java new file mode 100644 index 0000000000..875ec3263b --- /dev/null +++ b/ebean-test/src/test/java/org/tests/query/TestQueryFilterManyWithDeeperNestedFetch.java @@ -0,0 +1,73 @@ +package org.tests.query; + +import io.ebean.DB; +import io.ebean.test.LoggedSql; +import io.ebean.xtest.BaseTestCase; +import org.junit.jupiter.api.Test; +import org.tests.model.basic.Contact; +import org.tests.model.basic.Customer; +import org.tests.model.basic.ResetBasicData; + +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Reproduces a bug where filterMany() on a property of the many-root itself is misapplied + * to the ON clause of a deeper, unrelated nested fetch join instead of the many-root's own join + * clause - when an additional fetch path exists beneath the many property that the filterMany + * expression itself does not reference. + */ +public class TestQueryFilterManyWithDeeperNestedFetch extends BaseTestCase { + + @Test + void filterMany_onRootProperty_withUnrelatedDeeperNestedFetch_expectFilterOnOwnJoin() { + ResetBasicData.reset(); + + Customer customer = DB.find(Customer.class).where().ieq("name", "Rob").findOne(); + assertThat(customer).isNotNull(); + + List allContacts = DB.find(Contact.class).where().eq("customer", customer).findList(); + assertThat(allContacts).isNotEmpty(); + + // ensure at least one contact isMember=true and one isMember=false + Contact memberContact = allContacts.get(0); + memberContact.setMember(true); + DB.save(memberContact); + Contact nonMemberContact; + if (allContacts.size() > 1) { + nonMemberContact = allContacts.get(1); + } else { + nonMemberContact = new Contact(); + nonMemberContact.setFirstName("Extra"); + nonMemberContact.setLastName("NonMember"); + nonMemberContact.setCustomer(customer); + } + nonMemberContact.setMember(false); + DB.save(nonMemberContact); + + LoggedSql.start(); + // filterMany only references "isMember" (a property of Contact - the many-root itself) but + // the query ALSO fetches a further nested path beneath "contacts" (contacts.group) that the + // filterMany expression does NOT reference at all. + List found = DB.find(Customer.class) + .setBeanCacheMode(io.ebean.CacheMode.OFF) + .setPersistenceContextScope(io.ebean.PersistenceContextScope.QUERY) + .fetch("contacts", "id,firstName,lastName,isMember") + .fetch("contacts.group", "id,name") + .filterMany("contacts").eq("isMember", true) + .where().idEq(customer.getId()) + .findList(); + + List sql = LoggedSql.stop(); + assertThat(found).isNotEmpty(); + assertThat(sql).hasSize(1); + // the filterMany predicate must be on contact's own join (t1), not misapplied to the + // unrelated deeper contact_group join (t2) + assertThat(sql.get(0)).contains("left join contact t1 on t1.customer_id = t0.id and t1.is_member = ? left join contact_group t2 on t2.id = t1.group_id"); + + // every contact returned must be a member - the filter must actually exclude non-members + assertThat(found.get(0).getContacts()).isNotEmpty(); + assertThat(found.get(0).getContacts()).allMatch(Contact::isMember); + } +}