fix(core): support embedded properties with conflicting property names

Closes #5065
mikro-orm · Jan 2, 2024 · b43ef63 · b43ef63
1 parent 6c363e7
commit b43ef63
Show file tree

Hide file tree

Showing 10 changed files with 128 additions and 32 deletions.
diff --git a/packages/core/src/EntityManager.ts b/packages/core/src/EntityManager.ts
@@ -2185,12 +2185,14 @@ export class EntityManager<D extends IDatabaseDriver = IDatabaseDriver> {
           merge: true,
           convertCustomTypes: true,
           refresh,
+          recomputeSnapshot: true,
         })) as unknown as R;
       } else if (Utils.isObject<EntityData<T>>(cached) && merge) {
         data = em.entityFactory.create<T>(entityName, cached, {
           merge: true,
           convertCustomTypes: true,
           refresh,
+          recomputeSnapshot: true,
         }) as unknown as R;
       } else {
         data = cached;

diff --git a/packages/core/src/entity/EntityFactory.ts b/packages/core/src/entity/EntityFactory.ts
@@ -31,6 +31,7 @@ export interface FactoryOptions {
   merge?: boolean;
   refresh?: boolean;
   convertCustomTypes?: boolean;
+  recomputeSnapshot?: boolean;
   schema?: string;
 }
 
@@ -117,6 +118,10 @@ export class EntityFactory {
         newEntity: options.newEntity,
         loaded: options.initialized,
       });
+
+      if (options.recomputeSnapshot) {
+        wrapped.__originalEntityData = this.comparator.prepareEntity(entity);
+      }
     }
 
     if (this.eventManager.hasListeners(EventType.onInit, meta2)) {

diff --git a/packages/core/src/errors.ts b/packages/core/src/errors.ts
@@ -225,7 +225,7 @@ export class MetadataError<T extends AnyEntity = AnyEntity> extends ValidationEr
   }
 
   static duplicateFieldName(className: string, names: [string, string][]): MetadataError {
-    return new MetadataError(`Duplicate fieldNames are not allowed: ${names.map(n => `${className}.${n[0]} (fieldName: '${n[0]}')`).join(', ')}`);
+    return new MetadataError(`Duplicate fieldNames are not allowed: ${names.map(n => `${className}.${n[0]} (fieldName: '${n[1]}')`).join(', ')}`);
   }
 
   static multipleDecorators(entityName: string, propertyName: string): MetadataError {
@@ -236,10 +236,6 @@ export class MetadataError<T extends AnyEntity = AnyEntity> extends ValidationEr
     return new MetadataError(`Metadata for entity ${entity} not found`);
   }
 
-  static conflictingPropertyName(className: string, name: string, embeddedName: string): MetadataError {
-    return new MetadataError(`Property ${className}:${name} is being overwritten by its child property ${embeddedName}:${name}. Consider using a prefix to overcome this issue.`);
-  }
-
   static invalidPrimaryKey(meta: EntityMetadata, prop: EntityProperty, requiredName: string) {
     return this.fromMessage(meta, prop, `has wrong field name, '${requiredName}' is required in current driver`);
   }

