diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchControl.java b/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchControl.java index 24ed757e46..c59c20c087 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/persist/BatchControl.java +++ b/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 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 list) throws BatchedSqlException { } flushPstmtHolder(); } finally { + executing = oldExecuting; transaction.setFlushOnQuery(old); } } diff --git a/ebean-test/src/test/java/org/tests/delete/TestDeleteCascadeOrder.java b/ebean-test/src/test/java/org/tests/delete/TestDeleteCascadeOrder.java new file mode 100644 index 0000000000..80f1bbe407 --- /dev/null +++ b/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. + *

+ * See #1852. + */ +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 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 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 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)); + } +} diff --git a/ebean-test/src/test/java/org/tests/delete/TestDeleteTreeCascadeOrder.java b/ebean-test/src/test/java/org/tests/delete/TestDeleteTreeCascadeOrder.java new file mode 100644 index 0000000000..d026917280 --- /dev/null +++ b/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 #1852 : + * 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 : + * 

+   *   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 (?,?)
+   *
+ * 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(); + } +} diff --git a/ebean-test/src/test/java/org/tests/delete/TestInsertCascadeOrder.java b/ebean-test/src/test/java/org/tests/delete/TestInsertCascadeOrder.java new file mode 100644 index 0000000000..204a73e032 --- /dev/null +++ b/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. + *

+ * #3148 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 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 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 insertParents(int count) { + List 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 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 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)); + } +} diff --git a/ebean-test/src/test/java/org/tests/model/deleteorder/DcoAsset.java b/ebean-test/src/test/java/org/tests/model/deleteorder/DcoAsset.java new file mode 100644 index 0000000000..69d9ea21fc --- /dev/null +++ b/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; + } +} diff --git a/ebean-test/src/test/java/org/tests/model/deleteorder/DcoAudit.java b/ebean-test/src/test/java/org/tests/model/deleteorder/DcoAudit.java new file mode 100644 index 0000000000..aae719d94a --- /dev/null +++ b/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; + } +} diff --git a/ebean-test/src/test/java/org/tests/model/deleteorder/DcoLink.java b/ebean-test/src/test/java/org/tests/model/deleteorder/DcoLink.java new file mode 100644 index 0000000000..e4b8ac1a28 --- /dev/null +++ b/ebean-test/src/test/java/org/tests/model/deleteorder/DcoLink.java @@ -0,0 +1,46 @@ +package org.tests.model.deleteorder; + +import jakarta.persistence.CascadeType; +import jakarta.persistence.Entity; +import jakarta.persistence.GeneratedValue; +import jakarta.persistence.Id; +import jakarta.persistence.ManyToOne; +import jakarta.persistence.OneToOne; + +/** + * Join entity between {@link DcoParent} and {@link DcoAsset}. It owns the foreign key to the asset, + * so the link row has to be deleted before the asset it points at. + *

