From 1b2bfc5df86dc0be2b0d094c2fa80e7c39bac28c Mon Sep 17 00:00:00 2001 From: Roland Praml Date: Sat, 10 Nov 2018 01:53:02 +0100 Subject: [PATCH 1/2] FIX: use outer join on "is null" check --- .../server/expression/NullExpression.java | 3 + .../java/org/tests/query/TestOuterJoin.java | 197 ++++++++++++++++++ 2 files changed, 200 insertions(+) create mode 100644 src/test/java/org/tests/query/TestOuterJoin.java diff --git a/src/main/java/io/ebeaninternal/server/expression/NullExpression.java b/src/main/java/io/ebeaninternal/server/expression/NullExpression.java index 547bcb36d..ba81c0bef 100644 --- a/src/main/java/io/ebeaninternal/server/expression/NullExpression.java +++ b/src/main/java/io/ebeaninternal/server/expression/NullExpression.java @@ -40,6 +40,9 @@ class NullExpression extends AbstractExpression { propertyPath = SplitName.split(propName)[0]; propertyContainsMany(propertyPath, desc, manyWhereJoin); } else { + if (elProperty != null && elProperty.containsMany() && !notNull) { + manyWhereJoin.setRequireOuterJoins(true); + } propertyContainsMany(propName, desc, manyWhereJoin); } } diff --git a/src/test/java/org/tests/query/TestOuterJoin.java b/src/test/java/org/tests/query/TestOuterJoin.java new file mode 100644 index 000000000..29fdda09a --- /dev/null +++ b/src/test/java/org/tests/query/TestOuterJoin.java @@ -0,0 +1,197 @@ +package org.tests.query; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.*; + +import java.util.List; + +import org.junit.BeforeClass; +import org.junit.Test; +import org.tests.model.basic.Customer; +import org.tests.model.basic.Order; +import org.tests.model.basic.ResetBasicData; + +import io.ebean.BaseTestCase; +import io.ebean.Ebean; +import io.ebeantest.LoggedSql; + +/** + * Tests if outer joins are correctly used. + * + * @author Roland Praml, FOCONIS AG + * + */ +public class TestOuterJoin extends BaseTestCase { + + @BeforeClass + public static void setup() { + ResetBasicData.reset(); + } + + + @Test + public void testOuterOnNullQuery() throws Exception { + + LoggedSql.start(); + List list = Ebean.find(Customer.class).where() + .isNull("orders") + .le("id", 4) // ignore others than basicData + .findList(); + + assertThat(LoggedSql.collect().get(0)) + .contains(" where not exists "); // perfored with not exists query + assertThat(list).hasSize(2); + + // use OR construct + LoggedSql.start(); + list = Ebean.find(Customer.class).where() + .or() + .isNull("orders.details.product.name") + .isNull("orders") + .endOr() + .le("id", 4) // ignore others than basicData + .findList(); + + assertThat(LoggedSql.collect().get(0)) + .contains(" left join o_order ") + .contains(" left join o_order_detail ") + .contains(" left join o_product ") + .contains(" or not exists "); + + assertThat(list).hasSize(4); + + // use AND construct + LoggedSql.start(); + list = Ebean.find(Customer.class).where() + .and() + .isNull("orders.details.product.name") + .isNull("orders") + .endAnd() + .le("id", 4) // ignore others than basicData + .findList(); + + assertThat(LoggedSql.collect().get(0)) + .contains(" left join o_order ") + .contains(" left join o_order_detail ") + .contains(" left join o_product ") + .contains(" and not exists "); + + assertThat(list).hasSize(2); + + LoggedSql.start(); + list = Ebean.find(Customer.class).where() + .isNull("orders.details.product.name") + .le("id", 4) // ignore others than basicData + .findList(); + + assertThat(LoggedSql.collect().get(0)) + .contains(" left join o_order ") + .contains(" left join o_order_detail ") + .contains(" left join o_product "); + assertThat(list).hasSize(4); + LoggedSql.stop(); + } + + @Test + public void testInnerOnNonNullQuery() throws Exception { + + LoggedSql.start(); + List list = Ebean.find(Customer.class).where() + .isNotNull("orders") + .le("id", 4) // ignore others than basicData + .findList(); + + assertThat(LoggedSql.collect().get(0)) + .contains(" where exists "); // perfored with not exists query + assertThat(list).hasSize(2); + + // use OR construct + LoggedSql.start(); + list = Ebean.find(Customer.class).where() + .or() + .isNotNull("orders.details.product.name") + .isNotNull("orders") + .endOr() + .le("id", 4) // ignore others than basicData + .findList(); + + assertThat(LoggedSql.collect().get(0)) + .contains(" left join o_order ") + .contains(" left join o_order_detail ") + .contains(" left join o_product ") + .contains(" or exists "); + + assertThat(list).hasSize(2); + + // use AND construct + LoggedSql.start(); + list = Ebean.find(Customer.class).where() + .and() + .isNotNull("orders.details.product.name") + .isNotNull("orders") + .endAnd() + .le("id", 4) // ignore others than basicData + .findList(); + + assertThat(LoggedSql.collect().get(0)) + .doesNotContain(" left join ") + .contains(" and exists "); + + assertThat(list).hasSize(2); + + + LoggedSql.start(); + list = Ebean.find(Customer.class).where() + .isNotNull("orders.details.product.name") + .le("id", 4) // ignore others than basicData + .findList(); + + assertThat(LoggedSql.collect().get(0)) + .doesNotContain(" left join "); + assertThat(list).hasSize(2); + LoggedSql.stop(); + } + + @Test + public void testOuterOnFetch() throws Exception { + LoggedSql.start(); + + List orders1 = Ebean.find(Order.class).findList(); + + assertThat(LoggedSql.collect().get(0)) + .contains(" join o_customer") // ensure that we do not left join the customer + .doesNotContain(" left join o_customer"); + + + // now fetch "details" bean, which may be optional + LoggedSql.start(); + + List orders2 = Ebean.find(Order.class) + .fetch("details", "id").findList(); + + assertThat(LoggedSql.collect().get(0)) + .contains(" left join o_order_detail "); + + LoggedSql.stop(); + + // Note: You would expect, that the two lists are equal. + // but there is a @Where(clause = "${ta}.id > 0") on the details + // property, that will filter orders, where details is zero. + + // The produced SQL is: + // select * from o_order t0 + // join o_customer t2 on t2.id = t0.kcustomer_id + // left join o_order_detail t1 on t1.order_id = t0.id + // where t1.id > 0 + // order by t0.id, t1.id asc, t1.order_qty asc, t1.cretime desc; --bind() + // + // I think this is NOT correct as it may exclude orders without details or with negative ID + // + // Correct way could/would be, to add that extra where to the join + // + // + + // assertThat(orders2).isEqualTo(orders1); + } + +} From c5536aaeecc0c249c79a374cbf9949d8f4ab9619 Mon Sep 17 00:00:00 2001 From: Roland Praml Date: Sat, 10 Nov 2018 01:56:28 +0100 Subject: [PATCH 2/2] FIX: append extraWhere to join --- .../server/query/SqlTreeNodeBean.java | 19 ++++++++++++------- .../server/query/SqlTreeNodeManyRoot.java | 16 ++++++++++++++++ .../tests/model/view/TestViewBaseEntity.java | 2 +- .../java/org/tests/query/TestOuterJoin.java | 19 +------------------ 4 files changed, 30 insertions(+), 26 deletions(-) diff --git a/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java b/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java index c67d58600..9324016a0 100644 --- a/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java +++ b/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeBean.java @@ -510,6 +510,17 @@ class SqlTreeNodeBean implements SqlTreeNode { ctx.append(inheritInfo.getWhere()).append(" "); } } + + appendExtraWhere(ctx); + + for (SqlTreeNode aChildren : children) { + // recursively add to the where clause any + // fixed predicates (extraWhere etc) + aChildren.appendWhere(ctx); + } + } + + protected void appendExtraWhere(DbSqlContext ctx) { if (extraWhere != null) { if (ctx.length() > 0) { ctx.append(" and"); @@ -518,12 +529,6 @@ class SqlTreeNodeBean implements SqlTreeNode { String ew = StringHelper.replaceString(extraWhere, "${ta}", ta); ctx.append(" ").append(ew).append(" "); } - - for (SqlTreeNode aChildren : children) { - // recursively add to the where clause any - // fixed predicates (extraWhere etc) - aChildren.appendWhere(ctx); - } } /** @@ -604,7 +609,7 @@ class SqlTreeNodeBean implements SqlTreeNode { return sqlJoinType; } - private SqlJoinType appendFromAsJoin(DbSqlContext ctx, SqlJoinType joinType) { + protected SqlJoinType appendFromAsJoin(DbSqlContext ctx, SqlJoinType joinType) { if (nodeBeanProp instanceof STreePropertyAssocMany) { STreePropertyAssocMany manyProp = (STreePropertyAssocMany) nodeBeanProp; diff --git a/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyRoot.java b/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyRoot.java index edeff9fce..6b050f51a 100644 --- a/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyRoot.java +++ b/src/main/java/io/ebeaninternal/server/query/SqlTreeNodeManyRoot.java @@ -41,6 +41,22 @@ final class SqlTreeNodeManyRoot extends SqlTreeNodeBean { return detailBean; } + + /** + * append extraWhere to the join. + */ + @Override + protected SqlJoinType appendFromAsJoin(DbSqlContext ctx, SqlJoinType joinType) { + SqlJoinType join = super.appendFromAsJoin(ctx, joinType); + super.appendExtraWhere(ctx); + return join; + } + + @Override + protected void appendExtraWhere(DbSqlContext ctx) { + // extraWhere is already appended to the tableJoin + } + /** * Force outer join for everything after the many property. */ diff --git a/src/test/java/org/tests/model/view/TestViewBaseEntity.java b/src/test/java/org/tests/model/view/TestViewBaseEntity.java index e6da2b525..63c94a244 100644 --- a/src/test/java/org/tests/model/view/TestViewBaseEntity.java +++ b/src/test/java/org/tests/model/view/TestViewBaseEntity.java @@ -65,6 +65,6 @@ public class TestViewBaseEntity extends BaseTestCase { assertThat(details).isNotEmpty(); } - assertThat(query.getGeneratedSql()).contains("from order_agg_vw t0 left join o_order t1 on t1.id = t0.order_id left join o_customer t3 on t3.id = t1.kcustomer_id left join o_order_detail t2 on t2.order_id = t1.id where t2.id > 0 and t0.order_total > ?"); + assertThat(query.getGeneratedSql()).contains("from order_agg_vw t0 left join o_order t1 on t1.id = t0.order_id left join o_customer t3 on t3.id = t1.kcustomer_id left join o_order_detail t2 on t2.order_id = t1.id and t2.id > 0 where t0.order_total > ?"); } } diff --git a/src/test/java/org/tests/query/TestOuterJoin.java b/src/test/java/org/tests/query/TestOuterJoin.java index 29fdda09a..b2f8e6ae1 100644 --- a/src/test/java/org/tests/query/TestOuterJoin.java +++ b/src/test/java/org/tests/query/TestOuterJoin.java @@ -174,24 +174,7 @@ public class TestOuterJoin extends BaseTestCase { LoggedSql.stop(); - // Note: You would expect, that the two lists are equal. - // but there is a @Where(clause = "${ta}.id > 0") on the details - // property, that will filter orders, where details is zero. - - // The produced SQL is: - // select * from o_order t0 - // join o_customer t2 on t2.id = t0.kcustomer_id - // left join o_order_detail t1 on t1.order_id = t0.id - // where t1.id > 0 - // order by t0.id, t1.id asc, t1.order_qty asc, t1.cretime desc; --bind() - // - // I think this is NOT correct as it may exclude orders without details or with negative ID - // - // Correct way could/would be, to add that extra where to the join - // - // - - // assertThat(orders2).isEqualTo(orders1); + assertThat(orders2).isEqualTo(orders1); } }