diff --git a/packages/core/src/metadata/MetadataDiscovery.ts b/packages/core/src/metadata/MetadataDiscovery.ts
@@ -947,11 +947,7 @@ export class MetadataDiscovery {
     const prefix = embeddedProp.prefix === false ? '' : embeddedProp.prefix === true ? embeddedProp.embeddedPath?.join('_') ?? embeddedProp.fieldNames[0] + '_' : embeddedProp.prefix;
 
     for (const prop of Object.values(embeddable.properties).filter(p => p.persist !== false)) {
-      const name = prefix + prop.name;
-
-      if (meta.properties[name] !== undefined && getRootProperty(meta.properties[name]).kind !== ReferenceKind.EMBEDDED) {
-        throw MetadataError.conflictingPropertyName(meta.className, name, embeddedProp.name);
-      }
+      const name = (embeddedProp.embeddedPath?.join('_') ?? embeddedProp.fieldNames[0] + '_') + prop.name;
 
       meta.properties[name] = Utils.copy(prop, false);
       meta.properties[name].name = name;
@@ -963,12 +959,13 @@ export class MetadataDiscovery {
         meta.properties[name].nullable = true;
       }
 
-      if (prefix) {
-        if (meta.properties[name].fieldNames) {
-          meta.properties[name].fieldNames[0] = prefix + meta.properties[name].fieldNames[0];
-        } else {
-          this.initFieldName(meta.properties[name]);
-        }
+      if (meta.properties[name].fieldNames) {
+        meta.properties[name].fieldNames[0] = prefix + meta.properties[name].fieldNames[0];
+      } else {
+        const name2 = meta.properties[name].name;
+        meta.properties[name].name = prefix + prop.name;
+        this.initFieldName(meta.properties[name]);
+        meta.properties[name].name = name2;
       }
 
       if (object) {

diff --git a/packages/core/src/metadata/MetadataValidator.ts b/packages/core/src/metadata/MetadataValidator.ts
@@ -215,15 +215,17 @@ export class MetadataValidator {
 
   private validateDuplicateFieldNames(meta: EntityMetadata, options: MetadataDiscoveryOptions): void {
     const candidates = Object.values(meta.properties)
-      .filter(prop => prop.persist !== false && !prop.inherited && prop.fieldNames?.length === 1)
+      .filter(prop => prop.persist !== false && !prop.inherited && prop.fieldNames?.length === 1 && (prop.kind !== ReferenceKind.EMBEDDED || prop.object))
       .map(prop => prop.fieldNames[0]);
     const duplicates = Utils.findDuplicates(candidates);
 
     if (duplicates.length > 0 && options.checkDuplicateFieldNames) {
       const pairs = duplicates.flatMap(name => {
         return Object.values(meta.properties)
           .filter(p => p.fieldNames[0] === name)
-          .map(prop => [prop.name, prop.fieldNames[0]] as [string, string]);
+          .map(prop => {
+            return [prop.embedded ? prop.embedded.join('.') : prop.name, prop.fieldNames[0]] as [string, string];
+          });
       });
 
       throw MetadataError.duplicateFieldName(meta.className, pairs);

diff --git a/tests/MetadataValidator.test.ts b/tests/MetadataValidator.test.ts
@@ -184,7 +184,7 @@ describe('MetadataValidator', () => {
         age: { kind: 'scalar', name: 'age', fieldNames: ['name'], type: 'string' },
       },
     } as any).init();
-    expect(() => validator.validateEntityDefinition(new MetadataStorage({ Foo1: schema1.meta }), 'Foo1', options)).toThrow("Duplicate fieldNames are not allowed: Foo1.name (fieldName: 'name'), Foo1.age (fieldName: 'age')");
+    expect(() => validator.validateEntityDefinition(new MetadataStorage({ Foo1: schema1.meta }), 'Foo1', options)).toThrow("Duplicate fieldNames are not allowed: Foo1.name (fieldName: 'name'), Foo1.age (fieldName: 'name')");
     schema1.meta.properties.age.fieldNames[0] = 'age';
     expect(() => validator.validateDiscovered([schema1.meta], options)).not.toThrow();
   });

diff --git a/tests/features/embeddables/GH5065.test.ts b/tests/features/embeddables/GH5065.test.ts
@@ -0,0 +1,94 @@
+import { EntitySchema, MikroORM } from '@mikro-orm/sqlite';
+
+class Place {
+
+  id!: number;
+  name!: string;
+  altitude!: Altitude;
+  population!: Population;
+
+}
+
+class Altitude {
+
+  value1!: number;
+  value2!: number;
+
+}
+
+class Population {
+
+  value1!: number;
+  value2!: number;
+
+}
+
+const placeSchema = new EntitySchema({
+  class: Place,
+  properties: {
+    id: { primary: true, type: Number },
+    name: { type: String },
+    altitude: {
+      kind: 'embedded',
+      entity: () => Altitude,
+      prefix: false,
+    },
+    population: {
+      kind: 'embedded',
+      entity: () => Population,
+      prefix: false,
+    },
+  },
+  tableName: 'Place',
+});
+
+const altitudeSchema = new EntitySchema({
+  class: Altitude,
+  embeddable: true,
+  properties: {
+    value1: {
+      fieldName: 'altitude',
+      type: Number,
+    },
+    value2: {
+      fieldName: 'altitude2',
+      type: Number,
+    },
+  },
+});
+
+const populationSchema = new EntitySchema({
+  class: Population,
+  embeddable: true,
+  properties: {
+    value1: {
+      fieldName: 'population',
+      type: Number,
+    },
+    value2: {
+      fieldName: 'population2',
+      type: Number,
+    },
+  },
+});
+
+let orm: MikroORM;
+
+beforeAll(async () => {
+  orm = await MikroORM.init({
+    entities: [placeSchema],
+    dbName: ':memory:',
+  });
+});
+
+afterAll(async () => {
+  await orm.close(true);
+});
+
+test(`GH #5065`, async () => {
+  expect(await orm.schema.getCreateSchemaSQL({ wrap: false })).toMatchInlineSnapshot(`
+"create table \`Place\` (\`id\` integer not null primary key autoincrement, \`name\` text not null, \`altitude\` integer not null, \`altitude2\` integer not null, \`population\` integer not null, \`population2\` integer not null);
+
+"
+`);
+});
diff --git a/tests/features/embeddables/embedded-entities.mongo.test.ts b/tests/features/embeddables/embedded-entities.mongo.test.ts
@@ -230,8 +230,8 @@ describe('embedded entities in mongo', () => {
       kind: ReferenceKind.EMBEDDED,
       type: 'Address2',
     });
-    expect(orm.getMetadata().get('User').properties.addr_street).toMatchObject({
-      name: 'addr_street',
+    expect(orm.getMetadata().get('User').properties.address2_street).toMatchObject({
+      name: 'address2_street',
       kind: ReferenceKind.SCALAR,
       type: 'string',
       nullable: true,
@@ -241,8 +241,8 @@ describe('embedded entities in mongo', () => {
       kind: ReferenceKind.EMBEDDED,
       type: 'Address1',
     });
-    expect(orm.getMetadata().get('User').properties.street).toMatchObject({
-      name: 'street',
+    expect(orm.getMetadata().get('User').properties.address4_street).toMatchObject({
+      name: 'address4_street',
       kind: ReferenceKind.SCALAR,
       type: 'string',
     });

diff --git a/tests/features/embeddables/embedded-entities.mysql.test.ts b/tests/features/embeddables/embedded-entities.mysql.test.ts
@@ -129,8 +129,8 @@ describe('embedded entities in mysql', () => {
       kind: ReferenceKind.EMBEDDED,
       type: 'Address2',
     });
-    expect(orm.getMetadata().get('User').properties.addr_street).toMatchObject({
-      name: 'addr_street',
+    expect(orm.getMetadata().get('User').properties.address2_street).toMatchObject({
+      name: 'address2_street',
       kind: ReferenceKind.SCALAR,
       type: 'string',
       nullable: true,
@@ -140,8 +140,8 @@ describe('embedded entities in mysql', () => {
       kind: ReferenceKind.EMBEDDED,
       type: 'Address1',
     });
-    expect(orm.getMetadata().get('User').properties.street).toMatchObject({
-      name: 'street',
+    expect(orm.getMetadata().get('User').properties.address4_street).toMatchObject({
+      name: 'address4_street',
       kind: ReferenceKind.SCALAR,
       type: 'string',
     });
@@ -296,7 +296,7 @@ describe('embedded entities in mysql', () => {
   });
 
   test('should throw error with colliding definition of inlined embeddables without prefix', async () => {
-    const err = `Property UserWithCity:city is being overwritten by its child property address1:city. Consider using a prefix to overcome this issue.`;
+    const err = `Duplicate fieldNames are not allowed: UserWithCity.city (fieldName: 'city'), UserWithCity.address1.city (fieldName: 'city')`;
     await expect(MikroORM.init({
       entities: [Address1, UserWithCity],
       dbName: `mikro_orm_test_embeddables`,

diff --git a/tests/features/embeddables/embedded-entities.postgres.test.ts b/tests/features/embeddables/embedded-entities.postgres.test.ts
@@ -149,8 +149,8 @@ describe('embedded entities in postgresql', () => {
       kind: ReferenceKind.EMBEDDED,
       type: 'Address2',
     });
-    expect(orm.getMetadata().get('User').properties.addr_street).toMatchObject({
-      name: 'addr_street',
+    expect(orm.getMetadata().get('User').properties.address2_street).toMatchObject({
+      name: 'address2_street',
       kind: ReferenceKind.SCALAR,
       type: 'string',
       nullable: true,
@@ -160,8 +160,8 @@ describe('embedded entities in postgresql', () => {
       kind: ReferenceKind.EMBEDDED,
       type: 'Address1',
     });
-    expect(orm.getMetadata().get('User').properties.street).toMatchObject({
-      name: 'street',
+    expect(orm.getMetadata().get('User').properties.address4_street).toMatchObject({
+      name: 'address4_street',
       kind: ReferenceKind.SCALAR,
       type: 'string',
     });