From f8c52f30a17ffb0b62a8e280905c6fcc86c4a177 Mon Sep 17 00:00:00 2001
From: Lovro Puzar <l.puzar@synthace.com>
Date: Sat, 16 May 2020 17:12:55 +0100
Subject: [PATCH] fix: calling EntityManager.insert() with an empty array of
 entities (#5745)

As described in issue #5734, the current behaviour seems like an oversight and inconsistent with save() and remove() which already have this handled as a special case.

Test Plan: npm test, and more specifically npm test -- --grep='#5734'

Closes: #5734
---
 src/entity-manager/EntityManager.ts           | 10 +++++++
 test/github-issues/5734/entity/Post.ts        | 12 ++++++++
 test/github-issues/5734/issue-5734.ts         | 28 +++++++++++++++++++
 .../subscriber/CheckValidEntitySubscriber.ts  | 22 +++++++++++++++
 4 files changed, 72 insertions(+)
 create mode 100644 test/github-issues/5734/entity/Post.ts
 create mode 100644 test/github-issues/5734/issue-5734.ts
 create mode 100644 test/github-issues/5734/subscriber/CheckValidEntitySubscriber.ts

diff --git a/src/entity-manager/EntityManager.ts b/src/entity-manager/EntityManager.ts
index 8e3dcd8408..d846df1e43 100644
--- a/src/entity-manager/EntityManager.ts
+++ b/src/entity-manager/EntityManager.ts
@@ -582,6 +582,16 @@ export class EntityManager {
      */
     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)));
diff --git a/test/github-issues/5734/entity/Post.ts b/test/github-issues/5734/entity/Post.ts
new file mode 100644
index 0000000000..9d0337791a
--- /dev/null
+++ b/test/github-issues/5734/entity/Post.ts
@@ -0,0 +1,12 @@
+import {PrimaryColumn} from "../../../../src/decorator/columns/PrimaryColumn";
+import {Entity} from "../../../../src/decorator/entity/Entity";
+
+@Entity()
+export class Post {
+    @PrimaryColumn()
+    id: number;
+
+    constructor(id: number) {
+        this.id = id;
+    }
+}
diff --git a/test/github-issues/5734/issue-5734.ts b/test/github-issues/5734/issue-5734.ts
new file mode 100644
index 0000000000..1b0cfcf79c
--- /dev/null
+++ b/test/github-issues/5734/issue-5734.ts
@@ -0,0 +1,28 @@
+import "reflect-metadata";
+import {closeTestingConnections, createTestingConnections, reloadTestingDatabases} from "../../utils/test-utils";
+import {Connection} from "../../../src/connection/Connection";
+import {Post} from "./entity/Post";
+
+describe("github issues > #5734 insert([]) should not crash", () => {
+
+    let connections: Connection[];
+    before(async () => connections = await createTestingConnections({
+        entities: [__dirname + "/entity/*{.js,.ts}"],
+        subscribers: [__dirname + "/subscriber/*{.js,.ts}"],
+        schemaCreate: true,
+        dropSchema: true
+    }));
+    beforeEach(() => reloadTestingDatabases(connections));
+    after(() => closeTestingConnections(connections));
+
+    it("should not crash on insert([])", () => Promise.all(connections.map(async connection => {
+        const repository = connection.getRepository(Post);
+        await repository.insert([]);
+    })));
+
+    it("should still work with a nonempty array", () => Promise.all(connections.map(async connection => {
+        const repository = connection.getRepository(Post);
+        await repository.insert([new Post(1)]);
+        await repository.findOneOrFail({where: {id: 1}});
+    })));
+});
diff --git a/test/github-issues/5734/subscriber/CheckValidEntitySubscriber.ts b/test/github-issues/5734/subscriber/CheckValidEntitySubscriber.ts
new file mode 100644
index 0000000000..a2573c9236
--- /dev/null
+++ b/test/github-issues/5734/subscriber/CheckValidEntitySubscriber.ts
@@ -0,0 +1,22 @@
+import {Post} from "../entity/Post";
+import {EntitySubscriberInterface, EventSubscriber, InsertEvent} from "../../../../src";
+
+/**
+ * Subscriber which checks the validity of the entity passed to beforeInsert().
+ * Tests the fix for issue #5734 in which we saw an empty array leak into
+ * beforeInsert().
+ */
+@EventSubscriber()
+export class ValidEntityCheckSubscriber
+    implements EntitySubscriberInterface<Post> {
+    listenTo() {
+        return Post;
+    }
+
+    beforeInsert(event: InsertEvent<Post>) {
+        const entity = event.entity;
+        if (Array.isArray(entity) || !entity.id) {
+            throw new Error(`Subscriber saw invalid entity: ${JSON.stringify(entity)}`);
+        }
+    }
+}