From 267b1b730317048214a7ae6910a14d07d685b1a5 Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Mon, 14 Mar 2022 15:37:12 +1300 Subject: [PATCH] #2580 - Fix for Wrong join in query with @Inheritance Effectively delays an extra join added via BeanPropertyAssocOne with inheritance, such that this only occurs if a SqlTreeNode child doesn't add the join. --- .../server/deploy/AssocOneHelpRefInherit.java | 22 ++++- .../server/deploy/DbSqlContext.java | 10 +++ .../server/query/DefaultDbSqlContext.java | 23 +++++- .../server/query/SqlTreeBuilder.java | 2 +- .../server/query/SqlTreeJoin.java | 15 +++- .../server/query/SqlTreeNodeBean.java | 4 +- .../TestMTOInheritNoDiscriminator.java | 6 +- .../org/tests/model/join/AccountAccess.java | 4 +- .../org/tests/model/join/BankAccount.java | 6 +- .../org/tests/model/join/CustomerAccess.java | 4 +- .../java/org/tests/model/join/HAccess.java | 22 +++-- .../java/org/tests/model/join/HAccount.java | 12 ++- .../java/org/tests/model/join/HCustomer.java | 4 +- .../query/TestQueryMultiJoinFetchPath.java | 81 ++++++++----------- 14 files changed, 130 insertions(+), 85 deletions(-) diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/deploy/AssocOneHelpRefInherit.java b/ebean-core/src/main/java/io/ebeaninternal/server/deploy/AssocOneHelpRefInherit.java index b797c5a0c..72fb828c7 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/deploy/AssocOneHelpRefInherit.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/deploy/AssocOneHelpRefInherit.java @@ -3,6 +3,7 @@ package io.ebeaninternal.server.deploy; import io.ebean.bean.EntityBean; import io.ebean.bean.PersistenceContext; import io.ebeaninternal.server.query.SqlJoinType; +import io.ebeaninternal.server.query.SqlTreeJoin; import java.sql.SQLException; @@ -60,11 +61,28 @@ final class AssocOneHelpRefInherit extends AssocOneHelp { return ref; } - @Override void appendFrom(DbSqlContext ctx, SqlJoinType joinType) { // add join to support the discriminator column String relativePrefix = ctx.getRelativePrefix(property.name); - property.tableJoin.addJoin(joinType, relativePrefix, ctx); + ctx.addExtraJoin(new Extra(relativePrefix, joinType)); + } + + /** + * Extra join to support the discriminator column. + */ + final class Extra implements SqlTreeJoin { + final String relativePrefix; + final SqlJoinType joinType; + Extra(String relativePrefix, SqlJoinType joinType) { + this.relativePrefix = relativePrefix; + this.joinType = joinType; + } + + @Override + public void addJoin(DbSqlContext ctx) { + // add join to support the discriminator column *IF* join is not already present + property.tableJoin.addJoin(joinType, relativePrefix, ctx); + } } /** 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 501a8550f..1fccc46f3 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 @@ -1,6 +1,7 @@ package io.ebeaninternal.server.deploy; import io.ebeaninternal.server.query.SqlJoinType; +import io.ebeaninternal.server.query.SqlTreeJoin; /** * Used to provide context during sql construction. @@ -133,4 +134,13 @@ public interface DbSqlContext { */ void appendFromForUpdate(); + /** + * Delay adding an extra join to support inheritance discriminator in projection (IF required). + */ + void addExtraJoin(SqlTreeJoin treeJoin); + + /** + * Add extra joins *IF* required to support inheritance discriminator in projection. + */ + void flushExtraJoins(); } 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 036cda797..4e45c564c 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 @@ -7,6 +7,7 @@ import io.ebeaninternal.server.util.ArrayStack; import java.util.ArrayList; import java.util.HashSet; +import java.util.List; final class DefaultDbSqlContext implements DbSqlContext { @@ -43,7 +44,9 @@ final class DefaultDbSqlContext implements DbSqlContext { private String currentPrefix; - private ArrayList encryptedProps; + private List encryptedProps; + + private List extraJoins; private final CQueryDraftSupport draftSupport; @@ -70,6 +73,24 @@ final class DefaultDbSqlContext implements DbSqlContext { return alias.isIncludeSoftDelete(); } + @Override + public void addExtraJoin(SqlTreeJoin treeJoin) { + if (extraJoins == null) { + extraJoins = new ArrayList<>(); + } + extraJoins.add(treeJoin); + } + + @Override + public void flushExtraJoins() { + if (extraJoins != null) { + for (SqlTreeJoin extra : extraJoins) { + extra.addJoin(this); + } + extraJoins = null; + } + } + @Override public void appendFromForUpdate() { if (fromForUpdate != null) { 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 ed687aff8..0e4f11079 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 @@ -256,7 +256,7 @@ public final class SqlTreeBuilder { } addManyWhereJoins(myJoinList); } - extraProps.forEach(props::add); + extraProps.forEach(props::add); //addExtra if (!rawSql && manyWhereJoins.isFormulaWithJoin(prefix)) { for (String property : manyWhereJoins.getFormulaJoinProperties(prefix)) { diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeJoin.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeJoin.java index f7169fe0f..340ee22b7 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeJoin.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeJoin.java @@ -1,2 +1,15 @@ -package io.ebeaninternal.server.query;public interface SqlTreeJoin { +package io.ebeaninternal.server.query; + +import io.ebeaninternal.server.deploy.DbSqlContext; + +/** + * Extra Sql joins conditionally added if required (after children are joined). + */ +public interface SqlTreeJoin { + + /** + * Add the extra join if required. + */ + void addJoin(DbSqlContext ctx); + } 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 d5ec71aa4..1ed4df10d 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 @@ -279,18 +279,16 @@ class SqlTreeNodeBean implements SqlTreeNode { ctx.pushTableAlias(prefix); baseTableAlias = ctx.getTableAlias(prefix); - // join and return SqlJoinType to use for child joins joinType = appendFromBaseTable(ctx, joinType); - for (STreeProperty property : properties) { // usually nothing... except for 1-1 Exported property.appendFrom(ctx, joinType, null); } - for (SqlTreeNode child : children) { child.appendFrom(ctx, joinType); } + ctx.flushExtraJoins(); ctx.popTableAlias(); ctx.popJoin(); } diff --git a/ebean-test/src/test/java/org/tests/model/carwheeltruck/TestMTOInheritNoDiscriminator.java b/ebean-test/src/test/java/org/tests/model/carwheeltruck/TestMTOInheritNoDiscriminator.java index 107089063..9f6052b9a 100644 --- a/ebean-test/src/test/java/org/tests/model/carwheeltruck/TestMTOInheritNoDiscriminator.java +++ b/ebean-test/src/test/java/org/tests/model/carwheeltruck/TestMTOInheritNoDiscriminator.java @@ -9,10 +9,10 @@ import java.util.List; import static org.assertj.core.api.Assertions.assertThat; -public class TestMTOInheritNoDiscriminator extends BaseTestCase { +class TestMTOInheritNoDiscriminator extends BaseTestCase { @Test - public void test() { + void test() { final TTruckHolder holder = setup(); @@ -31,7 +31,7 @@ public class TestMTOInheritNoDiscriminator extends BaseTestCase { assertThat(sql).hasSize(1); if (isH2() || isPostgresCompatible()) { - assertSql(sql.get(0)).contains("select t0.id, t0.name, t0.version, t2.type, t0.truck_plate_no, t0.basic_id, t1.id, t1.some_uid, t1.foo, t1.owner_id from ttruck_holder t0 join tcar t2 on t2.plate_no = t0.truck_plate_no left join ttruck_holder_item t1 on t1.owner_id = t0.id where t0.id = ? order by t0.id"); + assertSql(sql.get(0)).contains("select t0.id, t0.name, t0.version, t2.type, t0.truck_plate_no, t0.basic_id, t1.id, t1.some_uid, t1.foo, t1.owner_id from ttruck_holder t0 left join ttruck_holder_item t1 on t1.owner_id = t0.id join tcar t2 on t2.plate_no = t0.truck_plate_no where t0.id = ? order by t0.id"); } } diff --git a/ebean-test/src/test/java/org/tests/model/join/AccountAccess.java b/ebean-test/src/test/java/org/tests/model/join/AccountAccess.java index bf04209de..052d91156 100644 --- a/ebean-test/src/test/java/org/tests/model/join/AccountAccess.java +++ b/ebean-test/src/test/java/org/tests/model/join/AccountAccess.java @@ -5,6 +5,6 @@ import javax.persistence.Entity; @DiscriminatorValue("A") @Entity -public class AccountAccess extends Access { +public class AccountAccess extends HAccess { -} \ No newline at end of file +} diff --git a/ebean-test/src/test/java/org/tests/model/join/BankAccount.java b/ebean-test/src/test/java/org/tests/model/join/BankAccount.java index 8cb93fb70..491303be7 100644 --- a/ebean-test/src/test/java/org/tests/model/join/BankAccount.java +++ b/ebean-test/src/test/java/org/tests/model/join/BankAccount.java @@ -5,6 +5,6 @@ import javax.persistence.Entity; @DiscriminatorValue("B") @Entity -public class BankAccount extends Account { - -} \ No newline at end of file +public class BankAccount extends HAccount { + +} diff --git a/ebean-test/src/test/java/org/tests/model/join/CustomerAccess.java b/ebean-test/src/test/java/org/tests/model/join/CustomerAccess.java index 5897a8833..c9336de17 100644 --- a/ebean-test/src/test/java/org/tests/model/join/CustomerAccess.java +++ b/ebean-test/src/test/java/org/tests/model/join/CustomerAccess.java @@ -5,6 +5,6 @@ import javax.persistence.Entity; @DiscriminatorValue("C") @Entity -public class CustomerAccess extends Access { +public class CustomerAccess extends HAccess { -} \ No newline at end of file +} diff --git a/ebean-test/src/test/java/org/tests/model/join/HAccess.java b/ebean-test/src/test/java/org/tests/model/join/HAccess.java index ebc14d681..e62484d96 100644 --- a/ebean-test/src/test/java/org/tests/model/join/HAccess.java +++ b/ebean-test/src/test/java/org/tests/model/join/HAccess.java @@ -8,13 +8,11 @@ import javax.persistence.Inheritance; import javax.persistence.JoinColumn; import javax.persistence.ManyToOne; -import org.tests.model.basic.Customer; - import io.ebean.annotation.DbForeignKey; @Inheritance @Entity -public abstract class Access { +public abstract class HAccess { @Id UUID id; @@ -22,17 +20,17 @@ public abstract class Access { @ManyToOne @DbForeignKey(noConstraint = true) @JoinColumn(name = "accessor_id") - ACustomer accessor; + HCustomer accessor; @ManyToOne @DbForeignKey(noConstraint = true) @JoinColumn(name = "principal_id") - ACustomer principal; + HCustomer principal; @ManyToOne @DbForeignKey(noConstraint = true) @JoinColumn(name = "access_account_number") - Account account; + HAccount account; public UUID getId() { return id; @@ -42,27 +40,27 @@ public abstract class Access { this.id = id; } - public ACustomer getAccessor() { + public HCustomer getAccessor() { return accessor; } - public void setAccessor(ACustomer accessor) { + public void setAccessor(HCustomer accessor) { this.accessor = accessor; } - public ACustomer getPrincipal() { + public HCustomer getPrincipal() { return principal; } - public void setPrincipal(ACustomer principal) { + public void setPrincipal(HCustomer principal) { this.principal = principal; } - public Account getAccount() { + public HAccount getAccount() { return account; } - public void setAccount(Account account) { + public void setAccount(HAccount account) { this.account = account; setPrincipal(account.getOwner()); } diff --git a/ebean-test/src/test/java/org/tests/model/join/HAccount.java b/ebean-test/src/test/java/org/tests/model/join/HAccount.java index 6820947bd..2ca896403 100644 --- a/ebean-test/src/test/java/org/tests/model/join/HAccount.java +++ b/ebean-test/src/test/java/org/tests/model/join/HAccount.java @@ -2,18 +2,16 @@ package org.tests.model.join; import javax.persistence.*; -import org.tests.model.basic.Customer; - @Entity @Inheritance -public abstract class Account { +public abstract class HAccount { @Id String accountNumber; @ManyToOne - @JoinColumn(name = "owner_id")//, referencedColumnName = "id") - ACustomer owner; + @JoinColumn(name = "owner_id") + HCustomer owner; public String getAccountNumber() { return accountNumber; @@ -23,11 +21,11 @@ public abstract class Account { this.accountNumber = accountNumber; } - public ACustomer getOwner() { + public HCustomer getOwner() { return owner; } - public void setOwner(ACustomer owner) { + public void setOwner(HCustomer owner) { this.owner = owner; } diff --git a/ebean-test/src/test/java/org/tests/model/join/HCustomer.java b/ebean-test/src/test/java/org/tests/model/join/HCustomer.java index a78aaf773..9a5ba3a9d 100644 --- a/ebean-test/src/test/java/org/tests/model/join/HCustomer.java +++ b/ebean-test/src/test/java/org/tests/model/join/HCustomer.java @@ -4,7 +4,7 @@ import javax.persistence.Entity; import javax.persistence.Id; @Entity -public class ACustomer { +public class HCustomer { @Id final String cid; @@ -13,7 +13,7 @@ public class ACustomer { String status = "A"; - public ACustomer(String cid, String name) { + public HCustomer(String cid, String name) { this.cid = cid; this.name = name; } diff --git a/ebean-test/src/test/java/org/tests/query/TestQueryMultiJoinFetchPath.java b/ebean-test/src/test/java/org/tests/query/TestQueryMultiJoinFetchPath.java index 18f77b155..e8f1a81be 100644 --- a/ebean-test/src/test/java/org/tests/query/TestQueryMultiJoinFetchPath.java +++ b/ebean-test/src/test/java/org/tests/query/TestQueryMultiJoinFetchPath.java @@ -1,80 +1,69 @@ package org.tests.query; -import static org.assertj.core.api.Assertions.assertThat; - -import java.util.List; - -import org.junit.jupiter.api.Test; -import org.tests.model.basic.Customer; -import org.tests.model.basic.Customer.Status; -import org.tests.model.basic.ResetBasicData; -import org.tests.model.join.Access; -import org.tests.model.join.Account; -import org.tests.model.join.AccountAccess; -import org.tests.model.join.BankAccount; -import org.tests.model.join.CustomerAccess; - import io.ebean.BaseTestCase; import io.ebean.DB; import io.ebean.Query; +import org.junit.jupiter.api.Test; +import org.tests.model.join.*; -public class TestQueryMultiJoinFetchPath extends BaseTestCase { +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +class TestQueryMultiJoinFetchPath extends BaseTestCase { @Test - public void test() { + void test() { - ResetBasicData.reset(); - - Customer c1 = new Customer(); - c1.setName("c1"); - c1.setStatus(Status.ACTIVE); + HCustomer c1 = new HCustomer("c1", "c1"); DB.save(c1); - - Customer c2 = new Customer(); - c2.setName("c2"); - c2.setStatus(Status.ACTIVE); + + HCustomer c2 = new HCustomer("c2", "c2"); DB.save(c2); - - Customer c3 = new Customer(); - c3.setName("c3"); - c3.setStatus(Status.ACTIVE); + + HCustomer c3 = new HCustomer("c3", "c3"); DB.save(c3); - - Account a1 = new BankAccount(); + + HAccount a1 = new BankAccount(); a1.setAccountNumber("a1"); a1.setOwner(c1); DB.save(a1); - + CustomerAccess ca = new CustomerAccess(); ca.setAccessor(c3); ca.setPrincipal(c1); DB.save(ca); - + AccountAccess aa = new AccountAccess(); aa.setAccessor(c2); aa.setAccount(a1); DB.save(aa); - - List ids = DB.find(Access.class) + + List ids = DB.find(HAccess.class) .where() - .eq("principal.status", Status.ACTIVE) - .eq("accessor.status", Status.ACTIVE) + .eq("principal.status", "A") + .eq("accessor.status", "A") .findIds(); - + assertThat(ids).hasSize(2); - - Query query = DB.find(Access.class) + + Query query = DB.find(HAccess.class) .fetch("account","accountNumber") .fetch("accessor","name") .where() - .eq("accessor.status", Status.ACTIVE) - .eq("principal.status", Status.ACTIVE) + .eq("accessor.status", "A") + .eq("principal.status", "A") .idIn(ids) .query(); - - List accesses = query.findList(); - + + List accesses = query.findList(); + assertThat(accesses).hasSize(2); + if (isH2()) { + assertThat(query.getGeneratedSql()).isEqualTo("select t0.dtype, t0.id, t0.accessor_id, t0.principal_id, t2.dtype, t0.access_account_number, t0.accessor_id, t2.dtype, t0.access_account_number, t1.cid, t1.name, t2.dtype, t2.account_number from haccess t0 left join hcustomer t1 on t1.cid = t0.accessor_id left join haccount t2 on t2.account_number = t0.access_account_number and t2.dtype = 'B' left join hcustomer t3 on t3.cid = t0.principal_id where t1.status = ? and t3.status = ? and t0.id in (?,?,?,?,?)"); + } else { + assertThat(query.getGeneratedSql()).contains("select t0.dtype, t0.id, t0.accessor_id, t0.principal_id, t2.dtype, t0.access_account_number, t0.accessor_id, t2.dtype, t0.access_account_number, t1.cid, t1.name, t2.dtype, t2.account_number from haccess t0 left join hcustomer t1 on t1.cid = t0.accessor_id left join haccount t2 on t2.account_number = t0.access_account_number and t2.dtype = 'B' left join hcustomer t3 on t3.cid = t0.principal_id where t1.status = ? and t3.status = ? and t0.id "); + } } - + }