fix(core): do not cascade persist removed entities

Previously when removed entity was part of a collection, it was wrongly cascade persisted again, resulting in insert query instead of delete.

Closes #369

lib/unit-of-work/ChangeSetComputer.ts

@@ -11,6 +11,7 @@ export class ChangeSetComputer {
               private readonly originalEntityData: Record<string, EntityData<AnyEntity>>,
               private readonly identifierMap: Record<string, EntityIdentifier>,
               private readonly collectionUpdates: Collection<AnyEntity>[],
+              private readonly removeStack: AnyEntity[],
               private readonly metadata: MetadataStorage,
               private readonly platform: Platform) { }
 
@@ -49,6 +50,13 @@ export class ChangeSetComputer {
   private processReference<T extends AnyEntity<T>>(changeSet: ChangeSet<T>, prop: EntityProperty<T>): void {
     const isToOneOwner = prop.reference === ReferenceType.MANY_TO_ONE || (prop.reference === ReferenceType.ONE_TO_ONE && prop.owner);
 
+    if ([ReferenceType.ONE_TO_MANY, ReferenceType.MANY_TO_MANY].includes(prop.reference) && (changeSet.entity[prop.name] as unknown as Collection<T>).isInitialized()) {
+      const collection = changeSet.entity[prop.name] as unknown as Collection<AnyEntity>;
+      collection.getItems()
+        .filter(item => this.removeStack.includes(item))
+        .forEach(item => collection.remove(item));
+    }
+
     if (prop.reference === ReferenceType.MANY_TO_MANY && prop.owner && (changeSet.entity[prop.name] as unknown as Collection<T>).isDirty()) {
       this.collectionUpdates.push(changeSet.entity[prop.name] as unknown as Collection<AnyEntity>);
     } else if (isToOneOwner && changeSet.entity[prop.name]) {

lib/unit-of-work/UnitOfWork.ts

@@ -26,7 +26,7 @@ export class UnitOfWork {
   private readonly extraUpdates: [AnyEntity, string, AnyEntity | Reference<AnyEntity>][] = [];
   private readonly metadata = this.em.getMetadata();
   private readonly platform = this.em.getDriver().getPlatform();
-  private readonly changeSetComputer = new ChangeSetComputer(this.em.getValidator(), this.originalEntityData, this.identifierMap, this.collectionUpdates, this.metadata, this.platform);
+  private readonly changeSetComputer = new ChangeSetComputer(this.em.getValidator(), this.originalEntityData, this.identifierMap, this.collectionUpdates, this.removeStack, this.metadata, this.platform);
   private readonly changeSetPersister = new ChangeSetPersister(this.em.getDriver(), this.identifierMap, this.metadata);
 
   constructor(private readonly em: EntityManager) { }
@@ -67,18 +67,22 @@ export class UnitOfWork {
     return this.identityMap;
   }
 
-  persist<T extends AnyEntity<T>>(entity: T, visited: AnyEntity[] = []): void {
+  persist<T extends AnyEntity<T>>(entity: T, visited: AnyEntity[] = [], checkRemoveStack = false): void {
     if (this.persistStack.includes(entity)) {
       return;
     }
 
+    if (checkRemoveStack && this.removeStack.includes(entity)) {
+      return;
+    }
+
     if (!wrap(entity).__primaryKey) {
       this.identifierMap[wrap(entity).__uuid] = new EntityIdentifier();
     }
 
     this.persistStack.push(entity);
     this.cleanUpStack(this.removeStack, entity);
-    this.cascade(entity, Cascade.PERSIST, visited);
+    this.cascade(entity, Cascade.PERSIST, visited, checkRemoveStack);
   }
 
   remove(entity: AnyEntity, visited: AnyEntity[] = []): void {
@@ -145,7 +149,7 @@ export class UnitOfWork {
 
     Object.values(this.identityMap)
       .filter(entity => !this.removeStack.includes(entity) && !this.orphanRemoveStack.includes(entity))
-      .forEach(entity => this.persist(entity));
+      .forEach(entity => this.persist(entity, [], true));
 
     while (this.persistStack.length) {
       this.findNewEntities(this.persistStack.shift()!);
@@ -294,27 +298,27 @@ export class UnitOfWork {
     this.extraUpdates.length = 0;
   }
 
-  private cascade<T extends AnyEntity<T>>(entity: T, type: Cascade, visited: AnyEntity[]): void {
+  private cascade<T extends AnyEntity<T>>(entity: T, type: Cascade, visited: AnyEntity[], checkRemoveStack = false): void {
     if (visited.includes(entity)) {
       return;
     }
 
     visited.push(entity);
 
     switch (type) {
-      case Cascade.PERSIST: this.persist(entity, visited); break;
+      case Cascade.PERSIST: this.persist(entity, visited, checkRemoveStack); break;
       case Cascade.MERGE: this.merge(entity, visited); break;
       case Cascade.REMOVE: this.remove(entity, visited); break;
     }
 
     const meta = this.metadata.get<T>(entity.constructor.name);
 
     for (const prop of Object.values<EntityProperty>(meta.properties).filter(prop => prop.reference !== ReferenceType.SCALAR)) {
-      this.cascadeReference<T>(entity, prop, type, visited);
+      this.cascadeReference<T>(entity, prop, type, visited, checkRemoveStack);
     }
   }
 
-  private cascadeReference<T extends AnyEntity<T>>(entity: T, prop: EntityProperty<T>, type: Cascade, visited: AnyEntity[]): void {
+  private cascadeReference<T extends AnyEntity<T>>(entity: T, prop: EntityProperty<T>, type: Cascade, visited: AnyEntity[], checkRemoveStack: boolean): void {
     this.fixMissingReference(entity, prop);
 
     if (!this.shouldCascade(prop, type)) {
@@ -324,14 +328,14 @@ export class UnitOfWork {
     const reference = this.unwrapReference(entity, prop);
 
     if ([ReferenceType.MANY_TO_ONE, ReferenceType.ONE_TO_ONE].includes(prop.reference) && reference) {
-      return this.cascade(reference as T, type, visited);
+      return this.cascade(reference as T, type, visited, checkRemoveStack);
     }
 
     const collection = reference as Collection<AnyEntity>;
     const requireFullyInitialized = type === Cascade.PERSIST; // only cascade persist needs fully initialized items
 
     if ([ReferenceType.ONE_TO_MANY, ReferenceType.MANY_TO_MANY].includes(prop.reference) && collection && collection.isInitialized(requireFullyInitialized)) {
-      collection.getItems().forEach(item => this.cascade(item, type, visited));
+      collection.getItems().forEach(item => this.cascade(item, type, visited, checkRemoveStack));
     }
   }
 

tests/issues/GH369.test.ts

@@ -0,0 +1,74 @@
+import { unlinkSync } from 'fs';
+import { Entity, ManyToOne, OneToMany, PrimaryKey, Property, Collection, MikroORM, ReflectMetadataProvider } from '../../lib';
+import { SqliteDriver } from '../../lib/drivers/SqliteDriver';
+import { Logger } from '../../lib/utils';
+
+@Entity()
+class A {
+
+  @PrimaryKey()
+  id!: number;
+
+  @OneToMany(() => B, b => b.a)
+  bItems = new Collection<B>(this);
+
+}
+
+@Entity()
+class B {
+
+  @PrimaryKey()
+  id!: number;
+
+  @ManyToOne(() => A)
+  a!: A;
+
+  @Property()
+  foo: string = 'bar';
+
+}
+
+describe('GH issue 369', () => {
+
+  let orm: MikroORM<SqliteDriver>;
+
+  beforeAll(async () => {
+    orm = await MikroORM.init({
+      entities: [A, B],
+      dbName: __dirname + '/../../temp/mikro_orm_test_gh228.db',
+      debug: false,
+      highlight: false,
+      type: 'sqlite',
+      metadataProvider: ReflectMetadataProvider,
+      cache: { enabled: false },
+    });
+    await orm.getSchemaGenerator().dropSchema();
+    await orm.getSchemaGenerator().createSchema();
+  });
+
+  afterAll(async () => {
+    await orm.close(true);
+    unlinkSync(orm.config.get('dbName'));
+  });
+
+  test(`removing entity that is inside a 1:m collection`, async () => {
+    const mock = jest.fn();
+    const logger = new Logger(mock, ['query']);
+    Object.assign(orm.config, { logger });
+
+    const a = new A();
+    const b = new B();
+    b.a = a;
+    await orm.em.persistAndFlush(b);
+    await orm.em.removeAndFlush(b);
+    expect(mock.mock.calls[0][0]).toMatch('begin');
+    expect(mock.mock.calls[1][0]).toMatch('insert into `a` default values');
+    expect(mock.mock.calls[2][0]).toMatch('insert into `b` (`a_id`, `foo`) values (?, ?)');
+    expect(mock.mock.calls[3][0]).toMatch('commit');
+    expect(mock.mock.calls[4][0]).toMatch('begin');
+    expect(mock.mock.calls[5][0]).toMatch('delete from `b` where `id` = ?');
+    expect(mock.mock.calls[6][0]).toMatch('commit');
+    expect(mock.mock.calls).toHaveLength(7);
+  });
+
+});