#1447 - Delete query ... should honour cascading delete if defined

See also PR #1445 with the failing test case

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<Object> ids = request.findIds();
+        if (ids.isEmpty()) {
+          return 0;
+        } else {
+          return persister.deleteByIds(request.getBeanDescriptor(), ids, request.getTransaction(), false);
+        }
+      }
     } finally {
       request.endTransIfRequired();
     }

src/main/java/io/ebeaninternal/server/core/OrmQueryRequest.java

@@ -116,6 +116,11 @@ public final class OrmQueryRequest<T> extends BeanRequest implements SpiOrmQuery
     }
   }
 
+  @Override
+  public boolean isDeleteByStatement() {
+    return beanDescriptor.isDeleteByStatement();
+  }
+
   @Override
   public boolean isMultiValueIdSupported() {
     return beanDescriptor.isMultiValueIdSupported();

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<Object> idList, Transaction transaction, boolean permanent);
+
   /**
    * Execute the Update.
    */

src/main/java/io/ebeaninternal/server/core/SpiOrmQueryRequest.java

@@ -171,4 +171,9 @@ public interface SpiOrmQueryRequest<T> extends BeanQueryRequest<T>, 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();
 }

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<Object> 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<Object> 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<Object> childIds = many.findIdsByParentId(id, idList, t, null);
+              if (!childIds.isEmpty()) {
+                delete(targetDesc, null, childIds, t, deleteMode);
+              }
             }
           }
         }

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<BBookmarkUser> query = server.find(BBookmarkUser.class)
+      .where().eq("org.name", "NahYeahMaybe")
+      .query();
+
+    LoggedSqlCollector.start();
+    query.delete();
+
+    List<String> 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<BBookmarkUser> 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<BBookmarkUser> 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<String> 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<Contact> 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<String> 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<String> 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<String> 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();
 

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;
+  }
+}

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;
+  }
 }

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<Order> bookmarks = new ArrayList<Order>();
-    // 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<BBookmark> bookmarks = new ArrayList<>();