From 588aaf93614a560bf75f96ad1830f28268dea867 Mon Sep 17 00:00:00 2001 From: rob bygrave Date: Thu, 5 Jul 2018 22:37:54 +1200 Subject: [PATCH] #1447 - Delete query ... should honour cascading delete if defined See also PR #1445 with the failing test case --- .../server/core/DefaultServer.java | 13 ++- .../server/core/OrmQueryRequest.java | 5 ++ .../ebeaninternal/server/core/Persister.java | 5 ++ .../server/core/SpiOrmQueryRequest.java | 5 ++ .../server/persist/DefaultPersister.java | 32 ++++--- .../org/tests/delete/TestDeleteByQuery.java | 83 +++++++++++++++++-- .../org/tests/model/basic/BBookmarkOrg.java | 35 ++++++++ .../org/tests/model/basic/BBookmarkUser.java | 60 ++++---------- .../transaction/TestInsertManyAndRef.java | 22 +---- 9 files changed, 174 insertions(+), 86 deletions(-) create mode 100644 src/test/java/org/tests/model/basic/BBookmarkOrg.java diff --git a/src/main/java/io/ebeaninternal/server/core/DefaultServer.java b/src/main/java/io/ebeaninternal/server/core/DefaultServer.java index a540ecb1b..2a7c4e1cc 100644 --- a/src/main/java/io/ebeaninternal/server/core/DefaultServer.java +++ b/src/main/java/io/ebeaninternal/server/core/DefaultServer.java @@ -1384,7 +1384,18 @@ public final class DefaultServer implements SpiServer, SpiEbeanServer { try { request.initTransIfRequired(); request.markNotQueryOnly(); - return request.delete(); + if (request.isDeleteByStatement()) { + return request.delete(); + } else { + // escalate to fetch the ids of the beans to delete due + // to cascading deletes or l2 caching etc + List ids = request.findIds(); + if (ids.isEmpty()) { + return 0; + } else { + return persister.deleteByIds(request.getBeanDescriptor(), ids, request.getTransaction(), false); + } + } } finally { request.endTransIfRequired(); } diff --git a/src/main/java/io/ebeaninternal/server/core/OrmQueryRequest.java b/src/main/java/io/ebeaninternal/server/core/OrmQueryRequest.java index 8c9420911..fd21d8f8a 100644 --- a/src/main/java/io/ebeaninternal/server/core/OrmQueryRequest.java +++ b/src/main/java/io/ebeaninternal/server/core/OrmQueryRequest.java @@ -116,6 +116,11 @@ public final class OrmQueryRequest extends BeanRequest implements SpiOrmQuery } } + @Override + public boolean isDeleteByStatement() { + return beanDescriptor.isDeleteByStatement(); + } + @Override public boolean isMultiValueIdSupported() { return beanDescriptor.isMultiValueIdSupported(); diff --git a/src/main/java/io/ebeaninternal/server/core/Persister.java b/src/main/java/io/ebeaninternal/server/core/Persister.java index 20cbfcce4..c64daaa4e 100644 --- a/src/main/java/io/ebeaninternal/server/core/Persister.java +++ b/src/main/java/io/ebeaninternal/server/core/Persister.java @@ -63,6 +63,11 @@ public interface Persister { */ int deleteMany(Class beanType, Collection ids, Transaction transaction, boolean permanent); + /** + * Delete multiple beans when escalated from a delete query. + */ + int deleteByIds(BeanDescriptor descriptor, List idList, Transaction transaction, boolean permanent); + /** * Execute the Update. */ diff --git a/src/main/java/io/ebeaninternal/server/core/SpiOrmQueryRequest.java b/src/main/java/io/ebeaninternal/server/core/SpiOrmQueryRequest.java index 68895bb88..c78661398 100644 --- a/src/main/java/io/ebeaninternal/server/core/SpiOrmQueryRequest.java +++ b/src/main/java/io/ebeaninternal/server/core/SpiOrmQueryRequest.java @@ -171,4 +171,9 @@ public interface SpiOrmQueryRequest extends BeanQueryRequest, DocQueryRequ * Set profile location for "find all" if not set. */ void profileLocationAll(); + + /** + * Return true if delete by statement is allowed for this type given cascade rules etc. + */ + boolean isDeleteByStatement(); } diff --git a/src/main/java/io/ebeaninternal/server/persist/DefaultPersister.java b/src/main/java/io/ebeaninternal/server/persist/DefaultPersister.java index a259f8b84..feb7337b3 100644 --- a/src/main/java/io/ebeaninternal/server/persist/DefaultPersister.java +++ b/src/main/java/io/ebeaninternal/server/persist/DefaultPersister.java @@ -680,6 +680,12 @@ public final class DefaultPersister implements Persister { return delete(descriptor, id, null, transaction, deleteMode); } + @Override + public int deleteByIds(BeanDescriptor descriptor, List idList, Transaction transaction, boolean permanent) { + DeleteMode deleteMode = (permanent || !descriptor.isSoftDelete()) ? DeleteMode.HARD : DeleteMode.SOFT; + return delete(descriptor, null, idList, transaction, deleteMode); + } + /** * Delete by Id or a List of Id's. */ @@ -739,18 +745,20 @@ public final class DefaultPersister implements Persister { // OneToMany's with delete cascade BeanPropertyAssocMany[] manys = descriptor.propertiesManyDelete(); for (BeanPropertyAssocMany many : manys) { - BeanDescriptor targetDesc = many.getTargetDescriptor(); - // only cascade soft deletes when supported by target - if (deleteMode.isHard() || targetDesc.isSoftDelete()) { - if (deleteMode.isHard() && targetDesc.isDeleteByStatement()) { - // we can just delete children with a single statement - SqlUpdate sqlDelete = many.deleteByParentId(id, idList); - executeSqlUpdate(sqlDelete, t); - } else { - // we need to fetch the Id's to delete (recurse or notify L2 cache) - List childIds = many.findIdsByParentId(id, idList, t, null); - if (!childIds.isEmpty()) { - delete(targetDesc, null, childIds, t, deleteMode); + if (!many.isManyToMany()) { + BeanDescriptor targetDesc = many.getTargetDescriptor(); + // only cascade soft deletes when supported by target + if (deleteMode.isHard() || targetDesc.isSoftDelete()) { + if (deleteMode.isHard() && targetDesc.isDeleteByStatement()) { + // we can just delete children with a single statement + SqlUpdate sqlDelete = many.deleteByParentId(id, idList); + executeSqlUpdate(sqlDelete, t); + } else { + // we need to fetch the Id's to delete (recurse or notify L2 cache) + List childIds = many.findIdsByParentId(id, idList, t, null); + if (!childIds.isEmpty()) { + delete(targetDesc, null, childIds, t, deleteMode); + } } } } diff --git a/src/test/java/org/tests/delete/TestDeleteByQuery.java b/src/test/java/org/tests/delete/TestDeleteByQuery.java index a4b7ce989..47c1c7ff8 100644 --- a/src/test/java/org/tests/delete/TestDeleteByQuery.java +++ b/src/test/java/org/tests/delete/TestDeleteByQuery.java @@ -7,6 +7,7 @@ import io.ebean.Query; import io.ebean.annotation.IgnorePlatform; import io.ebean.annotation.Platform; +import org.tests.model.basic.BBookmarkUser; import org.tests.model.basic.Contact; import org.tests.model.basic.Customer; import org.tests.model.basic.ResetBasicData; @@ -22,7 +23,46 @@ public class TestDeleteByQuery extends BaseTestCase { @Test @IgnorePlatform(Platform.MYSQL) // FIXME: MySql does not the sub query selecting from the delete table - public void test() { + public void deleteWithSubquery() { + + EbeanServer server = Ebean.getDefaultServer(); + + BBookmarkUser u1 = new BBookmarkUser("u1"); + Ebean.save(u1); + + Query query = server.find(BBookmarkUser.class) + .where().eq("org.name", "NahYeahMaybe") + .query(); + + LoggedSqlCollector.start(); + query.delete(); + + List loggedSql = LoggedSqlCollector.stop(); + assertThat(loggedSql).hasSize(1); + assertThat(trimSql(loggedSql.get(0), 1)).contains("delete from bbookmark_user where id in (select t0.id from bbookmark_user t0 left join bbookmark_org t1 on t1.id = t0.org_id where t1.name"); + + Query query2 = server.find(BBookmarkUser.class) + .where().eq("name", "NotARealFirstName").query(); + + LoggedSqlCollector.start(); + query2.delete(); + + loggedSql = LoggedSqlCollector.stop(); + assertThat(loggedSql).hasSize(1); + assertThat(loggedSql.get(0)).contains("delete from bbookmark_user where name ="); + + + server.find(BBookmarkUser.class).select("id").where().eq("name", "NotARealFirstName").delete(); + server.find(BBookmarkUser.class).select("id").where().eq("name", "TwoAlsoNotRealFirstName").query().delete(); + + List list = server.find(BBookmarkUser.class).select("id").where().eq("name", "NotARealFirstName").findList(); + assertThat(list).isEmpty(); + } + + @Test + @IgnorePlatform(Platform.MYSQL) + // FIXME: MySql does not the sub query selecting from the delete table + public void deleteWithSubquery_withEscalation() { EbeanServer server = Ebean.getDefaultServer(); @@ -33,7 +73,7 @@ public class TestDeleteByQuery extends BaseTestCase { List loggedSql = LoggedSqlCollector.stop(); assertThat(loggedSql).hasSize(1); - assertThat(trimSql(loggedSql.get(0), 1)).contains("delete from contact where id in (select t0.id from contact t0 left join"); + assertThat(trimSql(loggedSql.get(0), 1)).contains("select t0.id from contact t0 left join contact_group t1 on t1.id = t0.group_id where t1.name = ?"); Query query2 = server.find(Contact.class).where().eq("firstName", "NotARealFirstName").query(); @@ -42,7 +82,7 @@ public class TestDeleteByQuery extends BaseTestCase { loggedSql = LoggedSqlCollector.stop(); assertThat(loggedSql).hasSize(1); - assertThat(loggedSql.get(0)).contains("delete from contact where first_name ="); + assertThat(loggedSql.get(0)).contains("select t0.id from contact t0 where t0.first_name = ?"); server.find(Contact.class).select("id").where().eq("firstName", "NotARealFirstName").delete(); @@ -57,12 +97,29 @@ public class TestDeleteByQuery extends BaseTestCase { LoggedSqlCollector.start(); + Ebean.find(BBookmarkUser.class).where().eq("id", 7000).delete(); + Ebean.find(BBookmarkUser.class).setId(7000).delete(); + + List sql = LoggedSqlCollector.stop(); + assertThat(sql.get(0)).contains("delete from bbookmark_user where id = ?"); + assertThat(sql.get(1)).contains("delete from bbookmark_user where id = ?"); + + // and note this is the easiest option + Ebean.delete(BBookmarkUser.class, 7000); + } + + @Test + public void queryByIdDelete_withEscalation() { + + LoggedSqlCollector.start(); + Ebean.find(Contact.class).where().eq("id", 7000).delete(); Ebean.find(Contact.class).setId(7000).delete(); List sql = LoggedSqlCollector.stop(); - assertThat(sql.get(0)).contains("delete from contact where id = ?"); - assertThat(sql.get(1)).contains("delete from contact where id = ?"); + // escalate to fetch ids then delete ... but no rows found + assertThat(sql.get(0)).contains("select t0.id from contact t0 where t0.id = ?"); + assertThat(sql.get(1)).contains("select t0.id from contact t0 where t0.id = ?"); // and note this is the easiest option Ebean.delete(Contact.class, 7000); @@ -80,11 +137,23 @@ public class TestDeleteByQuery extends BaseTestCase { List sql = LoggedSqlCollector.stop(); assertThat(sql).hasSize(1); - assertThat(sql.get(0)).contains("delete from o_customer where name = ?"); + assertThat(sql.get(0)).contains("select t0.id from o_customer t0 where t0.name = ?"); } @Test - public void testCommit() { + public void deleteByPredicate() { + + BBookmarkUser ud = new BBookmarkUser("deleteQueryByPredicate"); + Ebean.save(ud); + + Ebean.find(BBookmarkUser.class).where().eq("name", "deleteQueryByPredicate").delete(); + + BBookmarkUser found = Ebean.find(BBookmarkUser.class, ud.getId()); + assertThat(found).isNull(); + } + + @Test + public void deleteByPredicate_withEscalation() { ResetBasicData.reset(); diff --git a/src/test/java/org/tests/model/basic/BBookmarkOrg.java b/src/test/java/org/tests/model/basic/BBookmarkOrg.java new file mode 100644 index 000000000..305ab9497 --- /dev/null +++ b/src/test/java/org/tests/model/basic/BBookmarkOrg.java @@ -0,0 +1,35 @@ +package org.tests.model.basic; + +import javax.persistence.Entity; +import javax.persistence.GeneratedValue; +import javax.persistence.Id; + +@Entity +public class BBookmarkOrg { + + @Id + @GeneratedValue + private int id; + + private String name; + + public BBookmarkOrg(String name) { + this.name = name; + } + + public int getId() { + return id; + } + + public void setId(int id) { + this.id = id; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } +} diff --git a/src/test/java/org/tests/model/basic/BBookmarkUser.java b/src/test/java/org/tests/model/basic/BBookmarkUser.java index 873af8096..064f4aa66 100644 --- a/src/test/java/org/tests/model/basic/BBookmarkUser.java +++ b/src/test/java/org/tests/model/basic/BBookmarkUser.java @@ -1,13 +1,11 @@ package org.tests.model.basic; -import javax.persistence.Column; import javax.persistence.Entity; import javax.persistence.GeneratedValue; import javax.persistence.Id; +import javax.persistence.ManyToOne; /** - * represents a user entity. A user contains a username and password. - * * @author Chris */ @Entity @@ -17,97 +15,69 @@ public class BBookmarkUser { @GeneratedValue private Integer id; - @Column private String name; - @Column private String password; - @Column private String emailAddress; - @Column private String country; -// @Version -// private Timestamp lastUpdate; - /** - * @return the id + * An optional non-cascading ManyToOne. */ + @ManyToOne + private BBookmarkOrg org; + + public BBookmarkUser(String name) { + this.name = name; + } + public Integer getId() { return this.id; } - /** - * @param id the id to set - */ public void setId(final Integer id) { this.id = id; } - /** - * @return the password - */ public String getPassword() { return this.password; } - /** - * @param password the password to set - */ public void setPassword(final String password) { this.password = password; } - /** - * @return the name - */ public String getName() { return this.name; } - /** - * @param name the name to set - */ public void setName(final String name) { this.name = name; } - /** - * @return the emailAddress - */ public String getEmailAddress() { return this.emailAddress; } - /** - * @param emailAddress the emailAddress to set - */ public void setEmailAddress(final String emailAddress) { this.emailAddress = emailAddress; } - /** - * @return the country - */ public String getCountry() { return this.country; } - /** - * @param country the country to set - */ public void setCountry(final String country) { this.country = country; } -// public Timestamp getLastUpdate() { -// return lastUpdate; -// } -// -// public void setLastUpdate(Timestamp lastUpdate) { -// this.lastUpdate = lastUpdate; -// } + public BBookmarkOrg getOrg() { + return org; + } + public void setOrg(BBookmarkOrg org) { + this.org = org; + } } diff --git a/src/test/java/org/tests/transaction/TestInsertManyAndRef.java b/src/test/java/org/tests/transaction/TestInsertManyAndRef.java index 029595eb7..de123a74d 100644 --- a/src/test/java/org/tests/transaction/TestInsertManyAndRef.java +++ b/src/test/java/org/tests/transaction/TestInsertManyAndRef.java @@ -14,28 +14,8 @@ public class TestInsertManyAndRef extends BaseTestCase { @Test public void testMe() { - // ResetBasicData.reset(); - // - // Customer u = new Customer(); - // u.setName("Mr Test"); - // - // final List bookmarks = new ArrayList(); - // final Order b1 = new Order(); - // b1.setCustomer(u); - // b1.setStatus(Status.NEW); - // - // final Order b2 = new Order(); - // b2.setStatus(Status.NEW); - // b2.setCustomer(u); - // - // bookmarks.add(b1); - // bookmarks.add(b2); - // - // Ebean.save(bookmarks); - - final BBookmarkUser u = new BBookmarkUser(); + final BBookmarkUser u = new BBookmarkUser("Mr Test"); u.setEmailAddress("test@test.com"); - u.setName("Mr Test"); u.setPassword("password"); final List bookmarks = new ArrayList<>();