Compare commits

...
Author SHA1 Message Date
robin.bygrave 99c3d8b07b Fix OrmQueryProperties.filterManyHasNestedProperty() with real nested property check 2026-07-15 23:08:51 +12:00
robin.bygrave 9947a41a9b Tidy up comments 2026-07-15 22:54:30 +12:00
robin.bygrave 055a3bf638 Simplify SpiExpressionValidation with boolean nestedProperty flag 2026-07-15 22:47:32 +12:00
robin.bygrave f6ff485041 Bug: filterMany() with nested-property predicates silently ineffective
filterMany() expressions referencing a nested/dotted property (e.g.
.eq("group.name", "x"), alone or mixed with root properties) were
attached to a LEFT JOIN's ON clause via the "extra join" mechanism.

That only nulls the extra join's own columns on non-match - it does
not exclude the parent/many-side row, so the predicate had no actual
filtering effect (confirmed by data, not just SQL shape).

## Fix:

force any many-property fetch whose filterMany expression
references a nested property onto a query-join (secondary select
restoring filterMany's original always-fetchQuery behaviour for this
with a genuine WHERE clause) instead of leaving it as an inline JOIN, case.

- SpiExpressionValidation: track all visited property names (allProperties()), not just unknown ones.
- OrmQueryProperties: filterManyHasNestedProperty() walks the filterMany expression for any dotted property reference.
- OrmQueryDetail.markQueryJoins(): route nested-property filterMany chunks to markForQueryJoin() instead of the inline fetch-join slot, without consuming that slot for other many-paths.

Also simplifies the earlier root-only-predicate join-placement fix
(CQueryPredicates/DefaultDbSqlContext/SqlTreeNodeManyRoot) now that
a nested-property filterMany can never reach the JOIN-based code
path: removed the now-unreachable "deepest path" fallback branch,
the includeFilterMany() idempotency guard, and the redundant fallback
call in SqlTreeNodeManyRoot - isFilterManyAttachPoint() remains as
the single, always-used attach mechanism.

Updated TestQueryFilterMany/TestQueryFilterManySimple assertions to
expect the corrected query-join (2 statement) behaviour.
2026-07-15 22:42:25 +12:00
robin.bygrave b67e28e60a Bug: filterMany() expressions when additional nested fetch has predicates in wrong place
When a filterMany() expression only references the many-root's own properties (e.g. .eq("status", ...)),
but the query also has an additional nested fetch beneath that many-path (e.g. .fetch("orders.customer")
or .fetch("contacts.group")), the filter predicate was incorrectly attached to the last/deepest join's
ON clause instead of the many-root's own join — silently misapplying the filter.

Fix: A hybrid approach:

- If the filterMany expression references only root-level properties → attach the predicate directly on the many-root's own join (correct).

 - If it references deeper/nested properties requiring "extra joins" (a separate existing mechanism) → fall back to the original end-of-subtree behavior, since no SqlTreeNode exists to attach to.

 - includeFilterMany() made idempotent so both mechanisms coexist safely.
2026-07-15 20:48:28 +12:00
12 changed files with 166 additions and 15 deletions
@@ -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 final class SpiExpressionValidation {
* 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 final class SpiExpressionValidation {
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;
}
}
@@ -181,4 +181,11 @@ public interface DbSqlContext {
* 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);
}
@@ -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 final class CQueryPredicates {
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 @@ public final class CQueryPredicates {
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.
*/
@@ -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
@@ -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());
}
/**
@@ -342,6 +342,10 @@ class SqlTreeNodeBean implements SqlTreeNode {
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;
}
@@ -45,6 +45,5 @@ final class SqlTreeNodeManyRoot extends SqlTreeNodeBean {
@Override
public void appendFrom(DbSqlContext ctx, SqlJoinType joinType) {
super.appendFrom(ctx, joinType.autoToOuter());
ctx.includeFilterMany();
}
}
@@ -384,14 +384,18 @@ public final class OrmQueryDetail implements Serializable {
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();
}
}
@@ -9,7 +9,10 @@ import io.ebean.util.SplitName;
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 final class OrmQueryProperties implements Serializable {
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.
*/
@@ -174,7 +174,7 @@ public class TestQueryFilterMany extends BaseTestCase {
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 class TestQueryFilterMany extends BaseTestCase {
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 class TestQueryFilterMany extends BaseTestCase {
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 class TestQueryFilterMany extends BaseTestCase {
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 @@ public class TestQueryFilterMany extends BaseTestCase {
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");
}
}
@@ -35,7 +35,7 @@ public class TestQueryFilterManySimple extends BaseTestCase {
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 @@ public class TestQueryFilterManySimple extends BaseTestCase {
.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 = ?");
}
}
@@ -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);
}
}