Batch statements issued out of order when a BeanPersistController persists from a callback (follow-up to #3148)

[AntoineDuComptoirDesPharmacies]

Hi everyone, we notice a bug when persisting an entity (DcoParent) containing a list of entities (DcoAsset) linked throuw a table of association which is Java Managed (DcoLink).
When we have an adapter doing insert into database, the batch is flushed but not respecting the correct entity order which result in DB rejecting due to FK constraints.

Version

18.4.0 : reproduced both against the published jar and against a build of master. Platform: H2 (the test below runs in ebean-test).

What happens

#3148 fixed the case where a query run from a BeanPersistController callback flushes the batch that is currently being executed: BatchControl.executeNow(list) turns flushOnQuery off for the duration.

A persist run from the same callback is not covered. It reaches BatchControl.executeOrQueue, which flushes, so the statements queued behind the one currently executing are issued early.

Saving a parent/child graph in batch while a row is written from preInsert:

insert into dco_link (parent_id, asset_id) values (?,?)
  NULL not allowed for column "PARENT_ID"

The children are issued before their parent has an id, the same symptom #3148 describes: "the later three will fail, because customer 2 is not yet saved and has no ID".

Persisting from a callback is documented behaviour, the BeanPersistController javadoc gives it as an example:

Object extaBeanToSave = ...;
Transaction t = request.getTransaction();
Database server = request.getEbeanServer();
database.save(extraBeanToSave, t);

Reproducer

Three entities, an adapter and a test.

@Entity
public class DcoParent {
  @Id @GeneratedValue Long id;
  String name;
  @OneToMany(cascade = CascadeType.ALL, mappedBy = "parent", orphanRemoval = true)
  List<DcoLink> links = new ArrayList<>();
}

@Entity
public class DcoLink {
  @Id @GeneratedValue Long id;
  @ManyToOne(optional = false) DcoParent parent;
  @OneToOne(optional = false, cascade = CascadeType.ALL, orphanRemoval = true) DcoAsset asset;
}

@Entity
public class DcoAsset {
  @Id @GeneratedValue Long id;
  @Version Long version;
  String name;
}

@Entity
public class DcoAudit {   // the row written from the callback
  @Id @GeneratedValue Long id;
  String message;
}

public class DcoParentAdapter extends BeanPersistAdapter {

  @Override
  public boolean isRegisterFor(Class<?> cls) {
    return DcoParent.class.equals(cls);
  }

  @Override
  public boolean preInsert(BeanPersistRequest<?> request) {
    DcoParent parent = (DcoParent) request.bean();
    request.database().save(new DcoAudit("inserting " + parent.getName()), request.transaction());
    return true;
  }
}

@Test
void insertParentBeforeItsLinks_whenCallbackWritesDuringFlush() {
  List<DcoParent> parents = new ArrayList<>();
  for (int i = 0; i < 3; i++) {
    DcoParent parent = new DcoParent("batch-parent-" + i);
    parent.addAsset(new DcoAsset("batch-asset-" + i));   // creates the DcoLink
    parents.add(parent);
  }

  try (Transaction txn = DB.beginTransaction()) {
    txn.setBatchMode(true);
    txn.setBatchSize(50);
    DB.saveAll(parents);
    txn.commit();     // DataIntegrityException here
  }
}

Why

• PersistRequestBean.executeInsert() and executeUpdate() call controller.preInsert / preUpdate from inside executeNow(), that is, during the batch execution.
• The persist done there reaches BatchControl.executeOrQueue, whose first branch calls flush().
• flushInternal runs executeAll() re-entrantly. The bean holder being executed has already had its list taken out of it, so the remaining holders are executed first : ahead of the statements that were queued before them.

The same thing is visible on the delete side with logSummary on:

BatchControl flush [DcoLink:0 d:2, DcoAsset:1 d:2]                  <- outer flush
BatchControl flush [DcoLink:0 d:0, DcoAsset:1 d:2, DcoAudit:2 i:1]  <- from the callback

Relation to the delete-side reportscontrollerPreDelete() ahead of the cascade : but only preDelete was moved, so preInsert and preUpdate still run inside the flush, which is what this issue is about.

Suggested fix

Guard executeOrQueue the way executeNow already guards flushOnQuery: while the batch is executing, queue instead of flushing. Statements added in the meantime are picked up by the do/while loop in executeAll().

  private boolean executing;

  void executeNow(ArrayList<PersistRequest> list) throws BatchedSqlException {
    boolean old = transaction.isFlushOnQuery();
    transaction.setFlushOnQuery(false);
    boolean oldExecuting = executing;
    executing = true;
    try {
      ...
    } finally {
      executing = oldExecuting;
      transaction.setFlushOnQuery(old);
    }
  }

  public int executeOrQueue(PersistRequestBean<?> request, boolean batch) throws BatchedSqlException {
    if (!executing && (!batch || (batchFlushOnMixed && !pstmtHolder.isEmpty()))) {
      flush();
    }
    ...
  }

With that change the test above passes, TestBatchInsertFlush (the test added by #3148) still passes, and the full ebean-test suite runs 2561 tests with no new failure. The only failing test, TestNatKeyCacheWithForeignKey#test_findOne, fails the same way without the patch and passes when run on its own, so it looks order-dependent and unrelated.

Will open a PR with the fix proposal and the test.