refactor: use brackets & object literal to do where-in-ids (#7832)

Fixes #4732

src/query-builder/DeleteQueryBuilder.ts

@@ -185,21 +185,21 @@ export class DeleteQueryBuilder<Entity> extends QueryBuilder<Entity> implements
      * Adds new AND WHERE with conditions for the given ids.
      */
     whereInIds(ids: any|any[]): this {
-        return this.where(this.createWhereIdsExpression(ids));
+        return this.where(this.getWhereInIdsCondition(ids));
     }
 
     /**
      * Adds new AND WHERE with conditions for the given ids.
      */
     andWhereInIds(ids: any|any[]): this {
-        return this.andWhere(this.createWhereIdsExpression(ids));
+        return this.andWhere(this.getWhereInIdsCondition(ids));
     }
 
     /**
      * Adds new OR WHERE with conditions for the given ids.
      */
     orWhereInIds(ids: any|any[]): this {
-        return this.orWhere(this.createWhereIdsExpression(ids));
+        return this.orWhere(this.getWhereInIdsCondition(ids));
     }
     /**
      * Optional returning/output clause.

src/query-builder/QueryBuilder.ts

@@ -813,44 +813,35 @@ export abstract class QueryBuilder<Entity> {
     }
 
     /**
-     * Creates "WHERE" expression and variables for the given "ids".
+     * Creates "WHERE" condition for an in-ids condition.
      */
-    protected createWhereIdsExpression(ids: any|any[]): string {
+    protected getWhereInIdsCondition(ids: any|any[]): ObjectLiteral | Brackets  {
         const metadata = this.expressionMap.mainAlias!.metadata;
         const normalized = (Array.isArray(ids) ? ids : [ids]).map(id => metadata.ensureEntityIdMap(id));
 
         // using in(...ids) for single primary key entities
-        if (!metadata.hasMultiplePrimaryKeys
-            && metadata.embeddeds.length === 0
-        ) {
+        if (!metadata.hasMultiplePrimaryKeys) {
             const primaryColumn = metadata.primaryColumns[0];
 
             // getEntityValue will try to transform `In`, it is a bug
             // todo: remove this transformer check after #2390 is fixed
-            if (!primaryColumn.transformer) {
-                return this.computeWhereParameter({
+            // This also fails for embedded & relation, so until that is fixed skip it.
+            if (!primaryColumn.transformer && !primaryColumn.relationMetadata && !primaryColumn.embeddedMetadata) {
+                return {
                     [primaryColumn.propertyName]: In(
                         normalized.map(id => primaryColumn.getEntityValue(id, false))
                     )
-                });
+                };
             }
         }
 
-        // create shortcuts for better readability
-        const alias = this.expressionMap.aliasNamePrefixingEnabled ? this.escape(this.expressionMap.mainAlias!.name) + "." : "";
-        const whereStrings = normalized.map((id, index) => {
-            const whereSubStrings: string[] = [];
-            metadata.primaryColumns.forEach((primaryColumn, secondIndex) => {
-                const parameterName = this.createParameter(primaryColumn.getEntityValue(id, true));
-                // whereSubStrings.push(alias + this.escape(primaryColumn.databaseName) + "=:id_" + index + "_" + secondIndex);
-                whereSubStrings.push(alias + this.escape(primaryColumn.databaseName) + " = " + parameterName);
-            });
-            return whereSubStrings.join(" AND ");
+        return new Brackets((qb) => {
+            for (const data of normalized) {
+                qb.orWhere(new Brackets(
+                    (qb) => qb.where(data)
+                ));
+            }
         });
-
-        return whereStrings.length > 1
-            ? "(" + whereStrings.map(whereString => "(" + whereString + ")").join(" OR ") + ")"
-            : whereStrings[0];
     }
 
     private findColumnsForPropertyPath(propertyPath: string): [ Alias, string[], ColumnMetadata[] ] {

src/query-builder/SelectQueryBuilder.ts

@@ -755,7 +755,7 @@ export class SelectQueryBuilder<Entity> extends QueryBuilder<Entity> implements
      * for example [{ firstId: 1, secondId: 2 }, { firstId: 2, secondId: 3 }, ...]
      */
     whereInIds(ids: any|any[]): this {
-        return this.where(this.createWhereIdsExpression(ids));
+        return this.where(this.getWhereInIdsCondition(ids));
     }
 
     /**
@@ -767,7 +767,7 @@ export class SelectQueryBuilder<Entity> extends QueryBuilder<Entity> implements
      * for example [{ firstId: 1, secondId: 2 }, { firstId: 2, secondId: 3 }, ...]
      */
     andWhereInIds(ids: any|any[]): this {
-        return this.andWhere(this.createWhereIdsExpression(ids));
+        return this.andWhere(this.getWhereInIdsCondition(ids));
     }
 
     /**
@@ -779,7 +779,7 @@ export class SelectQueryBuilder<Entity> extends QueryBuilder<Entity> implements
      * for example [{ firstId: 1, secondId: 2 }, { firstId: 2, secondId: 3 }, ...]
      */
     orWhereInIds(ids: any|any[]): this {
-        return this.orWhere(this.createWhereIdsExpression(ids));
+        return this.orWhere(this.getWhereInIdsCondition(ids));
     }
 
     /**

src/query-builder/SoftDeleteQueryBuilder.ts

@@ -189,21 +189,21 @@ export class SoftDeleteQueryBuilder<Entity> extends QueryBuilder<Entity> impleme
      * Adds new AND WHERE with conditions for the given ids.
      */
     whereInIds(ids: any|any[]): this {
-        return this.where(this.createWhereIdsExpression(ids));
+        return this.where(this.getWhereInIdsCondition(ids));
     }
 
     /**
      * Adds new AND WHERE with conditions for the given ids.
      */
     andWhereInIds(ids: any|any[]): this {
-        return this.andWhere(this.createWhereIdsExpression(ids));
+        return this.andWhere(this.getWhereInIdsCondition(ids));
     }
 
     /**
      * Adds new OR WHERE with conditions for the given ids.
      */
     orWhereInIds(ids: any|any[]): this {
-        return this.orWhere(this.createWhereIdsExpression(ids));
+        return this.orWhere(this.getWhereInIdsCondition(ids));
     }
     /**
      * Optional returning/output clause.

src/query-builder/UpdateQueryBuilder.ts

@@ -213,21 +213,21 @@ export class UpdateQueryBuilder<Entity> extends QueryBuilder<Entity> implements
      * Adds new AND WHERE with conditions for the given ids.
      */
     whereInIds(ids: any|any[]): this {
-        return this.where(this.createWhereIdsExpression(ids));
+        return this.where(this.getWhereInIdsCondition(ids));
     }
 
     /**
      * Adds new AND WHERE with conditions for the given ids.
      */
     andWhereInIds(ids: any|any[]): this {
-        return this.andWhere(this.createWhereIdsExpression(ids));
+        return this.andWhere(this.getWhereInIdsCondition(ids));
     }
 
     /**
      * Adds new OR WHERE with conditions for the given ids.
      */
     orWhereInIds(ids: any|any[]): this {
-        return this.orWhere(this.createWhereIdsExpression(ids));
+        return this.orWhere(this.getWhereInIdsCondition(ids));
     }
     /**
      * Optional returning/output clause.

test/functional/query-builder/select/entity/ExternalPost.ts

@@ -0,0 +1,17 @@
+import {
+    Entity,
+    Column,
+    PrimaryColumn,
+} from "../../../../../src";
+
+@Entity()
+export class ExternalPost {
+    @PrimaryColumn()
+    outlet: string;
+
+    @PrimaryColumn()
+    id: number;
+
+    @Column()
+    title: string;
+}

test/functional/query-builder/select/query-builder-select.ts

@@ -7,11 +7,12 @@ import { Category } from "./entity/Category";
 import {Post} from "./entity/Post";
 import { Tag } from "./entity/Tag";
 import { HeroImage } from "./entity/HeroImage";
+import { ExternalPost } from "./entity/ExternalPost";
 
 describe("query builder > select", () => {
     let connections: Connection[];
     before(async () => connections = await createTestingConnections({
-        entities: [Category, Post, Tag, HeroImage],
+        entities: [Category, Post, Tag, HeroImage, ExternalPost],
         enabledDrivers: ["sqlite"],
     }));
     beforeEach(() => reloadTestingDatabases(connections));
@@ -390,6 +391,51 @@ describe("query builder > select", () => {
 
     })
 
+    describe("where-in-ids", () => {
+        it("should create expected query with simple primary keys", () => Promise.all(connections.map(async connection => {
+            const [sql, params] = connection.createQueryBuilder(Post, "post")
+                .select("post.id")
+                .whereInIds([ 1, 2, 5, 9 ])
+                .disableEscaping()
+                .getQueryAndParameters();
+
+            expect(sql).to.equal("SELECT post.id AS post_id FROM post post WHERE post.id IN (?, ?, ?, ?)")
+            expect(params).to.eql([ 1, 2, 5, 9 ])
+        })))
+
+        it("should create expected query with composite primary keys", () => Promise.all(connections.map(async connection => {
+            const [sql, params] = connection.createQueryBuilder(ExternalPost, "post")
+                .select("post.id")
+                .whereInIds([ { outlet: "foo", id: 1 }, { outlet: "bar", id: 2 }, { outlet: "baz", id: 5 } ])
+                .disableEscaping()
+                .getQueryAndParameters();
+
+            expect(sql).to.equal(
+                'SELECT post.id AS post_id FROM external_post post WHERE ' +
+                '((post.outlet = ? AND post.id = ?) OR ' +
+                '(post.outlet = ? AND post.id = ?) OR ' +
+                '(post.outlet = ? AND post.id = ?))'
+            )
+            expect(params).to.eql([ "foo", 1, "bar", 2, "baz", 5 ])
+        })))
+
+        it("should create expected query with composite primary keys with missing value", () => Promise.all(connections.map(async connection => {
+            const [sql, params] = connection.createQueryBuilder(ExternalPost, "post")
+                .select("post.id")
+                .whereInIds([ { outlet: "foo", id: 1 }, { outlet: "bar", id: 2 }, { id: 5 } ])
+                .disableEscaping()
+                .getQueryAndParameters();
+
+            expect(sql).to.equal(
+                'SELECT post.id AS post_id FROM external_post post WHERE ' +
+                '((post.outlet = ? AND post.id = ?) OR ' +
+                '(post.outlet = ? AND post.id = ?) OR ' +
+                '(post.id = ?))'
+            )
+            expect(params).to.eql([ "foo", 1, "bar", 2, 5 ])
+        })))
+    })
+
     it("Support max execution time", () => Promise.all(connections.map(async connection => {
         // MAX_EXECUTION_TIME supports only in MySQL
         if (!(connection.driver instanceof MysqlDriver)) return