fix: resolve array modification bug in QueryRunner drop methods #11563 #11564
Conversation
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 typeorm#11563
WalkthroughThis update modifies several query runner methods for various database drivers to iterate over shallow copies of arrays when dropping columns, indices, foreign keys, and unique constraints. This prevents issues from array mutation during iteration. New tests are added to verify that all specified schema elements are dropped without skipping any due to array modification. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant QR as QueryRunner
participant DB as Database
Test->>QR: dropColumns(table, [col1, col2, col3, col4])
loop For each column in [...columns]
QR->>DB: DROP COLUMN colX
DB-->>QR: Success
end
QR-->>Test: Operation complete
Possibly related issues
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/driver/postgres/PostgresQueryRunner.ts (2)
2969-2972: Foreign-key batch drop safeguardedUsing a spread copy eliminates the mutation hazard. 👍
Minor nit: if the array is extremely large,slice()might be marginally faster than the spread operator, but that’s non-blocking and stylistic.
3097-3099: Indices loop fixed likewiseThe same issue is resolved here; implementation is sound and mirrors other methods.
Consider adding a small helper likefor (const i of toArray(indices))to DRY this pattern across drivers, but optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/driver/aurora-mysql/AuroraMysqlQueryRunner.ts(1 hunks)src/driver/cockroachdb/CockroachQueryRunner.ts(4 hunks)src/driver/mysql/MysqlQueryRunner.ts(1 hunks)src/driver/oracle/OracleQueryRunner.ts(1 hunks)src/driver/postgres/PostgresQueryRunner.ts(4 hunks)src/driver/sap/SapQueryRunner.ts(1 hunks)src/driver/spanner/SpannerQueryRunner.ts(3 hunks)src/driver/sqlserver/SqlServerQueryRunner.ts(1 hunks)test/github-issues/11563/entity/TestEntity.ts(1 hunks)test/github-issues/11563/issue-11563.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/github-issues/11563/entity/TestEntity.ts (2)
src/decorator/columns/PrimaryGeneratedColumn.ts (1)
PrimaryGeneratedColumn(55-119)src/decorator/columns/Column.ts (1)
Column(134-220)
🔇 Additional comments (20)
test/github-issues/11563/entity/TestEntity.ts (1)
1-10: Test entity is well-structured for testing the array mutation fix.The
TestEntityproperly uses TypeORM decorators and provides a simple but effective schema for testing the batch drop operations that were affected by the array iteration bug. The entity structure supports comprehensive testing ofdropColumns,dropIndices,dropForeignKeys, anddropUniqueConstraintsmethods.src/driver/aurora-mysql/AuroraMysqlQueryRunner.ts (1)
1417-1417: Excellent fix for the array mutation during iteration bug.The use of the spread operator
[...columns]creates a shallow copy that prevents the iteration from being affected whendropColumninternally modifies the original array. This ensures all columns are dropped without skipping elements, which was the core issue described in #11563.This change is minimal, targeted, and consistent with the fix pattern applied across other database drivers in this PR.
src/driver/sqlserver/SqlServerQueryRunner.ts (1)
2118-2118: Excellent fix for the array modification during iteration bug!The use of the spread operator
[...columns]correctly creates a shallow copy of the array, preventing the iteration from being affected when individualdropColumncalls modify the original array. This ensures all columns are properly dropped without any being skipped due to index shifts during iteration.src/driver/mysql/MysqlQueryRunner.ts (1)
1802-1802: LGTM! Excellent fix for the array modification bug.The spread operator
[...columns]creates a shallow copy of the input array, preventing the iteration from being affected if the original array is modified during thedropColumnoperations. This change correctly addresses the bug described in issue #11563 where elements could be skipped during batch drop operations.src/driver/oracle/OracleQueryRunner.ts (1)
1733-1733: LGTM! Correct fix for the array modification bug.The use of the spread operator
[...columns]creates a shallow copy of the array, preventing the iteration bug where dropping columns during the loop would modify the original array and cause elements to be skipped. This fix aligns perfectly with the PR objectives and ensures all specified columns are properly dropped.src/driver/sap/SapQueryRunner.ts (1)
1831-1831: LGTM! Excellent fix for the array modification during iteration bug.The use of the spread operator to create a shallow copy of the
columnsarray before iteration is the correct solution to prevent elements from being skipped when the underlying data structures are modified during thedropColumncalls. This defensive programming approach ensures that all specified columns are properly dropped without iterator invalidation issues.src/driver/postgres/PostgresQueryRunner.ts (2)
2530-2532: Iteration now safe against in-loop mutations – good fixIterating over a shallow copy (
[...columns]) prevents the skipped-element bug whendropColumnmutates the original array. ✔️
No further issues spotted here.
2731-2733: Consistent defensive copy for unique constraintsSame protective pattern applied to
dropUniqueConstraints. Looks correct and keeps behaviour consistent with the single-drop version.src/driver/spanner/SpannerQueryRunner.ts (3)
1055-1055: LGTM! Critical bug fix implemented correctly.The spread operator creates a shallow copy of the columns array, preventing the iteration bug where elements get skipped due to array modifications during the async
dropColumncalls.
1345-1345: LGTM! Critical bug fix implemented correctly.The spread operator creates a shallow copy of the foreignKeys array, preventing the iteration bug where elements get skipped due to array modifications during the async
dropForeignKeycalls.
1419-1419: LGTM! Critical bug fix implemented correctly.The spread operator creates a shallow copy of the indices array, preventing the iteration bug where elements get skipped due to array modifications during the async
dropIndexcalls.test/github-issues/11563/issue-11563.ts (5)
16-40: Excellent test suite setup with comprehensive driver coverage.The test configuration properly includes all affected drivers and handles schema setup/teardown correctly. The driver selection aligns with the drivers modified in this PR.
41-143: Well-structured test for column drop iteration bug.The test properly validates the core issue by:
- Adding multiple test columns to trigger the bug scenario
- Verifying all columns exist before the drop operation
- Using the batch
dropColumnsmethod that was affected by the bug- Confirming all test columns are removed while preserving entity columns
The error messages are descriptive and will help with debugging if the test fails.
145-213: Comprehensive test for index drop iteration bug.The test follows the same excellent pattern as the column test, properly validating that all indices are dropped without skipping any due to array modification during iteration.
215-349: Thorough foreign key test with proper driver filtering.The test correctly skips Spanner (which doesn't support foreign keys) and includes proper setup of a referenced table. The cleanup properly removes the auxiliary table, preventing test interference.
351-465: Complete unique constraint test with appropriate driver exclusions.The test properly excludes drivers that don't support unique constraints or handle them differently (Spanner, SAP, MySQL variants). This ensures the test only runs on databases where the functionality is supported.
src/driver/cockroachdb/CockroachQueryRunner.ts (4)
2314-2314: LGTM! Correctly fixes the array iteration bug.Using the spread operator
[...columns]creates a shallow copy of the array, preventing iteration issues when the original array is modified during individual drop operations. This matches the fix pattern described in the PR objectives.
2518-2518: LGTM! Correctly fixes the array iteration bug.Using the spread operator
[...uniqueConstraints]creates a shallow copy of the array, preventing iteration issues when the original array is modified during individual drop operations. This matches the fix pattern described in the PR objectives.
2715-2715: LGTM! Correctly fixes the array iteration bug.Using the spread operator
[...foreignKeys]creates a shallow copy of the array, preventing iteration issues when the original array is modified during individual drop operations. This matches the fix pattern described in the PR objectives.
2800-2800: LGTM! Correctly fixes the array iteration bug.Using the spread operator
[...indices]creates a shallow copy of the array, preventing iteration issues when the original array is modified during individual drop operations. This matches the fix pattern described in the PR objectives.
| import { Table } from "../../../src/schema-builder/table/Table" | ||
| import { TestEntity } from "./entity/TestEntity" | ||
|
|
||
| describe("github issues > #11563 QueryRunner array modification during iteration bug", () => { |
There was a problem hiding this comment.
We are moving away from creating tests for specific GitHub issues.
Can you move these tests to the appropriate places in test/functional/query-runner/drop-column.ts, test/functional/query-runner/drop-index.ts, etc?
There was a problem hiding this comment.
Thanks for suggestion @sgarner, I've created 4 separate tests in test/functional/query-runner.
commit: |
|
Nice :) |
Description of change
This PR fixes a critical array modification bug in QueryRunner implementations where iterating over arrays while simultaneously modifying them during iteration causes elements to be skipped.
Fixes:
Problem:
Multiple QueryRunner drop methods (
dropColumns,dropIndices,dropForeignKeys,dropUniqueConstraints) had a classic "iteration while modifying" bug. When these methods iterate over arrays usingfor (const item of array)and call individual drop methods, the individual methods internally remove items from the same array being iterated over. This causes the iterator to skip elements because array indices shift during iteration.Example of the bug:
When dropping columns
[A, B, C, D]:[B, C, D], but iterator moves to index 1Solution:
Changed all affected methods to iterate over a shallow copy of the input array using the spread operator
[...array]. This ensures that modifications to the original array don't affect the iteration.Before (buggy):
After (fixed):
Affected drivers and methods:
Verification:
Impact:
This fix prevents potential data loss scenarios where users expect all specified database objects to be dropped but some are silently left behind due to the iteration bug.
Pull-Request Checklist
masterbranchFixes #11563Summary by CodeRabbit
Bug Fixes
Tests