feat(core): respect naming strategy and explicit field names on embed…

…ded properties (#4866)

This is breaking mainly for SQL drivers, where the default naming
strategy is underscoring, and will now applied to the embedded
properties too. You can restore to the old behaviour by implementing
custom naming strategy, overriding the `propertyToColumnName` method. It
now has a second boolean parameter to indicate if the property is
defined inside a JSON object context.

```ts
import { UnderscoreNamingStrategy } from '@mikro-orm/core';

class CustomNamingStrategy extends UnderscoreNamingStrategy {

   propertyToColumnName(propertyName: string, object?: boolean): string {
      if (object) {
         return propertyName;
      }

      return super.propertyToColumnName(propertyName, object);
   }

}
```

Closes #4371
Closes #2165
Closes #2361

.github/workflows/tests.yml

@@ -150,35 +150,15 @@ jobs:
       - name: Set CC Required env vars
         run: export GIT_BRANCH=$GITHUB_HEAD_REF && export GIT_COMMIT_SHA=$(git rev-parse origin/$GITHUB_HEAD_REF)
 
-      - name: Check for changes
-        id: changed_packages
-        run: |
-          echo "::set-output name=changed_packages::$(node ./node_modules/.bin/lerna changed -p | wc -l | xargs)"
-
-      - name: Check for changes 2
-        if: steps.changed_packages.outputs.changed_packages == '0'
-        run: |
-          echo "no changes detected by lerna"
-
       - name: Test
-        if: steps.changed_packages.outputs.changed_packages != '0'
         run: |
           yarn coverage > COVERAGE_RESULT
           echo "$(cat COVERAGE_RESULT)"
           git status && git diff
 
       - name: Codecov
-        if: steps.changed_packages.outputs.changed_packages != '0'
         uses: codecov/codecov-action@v3
 
-#      - name: Codeclimate
-#        if: steps.changed_packages.outputs.changed_packages != '0'
-#        uses: paambaati/codeclimate-action@v4.0.0
-#        env:
-#          CC_TEST_REPORTER_ID: e2a39c5dc1a13674e97e94a467bacfaec953814982c7de89e9f0b55031e43bd8
-#        with:
-#          coverageCommand: echo "$(cat COVERAGE_RESULT)"
-
       - name: Teardown docker
         run: docker-compose down
 

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

@@ -397,3 +397,23 @@ Additional join conditions used to be implicitly aliased to the root entity, now
 // the `name` used to resolve to `b.name`, now it will resolve to `a.name` instead
 qb.join('b.author', 'a', { name: 'foo' });
 ```
+
+## Embedded properties respect `NamingStrategy`
+
+This is breaking mainly for SQL drivers, where the default naming strategy is underscoring, and will now applied to the embedded properties too. You can restore to the old behaviour by implementing custom naming strategy, overriding the `propertyToColumnName` method. It now has a second boolean parameter to indicate if the property is defined inside a JSON object context.
+
+```ts
+import { UnderscoreNamingStrategy } from '@mikro-orm/core';
+
+class CustomNamingStrategy extends UnderscoreNamingStrategy {
+
+   propertyToColumnName(propertyName: string, object?: boolean): string {
+      if (object) {
+         return propertyName;
+      }
+
+      return super.propertyToColumnName(propertyName, object);
+   }
+
+}
+```

packages/core/src/drivers/DatabaseDriver.ts

@@ -26,7 +26,7 @@ import type {
 } from '../typings';
 import type { MetadataStorage } from '../metadata';
 import type { Connection, QueryResult, Transaction } from '../connections';
-import { EntityComparator, Utils, type Configuration, type ConnectionOptions, Cursor } from '../utils';
+import { EntityComparator, Utils, type Configuration, type ConnectionOptions, Cursor, raw } from '../utils';
 import { type QueryOrder, ReferenceKind, type QueryOrderKeys, QueryOrderNumeric } from '../enums';
 import type { Platform } from '../platforms';
 import type { Collection } from '../entity/Collection';
@@ -260,7 +260,81 @@ export abstract class DatabaseDriver<C extends Connection> implements IDatabaseD
     } as FilterQuery<T>;
   }
 
+  /** @internal */
+  mapDataToFieldNames(data: Dictionary, stringifyJsonArrays: boolean, properties?: Record<string, EntityProperty>, convertCustomTypes?: boolean, object?: boolean) {
+    if (!properties || data == null) {
+      return data;
+    }
+
+    data = Object.assign({}, data); // copy first
+
+    Object.keys(data).forEach(k => {
+      const prop = properties[k];
+
+      if (!prop) {
+        return;
+      }
+
+      if (prop.embeddedProps && !prop.object && !object) {
+        const copy = data[k];
+        delete data[k];
+        Object.assign(data, this.mapDataToFieldNames(copy, stringifyJsonArrays, prop.embeddedProps, convertCustomTypes));
+
+        return;
+      }
+
+      if (prop.embeddedProps && (prop.object || object)) {
+        const copy = data[k];
+        delete data[k];
+
+        if (prop.array) {
+          data[prop.fieldNames[0]] = copy.map((item: Dictionary) => this.mapDataToFieldNames(item, stringifyJsonArrays, prop.embeddedProps, convertCustomTypes, true));
+        } else {
+          data[prop.fieldNames[0]] = this.mapDataToFieldNames(copy, stringifyJsonArrays, prop.embeddedProps, convertCustomTypes, true);
+        }
+
+        if (stringifyJsonArrays && prop.array) {
+          data[prop.fieldNames[0]] = this.platform.convertJsonToDatabaseValue(data[prop.fieldNames[0]]);
+        }
+
+        return;
+      }
+
+      if (prop.joinColumns && Array.isArray(data[k])) {
+        const copy = Utils.flatten(data[k]);
+        delete data[k];
+        prop.joinColumns.forEach((joinColumn, idx) => data[joinColumn] = copy[idx]);
+
+        return;
+      }
+
+      if (prop.customType && convertCustomTypes && !this.platform.isRaw(data[k])) {
+        data[k] = prop.customType.convertToDatabaseValue(data[k], this.platform, { fromQuery: true, key: k, mode: 'query-data' });
+      }
+
+      if (prop.hasConvertToDatabaseValueSQL && !this.platform.isRaw(data[k])) {
+        const quoted = this.platform.quoteValue(data[k]);
+        const sql = prop.customType.convertToDatabaseValueSQL!(quoted, this.platform);
+        data[k] = raw(sql.replace(/\?/g, '\\?'));
+      }
+
+      if (!prop.customType && (Array.isArray(data[k]) || Utils.isPlainObject(data[k]))) {
+        data[k] = JSON.stringify(data[k]);
+      }
+
+      if (prop.fieldNames) {
+        Utils.renameKey(data, k, prop.fieldNames[0]);
+      }
+    });
+
+    return data;
+  }
+
   protected inlineEmbeddables<T extends object>(meta: EntityMetadata<T>, data: T, where?: boolean): void {
+    if (data == null) {
+      return;
+    }
+
     Utils.keys(data).forEach(k => {
       if (Utils.isOperator(k as string)) {
         Utils.asArray(data[k]).forEach(payload => this.inlineEmbeddables(meta, payload as T, where));

packages/core/src/metadata/MetadataDiscovery.ts

@@ -420,13 +420,13 @@ export class MetadataDiscovery {
     }
   }
 
-  private initFieldName(prop: EntityProperty): void {
+  private initFieldName(prop: EntityProperty, object = false): void {
     if (prop.fieldNames && prop.fieldNames.length > 0) {
       return;
     }
 
-    if (prop.kind === ReferenceKind.SCALAR || (prop.kind === ReferenceKind.EMBEDDED && prop.object)) {
-      prop.fieldNames = [this.namingStrategy.propertyToColumnName(prop.name)];
+    if (prop.kind === ReferenceKind.SCALAR || prop.kind === ReferenceKind.EMBEDDED) {
+      prop.fieldNames = [this.namingStrategy.propertyToColumnName(prop.name, object)];
     } else if ([ReferenceKind.MANY_TO_ONE, ReferenceKind.ONE_TO_ONE].includes(prop.kind)) {
       prop.fieldNames = this.initManyToOneFieldName(prop, prop.name);
     } else if (prop.kind === ReferenceKind.MANY_TO_MANY && prop.owner) {
@@ -877,9 +877,19 @@ export class MetadataDiscovery {
     embeddedProp.embeddedProps = {};
     let order = meta.propertyOrder.get(embeddedProp.name)!;
     const getRootProperty: (prop: EntityProperty) => EntityProperty = (prop: EntityProperty) => prop.embedded ? getRootProperty(meta.properties[prop.embedded[0]]) : prop;
+    const isParentObject: (prop: EntityProperty) => boolean = (prop: EntityProperty) => {
+      if (prop.object || prop.array) {
+        return true;
+      }
+
+      return prop.embedded ? isParentObject(meta.properties[prop.embedded[0]]) : false;
+    };
+    const rootProperty = getRootProperty(embeddedProp);
+    const object = isParentObject(embeddedProp);
+    this.initFieldName(embeddedProp, rootProperty !== embeddedProp && object);
+    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 prefix = embeddedProp.prefix === false ? '' : embeddedProp.prefix === true ? embeddedProp.name + '_' : embeddedProp.prefix;
       const name = prefix + prop.name;
 
       if (meta.properties[name] !== undefined && getRootProperty(meta.properties[name]).kind !== ReferenceKind.EMBEDDED) {
@@ -896,18 +906,16 @@ export class MetadataDiscovery {
         meta.properties[name].nullable = true;
       }
 
-      const isParentObject: (prop: EntityProperty) => boolean = (prop: EntityProperty) => {
-        if (prop.object) {
-          return 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]);
         }
+      }
 
-        return prop.embedded ? isParentObject(meta.properties[prop.embedded[0]]) : false;
-      };
-      const rootProperty = getRootProperty(embeddedProp);
-
-      if (isParentObject(embeddedProp)) {
+      if (object) {
         embeddedProp.object = true;
-        this.initFieldName(embeddedProp);
         let path: string[] = [];
         let tmp = embeddedProp;
 
@@ -917,13 +925,17 @@ export class MetadataDiscovery {
         }
 
         if (tmp === rootProperty) {
-          path.unshift(this.namingStrategy.propertyToColumnName(rootProperty.name));
+          path.unshift(rootProperty.fieldNames[0]);
+        } else if (embeddedProp.embeddedPath) {
+          path = [...embeddedProp.embeddedPath];
         } else {
           path = [embeddedProp.fieldNames[0]];
         }
 
-        path.push(prop.name);
-        meta.properties[name].fieldNames = [path.join('.')]; // store path for ObjectHydrator
+        this.initFieldName(prop, true);
+        path.push(prop.fieldNames[0]);
+        meta.properties[name].fieldNames = prop.fieldNames;
+        meta.properties[name].embeddedPath = path;
         const fieldName = raw(this.platform.getSearchJsonPropertySQL(path.join('->'), prop.type, true));
         meta.properties[name].fieldNameRaw = fieldName.sql; // for querying in SQL drivers
         meta.properties[name].persist = false; // only virtual as we store the whole object

packages/core/src/naming-strategy/AbstractNamingStrategy.ts

@@ -59,7 +59,7 @@ export abstract class AbstractNamingStrategy implements NamingStrategy {
 
   abstract joinTableName(sourceEntity: string, targetEntity: string, propertyName?: string): string;
 
-  abstract propertyToColumnName(propertyName: string): string;
+  abstract propertyToColumnName(propertyName: string, object?: boolean): string;
 
   abstract referenceColumnName(): string;
 

packages/core/src/naming-strategy/NamingStrategy.ts

@@ -18,7 +18,7 @@ export interface NamingStrategy {
   /**
    * Return a column name for a property
    */
-  propertyToColumnName(propertyName: string): string;
+  propertyToColumnName(propertyName: string, object?: boolean): string;
 
   /**
    * Return a property for a column name (used in `EntityGenerator`).

packages/core/src/naming-strategy/UnderscoreNamingStrategy.ts

@@ -18,7 +18,7 @@ export class UnderscoreNamingStrategy extends AbstractNamingStrategy {
     return this.classToTableName(sourceEntity) + '_' + this.classToTableName(propertyName);
   }
 
-  propertyToColumnName(propertyName: string): string {
+  propertyToColumnName(propertyName: string, object?: boolean): string {
     return this.underscore(propertyName);
   }
 

packages/core/src/typings.ts

@@ -343,6 +343,7 @@ export interface EntityProperty<Owner = any, Target = any> {
   formula?: (alias: string) => string;
   prefix?: string | boolean;
   embedded?: [EntityKey<Owner>, EntityKey<Owner>];
+  embeddedPath?: string[];
   embeddable: Constructor<Owner>;
   embeddedProps: Dictionary<EntityProperty>;
   discriminatorColumn?: string; // only for poly embeddables currently

packages/core/src/utils/EntityComparator.ts

@@ -319,7 +319,7 @@ export class EntityComparator {
         return;
       }
 
-      if (prop.embedded && meta.properties[prop.embedded[0]].object && prop.runtimeType !== 'Date') {
+      if (prop.embedded && (meta.embeddable || meta.properties[prop.embedded[0]].object)) {
         return;
       }
 
@@ -329,33 +329,23 @@ export class EntityComparator {
         lines.push(`    ${propName(prop.fieldNames[0], 'mapped')} = true;`);
         lines.push(`  }`);
       } else if (prop.runtimeType === 'Date') {
-        if (prop.embedded && meta.properties[prop.embedded[0]].array) {
-          const parentKey = 'ret.' + meta.properties[prop.embedded[0]].fieldNames[0];
-          const idx = this.tmpIndex++;
-          lines.push(`  if (Array.isArray(${parentKey.replace(/\./g, '?.')})) {`);
-          lines.push(`    ${parentKey}.forEach((item_${idx}, idx_${idx}) => {`);
-          const childProp = this.wrap(prop.embedded[1]);
-          lines.push(`      if (typeof item_${idx}${childProp} !== 'undefined') {`);
-          parseDate(`${parentKey}[idx_${idx}]${childProp}`, `${parentKey}[idx_${idx}]${childProp}`, '    ');
-          lines.push(`      }`);
-          lines.push(`    });`);
-          lines.push(`  }`);
-        } else if (prop.embedded && meta.properties[prop.embedded[0]].object) {
-          const entityKey = 'ret.' + prop.fieldNames[0];
-          const entityKeyOptional = 'ret.' + prop.fieldNames[0].replace(/\./g, '?.');
-          lines.push(`  if (typeof ${entityKeyOptional} !== 'undefined') {`);
-          parseDate('ret.' + prop.fieldNames[0], entityKey);
-          lines.push(`  }`);
-        } else {
-          lines.push(`  if (typeof ${propName(prop.fieldNames[0])} !== 'undefined') {`);
-          parseDate('ret' + this.wrap(prop.name), propName(prop.fieldNames[0]));
-          lines.push(`    ${propName(prop.fieldNames[0], 'mapped')} = true;`);
-          lines.push(`  }`);
-        }
-      } else if (prop.kind === ReferenceKind.EMBEDDED && prop.object && !this.platform.convertsJsonAutomatically()) {
-        context.set('parseJsonSafe', parseJsonSafe);
         lines.push(`  if (typeof ${propName(prop.fieldNames[0])} !== 'undefined') {`);
-        lines.push(`    ret${this.wrap(prop.name)} = ${propName(prop.fieldNames[0])} == null ? ${propName(prop.fieldNames[0])} : parseJsonSafe(${propName(prop.fieldNames[0])});`);
+        parseDate('ret' + this.wrap(prop.name), propName(prop.fieldNames[0]));
+        lines.push(`    ${propName(prop.fieldNames[0], 'mapped')} = true;`);
+        lines.push(`  }`);
+      } else if (prop.kind === ReferenceKind.EMBEDDED && (prop.object || meta.embeddable)) {
+        const idx = this.tmpIndex++;
+        context.set(`mapEmbeddedResult_${idx}`, (data: Dictionary) => {
+          const item = parseJsonSafe(data);
+
+          if (Array.isArray(item)) {
+            return item.map(row => row == null ? row : this.getResultMapper(prop.type)(row));
+          }
+
+          return item == null ? item : this.getResultMapper(prop.type)(item);
+        });
+        lines.push(`  if (typeof ${propName(prop.fieldNames[0])} !== 'undefined') {`);
+        lines.push(`    ret${this.wrap(prop.name)} = ${propName(prop.fieldNames[0])} == null ? ${propName(prop.fieldNames[0])} : mapEmbeddedResult_${idx}(${propName(prop.fieldNames[0])});`);
         lines.push(`    ${propName(prop.fieldNames[0], 'mapped')} = true;`);
         lines.push(`  }`);
       } else {
@@ -369,21 +359,22 @@ export class EntityComparator {
 
     const code = `// compiled mapper for entity ${meta.className}\n`
       + `return function(result) {\n  const ret = {};\n${lines.join('\n')}\n  return ret;\n}`;
