From f2747437366a042ef4d2cf6f187929612358cde3 Mon Sep 17 00:00:00 2001 From: Noemi Szemenyei Date: Tue, 15 Mar 2022 15:37:43 +0100 Subject: [PATCH] Bug: Wrong join in query with @Inheritance and @DbForeignKey --- .../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 | 10 +++ .../org/tests/model/join/BankAccount.java | 10 +++ .../org/tests/model/join/CustomerAccess.java | 10 +++ .../java/org/tests/model/join/HAccess.java | 68 ++++++++++++++++++ .../java/org/tests/model/join/HAccount.java | 32 +++++++++ .../java/org/tests/model/join/HCustomer.java | 20 ++++++ .../query/TestQueryMultiJoinFetchPath.java | 69 +++++++++++++++++++ 14 files changed, 291 insertions(+), 10 deletions(-) create mode 100644 ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeJoin.java create mode 100644 ebean-test/src/test/java/org/tests/model/join/AccountAccess.java create mode 100644 ebean-test/src/test/java/org/tests/model/join/BankAccount.java create mode 100644 ebean-test/src/test/java/org/tests/model/join/CustomerAccess.java create mode 100644 ebean-test/src/test/java/org/tests/model/join/HAccess.java create mode 100644 ebean-test/src/test/java/org/tests/model/join/HAccount.java create mode 100644 ebean-test/src/test/java/org/tests/model/join/HCustomer.java create mode 100644 ebean-test/src/test/java/org/tests/query/TestQueryMultiJoinFetchPath.java 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 new file mode 100644 index 000000000..340ee22b7 --- /dev/null +++ b/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeJoin.java @@ -0,0 +1,15 @@ +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 new file mode 100644 index 000000000..052d91156 --- /dev/null +++ b/ebean-test/src/test/java/org/tests/model/join/AccountAccess.java @@ -0,0 +1,10 @@ +package org.tests.model.join; + +import javax.persistence.DiscriminatorValue; +import javax.persistence.Entity; + +@DiscriminatorValue("A") +@Entity +public class AccountAccess extends HAccess { + +} 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 new file mode 100644 index 000000000..491303be7 --- /dev/null +++ b/ebean-test/src/test/java/org/tests/model/join/BankAccount.java @@ -0,0 +1,10 @@ +package org.tests.model.join; + +import javax.persistence.DiscriminatorValue; +import javax.persistence.Entity; + +@DiscriminatorValue("B") +@Entity +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 new file mode 100644 index 000000000..c9336de17 --- /dev/null +++ b/ebean-test/src/test/java/org/tests/model/join/CustomerAccess.java @@ -0,0 +1,10 @@ +package org.tests.model.join; + +import javax.persistence.DiscriminatorValue; +import javax.persistence.Entity; + +@DiscriminatorValue("C") +@Entity +public class CustomerAccess extends HAccess { + +} 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 new file mode 100644 index 000000000..e62484d96 --- /dev/null +++ b/ebean-test/src/test/java/org/tests/model/join/HAccess.java @@ -0,0 +1,68 @@ +package org.tests.model.join; + +import java.util.UUID; + +import javax.persistence.Entity; +import javax.persistence.Id; +import javax.persistence.Inheritance; +import javax.persistence.JoinColumn; +import javax.persistence.ManyToOne; + +import io.ebean.annotation.DbForeignKey; + +@Inheritance +@Entity +public abstract class HAccess { + + @Id + UUID id; + + @ManyToOne + @DbForeignKey(noConstraint = true) + @JoinColumn(name = "accessor_id") + HCustomer accessor; + + @ManyToOne + @DbForeignKey(noConstraint = true) + @JoinColumn(name = "principal_id") + HCustomer principal; + + @ManyToOne + @DbForeignKey(noConstraint = true) + @JoinColumn(name = "access_account_number") + HAccount account; + + public UUID getId() { + return id; + } + + public void setId(UUID id) { + this.id = id; + } + + public HCustomer getAccessor() { + return accessor; + } + + public void setAccessor(HCustomer accessor) { + this.accessor = accessor; + } + + public HCustomer getPrincipal() { + return principal; + } + + public void setPrincipal(HCustomer principal) { + this.principal = principal; + } + + public HAccount getAccount() { + return 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 new file mode 100644 index 000000000..2ca896403 --- /dev/null +++ b/ebean-test/src/test/java/org/tests/model/join/HAccount.java @@ -0,0 +1,32 @@ +package org.tests.model.join; + +import javax.persistence.*; + +@Entity +@Inheritance +public abstract class HAccount { + + @Id + String accountNumber; + + @ManyToOne + @JoinColumn(name = "owner_id") + HCustomer owner; + + public String getAccountNumber() { + return accountNumber; + } + + public void setAccountNumber(String accountNumber) { + this.accountNumber = accountNumber; + } + + public HCustomer getOwner() { + return 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 new file mode 100644 index 000000000..9a5ba3a9d --- /dev/null +++ b/ebean-test/src/test/java/org/tests/model/join/HCustomer.java @@ -0,0 +1,20 @@ +package org.tests.model.join; + +import javax.persistence.Entity; +import javax.persistence.Id; + +@Entity +public class HCustomer { + + @Id + final String cid; + + final String name; + + String status = "A"; + + 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 new file mode 100644 index 000000000..e8f1a81be --- /dev/null +++ b/ebean-test/src/test/java/org/tests/query/TestQueryMultiJoinFetchPath.java @@ -0,0 +1,69 @@ +package org.tests.query; + +import io.ebean.BaseTestCase; +import io.ebean.DB; +import io.ebean.Query; +import org.junit.jupiter.api.Test; +import org.tests.model.join.*; + +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +class TestQueryMultiJoinFetchPath extends BaseTestCase { + + @Test + void test() { + + HCustomer c1 = new HCustomer("c1", "c1"); + DB.save(c1); + + HCustomer c2 = new HCustomer("c2", "c2"); + DB.save(c2); + + HCustomer c3 = new HCustomer("c3", "c3"); + DB.save(c3); + + 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(HAccess.class) + .where() + .eq("principal.status", "A") + .eq("accessor.status", "A") + .findIds(); + + assertThat(ids).hasSize(2); + + Query query = DB.find(HAccess.class) + .fetch("account","accountNumber") + .fetch("accessor","name") + .where() + .eq("accessor.status", "A") + .eq("principal.status", "A") + .idIn(ids) + .query(); + + 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 "); + } + } + +}