From 83d3ab27f63216aab385500ab73639fa39dcfe90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ad=C3=A1mek?= Date: Tue, 10 Nov 2020 00:29:21 +0100 Subject: [PATCH] fix(sql): convert custom types at query builder level --- packages/core/src/drivers/DatabaseDriver.ts | 8 ++-- packages/core/src/drivers/IDatabaseDriver.ts | 8 ++-- .../src/unit-of-work/ChangeSetPersister.ts | 16 ++++---- packages/knex/src/AbstractSqlDriver.ts | 39 ++++++++++--------- packages/knex/src/query/QueryBuilder.ts | 22 +++++++++-- packages/knex/src/query/QueryBuilderHelper.ts | 8 +++- tests/EntityManager.mongo.test.ts | 3 ++ tests/EntityManager.mysql.test.ts | 3 ++ tests/EntityManager.postgre.test.ts | 3 ++ tests/EntityManager.sqlite2.test.ts | 3 ++ tests/QueryBuilder.test.ts | 8 ++++ tests/entities/FooBar.ts | 4 +- 12 files changed, 84 insertions(+), 41 deletions(-) diff --git a/packages/core/src/drivers/DatabaseDriver.ts b/packages/core/src/drivers/DatabaseDriver.ts index 56e9d145516b..bcfbf49bac67 100644 --- a/packages/core/src/drivers/DatabaseDriver.ts +++ b/packages/core/src/drivers/DatabaseDriver.ts @@ -28,13 +28,13 @@ export abstract class DatabaseDriver implements IDatabaseD abstract async findOne>(entityName: string, where: FilterQuery, options?: FindOneOptions, ctx?: Transaction): Promise | null>; - abstract async nativeInsert>(entityName: string, data: EntityData, ctx?: Transaction): Promise; + abstract async nativeInsert>(entityName: string, data: EntityData, ctx?: Transaction, convertCustomTypes?: boolean): Promise; - abstract async nativeInsertMany>(entityName: string, data: EntityData[], ctx?: Transaction, processCollections?: boolean): Promise; + abstract async nativeInsertMany>(entityName: string, data: EntityData[], ctx?: Transaction, processCollections?: boolean, convertCustomTypes?: boolean): Promise; - abstract async nativeUpdate>(entityName: string, where: FilterQuery, data: EntityData, ctx?: Transaction): Promise; + abstract async nativeUpdate>(entityName: string, where: FilterQuery, data: EntityData, ctx?: Transaction, convertCustomTypes?: boolean): Promise; - async nativeUpdateMany>(entityName: string, where: FilterQuery[], data: EntityData[], ctx?: Transaction, processCollections?: boolean): Promise { + async nativeUpdateMany>(entityName: string, where: FilterQuery[], data: EntityData[], ctx?: Transaction, processCollections?: boolean, convertCustomTypes?: boolean): Promise { throw new Error(`Batch updates are not supported by ${this.constructor.name} driver`); } diff --git a/packages/core/src/drivers/IDatabaseDriver.ts b/packages/core/src/drivers/IDatabaseDriver.ts index 7aa081edca96..a29256f0afdf 100644 --- a/packages/core/src/drivers/IDatabaseDriver.ts +++ b/packages/core/src/drivers/IDatabaseDriver.ts @@ -33,13 +33,13 @@ export interface IDatabaseDriver { */ findOne>(entityName: string, where: FilterQuery, options?: FindOneOptions, ctx?: Transaction): Promise | null>; - nativeInsert>(entityName: string, data: EntityData, ctx?: Transaction): Promise; + nativeInsert>(entityName: string, data: EntityData, ctx?: Transaction, convertCustomTypes?: boolean): Promise; - nativeInsertMany>(entityName: string, data: EntityData[], ctx?: Transaction, processCollections?: boolean): Promise; + nativeInsertMany>(entityName: string, data: EntityData[], ctx?: Transaction, processCollections?: boolean, convertCustomTypes?: boolean): Promise; - nativeUpdate>(entityName: string, where: FilterQuery, data: EntityData, ctx?: Transaction): Promise; + nativeUpdate>(entityName: string, where: FilterQuery, data: EntityData, ctx?: Transaction, convertCustomTypes?: boolean): Promise; - nativeUpdateMany>(entityName: string, where: FilterQuery[], data: EntityData[], ctx?: Transaction, processCollections?: boolean): Promise; + nativeUpdateMany>(entityName: string, where: FilterQuery[], data: EntityData[], ctx?: Transaction, processCollections?: boolean, convertCustomTypes?: boolean): Promise; nativeDelete>(entityName: string, where: FilterQuery, ctx?: Transaction): Promise; diff --git a/packages/core/src/unit-of-work/ChangeSetPersister.ts b/packages/core/src/unit-of-work/ChangeSetPersister.ts index 480a535cd482..3367ac7ffc04 100644 --- a/packages/core/src/unit-of-work/ChangeSetPersister.ts +++ b/packages/core/src/unit-of-work/ChangeSetPersister.ts @@ -65,7 +65,7 @@ export class ChangeSetPersister { private async persistNewEntity>(meta: EntityMetadata, changeSet: ChangeSet, ctx?: Transaction): Promise { const wrapped = changeSet.entity.__helper!; - const res = await this.driver.nativeInsert(changeSet.name, changeSet.payload, ctx); + const res = await this.driver.nativeInsert(changeSet.name, changeSet.payload, ctx, false); if (!wrapped.hasPrimaryKey()) { this.mapPrimaryKey(meta, res.insertId, changeSet); @@ -93,7 +93,7 @@ export class ChangeSetPersister { } private async persistNewEntitiesBatch>(meta: EntityMetadata, changeSets: ChangeSet[], ctx?: Transaction): Promise { - const res = await this.driver.nativeInsertMany(meta.className, changeSets.map(cs => cs.payload), ctx, false); + const res = await this.driver.nativeInsertMany(meta.className, changeSets.map(cs => cs.payload), ctx, false, false); for (let i = 0; i < changeSets.length; i++) { const changeSet = changeSets[i]; @@ -131,13 +131,13 @@ export class ChangeSetPersister { private async persistManagedEntitiesBatch>(meta: EntityMetadata, changeSets: ChangeSet[], ctx?: Transaction): Promise { await this.checkOptimisticLocks(meta, changeSets, ctx); - await this.driver.nativeUpdateMany(meta.className, changeSets.map(cs => cs.entity.__helper!.getPrimaryKey() as Dictionary), changeSets.map(cs => cs.payload), ctx, false); + await this.driver.nativeUpdateMany(meta.className, changeSets.map(cs => cs.entity.__helper!.getPrimaryKey() as Dictionary), changeSets.map(cs => cs.payload), ctx, false, false); changeSets.forEach(cs => cs.persisted = true); } private mapPrimaryKey>(meta: EntityMetadata, value: IPrimaryKey, changeSet: ChangeSet): void { const prop = meta.properties[meta.primaryKeys[0]]; - const insertId = prop.customType ? prop.customType.convertToJSValue(value, this.driver.getPlatform()) : value; + const insertId = prop.customType ? prop.customType.convertToJSValue(value, this.platform) : value; const wrapped = changeSet.entity.__helper!; if (!wrapped.hasPrimaryKey()) { @@ -147,7 +147,7 @@ export class ChangeSetPersister { // some drivers might be returning bigint PKs as numbers when the number is small enough, // but we need to have it as string so comparison works in change set tracking, so instead // of using the raw value from db, we convert it back to the db value explicitly - value = prop.customType ? prop.customType.convertToDatabaseValue(insertId, this.driver.getPlatform()) : value; + value = prop.customType ? prop.customType.convertToDatabaseValue(insertId, this.platform) : value; changeSet.payload[wrapped.__meta.primaryKeys[0]] = value; wrapped.__identifier!.setValue(value); } @@ -174,7 +174,7 @@ export class ChangeSetPersister { private async updateEntity>(meta: EntityMetadata, changeSet: ChangeSet, ctx?: Transaction): Promise { if (!meta.versionProperty || !changeSet.entity[meta.versionProperty]) { - return this.driver.nativeUpdate(changeSet.name, changeSet.entity.__helper!.getPrimaryKey() as Dictionary, changeSet.payload, ctx); + return this.driver.nativeUpdate(changeSet.name, changeSet.entity.__helper!.getPrimaryKey() as Dictionary, changeSet.payload, ctx, false); } const cond = { @@ -182,7 +182,7 @@ export class ChangeSetPersister { [meta.versionProperty]: this.platform.quoteVersionValue(changeSet.entity[meta.versionProperty] as unknown as Date, meta.properties[meta.versionProperty]), } as FilterQuery; - return this.driver.nativeUpdate(changeSet.name, cond, changeSet.payload, ctx); + return this.driver.nativeUpdate(changeSet.name, cond, changeSet.payload, ctx, false); } private async checkOptimisticLocks>(meta: EntityMetadata, changeSets: ChangeSet[], ctx?: Transaction): Promise { @@ -257,7 +257,7 @@ export class ChangeSetPersister { } if (changeSet.payload[prop.name] as unknown instanceof Date) { - changeSet.payload[prop.name] = this.driver.getPlatform().processDateProperty(changeSet.payload[prop.name]); + changeSet.payload[prop.name] = this.platform.processDateProperty(changeSet.payload[prop.name]); } } diff --git a/packages/knex/src/AbstractSqlDriver.ts b/packages/knex/src/AbstractSqlDriver.ts index 461931d34e65..248660e2ef2b 100644 --- a/packages/knex/src/AbstractSqlDriver.ts +++ b/packages/knex/src/AbstractSqlDriver.ts @@ -38,7 +38,7 @@ export abstract class AbstractSqlDriver(entityName)!; const populate = this.autoJoinOneToOneOwner(meta, options.populate as PopulateOptions[], options.fields); const joinedProps = this.joinedProps(meta, populate); - const qb = this.createQueryBuilder(entityName, ctx, !!ctx).unsetFlag(QueryFlag.CONVERT_CUSTOM_TYPES); + const qb = this.createQueryBuilder(entityName, ctx, !!ctx, false); const fields = this.buildFields(meta, populate, joinedProps, qb, options.fields); if (Utils.isPrimaryKey(where, meta.compositePK)) { @@ -166,8 +166,7 @@ export abstract class AbstractSqlDriver>(entityName: string, where: any, options: CountOptions = {}, ctx?: Transaction): Promise { const pks = this.metadata.find(entityName)!.primaryKeys; - const qb = this.createQueryBuilder(entityName, ctx, !!ctx) - .unsetFlag(QueryFlag.CONVERT_CUSTOM_TYPES) + const qb = this.createQueryBuilder(entityName, ctx, !!ctx, false) .count(pks, true) .groupBy(options.groupBy!) .having(options.having!) @@ -178,11 +177,11 @@ export abstract class AbstractSqlDriver>(entityName: string, data: EntityData, ctx?: Transaction): Promise { + async nativeInsert>(entityName: string, data: EntityData, ctx?: Transaction, convertCustomTypes = true): Promise { const meta = this.metadata.find(entityName); const collections = this.extractManyToMany(entityName, data); const pks = this.getPrimaryKeyFields(entityName); - const qb = this.createQueryBuilder(entityName, ctx, true); + const qb = this.createQueryBuilder(entityName, ctx, true, convertCustomTypes); const res = await this.rethrow(qb.insert(data).execute('run', false)); res.row = res.row || {}; let pk: any; @@ -200,7 +199,7 @@ export abstract class AbstractSqlDriver>(entityName: string, data: EntityData[], ctx?: Transaction, processCollections = true): Promise { + async nativeInsertMany>(entityName: string, data: EntityData[], ctx?: Transaction, processCollections = true, convertCustomTypes = true): Promise { const meta = this.metadata.get(entityName); const collections = processCollections ? data.map(d => this.extractManyToMany(entityName, d)) : []; const pks = this.getPrimaryKeyFields(entityName); @@ -211,7 +210,7 @@ export abstract class AbstractSqlDriver>(entityName: string, where: FilterQuery, data: EntityData, ctx?: Transaction): Promise { + async nativeUpdate>(entityName: string, where: FilterQuery, data: EntityData, ctx?: Transaction, convertCustomTypes = true): Promise { const meta = this.metadata.find(entityName); const pks = this.getPrimaryKeyFields(entityName); const collections = this.extractManyToMany(entityName, data); @@ -267,8 +266,7 @@ export abstract class AbstractSqlDriver(entityName, ctx, true) - .unsetFlag(QueryFlag.CONVERT_CUSTOM_TYPES) + const qb = this.createQueryBuilder(entityName, ctx, true, convertCustomTypes) .update(data) .where(where); @@ -281,7 +279,7 @@ export abstract class AbstractSqlDriver>(entityName: string, where: FilterQuery[], data: EntityData[], ctx?: Transaction, processCollections = true): Promise { + async nativeUpdateMany>(entityName: string, where: FilterQuery[], data: EntityData[], ctx?: Transaction, processCollections = true, convertCustomTypes = true): Promise { const meta = this.metadata.get(entityName); const collections = processCollections ? data.map(d => this.extractManyToMany(entityName, d)) : []; const keys = new Set(); @@ -338,7 +336,7 @@ export abstract class AbstractSqlDriver>(entityName: string, ctx?: Transaction, write?: boolean): QueryBuilder { - return new QueryBuilder(entityName, this.metadata, this, ctx, undefined, write ? 'write' : 'read'); + protected createQueryBuilder>(entityName: string, ctx?: Transaction, write?: boolean, convertCustomTypes?: boolean): QueryBuilder { + const qb = new QueryBuilder(entityName, this.metadata, this, ctx, undefined, write ? 'write' : 'read'); + + if (!convertCustomTypes) { + qb.unsetFlag(QueryFlag.CONVERT_CUSTOM_TYPES); + } + + return qb; } protected extractManyToMany>(entityName: string, data: EntityData): EntityData { @@ -531,7 +534,7 @@ export abstract class AbstractSqlDriver 0) { - const qb1 = this.createQueryBuilder(prop.pivotTable, ctx, true); + const qb1 = this.createQueryBuilder(prop.pivotTable, ctx, true).unsetFlag(QueryFlag.CONVERT_CUSTOM_TYPES); const knex = qb1.getKnex(); if (Array.isArray(deleteDiff)) { @@ -558,12 +561,12 @@ export abstract class AbstractSqlDriver this.createQueryBuilder(prop.pivotTable, ctx, true).insert(item).execute('run', false)); + await Utils.runSerial(items, item => this.createQueryBuilder(prop.pivotTable, ctx, true).unsetFlag(QueryFlag.CONVERT_CUSTOM_TYPES).insert(item).execute('run', false)); } } async lockPessimistic>(entity: T, mode: LockMode, ctx?: Transaction): Promise { - const qb = this.createQueryBuilder(entity.constructor.name, ctx); + const qb = this.createQueryBuilder(entity.constructor.name, ctx).unsetFlag(QueryFlag.CONVERT_CUSTOM_TYPES); const meta = entity.__helper!.__meta; const cond = Utils.getPrimaryKeyCond(entity, meta.primaryKeys); qb.select('1').where(cond!).setLockMode(mode); diff --git a/packages/knex/src/query/QueryBuilder.ts b/packages/knex/src/query/QueryBuilder.ts index 94a42166903b..66f1f0945bd1 100644 --- a/packages/knex/src/query/QueryBuilder.ts +++ b/packages/knex/src/query/QueryBuilder.ts @@ -1,7 +1,23 @@ import { QueryBuilder as KnexQueryBuilder, Raw, Transaction, Value } from 'knex'; import { - AnyEntity, Dictionary, EntityMetadata, EntityProperty, FlatQueryOrderMap, GroupOperator, LockMode, MetadataStorage, EntityData, - PopulateOptions, QBFilterQuery, QueryFlag, QueryHelper, QueryOrderMap, ReferenceType, Utils, ValidationError, LoadStrategy, + AnyEntity, + Dictionary, + EntityData, + EntityMetadata, + EntityProperty, + FlatQueryOrderMap, + GroupOperator, + LoadStrategy, + LockMode, + MetadataStorage, + PopulateOptions, + QBFilterQuery, + QueryFlag, + QueryHelper, + QueryOrderMap, + ReferenceType, + Utils, + ValidationError, } from '@mikro-orm/core'; import { QueryType } from './enums'; import { AbstractSqlDriver } from '../AbstractSqlDriver'; @@ -522,7 +538,7 @@ export class QueryBuilder = AnyEntity> { } if (data) { - this._data = this.helper.processData(data); + this._data = this.helper.processData(data, this.flags.has(QueryFlag.CONVERT_CUSTOM_TYPES)); } if (cond) { diff --git a/packages/knex/src/query/QueryBuilderHelper.ts b/packages/knex/src/query/QueryBuilderHelper.ts index ab707ec7160a..5b57891c10f9 100644 --- a/packages/knex/src/query/QueryBuilderHelper.ts +++ b/packages/knex/src/query/QueryBuilderHelper.ts @@ -55,9 +55,9 @@ export class QueryBuilderHelper { return this.alias + '.' + ret; } - processData(data: Dictionary, multi = false): any { + processData(data: Dictionary, convertCustomTypes: boolean, multi = false): any { if (Array.isArray(data)) { - return data.map(d => this.processData(d, true)); + return data.map(d => this.processData(d, convertCustomTypes, true)); } data = Object.assign({}, data); // copy first @@ -78,6 +78,10 @@ export class QueryBuilderHelper { return; } + if (prop.customType && convertCustomTypes) { + data[k] = prop.customType.convertToDatabaseValue(data[k], this.platform, true); + } + if (!prop.customType && (Array.isArray(data[k]) || Utils.isPlainObject(data[k]))) { data[k] = JSON.stringify(data[k]); } diff --git a/tests/EntityManager.mongo.test.ts b/tests/EntityManager.mongo.test.ts index 722667b8eaed..92a11606ad55 100644 --- a/tests/EntityManager.mongo.test.ts +++ b/tests/EntityManager.mongo.test.ts @@ -1986,6 +1986,9 @@ describe('EntityManagerMongo', () => { }); test('custom types', async () => { + await orm.em.nativeInsert(FooBar, { name: 'n1', array: [1, 2, 3] }); + await orm.em.nativeInsert(FooBar, { name: 'n2', array: [] }); + const bar = FooBar.create('b1'); bar.blob = Buffer.from([1, 2, 3, 4, 5]); bar.array = []; diff --git a/tests/EntityManager.mysql.test.ts b/tests/EntityManager.mysql.test.ts index ad558fbd55ee..ec2dd5c7a3f7 100644 --- a/tests/EntityManager.mysql.test.ts +++ b/tests/EntityManager.mysql.test.ts @@ -2337,6 +2337,9 @@ describe('EntityManagerMySql', () => { }); test('custom types', async () => { + await orm.em.nativeInsert(FooBar2, { id: 123, name: 'n1', array: [1, 2, 3] }); + await orm.em.nativeInsert(FooBar2, { id: 456, name: 'n2', array: [] }); + const bar = FooBar2.create('b1'); bar.blob = Buffer.from([1, 2, 3, 4, 5]); bar.array = []; diff --git a/tests/EntityManager.postgre.test.ts b/tests/EntityManager.postgre.test.ts index ee65b49accc1..0b9e4b8f047d 100644 --- a/tests/EntityManager.postgre.test.ts +++ b/tests/EntityManager.postgre.test.ts @@ -1456,6 +1456,9 @@ describe('EntityManagerPostgre', () => { }); test('custom types', async () => { + await orm.em.nativeInsert(FooBar2, { id: 123, name: 'n1', array: [1, 2, 3] }); + await orm.em.nativeInsert(FooBar2, { id: 456, name: 'n2', array: [] }); + const bar = FooBar2.create('b1 "b" \'1\''); bar.blob = Buffer.from([1, 2, 3, 4, 5]); bar.array = []; diff --git a/tests/EntityManager.sqlite2.test.ts b/tests/EntityManager.sqlite2.test.ts index 35923fc48500..36e33a2b08e7 100644 --- a/tests/EntityManager.sqlite2.test.ts +++ b/tests/EntityManager.sqlite2.test.ts @@ -834,6 +834,9 @@ describe('EntityManagerSqlite2', () => { }); test('custom types', async () => { + await orm.em.nativeInsert(FooBar4, { id: 123, name: 'n1', array: [1, 2, 3] }); + await orm.em.nativeInsert(FooBar4, { id: 456, name: 'n2', array: [] }); + const bar = orm.em.create(FooBar4, { name: 'b1 \'the bad\' lol' }); bar.blob = Buffer.from([1, 2, 3, 4, 5]); bar.array = []; diff --git a/tests/QueryBuilder.test.ts b/tests/QueryBuilder.test.ts index 62bdddba04e6..a50f46051763 100644 --- a/tests/QueryBuilder.test.ts +++ b/tests/QueryBuilder.test.ts @@ -1628,6 +1628,14 @@ describe('QueryBuilder', () => { type: 'postgresql', }); + const qb01 = pg.em.createQueryBuilder(FooBar2); + qb01.insert({ array: [] }); + expect(qb01.getFormattedQuery()).toEqual(`insert into "foo_bar2" ("array") values ('{}') returning "id", "version"`); + + const qb02 = pg.em.createQueryBuilder(FooBar2); + qb02.insert({ array: [1, 2, 3] }); + expect(qb02.getFormattedQuery()).toEqual(`insert into "foo_bar2" ("array") values ('{1,2,3}') returning "id", "version"`); + const qb1 = pg.em.createQueryBuilder(Publisher2); qb1.select('*').where({ name: { $contains: 'test' } }); expect(qb1.getQuery()).toEqual('select "e0".* from "publisher2" as "e0" where "e0"."name" @> $1'); diff --git a/tests/entities/FooBar.ts b/tests/entities/FooBar.ts index 7ef6675bcaec..306e972b1ffc 100644 --- a/tests/entities/FooBar.ts +++ b/tests/entities/FooBar.ts @@ -14,10 +14,10 @@ export default class FooBar { @Property() name!: string; - @OneToOne({ entity: () => FooBaz, eager: true, orphanRemoval: true, serializedName: 'fooBaz', serializer: value => `FooBaz id: ${value.id}` }) + @OneToOne({ entity: () => FooBaz, eager: true, orphanRemoval: true, nullable: true, serializedName: 'fooBaz', serializer: value => `FooBaz id: ${value.id}` }) baz!: FooBaz | null; - @OneToOne(() => FooBar) + @OneToOne(() => FooBar, undefined, { nullable: true }) fooBar!: FooBar; @Property({ nullable: true })