fix: OneToManySubjectBuilder bug with multiple primary keys (#8221)

* Bugfix: OneToManySubjectBuilder generated invalid subjects because of failed matching of relation IDs.

* relation.getEntityValue does not always return an array. Fix by defaulting to empty array on falsy return value.

* Add tests

* test fixes

* Refactor tests ensuring composite keys on child side into a separate test suite @ functional tests

* Rewrite tests and notes to correctly document+show what's the actual issue

* Fix: test must not use Promise.all, parallel execution against different drivers would mess up the counter within the SettingSubscriber!

* code updates

* okay now I know we need this check

Co-authored-by: Jannik <jannik@jannikmewes.de>
Co-authored-by: jannik.wjm@gmail.com <>
Co-authored-by: Umed Khudoiberdiev <pleerock.me@gmail.com>

src/persistence/subject-builder/OneToManySubjectBuilder.ts

@@ -54,14 +54,21 @@ export class OneToManySubjectBuilder {
      * by example: subject is "post" entity we are saving here and relation is "categories" inside it here.
      */
     protected buildForSubjectRelation(subject: Subject, relation: RelationMetadata) {
-
         // prepare objects (relation id maps) for the database entity
-        // note: subject.databaseEntity contains relations with loaded relation ids only
         // by example: since subject is a post, we are expecting to get all post's categories saved in the database here,
         //             particularly their relation ids, e.g. category ids stored in the database
+
+		// in most cases relatedEntityDatabaseValues will contain only the entity key properties.
+		// this is because subject.databaseEntity contains relations with loaded relation ids only.
+		// however if the entity uses the afterLoad hook to calculate any properties, the fetched "key object" might include ADDITIONAL properties.
+		// to handle such situations, we pass the data to relation.inverseEntityMetadata.getEntityIdMap to extract the key without any other properties.
+
         let relatedEntityDatabaseRelationIds: ObjectLiteral[] = [];
         if (subject.databaseEntity) { // related entities in the database can exist only if this entity (post) is saved
-            relatedEntityDatabaseRelationIds = relation.getEntityValue(subject.databaseEntity);
+            const relatedEntityDatabaseRelation: ObjectLiteral[] | undefined = relation.getEntityValue(subject.databaseEntity)
+            if (relatedEntityDatabaseRelation) {
+                relatedEntityDatabaseRelationIds = relatedEntityDatabaseRelation.map((entity) => relation.inverseEntityMetadata.getEntityIdMap(entity)!);
+            }
         }
 
         // get related entities of persisted entity
@@ -72,7 +79,7 @@ export class OneToManySubjectBuilder {
         if (relatedEntities === undefined) // if relation is undefined then nothing to update
             return;
 
-        // extract only relation ids from the related entities, since we only need them for comparision
+        // extract only relation ids from the related entities, since we only need them for comparison
         // by example: extract from categories only relation ids (category id, or let's say category title, depend on join column options)
         const relatedPersistedEntityRelationIds: ObjectLiteral[] = [];
         relatedEntities.forEach(relatedEntity => { // by example: relatedEntity is a category here

test/functional/relations/multiple-primary-keys/multiple-primary-keys-one-to-many/entity/Setting.ts

@@ -0,0 +1,25 @@
+import { BaseEntity, Column, Entity, ManyToOne, PrimaryColumn } from "../../../../../../src";
+import {User} from "./User";
+
+@Entity()
+export class Setting extends BaseEntity {
+	@PrimaryColumn("int")
+	assetId?: number;
+
+	@ManyToOne("User","settings",{ cascade:false , orphanedRowAction: "delete", nullable:false })
+	asset?: User;
+
+	@PrimaryColumn("varchar")
+	name: string;
+
+	@Column({nullable:true})
+	value: string;
+
+	constructor(id: number, name: string, value: string) {
+		super();
+		this.assetId = id;
+		this.name = name;
+		this.value = value;
+	}
+
+}

test/functional/relations/multiple-primary-keys/multiple-primary-keys-one-to-many/entity/User.ts

@@ -0,0 +1,20 @@
+import { BaseEntity, Column, Entity, OneToMany, PrimaryColumn } from "../../../../../../src";
+import {Setting} from "./Setting";
+
+@Entity()
+export class User extends BaseEntity {
+    @PrimaryColumn()
+    id: number;
+
+	@Column()
+	name: string;
+
+	@OneToMany("Setting","asset",{ cascade:true })
+	settings: Setting[];
+
+	constructor(id: number, name: string) {
+		super();
+		this.id = id;
+		this.name = name;
+	}
+}

test/functional/relations/multiple-primary-keys/multiple-primary-keys-one-to-many/multiple-primary-keys-one-to-many.ts

@@ -0,0 +1,115 @@
+import { expect } from "chai";
+import "reflect-metadata";
+import { Connection } from "../../../../../src";
+import { closeTestingConnections, createTestingConnections } from "../../../../utils/test-utils";
+import { User } from "./entity/User";
+import { Setting } from "./entity/Setting";
+
+/**
+ *  Using OneToMany relation with composed primary key should not error and work correctly
+ */
+describe("relations > multiple-primary-keys > one-to-many", () => {
+
+	let connections: Connection[];
+
+	before(async () => connections = await createTestingConnections({
+		entities: [User,Setting],
+		schemaCreate: true,
+		dropSchema: true
+	}));
+
+	after(() => closeTestingConnections(connections));
+
+	function insertSimpleTestData(connection: Connection) {
+		const userRepo = connection.getRepository(User);
+		// const settingRepo = connection.getRepository(Setting);
+
+		const user = new User(1, "FooGuy");
+		const settingA = new Setting(1, "A", "foo");
+		const settingB = new Setting(1, "B", "bar");
+		user.settings = [settingA,settingB];
+
+		return userRepo.save(user);
+	}
+
+
+
+	it("should correctly insert relation items", () => Promise.all(connections.map(async connection => {
+
+		const userEntity = await insertSimpleTestData(connection);
+		const persistedSettings = await connection.getRepository(Setting).find();
+
+		expect(persistedSettings!).not.to.be.undefined;
+		expect(persistedSettings.length).to.equal(2);
+		expect(persistedSettings[0].assetId).to.equal(userEntity.id);
+		expect(persistedSettings[1].assetId).to.equal(userEntity.id);
+
+	})));
+
+	it("should correctly load relation items", () => Promise.all(connections.map(async connection => {
+
+		await insertSimpleTestData(connection);
+		const user = await connection.getRepository(User).findOne({relations:["settings"]});
+
+		expect(user!).not.to.be.undefined;
+		expect(user!.settings).to.be.an("array");
+		expect(user!.settings!.length).to.equal(2);
+
+	})));
+
+	it("should correctly update relation items", () => Promise.all(connections.map(async connection => {
+
+		await insertSimpleTestData(connection);
+		const userRepo = connection.getRepository(User);
+
+		await userRepo.save([{
+			id:1,
+			settings:[
+				{id:1,name:"A",value:"foobar"},
+				{id:1,name:"B",value:"testvalue"},
+			]
+		}]);
+
+		const user = await connection.getRepository(User).findOne({relations:["settings"]});
+
+		// check the saved items have correctly updated value
+		expect(user!).not.to.be.undefined;
+		expect(user!.settings).to.be.an("array");
+		expect(user!.settings!.length).to.equal(2);
+		user!.settings.forEach(setting=>{
+			if(setting.name==="A") expect(setting.value).to.equal("foobar");
+			else expect(setting.value).to.equal("testvalue");
+		});
+
+		// make sure only 2 entries are in db, initial ones should have been updated
+		const settings = await connection.getRepository(Setting).find();
+		expect(settings).to.be.an("array");
+		expect(settings!.length).to.equal(2);
+
+	})));
+
+	it("should correctly delete relation items", () => Promise.all(connections.map(async connection => {
+
+		await insertSimpleTestData(connection);
+		const userRepo = connection.getRepository(User);
+
+		await userRepo.save([{
+			id:1,
+			settings:[]
+		}]);
+
+		const user = await connection.getRepository(User).findOne({relations:["settings"]});
+
+		// check that no relational items are found
+		expect(user!).not.to.be.undefined;
+		expect(user!.settings).to.be.an("array");
+		expect(user!.settings!.length).to.equal(0);
+
+		// check there are no orphane relational items
+		const settings = await connection.getRepository(Setting).find();
+		expect(settings).to.be.an("array");
+		expect(settings!.length).to.equal(0);
+
+	})));
+
+});

test/github-issues/8221/entity/Setting.ts

@@ -0,0 +1,25 @@
+import { BaseEntity, Column, Entity, ManyToOne, PrimaryColumn } from "../../../../src";
+import {User} from "./User";
+
+@Entity()
+export class Setting extends BaseEntity {
+	@PrimaryColumn("int")
+	assetId?: number;
+
+	@ManyToOne("User","settings",{ cascade:false , orphanedRowAction: "delete", nullable:false })
+	asset?: User;
+
+	@PrimaryColumn("varchar")
+	name: string;
+
+	@Column({nullable:true})
+	value: string;
+
+	constructor(id: number, name: string, value: string) {
+		super();
+		this.assetId = id;
+		this.name = name;
+		this.value = value;
+	}
+
+}

test/github-issues/8221/entity/SettingSubscriber.ts

@@ -0,0 +1,41 @@
+import { EntitySubscriberInterface, EventSubscriber, LoadEvent, UpdateEvent } from "../../../../src";
+import {Setting} from "./Setting";
+
+
+@EventSubscriber()
+export class SettingSubscriber implements EntitySubscriberInterface {
+    counter: any;
+
+	constructor() {
+		this.reset();
+	}
+
+	listenTo() {
+		return Setting;
+	}
+
+	afterLoad(item: Setting, event?: LoadEvent<Setting>) {
+		// just an example, any entity modification on after load will lead to this issue
+		item.value = "x";
+	}
+
+    beforeUpdate(event: UpdateEvent<any>): void {
+        this.counter.updates++;
+    }
+
+    beforeInsert(event: UpdateEvent<any>): void {
+        this.counter.inserts++;
+    }
+
+    beforeRemove(event: UpdateEvent<any>): void {
+        this.counter.deletes++;
+    }
+
+	reset() {
+		this.counter = {
+			deletes:0,
+			inserts:0,
+			updates:0,
+		};
+	}
+}

test/github-issues/8221/entity/User.ts

@@ -0,0 +1,20 @@
+import { BaseEntity, Column, Entity, OneToMany, PrimaryColumn } from "../../../../src";
+import {Setting} from "./Setting";
+
+@Entity()
+export class User extends BaseEntity {
+    @PrimaryColumn()
+    id: number;
+
+	@Column()
+	name: string;
+
+	@OneToMany("Setting","asset",{ cascade:true })
+	settings: Setting[];
+
+	constructor(id: number, name: string) {
+		super();
+		this.id = id;
+		this.name = name;
+	}
+}

test/github-issues/8221/issue-8221.ts

@@ -0,0 +1,69 @@
+import { expect } from "chai";
+import "reflect-metadata";
+import { Connection } from "../../../src";
+import { closeTestingConnections, createTestingConnections } from "../../utils/test-utils";
+import { User } from "./entity/User";
+import { Setting } from "./entity/Setting";
+import { SettingSubscriber } from "./entity/SettingSubscriber";
+
+/**
+ *  Using OneToMany relation with composed primary key should not error and work correctly
+ */
+describe("github issues > #8221", () => {
+
+	let connections: Connection[];
+
+	before(async () => connections = await createTestingConnections({
+		entities: [User,Setting],
+		subscribers: [SettingSubscriber],
+		schemaCreate: true,
+		dropSchema: true
+	}));
+
+	after(() => closeTestingConnections(connections));
+
+	function insertSimpleTestData(connection: Connection) {
+		const userRepo = connection.getRepository(User);
+		// const settingRepo = connection.getRepository(Setting);
+
+		const user = new User(1, "FooGuy");
+		const settingA = new Setting(1, "A", "foo");
+		const settingB = new Setting(1, "B", "bar");
+		user.settings = [settingA,settingB];
+
+		return userRepo.save(user);
+	}
+
+	// important: must not use Promise.all! parallel execution against different drivers would mess up the counter within the SettingSubscriber!
+
+	it("afterLoad entity modifier must not make relation key matching fail", async () => {
+		for(const connection of connections) {
+
+			const userRepo = connection.getRepository(User);
+			const subscriber = (connection.subscribers[0] as SettingSubscriber);
+			subscriber.reset();
+
+			await insertSimpleTestData(connection);
+			subscriber.reset();
+
+			await userRepo.save([{
+				id:1,
+				settings: [
+					{ assertId:1, name:"A", value:"foobar" },
+					{ assertId:1, name:"B", value:"testvalue" },
+				]
+			}]);
+
+			// we use a subscriber to count generated Subjects based on how often beforeInsert/beforeRemove/beforeUpdate has been called.
+			// the save query should only update settings, so only beforeUpdate should have been called.
+			// if beforeInsert/beforeUpdate has been called, this would indicate that key matching has failed.
+			// the resulting state would be the same, but settings entities would be deleted and inserted instead.
+
+			expect(subscriber.counter.deletes).to.equal(0);
+			expect(subscriber.counter.inserts).to.equal(0);
+			expect(subscriber.counter.updates).to.equal(2);
+
+		}
+	});
+
+});