Skip to content

Commit

Permalink
fix: change InsertQueryBuilder.values() with an empty array into a no…
Browse files Browse the repository at this point in the history
…-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
  • Loading branch information
415mcc authored Aug 23, 2020
1 parent 5a9150e commit 9d2df28
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 15 deletions.
11 changes: 0 additions & 11 deletions src/entity-manager/EntityManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -581,17 +581,6 @@ export class EntityManager {
* You can execute bulk inserts using this method.
*/
async insert<Entity>(target: ObjectType<Entity>|EntitySchema<Entity>|string, entity: QueryDeepPartialEntity<Entity>|(QueryDeepPartialEntity<Entity>[])): Promise<InsertResult> {

// 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)));
Expand Down
19 changes: 15 additions & 4 deletions src/query-builder/InsertQueryBuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,20 @@ export class InsertQueryBuilder<Entity> extends QueryBuilder<Entity> {
* Executes sql generated by query builder and returns raw database results.
*/
async execute(): Promise<InsertResult> {
// 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();
Expand All @@ -55,9 +69,6 @@ export class InsertQueryBuilder<Entity> extends QueryBuilder<Entity> {
}

// 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) {
Expand Down Expand Up @@ -579,7 +590,7 @@ export class InsertQueryBuilder<Entity> extends QueryBuilder<Entity> {
* 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)
Expand Down
14 changes: 14 additions & 0 deletions test/github-issues/3111/entity/Test.ts
Original file line number Diff line number Diff line change
@@ -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;
}
30 changes: 30 additions & 0 deletions test/github-issues/3111/issue-3111.ts
Original file line number Diff line number Diff line change
@@ -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);
})));
});

0 comments on commit 9d2df28

Please sign in to comment.