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

@@ -40,6 +40,7 @@ export class EntitySchema<T extends AnyEntity<T> = AnyEntity, U extends AnyEntit
     }
 
     Object.assign(this._meta, { className: meta.name }, meta);
+    this._meta.root = this._meta.root ?? this._meta;
   }
 
   static fromMetadata<T extends AnyEntity<T> = AnyEntity, U extends AnyEntity<U> | undefined = undefined>(meta: EntityMetadata<T> | DeepPartial<EntityMetadata<T>>): EntitySchema<T, U> {

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;

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<T extends AnyEntity<T>> {
+
+  constructor(public entity: T,
+              public type: ChangeSetType,
+              public payload: EntityData<T>,
+              meta: EntityMetadata<T>) {
+    this.name = meta.className;
+    this.rootName = meta.root.className;
+    this.collection = meta.root.collection;
+  }
+
+}
 
 export interface ChangeSet<T extends AnyEntity<T>> {
   name: string;
+  rootName: string;
   collection: string;
   type: ChangeSetType;
   entity: T;

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

@@ -19,17 +19,14 @@ export class ChangeSetComputer {
               private readonly config: Configuration) { }
 
   computeChangeSet<T extends AnyEntity<T>>(entity: T): ChangeSet<T> | null {
-    const changeSet = { entity } as ChangeSet<T>;
     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;

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.

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<AnyEntity>);
+      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<string>();
-    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);
-  }
-
 }

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}` : '';

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');

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';