Stateless update on one-to-many relationship with children soft delete results in a hard delete instead of only inserting new child

Ichtil commented 2 years ago

Expected behavior

Domain has 3 entities - Goods, Workflow (softdelete), WorkflowOperation (softdelete).

Relationships are

Goods with one-to-one relationship to Workflow (orphanRemoval and cascade.all)
Workflow has one to many relationship to WorkflowOperation (orphanRemoval, and cascade.all)

When adding Operation to Workflow and saving Goods instance, we expect to see insert of the new operations (with possibly several updates). No delete should be issued on existing operation (and even if it was issued, it should have been soft delete).

Actual behavior

There is a hard delete (delete from) on existing Operation

The update will throw exception if there is one-to-many cascade.all relation on workflow_operation_entity that is still be referenced from other table - the delete is not cascaded (this error is not showcased in the test as I believe it shouldn't be an issue in the first place).

Steps to reproduce

Please see attached PR with unit test.

@Entity
public class GoodsEntity extends BaseDomain {
    private String name;

    @OneToOne(cascade = CascadeType.ALL, orphanRemoval = true)
    private WorkflowEntity workflowEntity;

@Entity
public class WorkflowEntity  extends BaseDomain {
    private String revision;

    @OneToMany(cascade = CascadeType.ALL, orphanRemoval = true)
    @JoinColumn(name = "workflow_id")
    private List<WorkflowOperationEntity> operations = new ArrayList<>();

    @SoftDelete
    private boolean deleted;

@Entity
public class WorkflowOperationEntity extends BaseDomain {

  private String name;

  @ManyToOne
  @JoinColumn(name = "workflow_id")
  private WorkflowEntity workflowEntity;

  @SoftDelete
  private boolean deleted;

    var goods = new GoodsEntity();
    var workflow = new WorkflowEntity();
    var operation1 = new WorkflowOperationEntity();
    goods.setWorkflowEntity(workflow);
    workflow.setOperations(List.of(operation1));

    DB.save(goods);

    LoggedSql.start();

    // statelessly add another operation to the workflow and save goods
    var goodsStateless = new GoodsEntity();
    goodsStateless.setId(1L);
    var workflowStateless = new WorkflowEntity();
    workflowStateless.setId(1L);
    var operation1Stateless = new WorkflowOperationEntity();
    operation1Stateless.setId(1L);
    var operation2 = new WorkflowOperationEntity();
    goodsStateless.setWorkflowEntity(workflowStateless);
    workflowStateless.setOperations(List.of(operation1Stateless, operation2));

    DB.update(goodsStateless);
    var updateSql = LoggedSql.stop();

    var dbGoodsAfterUpdate = DB.find(GoodsEntity.class).findOne();
    assertThat(dbGoodsAfterUpdate.getWorkflowEntity().getOperations()).hasSize(2);
    assertThat(dbGoodsAfterUpdate.getWorkflowEntity().getOperations().get(0).getId()).isEqualTo(1L);
    assertThat(dbGoodsAfterUpdate.getWorkflowEntity().getOperations().get(1).getId()).isEqualTo(2L);


txn[] delete from workflow_operation_entity where workflow_id=? 
txn[]  -- bind(1)
txn[] update workflow_entity set when_modified=? where id=?
txn[]  -- bind(2022-06-29 15:43:55.573,1)
txn[] update workflow_operation_entity set deleted=true where workflow_id = ? and not ( id in (?) ) 
txn[]  -- bind(1, Array[1]={1})
txn[] insert into workflow_operation_entity (name, version, when_created, when_modified, deleted, workflow_id) values (?,?,?,?,?,?)
txn[]  -- bind(null,1,2022-06-29 15:43:55.584,2022-06-29 15:43:55.584,false,1)
txn[] update goods_entity set when_modified=?, workflow_entity_id=? where id=?; -- bind(2022-06-29 15:43:55.573,1,1)```

Ichtil commented 2 years ago

I have added 2 more tests.

One is the copy of the original test but it is also setting other fields on the entities - which leads to en error javax.persistence.EntityNotFoundException: No rows updated.

Another test was supposed to demonstrate another weird behaviour, but I couldn't reproduce it due to the test failing in the code leading up to it. Instead of statelessly updating entity, ebean tries to insert entity with the same ID. See the unit test duplicateKeyWorkflowEntityInsertInsteadOfUpdate. I had some troubles running the tests, so this case may be my mistake because of my local ebean project setup.

The original error I was trying to demonstrate with the second added unit test is this - in our application setting config.setPersistBatchOnCascade(PersistBatch.NONE); hides the main issue, but after saving the goods instance, the workflow.operations as seen in debugger is populated correctly, altough after calling goods.getWorkflow().getOperations() the operations will be populated also with soft deleted records. Loading new instance from DB works as expected.

Thinking about it, I may have misplaced the tests in a wrong folder of o2m, as it could be more related to stateless tests.

rbygrave commented 2 years ago

Just noting that the 3rd test case had a possible misunderstanding in that we can't use save() for a "stateless update" and we instead must use update().

That is, save() uses the internal bean state to determine if something is an insert or update. As such, with the 3rd test case that meant it did an insert and we got the expected result of DuplicateKeyException. When we use update() we are explicitly telling ebean to perform an update and if that bean internally is actually in new state this is a "stateless update" (an update occurring on a bean that hasn't been fetched but instead just new'ed up). So for test case 3, I have changed that save() to be an update() and it works and passes as expected.

The underlying issue we hit here is in SaveManyBeans. In there we have 2 cases for doing bulk deletes for orphanRemoval. One is specifically for (A) "stateless updates" and the other for (B) "normal updates + non BeanCollections". The bug here was that we where effectively getting the bulk orphanRemoval from the (B) occurring rather than our specific one for (A). The (B) one doesn't take into account soft delete or the stateless update nature ... and hence those tests failed. The fix means that it will not use the (B) for stateless update orphan removal.

ebean-orm / ebean