EntityAssigner not marking removed items as deleted

[ballwood]

Describe the bug
Hi,

We have found an issue in EntityAssigner where items in a collection are not being marked as removed. We are using cascade.ALL and orphanRemoval: true

assignCollection() uses hydrate internally to replace the values in the collection hydrate() calls this.items.clear() which removes the items but does not inform whatever is tracking the entity that it has been removed therefore removing orphans does not work.

We have patched the code internally to use this.set() when we persist the entity with the collection it removes the orphaned value successfully but don't know a huge amount about the codebase so you might want to implement differently.

Line: https://github.com/mikro-orm/mikro-orm/blob/master/packages/core/src/entity/ArrayCollection.ts#L86

Patch

  hydrate(items: T[]): void {
    this.set(items);
  }

Expected behavior
When using assign if an entity is removed from a collection it should be deleted.

Versions

Dependency	Version
node	latest
typescript	latest
mikro-orm	latest
your-driver	mysql

[B4nan]

I'd be careful with such change, hydrate and set are different things. Hydration should not trigger orphan removal or cascading, so the issue might be more about how the EntityAssigner works rather than how Collection.hydrate works.

But to confirm that, we need to see... a reproduction :]

[ballwood]

No problem, I'll put one together :)

[B4nan]

I was thinking about this for some time, I'd say correct fix is to change EntityAssigner.assignCollection to do:

    if (collection.isInitialized() && !options.merge) {
      collection.set(items);
    } else {
      collection.hydrate(items, true, !!options.merge);
      collection.setDirty();
    }

set should be used only on loaded collections, and only when we do not have merging enabled

[B4nan]

I was a bit confused by the merge option of assign, but turns out we should actually just swap the hydrate with set inside the assigner. Calling set is fine for not initialized collections, and merge flag here is only about entity references so it should not matter.

Would be good to have more tests for assigning collections, so if you can craft something, please send a PR.

[ballwood]

Thanks the for the quick fix - I'll have a crack at that now. Me and my colleague found a few more edge cases around using EntityAssigner on Friday with nested data/collections.

We're using graphql so want to create some DTO's with a subset of the props of Entity in type-graphql and just do an assign so we don't have to muck around with lots of mapping functions. We have a complex data model - so always teases out bugs in any orm we use.

While I don't have really the knowledge to fix some of these I can definitely create a suite of tests to exhibit the behaviour. I'll put something up 👍 really been enjoying using mikro-orm, great work.

[B4nan]

with nested data/collections.

One thing to note upfront - assign is not working recursively. There is an open issue for that, although still without a reproduction.

I feel like people expect too much magic from it. It's just a helper that works like Object.assign(), but handles ORM specific invariants (references, collections).

Would be great to have more tests for that for sure.

[ballwood]

@B4nan I've added some tests here - #1052