fix(core): clean up bidirectional references after removal of entity

Closes #4234

packages/core/src/entity/ArrayCollection.ts

@@ -3,7 +3,7 @@ import type { EntityDTO, EntityProperty, IPrimaryKey, Primary } from '../typings
 import { Reference } from './Reference';
 import { helper, wrap } from './wrap';
 import { ReferenceType } from '../enums';
-import { MetadataError } from '../errors';
+import { MetadataError, ValidationError } from '../errors';
 import { Utils } from '../utils/Utils';
 
 export class ArrayCollection<T extends object, O extends object> {
@@ -80,10 +80,30 @@ export class ArrayCollection<T extends object, O extends object> {
   }
 
   set(items: (T | Reference<T>)[]): void {
+    if (this.compare(items.map(item => Reference.unwrapReference(item)))) {
+      return;
+    }
+
     this.removeAll();
     this.add(items);
   }
 
+  private compare(items: T[]): boolean {
+    if (items.length !== this.items.size) {
+      return false;
+    }
+
+    let idx = 0;
+
+    for (const item of this.items) {
+      if (item !== items[idx++]) {
+        return false;
+      }
+    }
+
+    return true;
+  }
+
   /**
    * @internal
    */
@@ -245,6 +265,14 @@ export class ArrayCollection<T extends object, O extends object> {
         helper(item).__pk = helper(item).getPrimaryKey()!;
       }
 
+      if (!prop2.nullable && prop2.onDelete !== 'cascade' && method === 'remove') {
+        if (!this.property.orphanRemoval) {
+          throw ValidationError.cannotRemoveFromCollectionWithoutOrphanRemoval(this.owner, this.property);
+        }
+
+        return;
+      }
+
       // skip if already propagated
       if (Reference.unwrapReference(item[this.property.mappedBy]) !== value) {
         item[this.property.mappedBy] = value;

packages/core/src/entity/EntityHelper.ts

@@ -184,12 +184,13 @@ export class EntityHelper {
         inverse.add(owner);
       }
 
-      if (prop.reference === ReferenceType.ONE_TO_ONE && entity && helper(entity).__initialized && Reference.unwrapReference(inverse) !== owner && value != null) {
-        EntityHelper.propagateOneToOne(entity, owner, prop, prop2, value, old);
-      }
-
-      if (prop.reference === ReferenceType.ONE_TO_ONE && entity && helper(entity).__initialized && entity[prop2.name] != null && value == null) {
-        EntityHelper.propagateOneToOne(entity, owner, prop, prop2, value, old as T);
+      if (prop.reference === ReferenceType.ONE_TO_ONE && entity && helper(entity).__initialized) {
+        if (
+          (value != null && Reference.unwrapReference(inverse) !== owner) ||
+          (value == null && entity[prop2.name] != null)
+        ) {
+          EntityHelper.propagateOneToOne(entity, owner, prop, prop2, value, old as T);
+        }
       }
     }
   }

packages/core/src/errors.ts

@@ -94,6 +94,15 @@ export class ValidationError<T extends AnyEntity = AnyEntity> extends Error {
     return new ValidationError(`You cannot modify collection ${owner.constructor.name}.${property.name} as it is marked as readonly.`, owner);
   }
 
+  static cannotRemoveFromCollectionWithoutOrphanRemoval(owner: AnyEntity, property: EntityProperty): ValidationError {
+    const options = [
+      ' - add `orphanRemoval: true` to the collection options',
+      ' - add `onDelete: \'cascade\'` to the owning side options',
+      ' - add `nullable: true` to the owning side options',
+    ].join('\n');
+    return new ValidationError(`Removing items from collection ${owner.constructor.name}.${property.name} without \`orphanRemoval: true\` would break non-null constraint on the owning side. You have several options: \n${options}`, owner);
+  }
+
   static invalidCompositeIdentifier(meta: EntityMetadata): ValidationError {
     return new ValidationError(`Composite key required for entity ${meta.className}.`);
   }

packages/core/src/unit-of-work/UnitOfWork.ts

@@ -258,7 +258,7 @@ export class UnitOfWork {
   }
 
   persist<T extends object>(entity: T, visited?: Set<AnyEntity>, options: { checkRemoveStack?: boolean; cascade?: boolean } = {}): void {
-    if (options.checkRemoveStack && (this.removeStack.has(entity))) {
+    if (options.checkRemoveStack && this.removeStack.has(entity)) {
       return;
     }
 
@@ -394,12 +394,17 @@ export class UnitOfWork {
       const rel = Reference.unwrapReference(entity[prop.name]);
 
       if (Utils.isCollection(rel) && rel.isInitialized()) {
-        rel.removeAll();
+        rel.getItems(false).forEach(item => rel.removeWithoutPropagation(item));
+        continue;
+      }
+
+      if (Utils.isCollection(rel?.[inverse]) && rel[inverse].isInitialized()) {
+        rel[inverse].removeWithoutPropagation(entity);
         continue;
       }
 
       // there is a value, and it is still a self-reference (e.g. not replaced by user manually)
-      if (rel?.[inverse] && entity === rel[inverse]) {
+      if (rel?.[inverse] && entity === Reference.unwrapReference(rel[inverse])) {
         delete helper(rel).__data[inverse];
       }
     }
@@ -569,7 +574,7 @@ export class UnitOfWork {
   }
 
   private initIdentifier<T extends object>(entity: T): void {
-    const wrapped = helper(entity);
+    const wrapped = entity && helper(entity);
 
     if (!wrapped || wrapped.__identifier || wrapped.hasPrimaryKey()) {
       return;
@@ -579,7 +584,7 @@ export class UnitOfWork {
 
     if (pk.reference === ReferenceType.SCALAR) {
       wrapped.__identifier = new EntityIdentifier();
-    } else {
+    } else if (entity[pk.name]) {
       this.initIdentifier(entity[pk.name] as object);
       wrapped.__identifier = helper(entity[pk.name] as AnyEntity)?.__identifier;
     }

tests/features/unit-of-work/GH4234-one-to-many.test.ts

@@ -0,0 +1,103 @@
+import { Collection, Entity, ManyToOne, OneToMany, PrimaryKey, Ref, RequiredEntityData } from '@mikro-orm/core';
+import { MikroORM } from '@mikro-orm/sqlite';
+
+@Entity()
+class Group {
+
+  @PrimaryKey()
+  id!: number;
+
+  @OneToMany({
+    entity: () => GroupMember,
+    mappedBy: member => member.group,
+    eager: true,
+    orphanRemoval: true,
+  })
+  members = new Collection<GroupMember>(this);
+
+  constructor(params: RequiredEntityData<Group>) {
+    Object.assign(this, params);
+  }
+
+}
+
+@Entity()
+class GroupMember {
+
+  @ManyToOne({
+    entity: () => Member,
+    primary: true,
+    ref: true,
+  })
+  member!: Ref<Member>;
+
+  @ManyToOne({
+    entity: () => Group,
+    primary: true,
+    ref: true,
+  })
+  group!: Ref<Group>;
+
+  constructor(params: RequiredEntityData<GroupMember>) {
+    Object.assign(this, params);
+  }
+
+}
+
+@Entity()
+class Member {
+
+  @PrimaryKey()
+  id!: number;
+
+  @OneToMany({
+    entity: () => GroupMember,
+    mappedBy: group => group.member,
+    eager: true,
+    orphanRemoval: true,
+  })
+  groups = new Collection<GroupMember>(this);
+
+  constructor(params: RequiredEntityData<Member>) {
+    Object.assign(this, params);
+  }
+
+}
+
+let orm: MikroORM;
+
+beforeAll(async () => {
+  orm = await MikroORM.init({
+    entities: [Group, Member, GroupMember],
+    dbName: ':memory:',
+  });
+  await orm.schema.refreshDatabase();
+});
+
+afterAll(() => orm.close(true));
+
+test(`GH issue 4234 with 1:m`, async () => {
+  const groupId = 1;
+  const memberId = 2;
+
+  {
+    const group = new Group({ id: groupId });
+    const member = new Member({ id: memberId });
+    member.groups.add(new GroupMember({ group, member }));
+    await orm.em.persist([group, member]).flush();
+    orm.em.clear();
+  }
+
+  {
+    await orm.em.findOne(Group, groupId);
+    const member = await orm.em.findOneOrFail(Member, memberId);
+    orm.em.assign(member, { groups: [] });
+    await orm.em.flush();
+    await orm.em.flush();
+  }
+
+  orm.em.clear();
+  await orm.em.findOneOrFail(Member, memberId);
+  await orm.em.findOneOrFail(Group, groupId);
+  await expect(orm.em.findOne(GroupMember, { group: groupId, member: memberId })).resolves.toBeNull();
+});

tests/features/unit-of-work/GH4234-one-to-one.test.ts

@@ -0,0 +1,104 @@
+import { Entity, OneToOne, PrimaryKey, ref, Ref, RequiredEntityData } from '@mikro-orm/core';
+import { MikroORM } from '@mikro-orm/sqlite';
+
+@Entity()
+class Group {
+
+  @PrimaryKey()
+  id!: number;
+
+  @OneToOne({
+    entity: () => GroupMember,
+    mappedBy: member => member.group,
+    ref: true,
+    eager: true,
+    orphanRemoval: true,
+  })
+  groupMember?: Ref<GroupMember>;
+
+  constructor(params: RequiredEntityData<Group>) {
+    Object.assign(this, params);
+  }
+
+}
+
+@Entity()
+class GroupMember {
+
+  @OneToOne({
+    entity: () => Member,
+    primary: true,
+    ref: true,
+  })
+  member!: Ref<Member>;
+
+  @OneToOne({
+    entity: () => Group,
+    primary: true,
+    ref: true,
+  })
+  group!: Ref<Group>;
+
+  constructor(params: RequiredEntityData<GroupMember>) {
+    Object.assign(this, params);
+  }
+
+}
+
+@Entity()
+class Member {
+
+  @PrimaryKey()
+  id!: number;
+
+  @OneToOne({
+    entity: () => GroupMember,
+    mappedBy: group => group.member,
+    eager: true,
+    ref: true,
+    orphanRemoval: true,
+  })
+  groupMember?: Ref<GroupMember>;
+
+  constructor(params: RequiredEntityData<Member>) {
+    Object.assign(this, params);
+  }
+
+}
+
+let orm: MikroORM;
+
+beforeAll(async () => {
+  orm = await MikroORM.init({
+    entities: [Group, Member, GroupMember],
+    dbName: ':memory:',
+  });
+  await orm.schema.refreshDatabase();
+});
+
+afterAll(() => orm.close(true));
+
+test(`GH issue 4234 with 1:1`, async () => {
+  const groupId = 1;
+  const memberId = 2;
+
+  {
+    const group = new Group({ id: groupId });
+    const member = new Member({ id: memberId });
+    member.groupMember = ref(new GroupMember({ group, member }));
+    await orm.em.persist([group, member]).flush();
+    orm.em.clear();
+  }
+
+  {
+    await orm.em.findOne(Group, groupId);
+    const member = await orm.em.findOneOrFail(Member, memberId);
+    orm.em.assign(member, { groupMember: null });
+    await orm.em.flush();
+    await orm.em.flush();
+  }
+
+  orm.em.clear();
+  await orm.em.findOneOrFail(Member, memberId);
+  await orm.em.findOneOrFail(Group, groupId);
+});

tests/issues/GH3543.test.ts

@@ -30,25 +30,6 @@ class Order {
 
 }
 
-@Entity({ tableName: 'order' })
-class Order2 {
-
-  [OptionalProps]?: 'orderId';
-
-  @PrimaryKey()
-  orderId: string = v4();
-
-  @PrimaryKey()
-  customerId!: string;
-
-  @PrimaryKey()
-  companyId!: string;
-
-  @OneToMany(() => OrderEvent, orderEvent => orderEvent.order)
-  events = new Collection<OrderEvent>(this);
-
-}
-
 @Entity()
 class OrderEvent {
 
@@ -131,7 +112,11 @@ test('GH issue 3543 without orphan removal builds correct query', async () => {
     orderId: order.orderId,
   }, { populate: true });
 
-  // disconnecting the relation without orphan removal means nulling it on the owning side, which fails as it is a non-null PK column
-  order.events.removeAll();
-  await expect(orm.em.flush()).rejects.toThrowError(NotNullConstraintViolationException);
+  // disconnecting the relation without orphan removal throws, as it means nulling it on the owning side, which would fail as it is a non-null PK column
+  expect(() => order.events.removeAll()).toThrowError(
+    'Removing items from collection Order.events without `orphanRemoval: true` would break non-null constraint on the owning side. You have several options: \n' +
+    ' - add `orphanRemoval: true` to the collection options\n' +
+    ' - add `onDelete: \'cascade\'` to the owning side options\n' +
+    ' - add `nullable: true` to the owning side options',
+  );
 });