fix(core): hydrate embeddable scalar properties

[dvlalex]

This PR fixes the issue with embeddables not having their inner properties hydrated properly.

fixes #1191

[B4nan]

Looking good, let's wait for the CI

[murbanowicz]

@dvlalex Can you add this or similar test?

import {
  Embeddable,
  Embedded,
  Entity,
  Logger,
  MikroORM,
  Platform,
  PrimaryKey,
  Property,
  Type,
} from '@mikro-orm/core';
import { PostgreSqlDriver, PostgreSqlPlatform } from '@mikro-orm/postgresql';

export class Numeric extends Type<number, string> {

  public convertToDatabaseValue(value: number, platform: Platform): string {
    this.validatePlatformSupport(platform);
    return value.toString();
  }

  public convertToJSValue(value: string, platform: Platform): number {
    this.validatePlatformSupport(platform);
    return Number(value);
  }

  public getColumnType(): string {
    return 'numeric(14,2)';
  }

  private validatePlatformSupport(platform: Platform): void {
    if (!(platform instanceof PostgreSqlPlatform)) {
      throw new Error('Numeric custom type implemented only for PG.');
    }
  }

}

@Embeddable()
class Savings {

  @Property({ type: Numeric })
  amount: number;

  constructor(amount: number) {
    this.amount = amount;
  }

}

@Entity()
class User {

  @PrimaryKey()
  id!: number;

  @Embedded()
  savings!: Savings;

  @Property({ nullable: true })
  after?: number; // property after embeddables to verify order props in resulting schema

}

describe('embedded entities in postgresql', () => {
  let orm: MikroORM<PostgreSqlDriver>;

  beforeAll(async () => {
    orm = await MikroORM.init({
      entities: [Savings, User],
      dbName: 'mikro_orm_test_embeddables_custom_types',
      type: 'postgresql',
    });
    await orm.getSchemaGenerator().ensureDatabase();
    await orm.getSchemaGenerator().dropSchema();
    await orm.getSchemaGenerator().createSchema();
  });

  afterAll(() => orm.close(true));

  test('schema', async () => {
    await expect(
      orm.getSchemaGenerator().getCreateSchemaSQL(false)
    ).resolves.toMatchSnapshot('embeddables custom types 1');
    await expect(
      orm.getSchemaGenerator().getUpdateSchemaSQL(false)
    ).resolves.toMatchSnapshot('embeddables custom types 2');
    await expect(
      orm.getSchemaGenerator().getDropSchemaSQL(false)
    ).resolves.toMatchSnapshot('embeddables custom types 3');
  });

  test('persist and load', async () => {
    const user = new User();
    user.savings = new Savings(15200.23);

    const mock = jest.fn();
    const logger = new Logger(mock, ['query']);
    Object.assign(orm.config, { logger });
    await orm.em.persistAndFlush(user);
    orm.em.clear();
    expect(mock.mock.calls[0][0]).toMatch('begin');
    expect(mock.mock.calls[2][0]).toMatch('commit');

    const u = await orm.em.findOneOrFail(User, user.id);
    expect(mock.mock.calls[3][0]).toMatch(
      'select "e0".* from "user" as "e0" where "e0"."id" = $1 limit $2'
    );
    expect(u.savings).toBeInstanceOf(Savings);
    expect(u.savings).toEqual({
      amount: 15200.23,
    });

    await orm.em.flush();
    expect(mock.mock.calls[4][0]).toMatch('begin');
    expect(mock.mock.calls[5][0]).toMatch(
      'update "user" set "addr_postal_code" = $1, "address4" = $2 where "id" = $3'
    );
    expect(mock.mock.calls[6][0]).toMatch('commit');
    orm.em.clear();

    const u1 = await orm.em.findOneOrFail(User, {
      savings: { amount: 15200.23 },
    });
    expect(mock.mock.calls[7][0]).toMatch(
      'select "e0".* from "user" as "e0" where "e0"."address1_city" = $1 and "e0"."address1_postal_code" = $2 limit $3'
    );
    expect(u1.savings.amount).toBe(15200.23);
  });
});

The reason is that I see Mongo test, but we all know mongo is pretty different, so I'd love to see this test for PG. Can be simplified to something similar as you've done for mongo.

[dvlalex]

@murbanowicz I had tested with your type initially, but then moved to mongo because I had to update the snapshots for postgres, and didn't want to do so.

[murbanowicz]

@dvlalex you can do whatI did when trying to write this test - add it as separate one? @B4nan what you think?

[B4nan]

Yeah sure, you can add it to the rest of the GH issue tests, those are isolated from the rest:

https://github.com/mikro-orm/mikro-orm/tree/master/tests/issues

[dvlalex]

Except for your logger, which is not called with the correct indexes, the test passes:

$ jest --runInBand -i GH1191
 PASS  tests/issues/GH1191.test.ts (5.106 s)
  embedded entities in postgresql
    ✓ schema (31 ms)
    ✓ persist and load (40 ms)

Test Suites: 1 passed, 1 total
Tests:       2 passed, 2 total
Snapshots:   3 passed, 3 total
Time:        5.652 s
Ran all test suites matching /GH1191/i.

[murbanowicz]

So, as Martin suggested, I'd be more than happy if you would commit it to make it stay with us :)
Thank you for your amazing work!

[murbanowicz]

The logger part can be removed as it's covered in other tests.

[murbanowicz]

@dvlalex @B4nan I am amazed how quickly was it solved. Are you able to merge it, so I could temporarily switch us to dev version and use it?

[B4nan]

Use the dev version for now (will be available in few minutes as dev.18). Will ship stable later this week probably.