-    const snapshotGenerator = Utils.createFunction(context, code);
-    this.mappers.set(entityName, snapshotGenerator);
+    const resultMapper = Utils.createFunction(context, code);
+    this.mappers.set(entityName, resultMapper);
 
-    return snapshotGenerator;
+    return resultMapper;
   }
 
-  private getPropertyCondition<T>(prop: EntityProperty<T>, entityKey: string, path: string[]): string {
+  private getPropertyCondition(path: string[]): string {
     const parts = path.slice(); // copy first
 
     if (parts.length > 1) {
       parts.pop();
     }
 
     let tail = '';
-    const ret = parts
+
+    return parts
       .map(k => {
         if (k.match(/^\[idx_\d+]$/)) {
           tail += k;
@@ -397,8 +388,6 @@ export class EntityComparator {
       })
       .filter(k => k)
       .join(' && ');
-
-    return ret;
   }
 
   private getEmbeddedArrayPropertySnapshot<T>(meta: EntityMetadata<T>, prop: EntityProperty<T>, context: Map<string, any>, level: number, path: string[], dataKey: string): string {
@@ -464,7 +453,7 @@ export class EntityComparator {
     }
 
     ret += meta.props.filter(p => p.embedded?.[0] === prop.name).map(childProp => {
-      const childDataKey = prop.object ? dataKey + this.wrap(childProp.embedded![1]) : this.wrap(childProp.name);
+      const childDataKey = meta.embeddable || prop.object ? dataKey + this.wrap(childProp.embedded![1]) : this.wrap(childProp.name);
       const childEntityKey = [...path, childProp.embedded![1]].map(k => this.wrap(k)).join('');
       const childCond = `typeof entity${childEntityKey} !== 'undefined'`;
 
@@ -501,7 +490,7 @@ export class EntityComparator {
   private getPropertySnapshot<T>(meta: EntityMetadata<T>, prop: EntityProperty<T>, context: Map<string, any>, dataKey: string, entityKey: string, path: string[], level = 1, object?: boolean): string {
     const convertorKey = this.safeKey(prop.name);
     const unwrap = prop.ref ? '?.unwrap()' : '';
-    let ret = `  if (${this.getPropertyCondition(prop, entityKey, path)}) {\n`;
+    let ret = `  if (${this.getPropertyCondition(path)}) {\n`;
 
     if (['number', 'string', 'boolean'].includes(prop.type.toLowerCase())) {
       return ret + `    ret${dataKey} = entity${entityKey}${unwrap};\n  }\n`;