archive-gh-me/typeorm
1
0
Fork 0
mirror of https://github.com/typeorm/typeorm.git synced 2025-12-08 21:26:23 +00:00
Code Issues Projects Releases Wiki Activity

fix: resolve array modification bug in QueryRunner drop methods (#11564)

Browse Source 
* 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
This commit is contained in:
Vampire 2025-07-09 13:23:47 +07:00 committed by GitHub
parent 1737e97d1a
commit f351757a15
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
12 changed files with 531 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
src/driver/aurora-mysql/AuroraMysqlQueryRunner.ts
View File

@ -1414,7 +1414,7 @@ export class AuroraMysqlQueryRunner
tableOrName: Table | string,
columns: TableColumn[] | string[],
): Promise<void> {
for (const column of columns) {
for (const column of [...columns]) {
await this.dropColumn(tableOrName, column)
}
}

8
src/driver/cockroachdb/CockroachQueryRunner.ts
View File

@ -2311,7 +2311,7 @@ export class CockroachQueryRunner
tableOrName: Table | string,
columns: TableColumn[] | string[],
): Promise<void> {
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<void> {
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<void> {
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<void> {
for (const index of indices) {
for (const index of [...indices]) {
await this.dropIndex(tableOrName, index)
}
}

2
src/driver/mysql/MysqlQueryRunner.ts
View File

@ -1799,7 +1799,7 @@ export class MysqlQueryRunner extends BaseQueryRunner implements QueryRunner {
tableOrName: Table | string,
columns: TableColumn[] | string[],
): Promise<void> {
for (const column of columns) {
for (const column of [...columns]) {
await this.dropColumn(tableOrName, column)
}
}

2
src/driver/oracle/OracleQueryRunner.ts
View File

@ -1730,7 +1730,7 @@ export class OracleQueryRunner extends BaseQueryRunner implements QueryRunner {
tableOrName: Table | string,
columns: TableColumn[] | string[],
): Promise<void> {
for (const column of columns) {
for (const column of [...columns]) {
await this.dropColumn(tableOrName, column)
}
}

8
src/driver/postgres/PostgresQueryRunner.ts
View File

@ -2527,7 +2527,7 @@ export class PostgresQueryRunner
tableOrName: Table | string,
columns: TableColumn[] | string[],
): Promise<void> {
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<void> {
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<void> {
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<void> {
for (const index of indices) {
for (const index of [...indices]) {
await this.dropIndex(tableOrName, index)
}
}

2
src/driver/sap/SapQueryRunner.ts
View File

@ -1828,7 +1828,7 @@ export class SapQueryRunner extends BaseQueryRunner implements QueryRunner {
tableOrName: Table | string,
columns: TableColumn[] | string[],
): Promise<void> {
for (const column of columns) {
for (const column of [...columns]) {
await this.dropColumn(tableOrName, column)
}
}

6
src/driver/spanner/SpannerQueryRunner.ts
View File

@ -1052,7 +1052,7 @@ export class SpannerQueryRunner extends BaseQueryRunner implements QueryRunner {
tableOrName: Table | string,
columns: TableColumn[] | string[],
): Promise<void> {
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<void> {
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<void> {
for (const index of indices) {
for (const index of [...indices]) {
await this.dropIndex(tableOrName, index)
}
}

2
src/driver/sqlserver/SqlServerQueryRunner.ts
View File

@ -2115,7 +2115,7 @@ export class SqlServerQueryRunner
tableOrName: Table | string,
columns: TableColumn[] | string[],
): Promise<void> {
for (const column of columns) {
for (const column of [...columns]) {
await this.dropColumn(tableOrName, column)
}
}

95
test/functional/query-runner/drop-column.ts
View File

@ -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()
}),
))
})
})

164
test/functional/query-runner/drop-foreign-key.ts
View File

@ -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()
}
}),
))
})

127
test/functional/query-runner/drop-index.ts
View File

@ -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()
}
}),
))
})

133
test/functional/query-runner/drop-unique-constraint.ts
View File

@ -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()
}
}),
))
})