From 8b7752545654b5a60cbc6eaf4f12e0b91e4d5cea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ad=C3=A1mek?= Date: Tue, 27 Oct 2020 15:03:49 +0100 Subject: [PATCH] fix(core): ensure correct grouping and commit order for STI STI entities were not grouped by the root entity, and the commit order did not respect relations in some cases (e.g. when the target is a root entity). Related: #845 --- packages/core/src/metadata/EntitySchema.ts | 1 + .../core/src/metadata/MetadataDiscovery.ts | 2 ++ packages/core/src/unit-of-work/ChangeSet.ts | 16 +++++++++++++- .../src/unit-of-work/ChangeSetComputer.ts | 7 ++---- .../src/unit-of-work/CommitOrderCalculator.ts | 17 +++++++++++++- packages/core/src/unit-of-work/UnitOfWork.ts | 22 +++++-------------- packages/knex/src/schema/SchemaGenerator.ts | 18 +++------------ tests/issues/GH845.test.ts | 12 +++++++++- tests/single-table-inheritance.mysql.test.ts | 15 +++++++------ 9 files changed, 63 insertions(+), 47 deletions(-) diff --git a/packages/core/src/metadata/EntitySchema.ts b/packages/core/src/metadata/EntitySchema.ts index cc033f72a99d..14504422df58 100644 --- a/packages/core/src/metadata/EntitySchema.ts +++ b/packages/core/src/metadata/EntitySchema.ts @@ -40,6 +40,7 @@ export class EntitySchema = AnyEntity, U extends AnyEntit } Object.assign(this._meta, { className: meta.name }, meta); + this._meta.root = this._meta.root ?? this._meta; } static fromMetadata = AnyEntity, U extends AnyEntity | undefined = undefined>(meta: EntityMetadata | DeepPartial>): EntitySchema { diff --git a/packages/core/src/metadata/MetadataDiscovery.ts b/packages/core/src/metadata/MetadataDiscovery.ts index 49985277e23c..9166c3b3ce04 100644 --- a/packages/core/src/metadata/MetadataDiscovery.ts +++ b/packages/core/src/metadata/MetadataDiscovery.ts @@ -395,6 +395,7 @@ export class MetadataDiscovery { private definePivotTableEntity(meta: EntityMetadata, prop: EntityProperty): EntityMetadata { const data = new EntityMetadata({ name: prop.pivotTable, + className: prop.pivotTable, collection: prop.pivotTable, pivotTable: true, }); @@ -460,6 +461,7 @@ export class MetadataDiscovery { } as EntityProperty; const meta = this.metadata.get(type); + ret.targetMeta = meta; ret.joinColumns = []; ret.inverseJoinColumns = []; ret.referencedTableName = meta.collection; diff --git a/packages/core/src/unit-of-work/ChangeSet.ts b/packages/core/src/unit-of-work/ChangeSet.ts index 5514cab167b2..fd7da15da243 100644 --- a/packages/core/src/unit-of-work/ChangeSet.ts +++ b/packages/core/src/unit-of-work/ChangeSet.ts @@ -1,7 +1,21 @@ -import { EntityData, AnyEntity } from '../typings'; +import { EntityData, AnyEntity, EntityMetadata } from '../typings'; + +export class ChangeSet> { + + constructor(public entity: T, + public type: ChangeSetType, + public payload: EntityData, + meta: EntityMetadata) { + this.name = meta.className; + this.rootName = meta.root.className; + this.collection = meta.root.collection; + } + +} export interface ChangeSet> { name: string; + rootName: string; collection: string; type: ChangeSetType; entity: T; diff --git a/packages/core/src/unit-of-work/ChangeSetComputer.ts b/packages/core/src/unit-of-work/ChangeSetComputer.ts index b3d730c788cc..585d1d74831f 100644 --- a/packages/core/src/unit-of-work/ChangeSetComputer.ts +++ b/packages/core/src/unit-of-work/ChangeSetComputer.ts @@ -19,17 +19,14 @@ export class ChangeSetComputer { private readonly config: Configuration) { } computeChangeSet>(entity: T): ChangeSet | null { - const changeSet = { entity } as ChangeSet; const meta = this.metadata.find(entity.constructor.name)!; if (meta.readonly) { return null; } - changeSet.name = meta.name!; - changeSet.type = entity.__helper!.__originalEntityData ? ChangeSetType.UPDATE : ChangeSetType.CREATE; - changeSet.collection = meta.collection; - changeSet.payload = this.computePayload(entity); + const type = entity.__helper!.__originalEntityData ? ChangeSetType.UPDATE : ChangeSetType.CREATE; + const changeSet = new ChangeSet(entity, type, this.computePayload(entity), meta); if (changeSet.type === ChangeSetType.UPDATE) { changeSet.originalEntity = entity.__helper!.__originalEntityData; diff --git a/packages/core/src/unit-of-work/CommitOrderCalculator.ts b/packages/core/src/unit-of-work/CommitOrderCalculator.ts index 860aec6ecbc9..aa72eeaf107d 100644 --- a/packages/core/src/unit-of-work/CommitOrderCalculator.ts +++ b/packages/core/src/unit-of-work/CommitOrderCalculator.ts @@ -1,4 +1,5 @@ -import { Dictionary } from '../typings'; +import { Dictionary, EntityProperty } from '../typings'; +import { ReferenceType } from '../enums'; export const enum NodeState { NOT_VISITED = 0, @@ -56,6 +57,20 @@ export class CommitOrderCalculator { this.nodes[from].dependencies[to] = { from, to, weight }; } + discoverProperty(prop: EntityProperty, entityName: string): void { + if (!(prop.reference === ReferenceType.ONE_TO_ONE && prop.owner) && prop.reference !== ReferenceType.MANY_TO_ONE) { + return; + } + + const propertyType = prop.targetMeta?.root.className; + + if (!propertyType || !this.hasNode(propertyType)) { + return; + } + + this.addDependency(propertyType, entityName, prop.nullable ? 0 : 1); + } + /** * Return a valid order list of all current nodes. * The desired topological sorting is the reverse post order of these searches. diff --git a/packages/core/src/unit-of-work/UnitOfWork.ts b/packages/core/src/unit-of-work/UnitOfWork.ts index 656f2e62a497..23e98f5937af 100644 --- a/packages/core/src/unit-of-work/UnitOfWork.ts +++ b/packages/core/src/unit-of-work/UnitOfWork.ts @@ -280,7 +280,7 @@ export class UnitOfWork { for (const entity of this.removeStack) { const meta = this.metadata.find(entity.constructor.name)!; - this.changeSets.set(entity, { entity, type: ChangeSetType.DELETE, name: meta.name, collection: meta.collection, payload: {} } as ChangeSet); + this.changeSets.set(entity, new ChangeSet(entity, ChangeSetType.DELETE, {}, meta)); } } @@ -645,8 +645,8 @@ export class UnitOfWork { this.changeSets.forEach(cs => { const group = groups[cs.type]; - group[cs.name] = group[cs.name] ?? []; - group[cs.name].push(cs); + group[cs.rootName] = group[cs.rootName] ?? []; + group[cs.rootName].push(cs); }); return groups; @@ -655,28 +655,16 @@ export class UnitOfWork { private getCommitOrder(): string[] { const calc = new CommitOrderCalculator(); const set = new Set(); - this.changeSets.forEach(cs => set.add(cs.name)); + this.changeSets.forEach(cs => set.add(cs.rootName)); set.forEach(entityName => calc.addNode(entityName)); for (const entityName of set) { for (const prop of this.metadata.find(entityName)!.props) { - if (!calc.hasNode(prop.type)) { - continue; - } - - this.addCommitDependency(calc, prop, entityName); + calc.discoverProperty(prop, entityName); } } return calc.sort(); } - private addCommitDependency(calc: CommitOrderCalculator, prop: EntityProperty, entityName: string): void { - if (!(prop.reference === ReferenceType.ONE_TO_ONE && prop.owner) && prop.reference !== ReferenceType.MANY_TO_ONE) { - return; - } - - calc.addDependency(prop.type, entityName, prop.nullable ? 0 : 1); - } - } diff --git a/packages/knex/src/schema/SchemaGenerator.ts b/packages/knex/src/schema/SchemaGenerator.ts index 45a266d02922..ad11f60e4228 100644 --- a/packages/knex/src/schema/SchemaGenerator.ts +++ b/packages/knex/src/schema/SchemaGenerator.ts @@ -580,20 +580,16 @@ export class SchemaGenerator { private getOrderedMetadata(): EntityMetadata[] { const metadata = Object.values(this.metadata.getAll()).filter(meta => { - const isRootEntity = !meta.root || meta.root === meta; + const isRootEntity = meta.root.className === meta.className; return isRootEntity && !meta.embeddable; }); const calc = new CommitOrderCalculator(); - metadata.forEach(meta => calc.addNode(meta.name!)); + metadata.forEach(meta => calc.addNode(meta.root.className)); let meta = metadata.pop(); while (meta) { for (const prop of meta.props) { - if (!calc.hasNode(prop.type)) { - continue; - } - - this.addCommitDependency(calc, prop, meta.name!); + calc.discoverProperty(prop, meta.root.className); } meta = metadata.pop(); @@ -602,14 +598,6 @@ export class SchemaGenerator { return calc.sort().map(cls => this.metadata.find(cls)!); } - private addCommitDependency(calc: CommitOrderCalculator, prop: EntityProperty, entityName: string): void { - if (!(prop.reference === ReferenceType.ONE_TO_ONE && prop.owner) && prop.reference !== ReferenceType.MANY_TO_ONE) { - return; - } - - calc.addDependency(prop.type, entityName, prop.nullable ? 0 : 1); - } - private dump(builder: SchemaBuilder, append = '\n\n'): string { const sql = builder.toQuery(); return sql.length > 0 ? `${sql};${append}` : ''; diff --git a/tests/issues/GH845.test.ts b/tests/issues/GH845.test.ts index 7d43d932c3f5..bbf715a6c7e5 100644 --- a/tests/issues/GH845.test.ts +++ b/tests/issues/GH845.test.ts @@ -1,4 +1,4 @@ -import { Entity, MikroORM, PrimaryKey, Property, OneToMany, ManyToOne, Collection, QueryOrder } from '@mikro-orm/core'; +import { Entity, MikroORM, PrimaryKey, Property, OneToMany, ManyToOne, Collection, QueryOrder, Logger } from '@mikro-orm/core'; import { SqliteDriver } from '@mikro-orm/sqlite'; abstract class Base { @@ -70,6 +70,10 @@ describe('GH issue 845', () => { }); test(`GH issue 845`, async () => { + const mock = jest.fn(); + const logger = new Logger(mock, ['query']); + Object.assign(orm.config, { logger }); + expect(true).toBe(true); const c1 = new Child1(); const c2 = new Child2(); @@ -82,6 +86,12 @@ describe('GH issue 845', () => { await orm.em.persistAndFlush([c1, c2]); orm.em.clear(); + expect(mock.mock.calls[0][0]).toMatch('begin'); + expect(mock.mock.calls[1][0]).toMatch('insert into `parent` (`type`) values (?), (?)'); + expect(mock.mock.calls[2][0]).toMatch('insert into `relation1` (`parent_id`) values (?), (?), (?)'); + expect(mock.mock.calls[3][0]).toMatch('insert into `child1specific` (`child1_id`) values (?), (?), (?)'); + expect(mock.mock.calls[4][0]).toMatch('commit'); + const parents = await orm.em.find(Parent, {}, ['qaInfo.parent', 'rel'], { type: QueryOrder.ASC }); expect(parents[0]).toBeInstanceOf(Child1); expect(parents[0].type).toBe('Child1'); diff --git a/tests/single-table-inheritance.mysql.test.ts b/tests/single-table-inheritance.mysql.test.ts index 901f87d07df5..00acdb1779cd 100644 --- a/tests/single-table-inheritance.mysql.test.ts +++ b/tests/single-table-inheritance.mysql.test.ts @@ -27,8 +27,9 @@ describe('single table inheritance in mysql', () => { await orm.em.persistAndFlush([owner, employee1]); orm.em.clear(); - expect(owner.state).toBe('created'); - expect(owner.baseState).toBe('created'); + // owner will be updated, as we first batch insert everything and handle the extra update for owner + expect(owner.state).toBe('updated'); + expect(owner.baseState).toBe('updated'); expect((owner as any).type).not.toBeDefined(); } @@ -63,7 +64,7 @@ describe('single table inheritance in mysql', () => { expect((users[3] as CompanyOwner2).favouriteEmployee).toBeInstanceOf(Employee2); expect((users[3] as CompanyOwner2).favouriteManager).toBeInstanceOf(Manager2); expect(users[0]).toMatchObject({ - id: 2, + id: 4, firstName: 'Emp', lastName: '1', employeeProp: 1, @@ -77,14 +78,14 @@ describe('single table inheritance in mysql', () => { type: Type.Employee, }); expect(users[2]).toMatchObject({ - id: 3, + id: 2, firstName: 'Man', lastName: '3', managerProp: 'i am manager', type: Type.Manager, }); expect(users[3]).toMatchObject({ - id: 4, + id: 3, firstName: 'Bruce', lastName: 'Almighty', managerProp: 'i said i am owner', @@ -98,9 +99,9 @@ describe('single table inheritance in mysql', () => { expect(Object.keys(users[2])).toEqual(['id', 'firstName', 'lastName', 'type', 'managerProp']); expect(Object.keys(users[3])).toEqual(['id', 'firstName', 'lastName', 'type', 'ownerProp', 'favouriteEmployee', 'favouriteManager', 'managerProp']); - expect([...orm.em.getUnitOfWork().getIdentityMap().keys()]).toEqual(['BaseUser2-2', 'BaseUser2-1', 'BaseUser2-3', 'BaseUser2-4']); + expect([...orm.em.getUnitOfWork().getIdentityMap().keys()]).toEqual(['BaseUser2-4', 'BaseUser2-1', 'BaseUser2-2', 'BaseUser2-3']); - const o = await orm.em.findOneOrFail(CompanyOwner2, 4); + const o = await orm.em.findOneOrFail(CompanyOwner2, 3); expect(o.state).toBeUndefined(); expect(o.baseState).toBeUndefined(); o.firstName = 'Changed';