Bugfix/3876

ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchControl.java

@@ -81,6 +81,13 @@ public final class BatchControl {
    */
   private int bufferMax;
 
+  /**
+   * True while the batched requests are being executed. A persist performed from a
+   * BeanPersistController callback must not flush the batch that is executing it, the same way
+   * executeNow() stops a query from doing so.
+   */
+  private boolean executing;
+
   private final Queue[] queues = new Queue[2];
 
   static final int DELETE_QUEUE = 0;
@@ -139,8 +146,9 @@ public void setGetGeneratedKeys(Boolean getGeneratedKeys) {
    * to the depth.
    */
   public int executeStatementOrBatch(PersistRequest request, boolean batch, boolean addBatch) throws BatchedSqlException {
-    if (!batch || (batchFlushOnMixed && !isBeansEmpty())) {
-      // flush when mixing beans and updateSql
+    if (!executing && (!batch || (batchFlushOnMixed && !isBeansEmpty()))) {
+      // flush when mixing beans and updateSql, unless we are inside the execution of the batch
+      // itself : flushing then would issue the statements queued behind the current one early
       flush();
     }
     if (!batch) {
@@ -163,8 +171,9 @@ public int executeStatementOrBatch(PersistRequest request, boolean batch, boolea
    * according to the depth (object graph depth).
    */
   public int executeOrQueue(PersistRequestBean<?> request, boolean batch) throws BatchedSqlException {
-    if (!batch || (batchFlushOnMixed && !pstmtHolder.isEmpty())) {
-      // flush when mixing beans and updateSql
+    if (!executing && (!batch || (batchFlushOnMixed && !pstmtHolder.isEmpty()))) {
+      // flush when mixing beans and updateSql, unless we are inside the execution of the batch
+      // itself : flushing then would issue the statements queued behind the current one early
       flush();
     }
     if (!batch) {
@@ -226,7 +235,9 @@ private void flushPstmtHolder(boolean reset) throws BatchedSqlException {
   void executeNow(ArrayList<PersistRequest> list) throws BatchedSqlException {
     boolean old = transaction.isFlushOnQuery();
     transaction.setFlushOnQuery(false);
-    // disable flush on query due transaction callbacks
+    boolean oldExecuting = executing;
+    executing = true;
+    // disable flush on query and on persist due transaction callbacks
     try {
       for (int i = 0; i < list.size(); i++) {
         if (i % batchSize == 0) {
@@ -237,6 +248,7 @@ void executeNow(ArrayList<PersistRequest> list) throws BatchedSqlException {
       }
       flushPstmtHolder();
     } finally {
+      executing = oldExecuting;
       transaction.setFlushOnQuery(old);
     }
   }

ebean-test/src/test/java/org/tests/delete/TestDeleteCascadeOrder.java

@@ -0,0 +1,95 @@
+package org.tests.delete;
+
+import io.ebean.DB;
+import io.ebean.Transaction;
+import io.ebean.test.LoggedSql;
+import io.ebean.xtest.BaseTestCase;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+import org.tests.model.deleteorder.DcoAsset;
+import org.tests.model.deleteorder.DcoLinkAdapter;
+import org.tests.model.deleteorder.DcoParent;
+
+import java.util.List;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * A join entity owns the foreign key to the bean its delete cascades to, so the join row has to be
+ * deleted first. When a persist callback writes to the database it flushes the batch from inside the
+ * flush that is already running : the outer flush has taken the join rows out of their bean holder,
+ * so the inner flush only finds the assets and executes them first.
+ * <p>
+ * See <a href="https://github.com/ebean-orm/ebean/issues/1852">#1852</a>.
+ */
+class TestDeleteCascadeOrder extends BaseTestCase {
+
+  @AfterEach
+  void after() {
+    DcoLinkAdapter.reset();
+  }
+
+  @Test
+  void deleteLinkBeforeAsset() {
+    assertLinkDeletedBeforeAsset(deleteAllLinks(newParent(2)));
+  }
+
+  @Test
+  void deleteLinkBeforeAsset_whenCallbackWritesOnPreDelete() {
+    DcoLinkAdapter.writeOnPreDelete(true);
+
+    assertLinkDeletedBeforeAsset(deleteAllLinks(newParent(2)));
+  }
+
+  @Test
+  void deleteLinkBeforeAsset_whenCallbackWritesOnPostDelete() {
+    DcoLinkAdapter.writeOnPostDelete(true);
+
+    assertLinkDeletedBeforeAsset(deleteAllLinks(newParent(2)));
+  }
+
+  private Long newParent(int assetCount) {
+    DcoParent parent = new DcoParent("parent-" + assetCount);
+    for (int i = 0; i < assetCount; i++) {
+      parent.addAsset(new DcoAsset("asset-" + i));
+    }
+    DB.save(parent);
+    return parent.getId();
+  }
+
+  /**
+   * Remove every link of the parent, which cascades the delete to the assets behind them. The graph is
+   * fetched up front : a lazy load would flush the batch on its own and hide the ordering.
+   */
+  private List<String> deleteAllLinks(Long parentId) {
+    try (Transaction txn = DB.beginTransaction()) {
+      txn.setBatchMode(true);
+      DcoParent parent = DB.find(DcoParent.class)
+        .fetch("links")
+        .fetch("links.asset")
+        .where().idEq(parentId)
+        .findOne();
+      parent.getLinks().clear();
+
+      LoggedSql.start();
+      DB.save(parent);
+      txn.commit();
+      return LoggedSql.stop();
+    }
+  }
+
+  private void assertLinkDeletedBeforeAsset(List<String> sql) {
+    assertThat(firstIndexOf(sql, "delete from dco_link"))
+      .as("the join row must be deleted before the asset it references, statements were :%n%s", String.join("\n", sql))
+      .isLessThan(firstIndexOf(sql, "delete from dco_asset"));
+  }
+
+  private int firstIndexOf(List<String> sql, String fragment) {
+    for (int i = 0; i < sql.size(); i++) {
+      if (sql.get(i).contains(fragment)) {
+        return i;
+      }
+    }
+    throw new AssertionError("no statement containing '" + fragment + "', statements were :\n" + String.join("\n", sql));
+  }
+}

ebean-test/src/test/java/org/tests/delete/TestDeleteTreeCascadeOrder.java

@@ -0,0 +1,50 @@
+package org.tests.delete;
+
+import io.ebean.DB;
+import io.ebean.xtest.BaseTestCase;
+import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.Test;
+import org.tests.model.deleteorder.DcoTree;
+import org.tests.model.deleteorder.DcoTreeContainer;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * The tree shape reported on <a href="https://github.com/ebean-orm/ebean/issues/1852">#1852</a> :
+ * deleting the container cascades down a self referencing tree, and the deletes have to reach the
+ * leaves before their parents.
+ */
+class TestDeleteTreeCascadeOrder extends BaseTestCase {
+
+  /**
+   * Still reproduces on 18.4.0. The tree is deleted level by level but not deepest first :
+   * <pre>
+   *   delete from dco_tree where id in (?)      -- the root, whose children are still there
+   *   delete from dco_tree where id in (?,?,?)
+   *   delete from dco_tree where id in (?,?)
+   * </pre>
+   * which fails with "Referential integrity constraint violation: FK_DCO_TREE_PARENT_ID". Disabled so
+   * that it does not break the build, remove the annotation to see the failure.
+   */
+  @Disabled("reproduces #1852, not fixed yet")
+  @Test
+  void deleteContainerOfNestedTree() {
+    DcoTreeContainer container = new DcoTreeContainer();
+    DcoTree root = new DcoTree("root");
+
+    DcoTree child1 = root.addChild("child 1");
+    child1.addChild("child 1a").addChild("child 1a1");
+
+    DcoTree child2 = root.addChild("child 2");
+    child2.addChild("child 2a");
+    child2.addChild("child 2b");
+
+    container.getTrees().add(root);
+    DB.save(container);
+
+    DB.delete(container);
+
+    assertThat(DB.find(DcoTree.class).findCount()).isZero();
+    assertThat(DB.find(DcoTreeContainer.class).where().idEq(container.getId()).findCount()).isZero();
+  }
+}

ebean-test/src/test/java/org/tests/delete/TestInsertCascadeOrder.java

@@ -0,0 +1,91 @@
+package org.tests.delete;
+
+import io.ebean.DB;
+import io.ebean.Transaction;
+import io.ebean.test.LoggedSql;
+import io.ebean.xtest.BaseTestCase;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+import org.tests.model.deleteorder.DcoAsset;
+import org.tests.model.deleteorder.DcoParent;
+import org.tests.model.deleteorder.DcoParentAdapter;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Same defect as the cascaded delete one, on the insert side : a persist done from a BeanPersistController
+ * flushes the batch from inside the flush that is already running, and the statements queued behind the
+ * one being executed are issued out of order.
+ * <p>
+ * <a href="https://github.com/ebean-orm/ebean/pull/3148">#3148</a> fixed this for a flush triggered by a
+ * query (BatchControl.executeNow disables flushOnQuery), but a flush triggered by a write goes through
+ * BatchControl.executeOrQueue, which that guard does not cover.
+ */
+class TestInsertCascadeOrder extends BaseTestCase {
+
+  @AfterEach
+  void after() {
+    DcoParentAdapter.writeOnPreInsert(false);
+    DcoParentAdapter.sqlUpdateOnPreInsert(false);
+  }
+
+  @Test
+  void insertParentBeforeItsLinks_whenCallbackWritesDuringFlush() {
+    DcoParentAdapter.writeOnPreInsert(true);
+
+    List<String> sql = insertParents(3);
+
+    // every parent has to be inserted before the link that points at it
+    assertThat(lastIndexOf(sql, "insert into dco_parent"))
+      .as("a parent must be inserted before the links referencing it, statements were :%n%s", String.join("\n", sql))
+      .isLessThan(lastIndexOf(sql, "insert into dco_link"));
+  }
+
+  /**
+   * Same as above but the callback runs a SqlUpdate, which reaches BatchControl by
+   * executeStatementOrBatch rather than executeOrQueue.
+   */
+  @Test
+  void insertParentBeforeItsLinks_whenCallbackRunsSqlUpdateDuringFlush() {
+    DcoParentAdapter.sqlUpdateOnPreInsert(true);
+
+    List<String> sql = insertParents(3);
+
+    assertThat(lastIndexOf(sql, "insert into dco_parent"))
+      .as("a parent must be inserted before the links referencing it, statements were :%n%s", String.join("\n", sql))
+      .isLessThan(lastIndexOf(sql, "insert into dco_link"));
+  }
+
+  private List<String> insertParents(int count) {
+    List<DcoParent> parents = new ArrayList<>();
+    for (int i = 0; i < count; i++) {
+      DcoParent parent = new DcoParent("batch-parent-" + i);
+      parent.addAsset(new DcoAsset("batch-asset-" + i));
+      parents.add(parent);
+    }
+
+    try (Transaction txn = DB.beginTransaction()) {
+      txn.setBatchMode(true);
+      txn.setBatchSize(50);
+      LoggedSql.start();
+      DB.saveAll(parents);
+      txn.commit();
+      List<String> sql = LoggedSql.stop();
+      System.out.println("---- insert order ----");
+      sql.stream().filter(s -> !s.contains("-- bind")).forEach(s -> System.out.println("   " + s));
+      return sql;
+    }
+  }
+
+  private int lastIndexOf(List<String> sql, String fragment) {
+    for (int i = sql.size() - 1; i >= 0; i--) {
+      if (sql.get(i).contains(fragment)) {
+        return i;
+      }
+    }
+    throw new AssertionError("no statement containing '" + fragment + "', statements were :\n" + String.join("\n", sql));
+  }
+}

ebean-test/src/test/java/org/tests/model/deleteorder/DcoAsset.java

@@ -0,0 +1,34 @@
+package org.tests.model.deleteorder;
+
+import jakarta.persistence.Entity;
+import jakarta.persistence.GeneratedValue;
+import jakarta.persistence.Id;
+import jakarta.persistence.Version;
+
+/**
+ * Owned by a {@link DcoLink} through a cascading OneToOne, so deleting the link deletes the asset.
+ */
+@Entity
+public class DcoAsset {
+
+  @Id
+  @GeneratedValue
+  Long id;
+
+  @Version
+  Long version;
+
+  String name;
+
+  public DcoAsset(String name) {
+    this.name = name;
+  }
+
+  public Long getId() {
+    return id;
+  }
+
+  public String getName() {
+    return name;
+  }
+}

ebean-test/src/test/java/org/tests/model/deleteorder/DcoAudit.java

@@ -0,0 +1,30 @@
+package org.tests.model.deleteorder;
+
+import jakarta.persistence.Entity;
+import jakarta.persistence.GeneratedValue;
+import jakarta.persistence.Id;
+
+/**
+ * Written from the delete callback of {@link DcoLink}, the way an audit or an outbox row is.
+ */
+@Entity
+public class DcoAudit {
+
+  @Id
+  @GeneratedValue
+  Long id;
+
+  String message;
+
+  public DcoAudit(String message) {
+    this.message = message;
+  }
+
+  public Long getId() {
+    return id;
+  }
+
+  public String getMessage() {
+    return message;
+  }
+}