refactor: add more tests and fix some minor issues

docs/docs/upgrading-v4-to-v5.md

@@ -128,3 +128,16 @@ Running migrations in production via node and ts-node is now handled the same.
 This should actually not be breaking, as old format with extension is still 
 supported (e.g. they still can be rolled back), but newly logged migrations
 will not contain the extension.
+
+## Changes in `assign()` helper
+
+Embeddable instances are now created via `EntityFactory` and they respect the
+`forceEntityConstructor` configuration. Due to this we need to have EM instance
+when assigning to embedded properties. 
+
+Using `em.assign()` should be preferred to get around this.
+
+Deep assigning of child entities now works by default based on the presence of PKs in the payload.
+This behaviour can be disable via updateByPrimaryKey: false in the assign options.
+
+`mergeObjects` option is now enabled by default.

packages/core/src/entity/EntityAssigner.ts

@@ -17,6 +17,7 @@ export class EntityAssigner {
     options = {
       updateNestedEntities: true,
       updateByPrimaryKey: true,
+      mergeObjects: true,
       schema: wrapped.__schema,
       ...options, // allow overriding the defaults
     };
@@ -75,7 +76,7 @@ export class EntityAssigner {
         return entity[prop as keyof T] = validator.validateProperty(props[prop], value, entity);
       }
 
-      if (props[prop]?.reference === ReferenceType.EMBEDDED) {
+      if (props[prop]?.reference === ReferenceType.EMBEDDED && EntityAssigner.validateEM(em)) {
         return EntityAssigner.assignEmbeddable(entity, value, props[prop], em, options);
       }
 
@@ -171,10 +172,8 @@ export class EntityAssigner {
     collection.set(items);
   }
 
-  private static assignEmbeddable<T extends AnyEntity<T>>(entity: T, value: any, prop: EntityProperty, em: EntityManager, options: AssignOptions): void {
-    const Embeddable = prop.embeddable;
+  private static assignEmbeddable<T extends AnyEntity<T>>(entity: T, value: any, prop: EntityProperty, em: EntityManager | undefined, options: AssignOptions): void {
     const propName = prop.embedded ? prop.embedded[1] : prop.name;
-    entity[propName] = prop.array || options.mergeObjects ? (entity[propName] || Object.create(Embeddable.prototype)) : Object.create(Embeddable.prototype);
 
     if (!value) {
       entity[propName] = value;
@@ -194,6 +193,12 @@ export class EntityAssigner {
       });
     }
 
+    const create = () => EntityAssigner.validateEM(em) && em!.getEntityFactory().createEmbeddable<T>(prop.type, value, {
+      convertCustomTypes: options.convertCustomTypes,
+      newEntity: options.mergeObjects ? !entity[propName] : true,
+    });
+    entity[propName] = options.mergeObjects ? (entity[propName] || create()) : create();
+
     Object.keys(value).forEach(key => {
       const childProp = prop.embeddedProps[key];
 

packages/core/src/entity/EntityFactory.ts

@@ -138,6 +138,15 @@ export class EntityFactory {
     return this.create<T>(entityName, id as EntityData<T>, { ...options, initialized: false }) as T;
   }
 
+  createEmbeddable<T>(entityName: EntityName<T>, data: EntityData<T>, options: Pick<FactoryOptions, 'newEntity' | 'convertCustomTypes'> = {}): T {
+    entityName = Utils.className(entityName);
+    data = { ...data };
+    const meta = this.metadata.get(entityName);
+    const meta2 = this.processDiscriminatorColumn<T>(meta, data);
+
+    return this.createEntity(data, meta2, options);
+  }
+
   private createEntity<T extends AnyEntity<T>>(data: EntityData<T>, meta: EntityMetadata<T>, options: FactoryOptions): T {
     if (options.newEntity || meta.forceConstructor) {
       const params = this.extractConstructorParams<T>(meta, data, options);
@@ -253,6 +262,15 @@ export class EntityFactory {
         return this.createReference(meta.properties[k].type, data[k], options);
       }
 
+      if (meta.properties[k]?.reference === ReferenceType.EMBEDDED && data[k]) {
+        /* istanbul ignore next */
+        if (Utils.isEntity<T>(data[k])) {
+          return data[k];
+        }
+
+        return this.createEmbeddable(meta.properties[k].type, data[k], options);
+      }
+
       if (!meta.properties[k]) {
         return data;
       }

packages/core/src/errors.ts

@@ -212,6 +212,10 @@ export class MetadataError<T extends AnyEntity = AnyEntity> extends ValidationEr
     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`);
+  }
+
   private static fromMessage(meta: EntityMetadata, prop: EntityProperty, message: string): MetadataError {
     return new MetadataError(`${meta.className}.${prop.name} ${message}`);
   }

packages/core/src/hydration/ObjectHydrator.ts

@@ -222,11 +222,11 @@ export class ObjectHydrator extends Hydrator {
           const childDataKey = prop.object ? dataKey + this.wrap(childProp.embedded![1]) : this.wrap(childProp.name);
           // weak comparison as we can have numbers that might have been converted to strings due to being object keys
           ret.push(`    if (data${childDataKey} == '${meta.discriminatorValue}') {`);
-          ret.push(`      entity${entityKey} = factory.create('${meta.className}', data${prop.object ? dataKey : ''}, { newEntity, convertCustomTypes });`);
+          ret.push(`      entity${entityKey} = factory.createEmbeddable('${meta.className}', data${prop.object ? dataKey : ''}, { newEntity, convertCustomTypes });`);
           ret.push(`    }`);
         });
       } else {
-        ret.push(`    entity${entityKey} = factory.create('${prop.targetMeta!.className}', data${prop.object ? dataKey : ''}, { newEntity, convertCustomTypes });`);
+        ret.push(`    entity${entityKey} = factory.createEmbeddable('${prop.targetMeta!.className}', data${prop.object ? dataKey : ''}, { newEntity, convertCustomTypes });`);
       }
 
       meta.props

packages/core/src/metadata/MetadataDiscovery.ts

@@ -41,7 +41,10 @@ export class MetadataDiscovery {
 
     this.discovered
       .filter(meta => meta.name)
-      .forEach(meta => discovered.set(meta.name!, meta));
+      .forEach(meta => {
+        this.platform.validateMetadata(meta);
+        discovered.set(meta.name!, meta);
+      });
 
     return discovered;
   }

packages/core/src/platforms/Platform.ts

@@ -2,7 +2,7 @@ import clone from 'clone';
 import { EntityRepository } from '../entity';
 import type { NamingStrategy } from '../naming-strategy';
 import { UnderscoreNamingStrategy } from '../naming-strategy';
-import type { AnyEntity, Constructor, EntityProperty, IEntityGenerator, IMigrator, IPrimaryKey, ISchemaGenerator, PopulateOptions, Primary } from '../typings';
+import type { AnyEntity, Constructor, EntityProperty, IEntityGenerator, IMigrator, IPrimaryKey, ISchemaGenerator, PopulateOptions, Primary, EntityMetadata } from '../typings';
 import { ExceptionConverter } from './ExceptionConverter';
 import type { EntityManager } from '../EntityManager';
 import type { Configuration } from '../utils/Configuration';
@@ -377,4 +377,8 @@ export abstract class Platform {
     return true;
   }
 
+  validateMetadata(meta: EntityMetadata): void {
+    return;
+  }
+
 }

packages/knex/src/query/QueryBuilderHelper.ts

@@ -559,12 +559,13 @@ export class QueryBuilderHelper {
 
     if (!this.isPrefixed(field)) {
       const alias = always ? (quote ? this.alias : this.platform.quoteIdentifier(this.alias)) + '.' : '';
-      const fieldName = this.fieldName(field, this.alias);
-      if (fieldName.startsWith('(')) {
-        ret = '(' + alias + fieldName.slice(1);
-      } else {
-        ret = alias + fieldName;
+      const fieldName = this.fieldName(field, this.alias, always);
+
+      if (QueryBuilderHelper.isCustomExpression(fieldName)) {
+        return fieldName;
       }
+
+      ret = alias + fieldName;
     } else {
       const [a, f] = field.split('.');
       ret = a + '.' + this.fieldName(f, a);
@@ -601,14 +602,25 @@ export class QueryBuilderHelper {
     return !!field.match(/[\w`"[\]]+\./);
   }
 
-  private fieldName(field: string, alias?: string): string {
+  private fieldName(field: string, alias?: string, always?: boolean): string {
     const prop = this.getProperty(field, alias);
 
     if (!prop) {
       return field;
     }
 
     if (prop.fieldNameRaw) {
+      if (!always) {
+        return prop.fieldNameRaw
+          .replace(/\[::alias::]\.?/g, '')
+          .replace(this.platform.quoteIdentifier('') + '.', '');
+      }
+
+      if (alias) {
+        return prop.fieldNameRaw.replace(/\[::alias::]/g, alias);
+      }
+
+      /* istanbul ignore next */
       return prop.fieldNameRaw;
     }
 

packages/mongodb/src/MongoPlatform.ts

@@ -1,6 +1,6 @@
 import { ObjectId } from 'mongodb';
-import type { IPrimaryKey, Primary, NamingStrategy, Constructor, EntityRepository, EntityProperty, PopulateOptions } from '@mikro-orm/core';
-import { Platform, MongoNamingStrategy, Utils, ReferenceType } from '@mikro-orm/core';
+import type { IPrimaryKey, Primary, NamingStrategy, Constructor, EntityRepository, EntityProperty, PopulateOptions, EntityMetadata } from '@mikro-orm/core';
+import { Platform, MongoNamingStrategy, Utils, ReferenceType, MetadataError } from '@mikro-orm/core';
 import { MongoExceptionConverter } from './MongoExceptionConverter';
 import { MongoEntityRepository } from './MongoEntityRepository';
 
@@ -63,4 +63,12 @@ export class MongoPlatform extends Platform {
     return prop.reference === ReferenceType.MANY_TO_MANY && prop.owner;
   }
 
+  validateMetadata(meta: EntityMetadata): void {
+    const pk = meta.getPrimaryProps()[0];
+
+    if (pk && pk.fieldNames?.[0] !== '_id') {
+      throw MetadataError.invalidPrimaryKey(meta, pk, '_id');
+    }
+  }
+
 }

packages/mysql-base/src/MySqlPlatform.ts

@@ -2,7 +2,7 @@ import { AbstractSqlPlatform } from '@mikro-orm/knex';
 import { MySqlSchemaHelper } from './MySqlSchemaHelper';
 import { MySqlExceptionConverter } from './MySqlExceptionConverter';
 import type { Type } from '@mikro-orm/core';
-import { Utils } from '@mikro-orm/core';
+import { expr, Utils } from '@mikro-orm/core';
 
 export class MySqlPlatform extends AbstractSqlPlatform {
 
@@ -15,7 +15,7 @@ export class MySqlPlatform extends AbstractSqlPlatform {
 
   getSearchJsonPropertyKey(path: string[], type: string): string {
     const [a, ...b] = path;
-    return `${this.quoteIdentifier(a)}->'$.${b.join('.')}'`;
+    return expr(alias => `${this.quoteIdentifier(`${alias}.${a}`)}->'$.${b.join('.')}'`);
   }
 
   getBooleanTypeDeclarationSQL(): string {

packages/postgresql/src/PostgreSqlPlatform.ts

@@ -1,6 +1,6 @@
 import { Client } from 'pg';
 import type { EntityProperty, Type } from '@mikro-orm/core';
-import { JsonProperty, Utils } from '@mikro-orm/core';
+import { expr, JsonProperty, Utils } from '@mikro-orm/core';
 import { AbstractSqlPlatform } from '@mikro-orm/knex';
 import { PostgreSqlSchemaHelper } from './PostgreSqlSchemaHelper';
 import { PostgreSqlExceptionConverter } from './PostgreSqlExceptionConverter';
@@ -107,7 +107,7 @@ export class PostgreSqlPlatform extends AbstractSqlPlatform {
   getSearchJsonPropertyKey(path: string[], type: string): string {
     const first = path.shift();
     const last = path.pop();
-    const root = this.quoteIdentifier(first!);
+    const root = expr(alias => this.quoteIdentifier(`${alias}.${first}`));
     const types = {
       number: 'float8',
       boolean: 'bool',

packages/sqlite/src/SqlitePlatform.ts

@@ -1,7 +1,7 @@
 // @ts-ignore
 import { escape } from 'sqlstring-sqlite';
 import type { EntityProperty } from '@mikro-orm/core';
-import { JsonProperty, Utils } from '@mikro-orm/core';
+import { expr, JsonProperty, Utils } from '@mikro-orm/core';
 import { AbstractSqlPlatform } from '@mikro-orm/knex';
 import { SqliteSchemaHelper } from './SqliteSchemaHelper';
 import { SqliteExceptionConverter } from './SqliteExceptionConverter';
@@ -104,7 +104,7 @@ export class SqlitePlatform extends AbstractSqlPlatform {
 
   getSearchJsonPropertyKey(path: string[], type: string): string {
     const [a, ...b] = path;
-    return `json_extract(${this.quoteIdentifier(a)}, '$.${b.join('.')}')`;
+    return expr(alias => `json_extract(${this.quoteIdentifier(`${alias}.${a}`)}, '$.${b.join('.')}')`);
   }
 
   getDefaultIntegrityRule(): string {

tests/EntityManager.mongo.test.ts

@@ -2,7 +2,7 @@ import { ObjectId } from 'mongodb';
 import c from 'ansi-colors';
 import chalk from 'chalk';
 import type { EntityProperty } from '@mikro-orm/core';
-import { Collection, Configuration, MikroORM, QueryOrder, Reference, wrap, Logger, UniqueConstraintViolationException, IdentityMap } from '@mikro-orm/core';
+import { Collection, Configuration, MikroORM, QueryOrder, Reference, wrap, Logger, UniqueConstraintViolationException, IdentityMap, EntitySchema } from '@mikro-orm/core';
 import { EntityManager, MongoConnection, MongoDriver } from '@mikro-orm/mongodb';
 import { MongoHighlighter } from '@mikro-orm/mongo-highlighter';
 
@@ -2189,6 +2189,15 @@ describe('EntityManagerMongo', () => {
     })).rejects.toThrowError('Mongo driver does not support `host` options, use `clientUrl` instead!');
   });
 
+  test('validation for `_id` PK field name', async () => {
+    const schema = new EntitySchema({ name: 'WrongPrimaryKeyEntity', properties: { id: { type: 'number', primary: true } } });
+    await expect(MikroORM.init({
+      entities: [schema],
+      dbName: 'bar',
+      type: 'mongo',
+    })).rejects.toThrowError(`WrongPrimaryKeyEntity.id has wrong field name, '_id' is required in current driver`);
+  });
+
   test('extracting child condition when populating (GH #1891)', async () => {
     const author = new Author('Jon Snow', 'snow@wall.st');
     const book1 = new Book('My Life on The Wall, part 1', author);

tests/EntityManager.postgre.test.ts

@@ -520,9 +520,9 @@ describe('EntityManagerPostgre', () => {
 
     expect(mock.mock.calls).toHaveLength(4);
     expect(mock.mock.calls[0][0]).toMatch(`select "f0".*, (select 123) as "random" from "foo_bar2" as "f0" where "f0"."id" = 1 limit 1`);
-    expect(mock.mock.calls[1][0]).toMatch(`select "f0".*, (select 123) as "random" from "foo_bar2" as "f0" where ("object_property"->'myPropName'->>'nestedProperty')::float8 = 123 limit 1`);
-    expect(mock.mock.calls[2][0]).toMatch(`select "f0".*, (select 123) as "random" from "foo_bar2" as "f0" where "object_property"->'myPropName'->>'somethingElse' is null limit 1`);
-    expect(mock.mock.calls[3][0]).toMatch(`select "f0".*, (select 123) as "random" from "foo_bar2" as "f0" where ("object_property"->'myPropName'->>'nestedProperty')::float8 = 123 and "object_property"->'myPropName'->>'somethingElse' is null limit 1`);
+    expect(mock.mock.calls[1][0]).toMatch(`select "f0".*, (select 123) as "random" from "foo_bar2" as "f0" where ("f0"."object_property"->'myPropName'->>'nestedProperty')::float8 = 123 limit 1`);
+    expect(mock.mock.calls[2][0]).toMatch(`select "f0".*, (select 123) as "random" from "foo_bar2" as "f0" where "f0"."object_property"->'myPropName'->>'somethingElse' is null limit 1`);
+    expect(mock.mock.calls[3][0]).toMatch(`select "f0".*, (select 123) as "random" from "foo_bar2" as "f0" where ("f0"."object_property"->'myPropName'->>'nestedProperty')::float8 = 123 and "f0"."object_property"->'myPropName'->>'somethingElse' is null limit 1`);
   });
 
   test('findOne should initialize entity that is already in IM', async () => {

tests/QueryBuilder.test.ts

@@ -441,7 +441,7 @@ describe('QueryBuilder', () => {
     const filter = Object.create(null);
     filter.meta = { foo: 'bar' };
     qb1.select('*').where(filter);
-    expect(qb1.getQuery()).toEqual('select `e0`.*, `e0`.price * 1.19 as `price_taxed` from `book2` as `e0` where `meta`->\'$.foo\' = ?');
+    expect(qb1.getQuery()).toEqual('select `e0`.*, `e0`.price * 1.19 as `price_taxed` from `book2` as `e0` where `e0`.`meta`->\'$.foo\' = ?');
     expect(qb1.getParams()).toEqual(['bar']);
   });
 
@@ -1942,19 +1942,19 @@ describe('QueryBuilder', () => {
     expect(qb10.getParams()).toEqual([timestamp, 'ignore@example.com', 'John Doe', timestamp, 'John Doe', timestamp, timestamp]);
 
     const qb11 = pg.em.createQueryBuilder(Book2).where({ meta: { foo: 123 } });
-    expect(qb11.getFormattedQuery()).toBe(`select "b0".*, "b0".price * 1.19 as "price_taxed" from "book2" as "b0" where ("meta"->>'foo')::float8 = 123`);
+    expect(qb11.getFormattedQuery()).toBe(`select "b0".*, "b0".price * 1.19 as "price_taxed" from "book2" as "b0" where ("b0"."meta"->>'foo')::float8 = 123`);
     const qb12 = pg.em.createQueryBuilder(Book2).where({ meta: { foo: { $eq: 123 } } });
-    expect(qb12.getFormattedQuery()).toBe(`select "b0".*, "b0".price * 1.19 as "price_taxed" from "book2" as "b0" where ("meta"->>'foo')::float8 = 123`);
+    expect(qb12.getFormattedQuery()).toBe(`select "b0".*, "b0".price * 1.19 as "price_taxed" from "book2" as "b0" where ("b0"."meta"->>'foo')::float8 = 123`);
     const qb13 = pg.em.createQueryBuilder(Book2).where({ meta: { foo: { $lte: 123 } } });
-    expect(qb13.getFormattedQuery()).toBe(`select "b0".*, "b0".price * 1.19 as "price_taxed" from "book2" as "b0" where ("meta"->>'foo')::float8 <= 123`);
+    expect(qb13.getFormattedQuery()).toBe(`select "b0".*, "b0".price * 1.19 as "price_taxed" from "book2" as "b0" where ("b0"."meta"->>'foo')::float8 <= 123`);
 
     // order by json property
     const qb14 = pg.em.createQueryBuilder(Book2).orderBy({ meta: { foo: 'asc' } });
-    expect(qb14.getFormattedQuery()).toBe(`select "b0".*, "b0".price * 1.19 as "price_taxed" from "book2" as "b0" order by "meta"->>'foo' asc`);
+    expect(qb14.getFormattedQuery()).toBe(`select "b0".*, "b0".price * 1.19 as "price_taxed" from "book2" as "b0" order by "b0"."meta"->>'foo' asc`);
     const qb15 = pg.em.createQueryBuilder(Book2).orderBy({ meta: { bar: { str: 'asc' } } });
-    expect(qb15.getFormattedQuery()).toBe(`select "b0".*, "b0".price * 1.19 as "price_taxed" from "book2" as "b0" order by "meta"->'bar'->>'str' asc`);
+    expect(qb15.getFormattedQuery()).toBe(`select "b0".*, "b0".price * 1.19 as "price_taxed" from "book2" as "b0" order by "b0"."meta"->'bar'->>'str' asc`);
     const qb16 = pg.em.createQueryBuilder(Book2).orderBy({ meta: { bar: { num: QueryOrder.DESC } } });
-    expect(qb16.getFormattedQuery()).toBe(`select "b0".*, "b0".price * 1.19 as "price_taxed" from "book2" as "b0" order by "meta"->'bar'->>'num' desc`);
+    expect(qb16.getFormattedQuery()).toBe(`select "b0".*, "b0".price * 1.19 as "price_taxed" from "book2" as "b0" order by "b0"."meta"->'bar'->>'num' desc`);
 
     // pessimistic locking
     await pg.em.transactional(async em => {