From f351757a15b9d2bd9d4222c69dcfd2316f46b5d1 Mon Sep 17 00:00:00 2001 From: Vampire Date: Wed, 9 Jul 2025 13:23:47 +0700 Subject: [PATCH] fix: resolve array modification bug in QueryRunner drop methods (#11564) * fix: resolve array modification bug in QueryRunner drop methods Fix iteration bug in QueryRunner implementations where dropping multiple database objects (columns, indices, foreign keys, unique constraints) would skip elements due to in-place array modification during iteration. The issue occurred when methods like dropColumns(), dropIndices(), dropForeignKeys(), and dropUniqueConstraints() iterated over arrays while simultaneously modifying them by removing elements. This caused a classic "off-by-one" iteration bug where alternate elements would be skipped. Changes: - Update all affected QueryRunner drop methods to iterate over a shallow copy of the input array using [...array] spread syntax - Add comprehensive regression tests in test/github-issues/11563/ - Test coverage includes all affected drivers: Postgres, MySQL, SQL Server, Oracle, CockroachDB, Spanner, SAP, and Aurora MySQL Affected drivers: - SpannerQueryRunner - PostgresQueryRunner - MysqlQueryRunner - SqlServerQueryRunner - OracleQueryRunner - CockroachQueryRunner - SapQueryRunner - AuroraMysqlQueryRunner Closes #11563 * fix: create multiple indices same column * chore: functional tests instead of github issues --- .../aurora-mysql/AuroraMysqlQueryRunner.ts | 2 +- .../cockroachdb/CockroachQueryRunner.ts | 8 +- src/driver/mysql/MysqlQueryRunner.ts | 2 +- src/driver/oracle/OracleQueryRunner.ts | 2 +- src/driver/postgres/PostgresQueryRunner.ts | 8 +- src/driver/sap/SapQueryRunner.ts | 2 +- src/driver/spanner/SpannerQueryRunner.ts | 6 +- src/driver/sqlserver/SqlServerQueryRunner.ts | 2 +- test/functional/query-runner/drop-column.ts | 95 +++++++++- .../query-runner/drop-foreign-key.ts | 164 +++++++++++++++++- test/functional/query-runner/drop-index.ts | 127 +++++++++++++- .../query-runner/drop-unique-constraint.ts | 133 +++++++++++++- 12 files changed, 531 insertions(+), 20 deletions(-) diff --git a/src/driver/aurora-mysql/AuroraMysqlQueryRunner.ts b/src/driver/aurora-mysql/AuroraMysqlQueryRunner.ts index 810e6735a..63787b313 100644 --- a/src/driver/aurora-mysql/AuroraMysqlQueryRunner.ts +++ b/src/driver/aurora-mysql/AuroraMysqlQueryRunner.ts @@ -1414,7 +1414,7 @@ export class AuroraMysqlQueryRunner tableOrName: Table | string, columns: TableColumn[] | string[], ): Promise { - for (const column of columns) { + for (const column of [...columns]) { await this.dropColumn(tableOrName, column) } } diff --git a/src/driver/cockroachdb/CockroachQueryRunner.ts b/src/driver/cockroachdb/CockroachQueryRunner.ts index c23f01837..a1c26d8d8 100644 --- a/src/driver/cockroachdb/CockroachQueryRunner.ts +++ b/src/driver/cockroachdb/CockroachQueryRunner.ts @@ -2311,7 +2311,7 @@ export class CockroachQueryRunner tableOrName: Table | string, columns: TableColumn[] | string[], ): Promise { - for (const column of columns) { + for (const column of [...columns]) { await this.dropColumn(tableOrName, column) } } @@ -2515,7 +2515,7 @@ export class CockroachQueryRunner tableOrName: Table | string, uniqueConstraints: TableUnique[], ): Promise { - for (const uniqueConstraint of uniqueConstraints) { + for (const uniqueConstraint of [...uniqueConstraints]) { await this.dropUniqueConstraint(tableOrName, uniqueConstraint) } } @@ -2712,7 +2712,7 @@ export class CockroachQueryRunner tableOrName: Table | string, foreignKeys: TableForeignKey[], ): Promise { - for (const foreignKey of foreignKeys) { + for (const foreignKey of [...foreignKeys]) { await this.dropForeignKey(tableOrName, foreignKey) } } @@ -2797,7 +2797,7 @@ export class CockroachQueryRunner tableOrName: Table | string, indices: TableIndex[], ): Promise { - for (const index of indices) { + for (const index of [...indices]) { await this.dropIndex(tableOrName, index) } } diff --git a/src/driver/mysql/MysqlQueryRunner.ts b/src/driver/mysql/MysqlQueryRunner.ts index 1b977f59b..643dd107a 100644 --- a/src/driver/mysql/MysqlQueryRunner.ts +++ b/src/driver/mysql/MysqlQueryRunner.ts @@ -1799,7 +1799,7 @@ export class MysqlQueryRunner extends BaseQueryRunner implements QueryRunner { tableOrName: Table | string, columns: TableColumn[] | string[], ): Promise { - for (const column of columns) { + for (const column of [...columns]) { await this.dropColumn(tableOrName, column) } } diff --git a/src/driver/oracle/OracleQueryRunner.ts b/src/driver/oracle/OracleQueryRunner.ts index ec58e155f..62af61291 100644 --- a/src/driver/oracle/OracleQueryRunner.ts +++ b/src/driver/oracle/OracleQueryRunner.ts @@ -1730,7 +1730,7 @@ export class OracleQueryRunner extends BaseQueryRunner implements QueryRunner { tableOrName: Table | string, columns: TableColumn[] | string[], ): Promise { - for (const column of columns) { + for (const column of [...columns]) { await this.dropColumn(tableOrName, column) } } diff --git a/src/driver/postgres/PostgresQueryRunner.ts b/src/driver/postgres/PostgresQueryRunner.ts index 3e0173da0..cdb6d2b0a 100644 --- a/src/driver/postgres/PostgresQueryRunner.ts +++ b/src/driver/postgres/PostgresQueryRunner.ts @@ -2527,7 +2527,7 @@ export class PostgresQueryRunner tableOrName: Table | string, columns: TableColumn[] | string[], ): Promise { - for (const column of columns) { + for (const column of [...columns]) { await this.dropColumn(tableOrName, column) } } @@ -2728,7 +2728,7 @@ export class PostgresQueryRunner tableOrName: Table | string, uniqueConstraints: TableUnique[], ): Promise { - for (const uniqueConstraint of uniqueConstraints) { + for (const uniqueConstraint of [...uniqueConstraints]) { await this.dropUniqueConstraint(tableOrName, uniqueConstraint) } } @@ -2966,7 +2966,7 @@ export class PostgresQueryRunner tableOrName: Table | string, foreignKeys: TableForeignKey[], ): Promise { - for (const foreignKey of foreignKeys) { + for (const foreignKey of [...foreignKeys]) { await this.dropForeignKey(tableOrName, foreignKey) } } @@ -3094,7 +3094,7 @@ export class PostgresQueryRunner tableOrName: Table | string, indices: TableIndex[], ): Promise { - for (const index of indices) { + for (const index of [...indices]) { await this.dropIndex(tableOrName, index) } } diff --git a/src/driver/sap/SapQueryRunner.ts b/src/driver/sap/SapQueryRunner.ts index 420b88f77..9920ffe76 100644 --- a/src/driver/sap/SapQueryRunner.ts +++ b/src/driver/sap/SapQueryRunner.ts @@ -1828,7 +1828,7 @@ export class SapQueryRunner extends BaseQueryRunner implements QueryRunner { tableOrName: Table | string, columns: TableColumn[] | string[], ): Promise { - for (const column of columns) { + for (const column of [...columns]) { await this.dropColumn(tableOrName, column) } } diff --git a/src/driver/spanner/SpannerQueryRunner.ts b/src/driver/spanner/SpannerQueryRunner.ts index e2b560e88..4c1a73b70 100644 --- a/src/driver/spanner/SpannerQueryRunner.ts +++ b/src/driver/spanner/SpannerQueryRunner.ts @@ -1052,7 +1052,7 @@ export class SpannerQueryRunner extends BaseQueryRunner implements QueryRunner { tableOrName: Table | string, columns: TableColumn[] | string[], ): Promise { - for (const column of columns) { + for (const column of [...columns]) { await this.dropColumn(tableOrName, column) } } @@ -1342,7 +1342,7 @@ export class SpannerQueryRunner extends BaseQueryRunner implements QueryRunner { tableOrName: Table | string, foreignKeys: TableForeignKey[], ): Promise { - for (const foreignKey of foreignKeys) { + for (const foreignKey of [...foreignKeys]) { await this.dropForeignKey(tableOrName, foreignKey) } } @@ -1416,7 +1416,7 @@ export class SpannerQueryRunner extends BaseQueryRunner implements QueryRunner { tableOrName: Table | string, indices: TableIndex[], ): Promise { - for (const index of indices) { + for (const index of [...indices]) { await this.dropIndex(tableOrName, index) } } diff --git a/src/driver/sqlserver/SqlServerQueryRunner.ts b/src/driver/sqlserver/SqlServerQueryRunner.ts index b21efe984..7a7dc4785 100644 --- a/src/driver/sqlserver/SqlServerQueryRunner.ts +++ b/src/driver/sqlserver/SqlServerQueryRunner.ts @@ -2115,7 +2115,7 @@ export class SqlServerQueryRunner tableOrName: Table | string, columns: TableColumn[] | string[], ): Promise { - for (const column of columns) { + for (const column of [...columns]) { await this.dropColumn(tableOrName, column) } } diff --git a/test/functional/query-runner/drop-column.ts b/test/functional/query-runner/drop-column.ts index 93b17dd15..a0868f978 100644 --- a/test/functional/query-runner/drop-column.ts +++ b/test/functional/query-runner/drop-column.ts @@ -4,7 +4,8 @@ import { closeTestingConnections, createTestingConnections, } from "../../utils/test-utils" -import { DataSource } from "../../../src" +import { DataSource, Table } from "../../../src" +import { DriverUtils } from "../../../src/driver/DriverUtils" describe("query runner > drop column", () => { let connections: DataSource[] @@ -146,4 +147,96 @@ describe("query runner > drop column", () => { }), )) }) + + describe("array modification during iteration", () => { + it("should drop all columns without skipping any when iterating over array", () => + Promise.all( + connections.map(async (connection) => { + // Skip drivers that don't support dropping multiple columns + if ( + connection.driver.options.type === "mongodb" || + connection.driver.options.type === "better-sqlite3" || + connection.driver.options.type === "capacitor" || + connection.driver.options.type === "cordova" || + connection.driver.options.type === "react-native" || + connection.driver.options.type === "nativescript" || + connection.driver.options.type === "expo" || + connection.driver.options.type === "sqljs" + ) { + return + } + + const queryRunner = connection.createQueryRunner() + + // Create test table with multiple columns + const table = new Table({ + name: "test_drop_columns_array", + columns: [ + { + name: "id", + type: DriverUtils.isSQLiteFamily( + connection.driver, + ) + ? "integer" + : "int", + isPrimary: true, + isGenerated: + connection.driver.options.type !== + "spanner", + generationStrategy: + connection.driver.options.type !== "spanner" + ? "increment" + : undefined, + }, + { + name: "col1", + type: "varchar", + length: "255", + }, + { + name: "col2", + type: "varchar", + length: "255", + }, + { + name: "col3", + type: "varchar", + length: "255", + }, + { + name: "col4", + type: "varchar", + length: "255", + }, + ], + }) + + await queryRunner.createTable(table) + + // Get the table to ensure it was created correctly + const createdTable = await queryRunner.getTable( + "test_drop_columns_array", + ) + expect(createdTable!.columns).to.have.length(5) // id + 4 test columns + + // Drop multiple columns at once - this tests the array modification bug fix + const columnsToRemove = ["col1", "col2", "col3", "col4"] + await queryRunner.dropColumns( + createdTable!, + columnsToRemove, + ) + + // Verify all columns were dropped (only 'id' should remain) + const updatedTable = await queryRunner.getTable( + "test_drop_columns_array", + ) + expect(updatedTable!.columns).to.have.length(1) + expect(updatedTable!.columns[0].name).to.equal("id") + + // Clean up + await queryRunner.dropTable("test_drop_columns_array") + await queryRunner.release() + }), + )) + }) }) diff --git a/test/functional/query-runner/drop-foreign-key.ts b/test/functional/query-runner/drop-foreign-key.ts index 2f7363a82..240121c9a 100644 --- a/test/functional/query-runner/drop-foreign-key.ts +++ b/test/functional/query-runner/drop-foreign-key.ts @@ -1,10 +1,12 @@ import "reflect-metadata" -import { DataSource } from "../../../src/data-source/DataSource" +import { expect } from "chai" +import { DataSource, Table, TableColumn, TableForeignKey } from "../../../src" import { closeTestingConnections, createTestingConnections, reloadTestingDatabases, } from "../../utils/test-utils" +import { DriverUtils } from "../../../src/driver/DriverUtils" describe("query runner > drop foreign key", () => { let connections: DataSource[] @@ -39,4 +41,164 @@ describe("query runner > drop foreign key", () => { await queryRunner.release() }), )) + + it("should drop all foreign keys without skipping any when iterating over array", () => + Promise.all( + connections.map(async (connection) => { + // Skip databases that don't support foreign keys + if (connection.driver.options.type === "spanner") { + return + } + + const queryRunner = connection.createQueryRunner() + await queryRunner.connect() + + try { + // Create a referenced table first + await queryRunner.createTable( + new Table({ + name: "referenced_table", + columns: [ + new TableColumn({ + name: "id", + type: DriverUtils.isSQLiteFamily( + connection.driver, + ) + ? "integer" + : "int", + isPrimary: true, + isGenerated: true, + generationStrategy: "increment", + }), + new TableColumn({ + name: "name", + type: "varchar", + length: "100", + }), + ], + }), + true, + ) + + // Create a test table for foreign keys + await queryRunner.createTable( + new Table({ + name: "test_fk_table", + columns: [ + new TableColumn({ + name: "id", + type: DriverUtils.isSQLiteFamily( + connection.driver, + ) + ? "integer" + : "int", + isPrimary: true, + isGenerated: true, + generationStrategy: "increment", + }), + new TableColumn({ + name: "ref_id_1", + type: "int", + isNullable: true, + }), + new TableColumn({ + name: "ref_id_2", + type: "int", + isNullable: true, + }), + new TableColumn({ + name: "ref_id_3", + type: "int", + isNullable: true, + }), + new TableColumn({ + name: "ref_id_4", + type: "int", + isNullable: true, + }), + ], + }), + true, + ) + + // Add multiple foreign keys + await queryRunner.createForeignKeys("test_fk_table", [ + new TableForeignKey({ + name: "test_fk_1", + columnNames: ["ref_id_1"], + referencedTableName: "referenced_table", + referencedColumnNames: ["id"], + }), + new TableForeignKey({ + name: "test_fk_2", + columnNames: ["ref_id_2"], + referencedTableName: "referenced_table", + referencedColumnNames: ["id"], + }), + new TableForeignKey({ + name: "test_fk_3", + columnNames: ["ref_id_3"], + referencedTableName: "referenced_table", + referencedColumnNames: ["id"], + }), + new TableForeignKey({ + name: "test_fk_4", + columnNames: ["ref_id_4"], + referencedTableName: "referenced_table", + referencedColumnNames: ["id"], + }), + ]) + + // Get the table with foreign keys + const table = await queryRunner.getTable("test_fk_table") + if (!table) { + throw new Error("Test table not found") + } + + // Find only our test foreign keys + const testForeignKeys = table.foreignKeys.filter((fk) => + fk.name?.startsWith("test_fk_"), + ) + + expect(testForeignKeys).to.have.length( + 4, + `Should have 4 test foreign keys before dropping, found: ${testForeignKeys + .map((fk) => fk.name) + .join(", ")}`, + ) + + // Drop all test foreign keys - this should not skip any due to array modification + await queryRunner.dropForeignKeys( + "test_fk_table", + testForeignKeys, + ) + + // Verify all test foreign keys were dropped + const finalTable = await queryRunner.getTable( + "test_fk_table", + ) + if (!finalTable) { + throw new Error("Final test table not found") + } + + const remainingTestForeignKeys = + finalTable.foreignKeys.filter((fk) => + fk.name?.startsWith("test_fk_"), + ) + + expect(remainingTestForeignKeys).to.have.length( + 0, + `All test foreign keys should be dropped, but found remaining: ${remainingTestForeignKeys + .map((fk) => fk.name) + .join(", ")}`, + ) + + // Clean up test tables + await queryRunner.dropTable("test_fk_table") + await queryRunner.dropTable("referenced_table") + } finally { + await queryRunner.release() + } + }), + )) }) diff --git a/test/functional/query-runner/drop-index.ts b/test/functional/query-runner/drop-index.ts index e81aa5925..c8027de60 100644 --- a/test/functional/query-runner/drop-index.ts +++ b/test/functional/query-runner/drop-index.ts @@ -1,10 +1,12 @@ import "reflect-metadata" -import { DataSource } from "../../../src/data-source/DataSource" +import { expect } from "chai" +import { DataSource, Table, TableColumn, TableIndex } from "../../../src" import { closeTestingConnections, createTestingConnections, reloadTestingDatabases, } from "../../utils/test-utils" +import { DriverUtils } from "../../../src/driver/DriverUtils" describe("query runner > drop index", () => { let connections: DataSource[] @@ -54,4 +56,127 @@ describe("query runner > drop index", () => { await queryRunner.release() }), )) + + it("should drop all indices without skipping any when iterating over array", () => + Promise.all( + connections.map(async (connection) => { + const queryRunner = connection.createQueryRunner() + await queryRunner.connect() + + try { + // Create a test table for this specific test + await queryRunner.createTable( + new Table({ + name: "test_index_table", + columns: [ + new TableColumn({ + name: "id", + type: DriverUtils.isSQLiteFamily( + connection.driver, + ) + ? "integer" + : "int", + isPrimary: true, + isGenerated: true, + generationStrategy: "increment", + }), + new TableColumn({ + name: "idx_col_1", + type: "varchar", + length: "100", + isNullable: true, + }), + new TableColumn({ + name: "idx_col_2", + type: "varchar", + length: "100", + isNullable: true, + }), + new TableColumn({ + name: "idx_col_3", + type: "varchar", + length: "100", + isNullable: true, + }), + new TableColumn({ + name: "idx_col_4", + type: "varchar", + length: "100", + isNullable: true, + }), + ], + }), + true, + ) + + // Add multiple test indices on different columns to avoid Oracle conflicts + await queryRunner.createIndices("test_index_table", [ + new TableIndex({ + name: "test_idx_1", + columnNames: ["idx_col_1"], + }), + new TableIndex({ + name: "test_idx_2", + columnNames: ["idx_col_2"], + }), + new TableIndex({ + name: "test_idx_3", + columnNames: ["idx_col_3"], + }), + new TableIndex({ + name: "test_idx_4", + columnNames: ["idx_col_4"], + }), + ]) + + // Get the table with indices + const table = await queryRunner.getTable("test_index_table") + if (!table) { + throw new Error("Test table not found") + } + + // Find only our test indices + const testIndices = table.indices.filter((idx) => + idx.name?.startsWith("test_idx_"), + ) + + expect(testIndices).to.have.length( + 4, + `Should have 4 test indices before dropping, found: ${testIndices + .map((i) => i.name) + .join(", ")}`, + ) + + // Drop all test indices - this should not skip any due to array modification + await queryRunner.dropIndices( + "test_index_table", + testIndices, + ) + + // Verify all test indices were dropped + const finalTable = await queryRunner.getTable( + "test_index_table", + ) + if (!finalTable) { + throw new Error("Final test table not found") + } + + const remainingTestIndices = finalTable.indices.filter( + (idx) => idx.name?.startsWith("test_idx_"), + ) + + expect(remainingTestIndices).to.have.length( + 0, + `All test indices should be dropped, but found remaining: ${remainingTestIndices + .map((i) => i.name) + .join(", ")}`, + ) + + // Clean up the test table + await queryRunner.dropTable("test_index_table") + } finally { + await queryRunner.release() + } + }), + )) }) diff --git a/test/functional/query-runner/drop-unique-constraint.ts b/test/functional/query-runner/drop-unique-constraint.ts index bc36246f7..4f4788e33 100644 --- a/test/functional/query-runner/drop-unique-constraint.ts +++ b/test/functional/query-runner/drop-unique-constraint.ts @@ -1,10 +1,12 @@ import "reflect-metadata" -import { DataSource } from "../../../src/data-source/DataSource" +import { expect } from "chai" +import { DataSource, Table, TableColumn, TableUnique } from "../../../src" import { closeTestingConnections, createTestingConnections, reloadTestingDatabases, } from "../../utils/test-utils" +import { DriverUtils } from "../../../src/driver/DriverUtils" describe("query runner > drop unique constraint", () => { let connections: DataSource[] @@ -51,4 +53,133 @@ describe("query runner > drop unique constraint", () => { await queryRunner.release() }), )) + + it("should drop all unique constraints without skipping any when iterating over array", () => + Promise.all( + connections.map(async (connection) => { + const queryRunner = connection.createQueryRunner() + await queryRunner.connect() + + try { + // Create a test table for unique constraints + await queryRunner.createTable( + new Table({ + name: "test_unique_table", + columns: [ + new TableColumn({ + name: "id", + type: DriverUtils.isSQLiteFamily( + connection.driver, + ) + ? "integer" + : "int", + isPrimary: true, + isGenerated: true, + generationStrategy: "increment", + }), + new TableColumn({ + name: "unique_col_1", + type: "varchar", + length: "100", + isNullable: true, + }), + new TableColumn({ + name: "unique_col_2", + type: "varchar", + length: "100", + isNullable: true, + }), + new TableColumn({ + name: "unique_col_3", + type: "varchar", + length: "100", + isNullable: true, + }), + new TableColumn({ + name: "unique_col_4", + type: "varchar", + length: "100", + isNullable: true, + }), + ], + }), + true, + ) + + // Add multiple unique constraints + await queryRunner.createUniqueConstraints( + "test_unique_table", + [ + new TableUnique({ + name: "test_uq_1", + columnNames: ["unique_col_1"], + }), + new TableUnique({ + name: "test_uq_2", + columnNames: ["unique_col_2"], + }), + new TableUnique({ + name: "test_uq_3", + columnNames: ["unique_col_3"], + }), + new TableUnique({ + name: "test_uq_4", + columnNames: ["unique_col_4"], + }), + ], + ) + + // Get the table with unique constraints + const table = await queryRunner.getTable( + "test_unique_table", + ) + if (!table) { + throw new Error("Test table not found") + } + + // Find only our test unique constraints + const testUniqueConstraints = table.uniques.filter((uq) => + uq.name?.startsWith("test_uq_"), + ) + + expect(testUniqueConstraints).to.have.length( + 4, + `Should have 4 test unique constraints before dropping, found: ${testUniqueConstraints + .map((uq) => uq.name) + .join(", ")}`, + ) + + // Drop all test unique constraints - this should not skip any due to array modification + await queryRunner.dropUniqueConstraints( + "test_unique_table", + testUniqueConstraints, + ) + + // Verify all test unique constraints were dropped + const finalTable = await queryRunner.getTable( + "test_unique_table", + ) + if (!finalTable) { + throw new Error("Final test table not found") + } + + const remainingTestUniqueConstraints = + finalTable.uniques.filter((uq) => + uq.name?.startsWith("test_uq_"), + ) + + expect(remainingTestUniqueConstraints).to.have.length( + 0, + `All test unique constraints should be dropped, but found remaining: ${remainingTestUniqueConstraints + .map((uq) => uq.name) + .join(", ")}`, + ) + + // Clean up the test table + await queryRunner.dropTable("test_unique_table") + } finally { + await queryRunner.release() + } + }), + )) })