Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@OneToMany orphanRemoval not occurring (was: insert instead update on changing @OneToMany field) #2952

Closed
buchtajz opened this issue Jan 30, 2023 · 8 comments · Fixed by #2960
Assignees
Labels
Milestone

Comments

@buchtajz
Copy link
Contributor

buchtajz commented Jan 30, 2023

Expected behavior

Should work as in 11.x version

DB.save/update should use SQL update not insert

Actual behavior

When changed @onetomany List via setter, ebean generate insert sql query.

When using getter and List clear/ addAll, it is using update.

EDIT by Rob:
No it is NOT using update here with clear/addAll. It IS using an extra delete for the orphans.

Steps to reproduce

    // this forces insert and throws exception due primary key conflict
    persistedGoods.setAttachments(marshaledGoods.getAttachments());

    // this works good
    persistedGoods.getAttachments().clear();
    persistedGoods.getAttachments().addAll(marshaledGoods.getAttachments());

    // Note: This is working because we mutate the collection and so the orphans that are 
    // removed by the clear() is known and results in the extra delete for those orphans
  io.ebean.DuplicateKeyException: Error when batch flush on sql: insert into attachment (id, goods_entity_id, name, version, when_created, when_modified, created_by, updated_by) values (?,?,?,?,?,?,?,?)

@rbygrave
Copy link
Member

Can you check the release notes of 12.1.1 - https://github.com/ebean-orm/ebean/releases/tag/ebean-12.1.1

I believe this is the change in behavior this issue is referring to. You can confirm that by comparing 12.1.1 and previous version which was 11.45.1

@rbygrave
Copy link
Member

We can also review this list: https://github.com/ebean-orm/ebean/issues?q=is%3Aissue+is%3Aclosed+label%3Areason-for-version-bump

Closed issues tagged with "reason-for-version-bump"

@buchtajz
Copy link
Contributor Author

Hi Rob, thanks for response.

I read these issues, it looks simillary but i think it's not the same problem. There is some correlation maybee.

There is difference on generated sql setAttachments() vs. getAttachments().clear() + .add()

I added one more attachment to marshaled object to see difference

var marshaledGoods = DB.json().toBean(GoodsEntity.class, DB.json().toJson(goods));
marshaledGoods.getAttachments().add(attachment3);

the first

setAttachments(marshaledGoods.getAttachments()))

on save generate

txn[] insert into attachment (id, goods_entity_id, name, version, when_created, when_modified, created_by, updated_by) values (?,?,?,?,?,?,?,?)
txn[]  -- bind(1,1,File1,1,2023-01-31 11:41:36.21,2023-01-31 11:41:36.21,null,null)
txn[]  -- bind(2,1,File2,1,2023-01-31 11:41:36.21,2023-01-31 11:41:36.21,null,null)
txn[] insert into attachment (goods_entity_id, name, version, when_created, when_modified, created_by, updated_by) values (?,?,?,?,?,?,?)
txn[]  -- bind(1,File3,1,2023-01-31 11:41:36.21,2023-01-31 11:41:36.21,null,null)

ant throws exception due primary key conflict on attachments id 1 and 2

second

persistedGoods.getAttachments().clear();
persistedGoods.getAttachments().addAll(marshaledGoods.getAttachments());

generate

txn[] delete from attachment where id=?
txn[]  -- bind(2)
txn[]  -- bind(1)
txn[] insert into attachment (id, goods_entity_id, name, version, when_created, when_modified, created_by, updated_by) values (?,?,?,?,?,?,?,?)
txn[]  -- bind(1,1,File1,1,2023-01-31 10:55:47.048,2023-01-31 10:55:47.048,null,null)
txn[]  -- bind(2,1,File2,1,2023-01-31 10:55:47.048,2023-01-31 10:55:47.048,null,null)
txn[] insert into attachment (goods_entity_id, name, version, when_created, when_modified, created_by, updated_by) values (?,?,?,?,?,?,?)
txn[]  -- bind(1,File3,1,2023-01-31 10:55:47.048,2023-01-31 10:55:47.048,null,null)

and works as expected

GoodsEntity.class have

  @OneToMany(cascade = CascadeType.ALL, orphanRemoval = true)
  private List<Attachment> attachments;

@rbygrave
Copy link
Member

Are you able to identify the version that introduced the behavior change. I believe you are saying that the test still passes with 12.1.1? If so, which version breaks the test?

@buchtajz
Copy link
Contributor Author

buchtajz commented Feb 1, 2023

I do some mystification in versions, i'm sorry.

11.17.5

  • we were used this version. Up to version 11.21.1 works.

