From 9d2df28b4c8c22024566a86a231db64ae2c09161 Mon Sep 17 00:00:00 2001 From: Lachlan McCarty Date: Sun, 23 Aug 2020 04:29:30 -0400 Subject: [PATCH] fix: change InsertQueryBuilder.values() with an empty array into a no-op (#6584) Change InsertQueryBuilder.values() with an empty array into a no-op instead of the current behavior of inserting a row of DEFAULT values. Closes: #3111 --- src/entity-manager/EntityManager.ts | 11 --------- src/query-builder/InsertQueryBuilder.ts | 19 ++++++++++++---- test/github-issues/3111/entity/Test.ts | 14 ++++++++++++ test/github-issues/3111/issue-3111.ts | 30 +++++++++++++++++++++++++ 4 files changed, 59 insertions(+), 15 deletions(-) create mode 100644 test/github-issues/3111/entity/Test.ts create mode 100644 test/github-issues/3111/issue-3111.ts diff --git a/src/entity-manager/EntityManager.ts b/src/entity-manager/EntityManager.ts index bb9979fa7b..5ed47b3690 100644 --- a/src/entity-manager/EntityManager.ts +++ b/src/entity-manager/EntityManager.ts @@ -581,17 +581,6 @@ export class EntityManager { * You can execute bulk inserts using this method. */ async insert(target: ObjectType|EntitySchema|string, entity: QueryDeepPartialEntity|(QueryDeepPartialEntity[])): Promise { - - // If user passed empty array of entities then we don't need to do - // anything. - // - // Fixes GitHub issue #5734. If we were to let this through we would - // run into problems downstream, like subscribers getting invoked with - // the empty array where they expect an entity, and SQL queries with an - // empty VALUES clause. - if (Array.isArray(entity) && entity.length === 0) - return Promise.resolve(new InsertResult()); - // TODO: Oracle does not support multiple values. Need to create another nice solution. if (this.connection.driver instanceof OracleDriver && Array.isArray(entity)) { const results = await Promise.all(entity.map(entity => this.insert(target, entity))); diff --git a/src/query-builder/InsertQueryBuilder.ts b/src/query-builder/InsertQueryBuilder.ts index 480a93ac04..106cd41494 100644 --- a/src/query-builder/InsertQueryBuilder.ts +++ b/src/query-builder/InsertQueryBuilder.ts @@ -41,6 +41,20 @@ export class InsertQueryBuilder extends QueryBuilder { * Executes sql generated by query builder and returns raw database results. */ async execute(): Promise { + // console.time(".value sets"); + const valueSets: ObjectLiteral[] = this.getValueSets(); + // console.timeEnd(".value sets"); + + // If user passed empty array of entities then we don't need to do + // anything. + // + // Fixes GitHub issues #3111 and #5734. If we were to let this through + // we would run into problems downstream, like subscribers getting + // invoked with the empty array where they expect an entity, and SQL + // queries with an empty VALUES clause. + if (valueSets.length === 0) + return new InsertResult(); + // console.time("QueryBuilder.execute"); // console.time(".database stuff"); const queryRunner = this.obtainQueryRunner(); @@ -55,9 +69,6 @@ export class InsertQueryBuilder extends QueryBuilder { } // console.timeEnd(".database stuff"); - // console.time(".value sets"); - const valueSets: ObjectLiteral[] = this.getValueSets(); - // console.timeEnd(".value sets"); // call before insertion methods in listeners and subscribers if (this.expressionMap.callListeners === true && this.expressionMap.mainAlias!.hasMetadata) { @@ -579,7 +590,7 @@ export class InsertQueryBuilder extends QueryBuilder { * Gets array of values need to be inserted into the target table. */ protected getValueSets(): ObjectLiteral[] { - if (Array.isArray(this.expressionMap.valuesSet) && this.expressionMap.valuesSet.length > 0) + if (Array.isArray(this.expressionMap.valuesSet)) return this.expressionMap.valuesSet; if (this.expressionMap.valuesSet instanceof Object) diff --git a/test/github-issues/3111/entity/Test.ts b/test/github-issues/3111/entity/Test.ts new file mode 100644 index 0000000000..dfcf3d2277 --- /dev/null +++ b/test/github-issues/3111/entity/Test.ts @@ -0,0 +1,14 @@ +import {Column} from "../../../../src/decorator/columns/Column"; +import {Entity} from "../../../../src/decorator/entity/Entity"; +import {PrimaryGeneratedColumn} from "../../../../src/decorator/columns/PrimaryGeneratedColumn"; + +export const DEFAULT_VALUE = "default-value"; + +@Entity() +export class Test { + @PrimaryGeneratedColumn() + id: number; + + @Column({default: DEFAULT_VALUE}) + value: string; +} diff --git a/test/github-issues/3111/issue-3111.ts b/test/github-issues/3111/issue-3111.ts new file mode 100644 index 0000000000..614525200f --- /dev/null +++ b/test/github-issues/3111/issue-3111.ts @@ -0,0 +1,30 @@ +import "reflect-metadata"; +import {createTestingConnections, closeTestingConnections, reloadTestingDatabases} from "../../utils/test-utils"; +import {Connection} from "../../../src/connection/Connection"; +import {expect} from "chai"; +import {InsertValuesMissingError} from "../../../src/error/InsertValuesMissingError"; +import {Test, DEFAULT_VALUE} from "./entity/Test"; + +describe("github issues > #3111 Inserting with query builder attempts to insert a default row when values is empty array", () => { + + let connections: Connection[]; + before(async () => connections = await createTestingConnections({ + entities: [__dirname + "/entity/*{.js,.ts}"], + schemaCreate: true, + dropSchema: true, + })); + beforeEach(() => reloadTestingDatabases(connections)); + after(() => closeTestingConnections(connections)); + + it("should not insert with default values on .values([])", () => Promise.all(connections.map(async connection => { + const repo = connection.getRepository(Test); + await repo.createQueryBuilder().insert().values([]).execute(); + const rowsWithDefaultValue = await repo.find({ where: {value: DEFAULT_VALUE}}); + expect(rowsWithDefaultValue).to.have.lengthOf(0); + }))); + + it("should still error on missing .values()", () => Promise.all(connections.map(async connection => { + const repo = connection.getRepository(Test); + await repo.createQueryBuilder().insert().execute().should.be.rejectedWith(InsertValuesMissingError); + }))); +});