Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: change InsertQueryBuilder.values() with an empty array into a no-op #6584

Merged
merged 1 commit into from Aug 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 0 additions & 11 deletions src/entity-manager/EntityManager.ts
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
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
@@ -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
@@ -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);
})));
});