setAttachments()

txn[1002] insert into attachment (goods_entity_id, name, version, when_created, when_modified, created_by, updated_by) values (?,?,?,?,?,?,?); --bind(1,File3,1,2023-02-01 09:30:20.622,2023-02-01 09:30:20.622,null,null)
txn[1002] update attachment set name=?, version=?, when_created=?, when_modified=?, created_by=?, updated_by=? where id=? and version=?; --bind(File1,2,2023-02-01 09:30:20.384,2023-02-01 09:30:20.622,null,null,1,1)
txn[1002] update attachment set name=?, version=?, when_created=?, when_modified=?, created_by=?, updated_by=? where id=? and version=?; --bind(File2,2,2023-02-01 09:30:20.396,2023-02-01 09:30:20.622,null,null,2,1)

clear()/addAll()

txn[1006] insert into attachment (goods_entity_id, name, version, when_created, when_modified, created_by, updated_by) values (?,?,?,?,?,?,?); --bind(1,File3,1,2023-02-01 11:07:19.819,2023-02-01 11:07:19.819,null,null)
txn[1006] update attachment set name=?, version=?, when_created=?, when_modified=?, created_by=?, updated_by=? where id=? and version=?; --bind(File1,2,2023-02-01 11:07:19.763,2023-02-01 11:07:19.819,null,null,1,1)
txn[1006] update attachment set name=?, version=?, when_created=?, when_modified=?, created_by=?, updated_by=? where id=? and version=?; --bind(File2,2,2023-02-01 11:07:19.765,2023-02-01 11:07:19.819,null,null,2,1)

11.22.1

setAttachments()

txn[1002] insert into attachment (id, goods_entity_id, name, version, when_created, when_modified, created_by, updated_by) values (?,?,?,?,?,?,?,?); --bind(1,1,File1,1,2023-02-01 09:58:57.099,2023-02-01 09:58:57.099,null,null)
txn[1002] insert into attachment (id, goods_entity_id, name, version, when_created, when_modified, created_by, updated_by) values (?,?,?,?,?,?,?,?); --bind(2,1,File2,1,2023-02-01 09:58:57.099,2023-02-01 09:58:57.099,null,null)
txn[1002] insert into attachment (goods_entity_id, name, version, when_created, when_modified, created_by, updated_by) values (?,?,?,?,?,?,?); --bind(1,File3,1,2023-02-01 09:58:57.099,2023-02-01 09:58:57.099,null,null)

clear()/addAll()

txn[1003] insert into attachment (id, goods_entity_id, name, version, when_created, when_modified, created_by, updated_by) values (?,?,?,?,?,?,?,?); --bind(1,1,File1,1,2023-02-01 09:58:57.132,2023-02-01 09:58:57.132,null,null)
txn[1003] insert into attachment (id, goods_entity_id, name, version, when_created, when_modified, created_by, updated_by) values (?,?,?,?,?,?,?,?); --bind(2,1,File2,1,2023-02-01 09:58:57.132,2023-02-01 09:58:57.132,null,null)
txn[1003] insert into attachment (goods_entity_id, name, version, when_created, when_modified, created_by, updated_by) values (?,?,?,?,?,?,?); --bind(1,File3,1,2023-02-01 09:58:57.132,2023-02-01 09:58:57.132,null,null)

@buchtajz
Copy link
Contributor Author

buchtajz commented Feb 1, 2023

one more info. I have some problems with testing 12.x,
but in version 13.0.0 there is working clear()/addAll (even there is delete + insert instead only update)

setAttachments()

txn[] insert into attachment (id, goods_entity_id, name, version, when_created, when_modified, created_by, updated_by) values (?,?,?,?,?,?,?,?)
txn[]  -- bind(1,1,File1,1,2023-02-01 10:49:14.938,2023-02-01 10:49:14.938,null,null)
txn[]  -- bind(2,1,File2,1,2023-02-01 10:49:14.938,2023-02-01 10:49:14.938,null,null)
txn[] insert into attachment (goods_entity_id, name, version, when_created, when_modified, created_by, updated_by) values (?,?,?,?,?,?,?)
txn[]  -- bind(1,File3,1,2023-02-01 10:49:14.938,2023-02-01 10:49:14.938,null,null)

clear()/addAll()

