Support mapping the same column across discriminator subclasses

[parisholley]

Describe the bug
In order to make it easier for developers to understand models that rely on single-table design (in particular, managed ManyToMany tables), we "alias" some of our ManyToOne's so that you don't have to remember which logical data model maps to which generically named column. Other examples would be tables with a to/from or parent/child naming convention.

@Entity({ discriminatorValue: MemberRelationshipType.CUSTOMER })
export class BusinessCustomerMemberRelationship extends MemberRelationship<MemberRelationshipType.CUSTOMER> extends {
  @ManyToOne(() => Member, { fieldName: 'activeId', wrappedReference: true })
  customer: IdentifiedReference<Member>;

  @ManyToOne(() => Member, { fieldName: 'passiveId', wrappedReference: true })
  business: IdentifiedReference<Member>;

  // additional relationships/columns/metadata that only apply for customers
}

@Entity({ discriminatorValue: MemberRelationshipType.VENDOR })
export class BusinessVendorMemberRelationship extends MemberRelationship<MemberRelationshipType.VENDOR> extends {
  @ManyToOne(() => Member, { fieldName: 'activeId', wrappedReference: true })
  vendor: IdentifiedReference<Member>;

  @ManyToOne(() => Member, { fieldName: 'passiveId', wrappedReference: true })
  business: IdentifiedReference<Member>;

  // additional relationships/columns/metadata that only apply for vendors
}

When attempting to insert a discriminated subclass, the ORM will inject the activeId twice into the SQL query and it will fail. Using a getter/setter per subclass to "mimick" the alias isn't useful because it will likely cause problems when using the API (eg: filtering or populating).

Expected behavior
ORM should only use metadata of parent class and subclass to generate queries and access identity map.

Versions

    "@mikro-orm/cli": "^4.5.9",
    "@mikro-orm/core": "^4.5.9",
    "@mikro-orm/migrations": "^4.5.9",
    "@mikro-orm/nestjs": "^4.3.1",
    "@mikro-orm/postgresql": "^4.5.9",
    "@mikro-orm/reflection": "^4.5.9",

[parisholley]

Interesting thing to note, this behavior seems to be related to the fact that the class property names are different. In the example above, the passiveId is not inserted into the SQL twice because it is both mapped to business, however if you rename the fields to business1 and business2, it will also duplicate passiveId.

[B4nan]

I believe we have a single hydration functions for this, and in general everything is processed in the same way, just the entity class is picked based on the discriminator. This is simply not supported use case right now, I believe there is even a validation that should forbid overriding properties defined in base STI entities, but here you are defining it on the same level so it fails to trigger.

[parisholley]

i'm not sure what the implications are w/r/t propagation and other internals, but as far as fixing my insertion issues, it just requires some de-dupe work inside here:

mikro-orm/packages/knex/src/AbstractSqlDriver.ts

Lines 243 to 274 in f0cbcc2

	const set = new Set<string>();
	data.forEach(row => Object.keys(row).forEach(k => set.add(k)));
	const props = [...set].map(name => meta.properties[name] ?? { name, fieldNames: [name] }) as EntityProperty<T>[];
	const fields = Utils.flatten(props.map(prop => prop.fieldNames));
	let res: QueryResult;
	if (fields.length === 0) {
	const qb = this.createQueryBuilder<T>(entityName, options.ctx, true, options.convertCustomTypes).withSchema(this.getSchemaName(meta, options));
	res = await this.rethrow(qb.insert(data).execute('run', false));
	} else {
	let sql = `insert into ${(this.getTableName(meta, options))} `;
	/* istanbul ignore next */
	sql += fields.length > 0 ? '(' + fields.map(k => this.platform.quoteIdentifier(k)).join(', ') + ')' : 'default';
	sql += ` values `;
	const params: any[] = [];
	sql += data.map(row => {
	const keys: string[] = [];
	props.forEach(prop => {
	if (prop.fieldNames.length > 1) {
	params.push(...(row[prop.name] as unknown[]));
	keys.push(...prop.fieldNames.map(_ => '?'));
	} else if (prop.customType && 'convertToDatabaseValueSQL' in prop.customType && !this.platform.isRaw(row[prop.name])) {
	keys.push(prop.customType.convertToDatabaseValueSQL!('?', this.platform));
	params.push(row[prop.name]);
	} else {
	params.push(row[prop.name]);
	keys.push('?');
	}
	});
	return '(' + keys.join(', ') + ')';
	}).join(', ');

i have a local patch working that i'll run with and see if i run into any weird behavior before considering a PR

[B4nan]

Fixed in v6 via #4769