+ * A real entity (rather than a plain join table) so that its delete fires a persistence callback, + * see {@link DcoLinkAdapter}. + */ +@Entity +public class DcoLink { + + @Id + @GeneratedValue + Long id; + + @ManyToOne(optional = false) + DcoParent parent; + + @OneToOne(optional = false, cascade = CascadeType.ALL, orphanRemoval = true) + DcoAsset asset; + + public DcoLink(DcoParent parent, DcoAsset asset) { + this.parent = parent; + this.asset = asset; + } + + public Long getId() { + return id; + } + + public DcoParent getParent() { + return parent; + } + + public DcoAsset getAsset() { + return asset; + } +} diff --git a/ebean-test/src/test/java/org/tests/model/deleteorder/DcoLinkAdapter.java b/ebean-test/src/test/java/org/tests/model/deleteorder/DcoLinkAdapter.java new file mode 100644 index 0000000000..495f9d6c22 --- /dev/null +++ b/ebean-test/src/test/java/org/tests/model/deleteorder/DcoLinkAdapter.java @@ -0,0 +1,54 @@ +package org.tests.model.deleteorder; + +import io.ebean.event.BeanPersistAdapter; +import io.ebean.event.BeanPersistRequest; + +/** + * Persists a bean from inside a delete callback of {@link DcoLink}, the way an audit or an outbox row + * is written. The BeanPersistController javadoc documents this as a supported use case. + *

+ * Off by default so that the callbacks only fire for the tests that ask for them. + */ +public class DcoLinkAdapter extends BeanPersistAdapter { + + private static boolean writeOnPreDelete; + private static boolean writeOnPostDelete; + + public static void writeOnPreDelete(boolean enabled) { + writeOnPreDelete = enabled; + } + + public static void writeOnPostDelete(boolean enabled) { + writeOnPostDelete = enabled; + } + + public static void reset() { + writeOnPreDelete = false; + writeOnPostDelete = false; + } + + @Override + public boolean isRegisterFor(Class cls) { + return DcoLink.class.equals(cls); + } + + @Override + public boolean preDelete(BeanPersistRequest request) { + if (writeOnPreDelete) { + audit(request, "pre"); + } + return true; + } + + @Override + public void postDelete(BeanPersistRequest request) { + if (writeOnPostDelete) { + audit(request, "post"); + } + } + + private void audit(BeanPersistRequest request, String phase) { + DcoLink link = (DcoLink) request.bean(); + request.database().save(new DcoAudit(phase + " delete of link " + link.getId()), request.transaction()); + } +} diff --git a/ebean-test/src/test/java/org/tests/model/deleteorder/DcoParent.java b/ebean-test/src/test/java/org/tests/model/deleteorder/DcoParent.java new file mode 100644 index 0000000000..bbb65f27c9 --- /dev/null +++ b/ebean-test/src/test/java/org/tests/model/deleteorder/DcoParent.java @@ -0,0 +1,50 @@ +package org.tests.model.deleteorder; + +import jakarta.persistence.CascadeType; +import jakarta.persistence.Entity; +import jakarta.persistence.GeneratedValue; +import jakarta.persistence.Id; +import jakarta.persistence.OneToMany; + +import java.util.ArrayList; +import java.util.List; + +/** + * Parent of a join entity, see {@link DcoLink}. + */ +@Entity +public class DcoParent { + + @Id + @GeneratedValue + Long id; + + String name; + + @OneToMany(cascade = CascadeType.ALL, mappedBy = "parent", orphanRemoval = true) + List links = new ArrayList<>(); + + public DcoParent(String name) { + this.name = name; + } + + public Long getId() { + return id; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + public List getLinks() { + return links; + } + + public void addAsset(DcoAsset asset) { + links.add(new DcoLink(this, asset)); + } +} diff --git a/ebean-test/src/test/java/org/tests/model/deleteorder/DcoParentAdapter.java b/ebean-test/src/test/java/org/tests/model/deleteorder/DcoParentAdapter.java new file mode 100644 index 0000000000..985ac1d48c --- /dev/null +++ b/ebean-test/src/test/java/org/tests/model/deleteorder/DcoParentAdapter.java @@ -0,0 +1,49 @@ +package org.tests.model.deleteorder; + +import io.ebean.event.BeanPersistAdapter; +import io.ebean.event.BeanPersistRequest; + +/** + * Persists a bean from inside the insert callback of {@link DcoParent}, the way an audit or an outbox + * row is written. The BeanPersistController javadoc documents this as a supported use case. + *

+ * Off by default so that the callback only fires for the tests that ask for it. + */ +public class DcoParentAdapter extends BeanPersistAdapter { + + private static boolean writeOnPreInsert; + private static boolean sqlUpdateOnPreInsert; + + public static void writeOnPreInsert(boolean enabled) { + writeOnPreInsert = enabled; + } + + /** + * Same as {@link #writeOnPreInsert(boolean)} but through SqlUpdate, which takes the + * BatchControl.executeStatementOrBatch path rather than executeOrQueue. + */ + public static void sqlUpdateOnPreInsert(boolean enabled) { + sqlUpdateOnPreInsert = enabled; + } + + @Override + public boolean isRegisterFor(Class cls) { + return DcoParent.class.equals(cls); + } + + @Override + public boolean preInsert(BeanPersistRequest request) { + DcoParent parent = (DcoParent) request.bean(); + if (writeOnPreInsert) { + request.database().save(new DcoAudit("inserting " + parent.getName()), request.transaction()); + } + if (sqlUpdateOnPreInsert) { + request.database().sqlUpdate("update dco_audit set message = ? where id = ?") + .setParameter(1, "inserting " + parent.getName()) + .setParameter(2, -1L) + .usingTransaction(request.transaction()) + .execute(); + } + return true; + } +} diff --git a/ebean-test/src/test/java/org/tests/model/deleteorder/DcoTree.java b/ebean-test/src/test/java/org/tests/model/deleteorder/DcoTree.java new file mode 100644 index 0000000000..58b9e2be71 --- /dev/null +++ b/ebean-test/src/test/java/org/tests/model/deleteorder/DcoTree.java @@ -0,0 +1,52 @@ +package org.tests.model.deleteorder; + +import jakarta.persistence.CascadeType; +import jakarta.persistence.Entity; +import jakarta.persistence.GeneratedValue; +import jakarta.persistence.Id; +import jakarta.persistence.ManyToOne; +import jakarta.persistence.OneToMany; + +import java.util.ArrayList; +import java.util.List; + +/** + * Self referencing tree, as reported on #1852. + */ +@Entity +public class DcoTree { + + @Id + @GeneratedValue + Long id; + + String name; + + @ManyToOne + DcoTree parent; + + @ManyToOne + DcoTreeContainer container; + + @OneToMany(cascade = CascadeType.ALL, mappedBy = "parent") + List children = new ArrayList<>(); + + public DcoTree(String name) { + this.name = name; + } + + public Long getId() { + return id; + } + + public List getChildren() { + return children; + } + + public DcoTree addChild(String name) { + DcoTree child = new DcoTree(name); + child.parent = this; + children.add(child); + return child; + } +} diff --git a/ebean-test/src/test/java/org/tests/model/deleteorder/DcoTreeContainer.java b/ebean-test/src/test/java/org/tests/model/deleteorder/DcoTreeContainer.java new file mode 100644 index 0000000000..8a4cd988f3 --- /dev/null +++ b/ebean-test/src/test/java/org/tests/model/deleteorder/DcoTreeContainer.java @@ -0,0 +1,32 @@ +package org.tests.model.deleteorder; + +import jakarta.persistence.CascadeType; +import jakarta.persistence.Entity; +import jakarta.persistence.GeneratedValue; +import jakarta.persistence.Id; +import jakarta.persistence.OneToMany; + +import java.util.ArrayList; +import java.util.List; + +/** + * Holds the roots of a {@link DcoTree}, as reported on #1852. + */ +@Entity +public class DcoTreeContainer { + + @Id + @GeneratedValue + Long id; + + @OneToMany(cascade = CascadeType.ALL) + List trees = new ArrayList<>(); + + public Long getId() { + return id; + } + + public List getTrees() { + return trees; + } +}