From bfd3840a81ae01cc9f2b8fce77edf2f7e814d5fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ad=C3=A1mek?= Date: Fri, 21 Apr 2023 23:25:05 +0200 Subject: [PATCH] fix(core): clean up bidirectional references after removal of entity Closes #4234 --- packages/core/src/entity/ArrayCollection.ts | 30 ++++- packages/core/src/entity/EntityHelper.ts | 13 ++- packages/core/src/errors.ts | 9 ++ packages/core/src/unit-of-work/UnitOfWork.ts | 15 ++- .../unit-of-work/GH4234-one-to-many.test.ts | 103 +++++++++++++++++ .../unit-of-work/GH4234-one-to-one.test.ts | 104 ++++++++++++++++++ tests/issues/GH3543.test.ts | 29 ++--- 7 files changed, 269 insertions(+), 34 deletions(-) create mode 100644 tests/features/unit-of-work/GH4234-one-to-many.test.ts create mode 100644 tests/features/unit-of-work/GH4234-one-to-one.test.ts diff --git a/packages/core/src/entity/ArrayCollection.ts b/packages/core/src/entity/ArrayCollection.ts index 538bcf21f31b..7fefa5f1fe8c 100644 --- a/packages/core/src/entity/ArrayCollection.ts +++ b/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 { @@ -80,10 +80,30 @@ export class ArrayCollection { } set(items: (T | Reference)[]): 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 { 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; diff --git a/packages/core/src/entity/EntityHelper.ts b/packages/core/src/entity/EntityHelper.ts index 1fb1ddc5ed28..06b1ed5075aa 100644 --- a/packages/core/src/entity/EntityHelper.ts +++ b/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); + } } } } diff --git a/packages/core/src/errors.ts b/packages/core/src/errors.ts index 3469598bff78..4821fa1f9f74 100644 --- a/packages/core/src/errors.ts +++ b/packages/core/src/errors.ts @@ -94,6 +94,15 @@ export class ValidationError 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}.`); } diff --git a/packages/core/src/unit-of-work/UnitOfWork.ts b/packages/core/src/unit-of-work/UnitOfWork.ts index 7ebdcf22a0ca..3c2724977c6c 100644 --- a/packages/core/src/unit-of-work/UnitOfWork.ts +++ b/packages/core/src/unit-of-work/UnitOfWork.ts @@ -258,7 +258,7 @@ export class UnitOfWork { } persist(entity: T, visited?: Set, 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(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; } diff --git a/tests/features/unit-of-work/GH4234-one-to-many.test.ts b/tests/features/unit-of-work/GH4234-one-to-many.test.ts new file mode 100644 index 000000000000..c00d77fba508 --- /dev/null +++ b/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(this); + + constructor(params: RequiredEntityData) { + Object.assign(this, params); + } + +} + +@Entity() +class GroupMember { + + @ManyToOne({ + entity: () => Member, + primary: true, + ref: true, + }) + member!: Ref; + + @ManyToOne({ + entity: () => Group, + primary: true, + ref: true, + }) + group!: Ref; + + constructor(params: RequiredEntityData) { + Object.assign(this, params); + } + +} + +@Entity() +class Member { + + @PrimaryKey() + id!: number; + + @OneToMany({ + entity: () => GroupMember, + mappedBy: group => group.member, + eager: true, + orphanRemoval: true, + }) + groups = new Collection(this); + + constructor(params: RequiredEntityData) { + 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(); +}); diff --git a/tests/features/unit-of-work/GH4234-one-to-one.test.ts b/tests/features/unit-of-work/GH4234-one-to-one.test.ts new file mode 100644 index 000000000000..755c8f1a6da2 --- /dev/null +++ b/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; + + constructor(params: RequiredEntityData) { + Object.assign(this, params); + } + +} + +@Entity() +class GroupMember { + + @OneToOne({ + entity: () => Member, + primary: true, + ref: true, + }) + member!: Ref; + + @OneToOne({ + entity: () => Group, + primary: true, + ref: true, + }) + group!: Ref; + + constructor(params: RequiredEntityData) { + 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; + + constructor(params: RequiredEntityData) { + 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); +}); diff --git a/tests/issues/GH3543.test.ts b/tests/issues/GH3543.test.ts index 93cb2d032ca7..cf727ff3926d 100644 --- a/tests/issues/GH3543.test.ts +++ b/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(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', + ); });