txn[] delete from attachment where id=?
txn[]  -- bind(1)
txn[]  -- bind(2)
txn[] insert into attachment (id, goods_entity_id, name, version, when_created, when_modified, created_by, updated_by) values (?,?,?,?,?,?,?,?)
txn[]  -- bind(1,1,File1,1,2023-02-01 10:49:14.972,2023-02-01 10:49:14.972,null,null)
txn[]  -- bind(2,1,File2,1,2023-02-01 10:49:14.973,2023-02-01 10:49:14.973,null,null)
txn[] insert into attachment (goods_entity_id, name, version, when_created, when_modified, created_by, updated_by) values (?,?,?,?,?,?,?)
txn[]  -- bind(1,File3,1,2023-02-01 10:49:14.973,2023-02-01 10:49:14.973,null,null)

@Ichtil
Copy link
Contributor

Ichtil commented Feb 6, 2023

While debugging a bit, I found a bit of strangeness in the class SaveManyBeans method removeAssocManyOrphans
line 346 if (!(value instanceof BeanCollection<?>)) {
and then line 348 many.deleteByParentId

  • if the objects in the many relationship stay the same, but the container is different (eg. when it is loaded from DB, it is BeanCollection, but then we replace that property with ArrayList with the same elements), this condition gets triggered and thus ebean tries to delete items by parent id (there is an easy workaround by using .clear() + .addAll(..) instead of .set(new))
  • the deleteByParentId is hard delete and does not care for soft delete

Please consider this unit test

  @Test
  void setManyAssocWithSameElementsShouldNotDelete() {
       var goods = new GoodsEntity();
    goods.setName("g");
    var workflow = new WorkflowEntity();
    goods.setWorkflowEntity(workflow);
    var operation = new WorkflowOperationEntity();
    operation.setName("o");
    workflow.setOperations(new ArrayList<>(List.of(operation)));
    DB.save(goods);

    var dbGoods = DB.find(GoodsEntity.class, goods.getId());
    var dbOperation = dbGoods.getWorkflowEntity().getOperations().get(0);
    dbGoods.getWorkflowEntity().setOperations(new ArrayList<>(List.of(dbOperation)));
    LoggedSql.start();
    // shouldn't delete existing operation
    // (and also shouldn't hard delete - WorkflowOperationEntity has soft delete)
    DB.update(dbGoods);
    var sql = LoggedSql.stop();
    sql.forEach(s -> assertThat(s).doesNotContain("delete from workflow_operation_entity"));
  }

rbygrave added a commit that referenced this issue Feb 9, 2023
orphanRemoval was not occurring when a loaded bean had a collection
replaced by a new BeanCollection (as opposed to a vanilla collection
like java.util.ArrayList).

Json marshalling a collection puts beans into BeanCollection and this
is part of the test that reproduced this issue.
@rbygrave rbygrave changed the title insert instead update on changing @OneToMany field @OneToMany orphanRemoval not occurring (was: insert instead update on changing @OneToMany field) Feb 9, 2023
@rbygrave
Copy link
Member

rbygrave commented Feb 9, 2023

if the objects in the many relationship stay the same, but the container is different ... and thus ebean tries to delete items by parent id

I have a refactor to merge the handling of this case into the other one so that we get back to:

  1. orphans explicitly from BeanCollection modifications
  2. orphans as everything NOT in the collection that is going to be updated

the deleteByParentId is hard delete and does not care for soft delete

Yeah, that's another bug there. I have a refactor to fix that by getting rid of that part of the code.

rbygrave added a commit that referenced this issue Feb 10, 2023
rbygrave added a commit that referenced this issue Feb 10, 2023
… + non-BeanCollection collection results in hard deletes

As noted in comments in #2952

In the internals of SaveManyBeans we have:

- BUG: the deleteByParentId is hard delete and does not care for soft delete
- YUK: internally we have 3 ways of performing the orphan removal when we really want 2

This change fixes the BUG and fixes the YUK. It does this by removing the special case at: https://github.com/ebean-orm/ebean/blob/ebean-parent-13.11.3/ebean-core/src/main/java/io/ebeaninternal/server/persist/SaveManyBeans.java#L347-L350 ... and replacing it with the more common orphan removal code used when we do not have BeanCollection modifications.

The result of this change is that in SaveManyBeans internals we get back to have 2 ways to remove orphans.

- A BeanCollection with modifications: Orphans explicitly deleted using the known elements removed from the collection
- All other cases: Orphans as everything NOT in the collection that is going to be updated
rbygrave added a commit that referenced this issue Feb 10, 2023
#2961 - Followup for #2952 - @onetomany + orphanRemoval + @SoftDelete + non-BeanCollection collection results in hard deletes
@rbygrave rbygrave self-assigned this Feb 10, 2023
@rbygrave rbygrave added this to the 13.11.4 milestone Feb 10, 2023
@rbygrave rbygrave added the bug label Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants