Bug: filterMany() expressions when additional nested fetch has predicates in wrong place

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<String> unknown = new LinkedHashSet<>();
+  private final LinkedHashSet<String> 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<String> 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<String> allProperties() {
+    return all;
+  }
+
 }

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);
 }

ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryPredicates.java

@@ -78,6 +78,11 @@ public final class CQueryPredicates {
    */
   private Set<String> predicateIncludes;
   private Set<String> 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.
    */

ebean-core/src/main/java/io/ebeaninternal/server/query/DefaultDbSqlContext.java

@@ -23,6 +23,7 @@ final class DefaultDbSqlContext implements DbSqlContext {
   private final ArrayStack<String> 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

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());
   }
 
   /**

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;
   }
 

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();
   }
 }

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();
           }
         }

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.
    */

ebean-test/src/test/java/org/tests/query/TestQueryFilterMany.java

@@ -174,7 +174,7 @@ public void filterManyRaw_singleQuery() {
     assertThat(customers).isNotEmpty();
     List<String> 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<String> 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<String> 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<String> 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<String> 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");
   }
 }

ebean-test/src/test/java/org/tests/query/TestQueryFilterManySimple.java

@@ -35,7 +35,7 @@ public void test() {
     list.get(0).getOrders().size();
     List<String> 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<String> 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 = ?");
   }
 }

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<Contact> 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<Customer> 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<String> 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);
+  }
+}