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

@@ -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)));

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;
+    }
+}

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}});
+    })));
+});

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)}`);
+        }
+    }
+}