fix: use ALTER COLUMN instead of DROP and ADD to prevent data loss#11997
Open
veeceey wants to merge 2 commits intotypeorm:masterfrom
Open
fix: use ALTER COLUMN instead of DROP and ADD to prevent data loss#11997veeceey wants to merge 2 commits intotypeorm:masterfrom
veeceey wants to merge 2 commits intotypeorm:masterfrom
Conversation
User descriptionDescription of changeThis has been bugging me for a while — when you change a column's length or type in your entity, TypeORM generates a migration that does The fix is straightforward: for type and length changes, use DROP+ADD is still kept for cases where ALTER genuinely can't work:
For Postgres specifically, the Tested locally by stepping through the changeColumn logic with a few scenarios (length change, type change varchar→text) and verifying the generated SQL uses ALTER instead of DROP+ADD. Fixes #3357 Pull-Request Checklist
PR TypeBug fix Description
Diagram Walkthroughflowchart LR
A["Column Type/Length Change"] --> B{Incompatible Change?}
B -->|Array/Generated/IDENTITY| C["DROP + ADD Column"]
B -->|Type or Length Only| D["ALTER COLUMN"]
D --> E["Postgres: ALTER TYPE with USING"]
D --> F["MySQL/MariaDB: MODIFY COLUMN"]
D --> G["CockroachDB: ALTER TYPE"]
D --> H["SQL Server: ALTER COLUMN"]
C --> I["Preserve Data"]
E --> I
F --> I
G --> I
H --> I
|
| Relevant files | |||||||||
|---|---|---|---|---|---|---|---|---|---|
| Bug fix |
| ||||||||
| Tests |
|
PR Code Suggestions ✨Latest suggestions up to e610d3d
Previous suggestionsSuggestions up to commit 78f5e5e
Suggestions up to commit 3a53020
✅ Suggestions up to commit aa1dfb0
✅ Suggestions up to commit 5232a03
✅ Suggestions up to commit e73d5c2
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Code Review by Qodo
✅ 1.
|
Comment on lines
1
to
12
| import "reflect-metadata" | ||
| import { | ||
| createTestingConnections, | ||
| closeTestingConnections, | ||
| } from "../../utils/test-utils" | ||
| import { DataSource } from "../../../src/data-source/DataSource" | ||
| import { expect } from "chai" | ||
| import { Post } from "./entity/Post" | ||
|
|
||
| describe("github issues > #3357 Migration generation should use ALTER COLUMN instead of DROP+ADD to prevent data loss", () => { | ||
| let connections: DataSource[] | ||
|
|
There was a problem hiding this comment.
1. Test added under github-issues 📘 Rule violation ⛯ Reliability
The fix adds a new test only under test/github-issues/3357, rather than the functional test suite. This risks issue-specific test sprawl and does not meet the requirement to keep issue fixes in functional tests.
Agent Prompt
## Issue description
The new coverage for issue #3357 was added only under `test/github-issues`, but the checklist requires issue fixes to live in the functional test suite.
## Issue Context
Per compliance, issue-specific tests should be placed in `test/functional` with an issue reference when applicable.
## Fix Focus Areas
- test/github-issues/3357/issue-3357.test.ts[1-138]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Thanks for catching this. You're right that the test should be in the functional test suite rather than under github-issues. I'll move it to test/functional/schema-builder/ with a proper reference to the issue.
There was a problem hiding this comment.
Code Implementation 🛠️
Implementation: Update the test header imports and suite description to match the functional test suite conventions: import expect first, use reflect-metadata, use DataSource from the public src entrypoint, and reference the correct functional fixture entity (Album) and suite name.
Suggested change
| import "reflect-metadata" | |
| import { | |
| createTestingConnections, | |
| closeTestingConnections, | |
| } from "../../utils/test-utils" | |
| import { DataSource } from "../../../src/data-source/DataSource" | |
| import { expect } from "chai" | |
| import { Post } from "./entity/Post" | |
| describe("github issues > #3357 Migration generation should use ALTER COLUMN instead of DROP+ADD to prevent data loss", () => { | |
| let connections: DataSource[] | |
| import { expect } from "chai" | |
| import "reflect-metadata" | |
| import { DataSource } from "../../../src" | |
| import { | |
| closeTestingConnections, | |
| createTestingConnections, | |
| } from "../../utils/test-utils" | |
| import { Album } from "./entity/Album" | |
| describe("schema builder > alter column type", () => { | |
| let connections: DataSource[] |
See review comment here
Comment on lines
1295
to
1318
| } else if ( | ||
| oldColumn.type !== newColumn.type || | ||
| oldColumn.length !== newColumn.length | ||
| ) { | ||
| // Use ALTER COLUMN TYPE instead of DROP+ADD to preserve existing data. | ||
| // PostgreSQL supports ALTER COLUMN TYPE with USING for type conversions. | ||
| const newFullType = this.driver.createFullType(newColumn) | ||
| const oldFullType = this.driver.createFullType(oldColumn) | ||
|
|
||
| upQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | ||
| newColumn.name | ||
| }" TYPE ${newFullType} USING "${newColumn.name}"::${newFullType}`, | ||
| ), | ||
| ) | ||
| downQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | ||
| oldColumn.name | ||
| }" TYPE ${oldFullType} USING "${oldColumn.name}"::${oldFullType}`, | ||
| ), | ||
| ) | ||
| } else { |
There was a problem hiding this comment.
2. Skips combined column changes 🐞 Bug ✓ Correctness
When type/length changes, the new else-if branch emits only ALTER TYPE/MODIFY and skips the rest of changeColumn logic (rename, nullability/default/PK/unique/FK renames, etc.). Any scenario where type/length changes together with other attributes can generate incomplete/incorrect migrations (e.g., rename+type change won’t rename).
Agent Prompt
### Issue description
Type/length changes currently take an `else if` branch that prevents executing the rest of the `changeColumn` logic. This causes incomplete migrations when multiple properties (rename, nullability, constraints, etc.) change together with type/length.
### Issue Context
`changeColumn` is invoked per changed column and is expected to handle all differences between `oldColumn` and `newColumn` in one pass. The new `else if` short-circuits that.
### Fix Focus Areas
- src/driver/postgres/PostgresQueryRunner.ts[1282-1335]
- src/driver/cockroachdb/CockroachQueryRunner.ts[1363-1405]
- src/driver/mysql/MysqlQueryRunner.ts[1095-1135]
- src/driver/sqlserver/SqlServerQueryRunner.ts[1303-1348]
### Notes
A robust approach is:
1) Handle truly-incompatible cases via DROP+ADD (keep existing first `if`).
2) Otherwise enter the main change flow, and:
- if rename: emit rename queries and update in-memory column names used for subsequent statements.
- if type/length: emit ALTER TYPE/MODIFY/ALTER COLUMN queries.
- continue with existing nullability/default/PK/unique/etc handling.
3) Ensure downQueries reverse the order (e.g., type back, then rename back).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Valid point. The current else if branch does short-circuit the rest of the changeColumn logic, which means combined changes (e.g., rename + type change) won't be handled correctly. I'll restructure this so the type/length ALTER is emitted within the main flow rather than as a separate branch, allowing rename, nullability, defaults, and constraint changes to still be processed in the same pass.
There was a problem hiding this comment.
Code Implementation 🛠️
Implementation: Adjust ALTER COLUMN TYPE generation to reference the correct column name in down migrations when a rename occurs, by using oldColumn.name for the up query and newColumn.name for the down query.
Suggested change
| } else if ( | |
| oldColumn.type !== newColumn.type || | |
| oldColumn.length !== newColumn.length | |
| ) { | |
| // Use ALTER COLUMN TYPE instead of DROP+ADD to preserve existing data. | |
| // PostgreSQL supports ALTER COLUMN TYPE with USING for type conversions. | |
| const newFullType = this.driver.createFullType(newColumn) | |
| const oldFullType = this.driver.createFullType(oldColumn) | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | |
| newColumn.name | |
| }" TYPE ${newFullType} USING "${newColumn.name}"::${newFullType}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | |
| oldColumn.name | |
| }" TYPE ${oldFullType} USING "${oldColumn.name}"::${oldFullType}`, | |
| ), | |
| ) | |
| } else { | |
| if ( | |
| oldColumn.type !== newColumn.type || | |
| oldColumn.length !== newColumn.length | |
| ) { | |
| // Use ALTER COLUMN TYPE instead of DROP+ADD to preserve existing data. | |
| // PostgreSQL supports ALTER COLUMN TYPE with USING for type conversions. | |
| const newFullType = this.driver.createFullType(newColumn) | |
| const oldFullType = this.driver.createFullType(oldColumn) | |
| // In the up-query, the column has not been renamed yet. | |
| const upColumnName = oldColumn.name | |
| // In the down-query, we're starting from the post-up state. | |
| // If a rename happened, the current column name is newColumn.name. | |
| const downColumnName = newColumn.name | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath( | |
| table, | |
| )} ALTER COLUMN "${upColumnName}" TYPE ${newFullType} USING "${upColumnName}"::${newFullType}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath( | |
| table, | |
| )} ALTER COLUMN "${downColumnName}" TYPE ${oldFullType} USING "${downColumnName}"::${oldFullType}`, | |
| ), | |
| ) | |
| // update cloned table column type/length | |
| const clonedColumn = clonedTable.columns.find( | |
| (column) => column.name === oldColumn.name, | |
| ) | |
| if (clonedColumn) { | |
| clonedColumn.type = newColumn.type | |
| clonedColumn.length = newColumn.length | |
| clonedColumn.precision = newColumn.precision | |
| clonedColumn.scale = newColumn.scale | |
| } | |
| } |
See review comment here
src/driver/mysql/MysqlQueryRunner.ts
Outdated
Comment on lines
1111
to
1126
| } else if ( | ||
| oldColumn.type !== newColumn.type || | ||
| oldColumn.length !== newColumn.length | ||
| ) { | ||
| // Use MODIFY COLUMN instead of DROP+ADD to preserve existing data. | ||
| upQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(newColumn, true)}`, | ||
| ), | ||
| ) | ||
| downQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(oldColumn, true)}`, | ||
| ), | ||
| ) | ||
| } else { |
There was a problem hiding this comment.
3. Stale schema cache after alter 🐞 Bug ⛯ Reliability
The new ALTER TYPE/MODIFY branches don’t update clonedTable/oldColumn, but changeColumn still replaces the cached table with clonedTable. This leaves the QueryRunner’s cached schema stale after a type/length change, which can corrupt subsequent diffing and SQL generation within the same schema sync/migration generation.
Agent Prompt
### Issue description
After emitting ALTER TYPE/MODIFY statements, `clonedTable` is not updated, but it is still used to replace the cached table. This leaves the cached schema out of sync with the SQL that was generated/executed.
### Issue Context
`changeColumn` uses `clonedTable` as the new authoritative cached schema at the end of the method.
### Fix Focus Areas
- src/driver/postgres/PostgresQueryRunner.ts[1295-1318]
- src/driver/postgres/PostgresQueryRunner.ts[2441-2443]
- src/driver/cockroachdb/CockroachQueryRunner.ts[1374-1396]
- src/driver/cockroachdb/CockroachQueryRunner.ts[2151-2153]
- src/driver/mysql/MysqlQueryRunner.ts[1111-1126]
- src/driver/mysql/MysqlQueryRunner.ts[1612-1614]
- src/driver/sqlserver/SqlServerQueryRunner.ts[1316-1347]
- src/driver/sqlserver/SqlServerQueryRunner.ts[1956-1958]
### Notes
Minimum fix: locate the column in `clonedTable.columns` and update type/length/precision/scale/isArray/etc. to match `newColumn` when the ALTER/MODIFY branch is taken.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Good catch — the clonedTable cache definitely needs to be updated after emitting the ALTER TYPE/MODIFY statements. I'll add the column property updates (type, length, precision, scale, isArray, etc.) on clonedTable.columns to match newColumn in each driver's type-change branch.
There was a problem hiding this comment.
Code Implementation 🛠️
Implementation: Update the in-memory schema cache (clonedTable) when the MySQL MODIFY branch is used by syncing all relevant column properties from newColumn onto the corresponding column in clonedTable, and expand the MODIFY-trigger condition to include precision/scale/width changes.
Suggested change
| } else if ( | |
| oldColumn.type !== newColumn.type || | |
| oldColumn.length !== newColumn.length | |
| ) { | |
| // Use MODIFY COLUMN instead of DROP+ADD to preserve existing data. | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(newColumn, true)}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(oldColumn, true)}`, | |
| ), | |
| ) | |
| } else { | |
| } else { | |
| if ( | |
| oldColumn.type !== newColumn.type || | |
| oldColumn.length !== newColumn.length || | |
| oldColumn.width !== newColumn.width || | |
| oldColumn.precision !== newColumn.precision || | |
| oldColumn.scale !== newColumn.scale | |
| ) { | |
| // Use MODIFY COLUMN instead of DROP+ADD to preserve existing data. | |
| // Use oldColumn.name in the up-query because the column has not | |
| // been renamed yet at this point in the query sequence; the | |
| // CHANGE (rename) statement is emitted afterwards. | |
| const typeChangeColumn = newColumn.clone() | |
| typeChangeColumn.name = oldColumn.name | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(typeChangeColumn, true)}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(oldColumn, true)}`, | |
| ), | |
| ) | |
| // Keep cached table schema in sync with the emitted MODIFY statement. | |
| const clonedColumn = clonedTable.columns.find( | |
| (column) => column.name === oldColumn.name, | |
| ) | |
| if (clonedColumn) { | |
| clonedColumn.type = newColumn.type | |
| clonedColumn.length = newColumn.length | |
| clonedColumn.width = newColumn.width | |
| clonedColumn.precision = newColumn.precision | |
| clonedColumn.scale = newColumn.scale | |
| clonedColumn.unsigned = newColumn.unsigned | |
| clonedColumn.zerofill = newColumn.zerofill | |
| clonedColumn.charset = newColumn.charset | |
| clonedColumn.collation = newColumn.collation | |
| } | |
| } | |
| if (newColumn.name !== oldColumn.name) { | |
| // We don't change any column properties, just rename it. | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} CHANGE \`${ | |
| oldColumn.name | |
| }\` \`${newColumn.name}\` ${this.buildCreateColumnSql( | |
| oldColumn, | |
| true, | |
| true, | |
| )}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} CHANGE \`${ | |
| newColumn.name | |
| }\` \`${oldColumn.name}\` ${this.buildCreateColumnSql( | |
| oldColumn, | |
| true, | |
| true, | |
| )}`, | |
| ), | |
| ) |
See review comment here
Comment on lines
1374
to
1396
| } else if ( | ||
| oldColumn.type !== newColumn.type || | ||
| oldColumn.length !== newColumn.length | ||
| ) { | ||
| // Use ALTER COLUMN TYPE instead of DROP+ADD to preserve existing data. | ||
| const newFullType = this.driver.createFullType(newColumn) | ||
| const oldFullType = this.driver.createFullType(oldColumn) | ||
|
|
||
| upQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | ||
| newColumn.name | ||
| }" TYPE ${newFullType}`, | ||
| ), | ||
| ) | ||
| downQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | ||
| oldColumn.name | ||
| }" TYPE ${oldFullType}`, | ||
| ), | ||
| ) | ||
| } else { |
There was a problem hiding this comment.
4. Enum conversions may emit invalid type 🐞 Bug ✓ Correctness
Postgres/Cockroach ALTER TYPE branches use driver.createFullType(), but enums are represented with TableColumn.type = "enum" and the real DB type name is derived via buildEnumName()/enumName. If a column changes between enum and non-enum types, the new branch can emit TYPE enum (and invalid down SQL) and it also bypasses enum type lifecycle handling (create/drop/rename) used elsewhere.
Agent Prompt
### Issue description
Enum columns require special type naming (via `buildEnumName`/`enumName`) and often enum type lifecycle operations. The new type/length ALTER branch uses `createFullType`, which will produce `enum` for enum columns and bypasses existing enum-specific handling.
### Issue Context
Postgres introspection sets `TableColumn.type = "enum"` and stores `enumName` separately, and SQL generation uses `buildEnumName()` for enum types.
### Fix Focus Areas
- src/driver/postgres/PostgresQueryRunner.ts[1295-1318]
- src/driver/postgres/PostgresQueryRunner.ts[3819-3835]
- src/driver/postgres/PostgresQueryRunner.ts[4997-5027]
- src/driver/cockroachdb/CockroachQueryRunner.ts[1374-1396]
- src/driver/cockroachdb/CockroachQueryRunner.ts[4352-4372]
### Notes
Two viable paths:
- Conservative: fall back to DROP+ADD when converting to/from enum.
- Full support: ensure enum type exists (create), then ALTER COLUMN TYPE to the correct enum identifier (using `buildEnumName`), and drop old enum types when appropriate; ensure down migration mirrors this.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Agreed — enum columns need special handling. For the initial implementation, I'll take the conservative approach and fall back to DROP+ADD for any enum-related type conversions. This keeps the scope manageable while still fixing the core issue for non-enum type/length changes.
There was a problem hiding this comment.
Code Implementation 🛠️
Implementation: Update CockroachDB changeColumn compatibility checks to conservatively fall back to DROP+ADD when converting between enum/simple-enum and non-enum types. This prevents generating invalid ALTER COLUMN ... TYPE SQL for enum-related conversions and keeps enum lifecycle handling in the existing enum-specific branch.
Suggested change
| } else if ( | |
| oldColumn.type !== newColumn.type || | |
| oldColumn.length !== newColumn.length | |
| ) { | |
| // Use ALTER COLUMN TYPE instead of DROP+ADD to preserve existing data. | |
| const newFullType = this.driver.createFullType(newColumn) | |
| const oldFullType = this.driver.createFullType(oldColumn) | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | |
| newColumn.name | |
| }" TYPE ${newFullType}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | |
| oldColumn.name | |
| }" TYPE ${oldFullType}`, | |
| ), | |
| ) | |
| } else { | |
| const oldIsEnum = oldColumn.type === "enum" || oldColumn.type === "simple-enum" | |
| const newIsEnum = newColumn.type === "enum" || newColumn.type === "simple-enum" | |
| if ( | |
| newColumn.isArray !== oldColumn.isArray || | |
| oldColumn.generatedType !== newColumn.generatedType || | |
| oldColumn.asExpression !== newColumn.asExpression || | |
| // Conservative approach: enum-related conversions are not handled by the generic ALTER TYPE path. | |
| // This includes enum <-> non-enum and enum <-> simple-enum conversions. | |
| (oldIsEnum !== newIsEnum || (oldIsEnum && newIsEnum && oldColumn.type !== newColumn.type)) | |
| ) { | |
| // These changes are incompatible with ALTER COLUMN, so we must recreate | |
| await this.dropColumn(table, oldColumn) | |
| await this.addColumn(table, newColumn) | |
| // update cloned table | |
| clonedTable = table.clone() | |
| } else { |
See review comment here
|
Persistent review updated to latest commit 5232a03 |
Comment on lines
+1323
to
1368
| if ( | ||
| newColumn.type !== oldColumn.type || | ||
| newColumn.length !== oldColumn.length | ||
| ) { | ||
| // Use ALTER COLUMN instead of DROP+ADD to preserve existing data. | ||
| upQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath( | ||
| table, | ||
| )} ALTER COLUMN ${this.buildCreateColumnSql( | ||
| table, | ||
| newColumn, | ||
| true, | ||
| false, | ||
| true, | ||
| )}`, | ||
| ), | ||
| ) | ||
| downQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath( | ||
| table, | ||
| )} ALTER COLUMN ${this.buildCreateColumnSql( | ||
| table, | ||
| oldColumn, | ||
| true, | ||
| false, | ||
| true, | ||
| )}`, | ||
| ), | ||
| ) | ||
|
|
||
| // update cloned table column type/length | ||
| const clonedColumn = clonedTable.columns.find( | ||
| (column) => column.name === oldColumn.name, | ||
| ) | ||
| if (clonedColumn) { | ||
| clonedColumn.type = newColumn.type | ||
| clonedColumn.length = newColumn.length | ||
| clonedColumn.precision = newColumn.precision | ||
| clonedColumn.scale = newColumn.scale | ||
| } | ||
| } | ||
| if (newColumn.name !== oldColumn.name) { | ||
| // we need database name and schema name to rename FK constraints | ||
| let dbName: string | undefined = undefined |
There was a problem hiding this comment.
1. Sqlserver alter wrong column 🐞 Bug ✓ Correctness
SQL Server ALTER COLUMN is built from newColumn (including its name) before sp_rename runs, so name+type/length changes in one call can reference a non-existent column. This breaks manual migrations (QueryRunner API) and any future schema-builder paths that pass both changes together.
Agent Prompt
### Issue description
SQL Server `changeColumn` emits `ALTER COLUMN` using a column definition built from `newColumn` (including `newColumn.name`) before it emits `sp_rename`. If a caller changes name + type/length in one call, the ALTER targets the wrong name.
### Issue Context
`buildCreateColumnSql` always embeds the column name, so passing `newColumn` directly is unsafe unless the rename has already happened.
### Fix Focus Areas
- src/driver/sqlserver/SqlServerQueryRunner.ts[1323-1353]
- src/driver/sqlserver/SqlServerQueryRunner.ts[1386-1400]
- src/driver/sqlserver/SqlServerQueryRunner.ts[4167-4176]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Good catch. The ALTER COLUMN should target oldColumn.name (since the rename hasn't happened yet at that point), not newColumn.name. I'll fix the ordering so the type change uses the current column name, and the rename follows afterward.
There was a problem hiding this comment.
Code Implementation 🛠️
Implementation: Update SQL Server changeColumn type/length ALTER COLUMN generation to target the current (old) column name when a rename is also included in the same changeColumn call, by cloning newColumn and overriding its name to oldColumn.name for the ALTER COLUMN statement.
Suggested change
| if ( | |
| newColumn.type !== oldColumn.type || | |
| newColumn.length !== oldColumn.length | |
| ) { | |
| // Use ALTER COLUMN instead of DROP+ADD to preserve existing data. | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath( | |
| table, | |
| )} ALTER COLUMN ${this.buildCreateColumnSql( | |
| table, | |
| newColumn, | |
| true, | |
| false, | |
| true, | |
| )}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath( | |
| table, | |
| )} ALTER COLUMN ${this.buildCreateColumnSql( | |
| table, | |
| oldColumn, | |
| true, | |
| false, | |
| true, | |
| )}`, | |
| ), | |
| ) | |
| // update cloned table column type/length | |
| const clonedColumn = clonedTable.columns.find( | |
| (column) => column.name === oldColumn.name, | |
| ) | |
| if (clonedColumn) { | |
| clonedColumn.type = newColumn.type | |
| clonedColumn.length = newColumn.length | |
| clonedColumn.precision = newColumn.precision | |
| clonedColumn.scale = newColumn.scale | |
| } | |
| } | |
| if (newColumn.name !== oldColumn.name) { | |
| // we need database name and schema name to rename FK constraints | |
| let dbName: string | undefined = undefined | |
| if ( | |
| newColumn.type !== oldColumn.type || | |
| newColumn.length !== oldColumn.length | |
| ) { | |
| // Use ALTER COLUMN instead of DROP+ADD to preserve existing data. | |
| // Use oldColumn.name in the up-query because the column has not | |
| // been renamed yet at this point in the query sequence; the RENAME | |
| // statement is emitted afterwards. | |
| const typeChangeColumn = newColumn.clone() | |
| typeChangeColumn.name = oldColumn.name | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath( | |
| table, | |
| )} ALTER COLUMN ${this.buildCreateColumnSql( | |
| table, | |
| typeChangeColumn, | |
| true, | |
| false, | |
| true, | |
| )}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath( | |
| table, | |
| )} ALTER COLUMN ${this.buildCreateColumnSql( | |
| table, | |
| oldColumn, | |
| true, | |
| false, | |
| true, | |
| )}`, | |
| ), | |
| ) | |
| // update cloned table column type/length | |
| const clonedColumn = clonedTable.columns.find( | |
| (column) => column.name === oldColumn.name, | |
| ) | |
| if (clonedColumn) { | |
| clonedColumn.type = newColumn.type | |
| clonedColumn.length = newColumn.length | |
| clonedColumn.precision = newColumn.precision | |
| clonedColumn.scale = newColumn.scale | |
| } | |
| } |
See review comment here
Comment on lines
+1119
to
1148
| if ( | ||
| oldColumn.type !== newColumn.type || | ||
| oldColumn.length !== newColumn.length | ||
| ) { | ||
| // Use MODIFY COLUMN instead of DROP+ADD to preserve existing data. | ||
| upQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(newColumn, true)}`, | ||
| ), | ||
| ) | ||
| downQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(oldColumn, true)}`, | ||
| ), | ||
| ) | ||
|
|
||
| // update cloned table column type/length | ||
| const clonedColumn = clonedTable.columns.find( | ||
| (column) => column.name === oldColumn.name, | ||
| ) | ||
| if (clonedColumn) { | ||
| clonedColumn.type = newColumn.type | ||
| clonedColumn.length = newColumn.length | ||
| clonedColumn.precision = newColumn.precision | ||
| clonedColumn.scale = newColumn.scale | ||
| } | ||
| } | ||
| if (newColumn.name !== oldColumn.name) { | ||
| // We don't change any column properties, just rename it. | ||
| upQueries.push( |
There was a problem hiding this comment.
2. Mysql rename overwrites type 🐞 Bug ✓ Correctness
If changeColumn is called with both name and type/length changes, it emits MODIFY using newColumn (new name) and then emits CHANGE old→new using the *old column definition*, which can fail and/or revert the type/length change. This breaks manual migrations (QueryRunner API) for combined rename+type operations.
Agent Prompt
### Issue description
MySQL `changeColumn` can generate conflicting SQL when both the column name and type/length change in the same call: it first emits `MODIFY` using `newColumn` (includes new name), then emits `CHANGE old→new` using `oldColumn`’s definition (can revert the type/length change).
### Issue Context
`buildCreateColumnSql(newColumn, true)` includes the column name by default, and `CHANGE` requires a full column definition. Using `oldColumn` in the rename step is only correct when renaming without other column changes.
### Fix Focus Areas
- src/driver/mysql/MysqlQueryRunner.ts[1119-1169]
- src/driver/mysql/MysqlQueryRunner.ts[3475-3487]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
You're right — the CHANGE statement using oldColumn's definition will undo the type/length change from the preceding MODIFY. For MySQL, when both name and type/length change, I should emit a single CHANGE statement with the new name AND new column definition, rather than separate MODIFY + CHANGE steps.
There was a problem hiding this comment.
Code Implementation 🛠️
Implementation: Update MySQL changeColumn SQL generation to avoid emitting separate MODIFY + CHANGE statements when both rename and type/length changes occur. Instead, emit a single CHANGE oldName -> newName statement using the new column definition, and adjust the down query accordingly. Keep the existing MODIFY-only path for type/length changes without rename.
Suggested change
| if ( | |
| oldColumn.type !== newColumn.type || | |
| oldColumn.length !== newColumn.length | |
| ) { | |
| // Use MODIFY COLUMN instead of DROP+ADD to preserve existing data. | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(newColumn, true)}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(oldColumn, true)}`, | |
| ), | |
| ) | |
| // update cloned table column type/length | |
| const clonedColumn = clonedTable.columns.find( | |
| (column) => column.name === oldColumn.name, | |
| ) | |
| if (clonedColumn) { | |
| clonedColumn.type = newColumn.type | |
| clonedColumn.length = newColumn.length | |
| clonedColumn.precision = newColumn.precision | |
| clonedColumn.scale = newColumn.scale | |
| } | |
| } | |
| if (newColumn.name !== oldColumn.name) { | |
| // We don't change any column properties, just rename it. | |
| upQueries.push( | |
| } else { | |
| const isTypeOrLengthChanged = | |
| oldColumn.type !== newColumn.type || | |
| oldColumn.length !== newColumn.length | |
| if (isTypeOrLengthChanged && newColumn.name !== oldColumn.name) { | |
| // If both rename and type/length change, emit a single CHANGE with the new definition. | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} CHANGE \`${ | |
| oldColumn.name | |
| }\` \`${newColumn.name}\` ${this.buildCreateColumnSql( | |
| newColumn, | |
| true, | |
| true, | |
| true, | |
| )}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} CHANGE \`${ | |
| newColumn.name | |
| }\` \`${oldColumn.name}\` ${this.buildCreateColumnSql( | |
| oldColumn, | |
| true, | |
| true, | |
| true, | |
| )}`, | |
| ), | |
| ) | |
| // update cloned table column properties + name | |
| const clonedColumn = clonedTable.columns.find( | |
| (column) => column.name === oldColumn.name, | |
| ) | |
| if (clonedColumn) { | |
| clonedColumn.name = newColumn.name | |
| clonedColumn.type = newColumn.type | |
| clonedColumn.length = newColumn.length | |
| clonedColumn.precision = newColumn.precision | |
| clonedColumn.scale = newColumn.scale | |
| } | |
| } else { | |
| if (isTypeOrLengthChanged) { | |
| // Use MODIFY COLUMN instead of DROP+ADD to preserve existing data. | |
| // Use oldColumn.name in the up-query because the column has not | |
| // been renamed yet at this point in the query sequence. | |
| const typeChangeColumn = newColumn.clone() | |
| typeChangeColumn.name = oldColumn.name | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(typeChangeColumn, true)}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(oldColumn, true)}`, | |
| ), | |
| ) | |
| // update cloned table column type/length | |
| const clonedColumn = clonedTable.columns.find( | |
| (column) => column.name === oldColumn.name, | |
| ) | |
| if (clonedColumn) { | |
| clonedColumn.type = newColumn.type | |
| clonedColumn.length = newColumn.length | |
| clonedColumn.precision = newColumn.precision | |
| clonedColumn.scale = newColumn.scale | |
| } | |
| } | |
| if (newColumn.name !== oldColumn.name) { | |
| // We don't change any column properties, just rename it. | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} CHANGE \`${ | |
| oldColumn.name | |
| }\` \`${newColumn.name}\` ${this.buildCreateColumnSql( | |
| oldColumn, | |
| true, | |
| true, | |
| true, | |
| )}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} CHANGE \`${ | |
| newColumn.name | |
| }\` \`${oldColumn.name}\` ${this.buildCreateColumnSql( | |
| oldColumn, | |
| true, | |
| true, | |
| true, | |
| )}`, | |
| ), | |
| ) | |
| // rename index constraints | |
| clonedTable.findColumnIndices(oldColumn).forEach((index) => { | |
| const oldUniqueName = | |
| this.connection.namingStrategy.indexName( | |
| clonedTable, | |
| index.columnNames, | |
| ) | |
| // Skip renaming if Index has user defined constraint name | |
| if (index.name !== oldUniqueName) return | |
| // build new constraint name | |
| index.columnNames.splice( | |
| index.columnNames.indexOf(oldColumn.name), | |
| 1, | |
| ) | |
| index.columnNames.push(newColumn.name) | |
| const columnNames = index.columnNames | |
| .map((column) => `\`${column}\``) | |
| .join(", ") | |
| const newIndexName = | |
| this.connection.namingStrategy.indexName( | |
| clonedTable, | |
| index.columnNames, | |
| index.where, | |
| ) | |
| // build queries | |
| let indexType = "" | |
| if (index.isUnique) indexType += "UNIQUE " | |
| if (index.isSpatial) indexType += "SPATIAL " | |
| if (index.isFulltext) indexType += "FULLTEXT " | |
| const indexParser = | |
| index.isFulltext && index.parser | |
| ? ` WITH PARSER ${index.parser}` | |
| : "" | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath( | |
| table, | |
| )} DROP INDEX \`${ | |
| index.name | |
| }\`, ADD ${indexType}INDEX \`${newIndexName}\` (${columnNames})${indexParser}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath( | |
| table, | |
| )} DROP INDEX \`${newIndexName}\`, ADD ${indexType}INDEX \`${ | |
| index.name | |
| }\` (${columnNames})${indexParser}`, | |
| ), | |
| ) | |
| // replace constraint name | |
| index.name = newIndexName | |
| }) | |
| } | |
| } |
See review comment here
commit: |
|
Persistent review updated to latest commit aa1dfb0 |
Comment on lines
+1302
to
1339
| if ( | ||
| oldColumn.type !== newColumn.type || | ||
| oldColumn.length !== newColumn.length | ||
| ) { | ||
| // Use ALTER COLUMN TYPE instead of DROP+ADD to preserve existing data. | ||
| // PostgreSQL supports ALTER COLUMN TYPE with USING for type conversions. | ||
| const newFullType = this.driver.createFullType(newColumn) | ||
| const oldFullType = this.driver.createFullType(oldColumn) | ||
|
|
||
| upQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | ||
| newColumn.name | ||
| }" TYPE ${newFullType} USING "${newColumn.name}"::${newFullType}`, | ||
| ), | ||
| ) | ||
| downQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | ||
| oldColumn.name | ||
| }" TYPE ${oldFullType} USING "${oldColumn.name}"::${oldFullType}`, | ||
| ), | ||
| ) | ||
|
|
||
| // update cloned table column type/length | ||
| const clonedColumn = clonedTable.columns.find( | ||
| (column) => column.name === oldColumn.name, | ||
| ) | ||
| if (clonedColumn) { | ||
| clonedColumn.type = newColumn.type | ||
| clonedColumn.length = newColumn.length | ||
| clonedColumn.precision = newColumn.precision | ||
| clonedColumn.scale = newColumn.scale | ||
| } | ||
| } | ||
| if (oldColumn.name !== newColumn.name) { | ||
| // rename column | ||
| upQueries.push( |
There was a problem hiding this comment.
1. Postgres alter uses new name 🐞 Bug ✓ Correctness
In Postgres changeColumn, the new ALTER COLUMN TYPE query targets newColumn.name even though the column rename happens later, so rename+type/length changes can generate migrations that fail at runtime. The down query similarly targets oldColumn.name even though the down-rename happens later.
Agent Prompt
### Issue description
Postgres changeColumn emits `ALTER COLUMN ... TYPE` using `newColumn.name` before the column is renamed, and emits the down-type change using `oldColumn.name` before the down-rename. This breaks migrations that both rename and change type/length.
### Issue Context
`upQueries` and `downQueries` are executed in the order they are pushed. When a rename is involved, the column’s name in the database changes only when the rename query runs.
### Fix Focus Areas
- src/driver/postgres/PostgresQueryRunner.ts[1302-1352]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Right, same issue as the SQL Server one — the ALTER COLUMN TYPE query needs to use oldColumn.name since the rename hasn't happened yet. I'll fix this across both Postgres up and down queries.
There was a problem hiding this comment.
Code Implementation 🛠️
Implementation: Update Postgres changeColumn to generate ALTER COLUMN ... TYPE statements using the correct column identifier when a rename is also part of the change: use oldColumn.name for the up ALTER COLUMN (since rename happens later), and use newColumn.name for the down ALTER COLUMN (since the down-rename happens later). Adjust the USING clause to match the targeted column name in each query.
Suggested change
| if ( | |
| oldColumn.type !== newColumn.type || | |
| oldColumn.length !== newColumn.length | |
| ) { | |
| // Use ALTER COLUMN TYPE instead of DROP+ADD to preserve existing data. | |
| // PostgreSQL supports ALTER COLUMN TYPE with USING for type conversions. | |
| const newFullType = this.driver.createFullType(newColumn) | |
| const oldFullType = this.driver.createFullType(oldColumn) | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | |
| newColumn.name | |
| }" TYPE ${newFullType} USING "${newColumn.name}"::${newFullType}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | |
| oldColumn.name | |
| }" TYPE ${oldFullType} USING "${oldColumn.name}"::${oldFullType}`, | |
| ), | |
| ) | |
| // update cloned table column type/length | |
| const clonedColumn = clonedTable.columns.find( | |
| (column) => column.name === oldColumn.name, | |
| ) | |
| if (clonedColumn) { | |
| clonedColumn.type = newColumn.type | |
| clonedColumn.length = newColumn.length | |
| clonedColumn.precision = newColumn.precision | |
| clonedColumn.scale = newColumn.scale | |
| } | |
| } | |
| if (oldColumn.name !== newColumn.name) { | |
| // rename column | |
| upQueries.push( | |
| if ( | |
| oldColumn.type !== newColumn.type || | |
| oldColumn.length !== newColumn.length | |
| ) { | |
| // Use ALTER COLUMN TYPE instead of DROP+ADD to preserve existing data. | |
| // PostgreSQL supports ALTER COLUMN TYPE with USING for type conversions. | |
| const newFullType = this.driver.createFullType(newColumn) | |
| const oldFullType = this.driver.createFullType(oldColumn) | |
| // Use oldColumn.name in the up-query because the column has not been renamed yet | |
| // at this point in the query sequence (the RENAME COLUMN statement is emitted afterwards). | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | |
| oldColumn.name | |
| }" TYPE ${newFullType} USING "${oldColumn.name}"::${newFullType}`, | |
| ), | |
| ) | |
| // Use newColumn.name in the down-query because the column will be renamed back afterwards | |
| // in the down query sequence. | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | |
| newColumn.name | |
| }" TYPE ${oldFullType} USING "${newColumn.name}"::${oldFullType}`, | |
| ), | |
| ) | |
| // update cloned table column type/length | |
| const clonedColumn = clonedTable.columns.find( | |
| (column) => column.name === oldColumn.name, | |
| ) | |
| if (clonedColumn) { | |
| clonedColumn.type = newColumn.type | |
| clonedColumn.length = newColumn.length | |
| clonedColumn.precision = newColumn.precision | |
| clonedColumn.scale = newColumn.scale | |
| } | |
| } |
See review comment here
Comment on lines
+1119
to
1148
| if ( | ||
| oldColumn.type !== newColumn.type || | ||
| oldColumn.length !== newColumn.length | ||
| ) { | ||
| // Use MODIFY COLUMN instead of DROP+ADD to preserve existing data. | ||
| upQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(newColumn, true)}`, | ||
| ), | ||
| ) | ||
| downQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(oldColumn, true)}`, | ||
| ), | ||
| ) | ||
|
|
||
| // update cloned table column type/length | ||
| const clonedColumn = clonedTable.columns.find( | ||
| (column) => column.name === oldColumn.name, | ||
| ) | ||
| if (clonedColumn) { | ||
| clonedColumn.type = newColumn.type | ||
| clonedColumn.length = newColumn.length | ||
| clonedColumn.precision = newColumn.precision | ||
| clonedColumn.scale = newColumn.scale | ||
| } | ||
| } | ||
| if (newColumn.name !== oldColumn.name) { | ||
| // We don't change any column properties, just rename it. | ||
| upQueries.push( |
There was a problem hiding this comment.
2. Mysql modify+change inconsistent 🐞 Bug ✓ Correctness
In MySQL changeColumn, MODIFY is emitted using newColumn (new name) before the CHANGE rename runs, so rename+type/length changes can reference a non-existent column name. Even if name targeting is fixed, the CHANGE statement uses oldColumn’s definition, which can undo the earlier type/length change.
Agent Prompt
### Issue description
MySQL changeColumn emits `MODIFY` using `newColumn` before renaming, and the rename `CHANGE` statement uses `oldColumn`’s definition, which can both (1) target a non-existent name and (2) revert the type/length.
### Issue Context
In MySQL, `CHANGE old new <definition>` includes a full column definition.
### Fix Focus Areas
- src/driver/mysql/MysqlQueryRunner.ts[1119-1169]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Same root cause as the other MySQL comment — I'll consolidate rename + type/length into a single CHANGE statement when both are needed.
There was a problem hiding this comment.
Code Implementation 🛠️
Implementation: Consolidate MySQL rename + type/length changes into a single ALTER TABLE ... CHANGE statement when both are needed, and avoid emitting a preceding MODIFY that could target a non-existent column name or be reverted by a later CHANGE. Keep MODIFY only for non-rename type/length changes, and keep CHANGE (rename-only) using the old definition when no other definition changes are involved.
Suggested change
| if ( | |
| oldColumn.type !== newColumn.type || | |
| oldColumn.length !== newColumn.length | |
| ) { | |
| // Use MODIFY COLUMN instead of DROP+ADD to preserve existing data. | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(newColumn, true)}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(oldColumn, true)}`, | |
| ), | |
| ) | |
| // update cloned table column type/length | |
| const clonedColumn = clonedTable.columns.find( | |
| (column) => column.name === oldColumn.name, | |
| ) | |
| if (clonedColumn) { | |
| clonedColumn.type = newColumn.type | |
| clonedColumn.length = newColumn.length | |
| clonedColumn.precision = newColumn.precision | |
| clonedColumn.scale = newColumn.scale | |
| } | |
| } | |
| if (newColumn.name !== oldColumn.name) { | |
| // We don't change any column properties, just rename it. | |
| upQueries.push( | |
| } else { | |
| const isTypeOrLengthChanged = | |
| oldColumn.type !== newColumn.type || | |
| oldColumn.length !== newColumn.length | |
| // If both definition (type/length) AND name are changing, emit a single CHANGE. | |
| // In MySQL, CHANGE includes the full column definition, so a later CHANGE can | |
| // otherwise revert an earlier MODIFY. | |
| if (isTypeOrLengthChanged && newColumn.name !== oldColumn.name) { | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} CHANGE \`${ | |
| oldColumn.name | |
| }\` \`${newColumn.name}\` ${this.buildCreateColumnSql( | |
| newColumn, | |
| true, | |
| true, | |
| true, | |
| )}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} CHANGE \`${ | |
| newColumn.name | |
| }\` \`${oldColumn.name}\` ${this.buildCreateColumnSql( | |
| oldColumn, | |
| true, | |
| true, | |
| true, | |
| )}`, | |
| ), | |
| ) | |
| // update cloned table column definition + name | |
| const clonedColumn = clonedTable.columns.find( | |
| (column) => column.name === oldColumn.name, | |
| ) | |
| if (clonedColumn) { | |
| clonedColumn.type = newColumn.type | |
| clonedColumn.length = newColumn.length | |
| clonedColumn.precision = newColumn.precision | |
| clonedColumn.scale = newColumn.scale | |
| clonedColumn.name = newColumn.name | |
| } | |
| oldColumn.name = newColumn.name | |
| } else { | |
| if (isTypeOrLengthChanged) { | |
| // Use MODIFY COLUMN instead of DROP+ADD to preserve existing data. | |
| // Use oldColumn.name in the up-query because the column has not | |
| // been renamed yet at this point in the query sequence. | |
| const typeChangeColumn = newColumn.clone() | |
| typeChangeColumn.name = oldColumn.name | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(typeChangeColumn, true)}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(oldColumn, true)}`, | |
| ), | |
| ) | |
| // update cloned table column type/length | |
| const clonedColumn = clonedTable.columns.find( | |
| (column) => column.name === oldColumn.name, | |
| ) | |
| if (clonedColumn) { | |
| clonedColumn.type = newColumn.type | |
| clonedColumn.length = newColumn.length | |
| clonedColumn.precision = newColumn.precision | |
| clonedColumn.scale = newColumn.scale | |
| } | |
| } | |
| if (newColumn.name !== oldColumn.name) { | |
| // Rename only (no definition changes handled above). | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} CHANGE \`${ | |
| oldColumn.name | |
| }\` \`${newColumn.name}\` ${this.buildCreateColumnSql( | |
| oldColumn, | |
| true, | |
| true, | |
| true, | |
| )}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} CHANGE \`${ | |
| newColumn.name | |
| }\` \`${oldColumn.name}\` ${this.buildCreateColumnSql( | |
| oldColumn, | |
| true, | |
| true, | |
| true, | |
| )}`, | |
| ), | |
| ) | |
| // rename index constraints | |
| clonedTable.findColumnIndices(oldColumn).forEach((index) => { | |
| const oldUniqueName = | |
| this.connection.namingStrategy.indexName( | |
| clonedTable, | |
| index.columnNames, | |
| ) | |
| // Skip renaming if Index has user defined constraint name | |
| if (index.name !== oldUniqueName) return | |
| // build new constraint name | |
| index.columnNames.splice( | |
| index.columnNames.indexOf(oldColumn.name), | |
| 1, | |
| ) | |
| index.columnNames.push(newColumn.name) | |
| const columnNames = index.columnNames | |
| .map((column) => `\`${column}\``) | |
| .join(", ") | |
| const newIndexName = | |
| this.connection.namingStrategy.indexName( | |
| clonedTable, | |
| index.columnNames, | |
| index.where, | |
| ) | |
| // build queries | |
| let indexType = "" | |
| if (index.isUnique) indexType += "UNIQUE " | |
| if (index.isSpatial) indexType += "SPATIAL " | |
| if (index.isFulltext) indexType += "FULLTEXT " | |
| const indexParser = | |
| index.isFulltext && index.parser | |
| ? ` WITH PARSER ${index.parser}` | |
| : "" | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath( | |
| table, | |
| )} DROP INDEX \`${ | |
| index.name | |
| }\`, ADD ${indexType}INDEX \`${newIndexName}\` (${columnNames})${indexParser}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath( | |
| table, | |
| )} DROP INDEX \`${newIndexName}\`, ADD ${indexType}INDEX \`${ | |
| index.name | |
| }\` (${columnNames})${indexParser}`, | |
| ), | |
| ) | |
| // replace constraint name | |
| index.name = newIndexName | |
| }) | |
| // rename foreign key constraints | |
| clonedTable | |
| .findColumnForeignKeys(oldColumn) | |
| .forEach((foreignKey) => { | |
| const foreignKeyName = | |
| this.connection.namingStrategy.foreignKeyName( | |
| clonedTable, | |
| foreignKey.columnNames, | |
| this.getTablePath(foreignKey), | |
| foreignKey.referencedColumnNames, | |
| ) | |
| // Skip renaming if foreign key has user defined constraint name | |
| if (foreignKey.name !== foreignKeyName) return | |
| // build new constraint name | |
| foreignKey.columnNames.splice( | |
| foreignKey.columnNames.indexOf(oldColumn.name), | |
| 1, | |
| ) | |
| foreignKey.columnNames.push(newColumn.name) | |
| const columnNames = foreignKey.columnNames | |
| .map((column) => `\`${column}\``) | |
| .join(", ") | |
| const referencedColumnNames = | |
| foreignKey.referencedColumnNames | |
| .map((column) => `\`${column}\``) | |
| .join(",") | |
| const newForeignKeyName = | |
| this.connection.namingStrategy.foreignKeyName( | |
| clonedTable, | |
| foreignKey.columnNames, | |
| this.getTablePath(foreignKey), | |
| foreignKey.referencedColumnNames, | |
| ) | |
| // build queries | |
| let up = | |
| `ALTER TABLE ${this.escapePath( | |
| table, | |
| )} DROP FOREIGN KEY \`${ | |
| foreignKey.name | |
| }\`, ADD CONSTRAINT \`${newForeignKeyName}\` FOREIGN KEY (${columnNames}) ` + | |
| `REFERENCES ${this.escapePath( | |
| this.getTablePath(foreignKey), | |
| )}(${referencedColumnNames})` | |
| if (foreignKey.onDelete) | |
| up += ` ON DELETE ${foreignKey.onDelete}` | |
| if (foreignKey.onUpdate) | |
| up += ` ON UPDATE ${foreignKey.onUpdate}` | |
| let down = | |
| `ALTER TABLE ${this.escapePath( | |
| table, | |
| )} DROP FOREIGN KEY \`${newForeignKeyName}\`, ADD CONSTRAINT \`${ | |
| foreignKey.name | |
| }\` FOREIGN KEY (${columnNames}) ` + | |
| `REFERENCES ${this.escapePath( | |
| this.getTablePath(foreignKey), | |
| )}(${referencedColumnNames})` | |
| if (foreignKey.onDelete) | |
| down += ` ON DELETE ${foreignKey.onDelete}` | |
| if (foreignKey.onUpdate) | |
| down += ` ON UPDATE ${foreignKey.onUpdate}` | |
| upQueries.push(new Query(up)) | |
| downQueries.push(new Query(down)) | |
| // replace constraint name | |
| foreignKey.name = newForeignKeyName | |
| }) | |
| // rename old column in the Table object | |
| const oldTableColumn = clonedTable.columns.find( | |
| (column) => column.name === oldColumn.name, | |
| ) | |
| clonedTable.columns[ | |
| clonedTable.columns.indexOf(oldTableColumn!) | |
| ].name = newColumn.name | |
| oldColumn.name = newColumn.name | |
| } | |
| } |
See review comment here
|
Persistent review updated to latest commit 3a53020 |
Comment on lines
1074
to
1108
| if ( | ||
| oldColumn.type !== newColumn.type || | ||
| oldColumn.length !== newColumn.length | ||
| ) { | ||
| // Use MODIFY COLUMN instead of DROP+ADD to preserve existing data. | ||
| // Use oldColumn.name in the up-query because the column has not | ||
| // been renamed yet at this point in the query sequence; the | ||
| // CHANGE (rename) statement is emitted afterwards. | ||
| const typeChangeColumn = newColumn.clone() | ||
| typeChangeColumn.name = oldColumn.name | ||
| upQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(typeChangeColumn, true)}`, | ||
| ), | ||
| ) | ||
| downQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath(table)} MODIFY ${this.buildCreateColumnSql(oldColumn, true)}`, | ||
| ), | ||
| ) | ||
|
|
||
| // update cloned table column type/length | ||
| const clonedColumn = clonedTable.columns.find( | ||
| (column) => column.name === oldColumn.name, | ||
| ) | ||
| if (clonedColumn) { | ||
| clonedColumn.type = newColumn.type | ||
| clonedColumn.length = newColumn.length | ||
| clonedColumn.precision = newColumn.precision | ||
| clonedColumn.scale = newColumn.scale | ||
| } | ||
| } | ||
| if (newColumn.name !== oldColumn.name) { | ||
| // We don't change any column properties, just rename it. | ||
| upQueries.push( |
There was a problem hiding this comment.
1. Mysql rename reverts type 🐞 Bug ✓ Correctness
If a column is renamed and its type/length changes in the same changeColumn call, the emitted MySQL/Aurora CHANGE statement uses the *old* column definition and can undo the earlier MODIFY type/length change.
Agent Prompt
### Issue description
MySQL/Aurora `changeColumn` can emit `MODIFY` (type/length) and then `CHANGE` (rename) where the `CHANGE` uses the old column definition, potentially reverting the earlier type/length change.
### Issue Context
In MySQL, `ALTER TABLE ... CHANGE old new <column_definition>` always applies the provided column definition. If the definition is built from `oldColumn` while a type/length change was just applied, the rename statement can undo it.
### Fix Focus Areas
- src/driver/mysql/MysqlQueryRunner.ts[1074-1129]
- src/driver/aurora-mysql/AuroraMysqlQueryRunner.ts[857-912]
### Implementation notes
- If `(type/length changed) && (name changed)`: prefer emitting **one** `CHANGE` statement using the **new** column definition (with `newColumn.name`) and skip the separate `MODIFY` + rename block.
- Ensure `downQueries` reverses both rename and type/length in the correct order (often by also using a single `CHANGE` back to the old name with the old definition).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Acknowledged — this is the same issue flagged earlier. The fix will be to use a single CHANGE with the correct (new) column definition when both rename and type/length change together.
Comment on lines
1302
to
1327
| if ( | ||
| oldColumn.type !== newColumn.type || | ||
| oldColumn.length !== newColumn.length | ||
| ) { | ||
| // Use ALTER COLUMN TYPE instead of DROP+ADD to preserve existing data. | ||
| // PostgreSQL supports ALTER COLUMN TYPE with USING for type conversions. | ||
| const newFullType = this.driver.createFullType(newColumn) | ||
| const oldFullType = this.driver.createFullType(oldColumn) | ||
|
|
||
| // Use oldColumn.name in the up-query because the column has not | ||
| // been renamed yet at this point in the query sequence; the RENAME | ||
| // statement is emitted afterwards. | ||
| upQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | ||
| oldColumn.name | ||
| }" TYPE ${newFullType} USING "${oldColumn.name}"::${newFullType}`, | ||
| ), | ||
| ) | ||
| downQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | ||
| oldColumn.name | ||
| }" TYPE ${oldFullType} USING "${oldColumn.name}"::${oldFullType}`, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
2. Alter type keeps defaults 🐞 Bug ⛯ Reliability
Postgres/Cockroach now run ALTER COLUMN ... TYPE for type/length changes without temporarily dropping DEFAULTs; type changes can fail when an existing default expression cannot be cast to the new type.
Agent Prompt
### Issue description
`ALTER COLUMN TYPE` is now used for type/length changes, but defaults are not dropped first. Some defaults are not castable to the new type and will cause migrations to fail.
### Issue Context
The code already drops defaults for enum changes to avoid cast failures, but not for general type/length changes. This creates an inconsistency and a regression risk for columns with defaults.
### Fix Focus Areas
- src/driver/postgres/PostgresQueryRunner.ts[1302-1327]
- src/driver/cockroachdb/CockroachQueryRunner.ts[1334-1358]
- src/driver/postgres/PostgresQueryRunner.ts[1755-1780]
- src/driver/cockroachdb/CockroachQueryRunner.ts[1898-1921]
### Implementation notes
- In the type/length-change block, if `oldColumn.default` is non-null/undefined:
- push `ALTER TABLE ... ALTER COLUMN "<name>" DROP DEFAULT` before the TYPE change
- mark `defaultValueChanged = true`
- after the TYPE change, re-apply `newColumn.default` (if defined) or re-apply `oldColumn.default` as appropriate for up/down.
- Ensure existing later default-handling logic respects `defaultValueChanged` to prevent duplicate SET/DROP DEFAULT statements.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Good point. I'll add DROP DEFAULT before the ALTER TYPE and re-apply the default afterward, similar to how it's already done for enum changes. This prevents cast failures when the existing default isn't compatible with the new type.
There was a problem hiding this comment.
Code Implementation 🛠️
Implementation: Update the type/length change path to temporarily DROP DEFAULT before ALTER COLUMN ... TYPE and then re-apply the appropriate default afterward. Mark the default as changed to avoid duplicate default handling later in changeColumn.
Suggested change
| if ( | |
| oldColumn.type !== newColumn.type || | |
| oldColumn.length !== newColumn.length | |
| ) { | |
| // Use ALTER COLUMN TYPE instead of DROP+ADD to preserve existing data. | |
| // PostgreSQL supports ALTER COLUMN TYPE with USING for type conversions. | |
| const newFullType = this.driver.createFullType(newColumn) | |
| const oldFullType = this.driver.createFullType(oldColumn) | |
| // Use oldColumn.name in the up-query because the column has not | |
| // been renamed yet at this point in the query sequence; the RENAME | |
| // statement is emitted afterwards. | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | |
| oldColumn.name | |
| }" TYPE ${newFullType} USING "${oldColumn.name}"::${newFullType}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | |
| oldColumn.name | |
| }" TYPE ${oldFullType} USING "${oldColumn.name}"::${oldFullType}`, | |
| ), | |
| ) | |
| if ( | |
| oldColumn.type !== newColumn.type || | |
| oldColumn.length !== newColumn.length | |
| ) { | |
| // Use ALTER COLUMN TYPE instead of DROP+ADD to preserve existing data. | |
| // PostgreSQL supports ALTER COLUMN TYPE with USING for type conversions. | |
| const newFullType = this.driver.createFullType(newColumn) | |
| const oldFullType = this.driver.createFullType(oldColumn) | |
| // If column has a default value, temporarily drop it to avoid type cast issues. | |
| if (oldColumn.default !== null && oldColumn.default !== undefined) { | |
| // mark default as changed to prevent double update later in this method | |
| defaultValueChanged = true | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | |
| oldColumn.name | |
| }" DROP DEFAULT`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | |
| oldColumn.name | |
| }" SET DEFAULT ${oldColumn.default}`, | |
| ), | |
| ) | |
| } | |
| // Use oldColumn.name in the up-query because the column has not | |
| // been renamed yet at this point in the query sequence; the RENAME | |
| // statement is emitted afterwards. | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | |
| oldColumn.name | |
| }" TYPE ${newFullType} USING "${oldColumn.name}"::${newFullType}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | |
| oldColumn.name | |
| }" TYPE ${oldFullType} USING "${oldColumn.name}"::${oldFullType}`, | |
| ), | |
| ) | |
| // Re-apply default after type change (if it was dropped above). | |
| if (oldColumn.default !== null && oldColumn.default !== undefined) { | |
| if (newColumn.default !== null && newColumn.default !== undefined) { | |
| upQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | |
| oldColumn.name | |
| }" SET DEFAULT ${newColumn.default}`, | |
| ), | |
| ) | |
| downQueries.push( | |
| new Query( | |
| `ALTER TABLE ${this.escapePath(table)} ALTER COLUMN "${ | |
| oldColumn.name | |
| }" DROP DEFAULT`, | |
| ), | |
| ) | |
| } else { | |
| // old had default, new does not => keep it dropped in up | |
| // down should restore old default (already handled above before TYPE) | |
| } | |
| } | |
| // update cloned table column type/length | |
| const clonedColumn = clonedTable.columns.find( | |
| (column) => column.name === oldColumn.name, | |
| ) | |
| if (clonedColumn) { | |
| clonedColumn.type = newColumn.type | |
| clonedColumn.length = newColumn.length | |
| clonedColumn.precision = newColumn.precision | |
| clonedColumn.scale = newColumn.scale | |
| } | |
| } |
See review comment here
Comment on lines
+3
to
+14
| import { PrimaryGeneratedColumn } from "../../../../src/decorator/columns/PrimaryGeneratedColumn" | ||
|
|
||
| @Entity({ synchronize: false }) | ||
| @Entity() | ||
| export class Album { | ||
| @PrimaryColumn() | ||
| @PrimaryGeneratedColumn() | ||
| id: number | ||
|
|
||
| @Column() | ||
| name: string | ||
| @Column({ type: "varchar", length: "50" }) | ||
| title: string | ||
|
|
||
| @Column({ type: "varchar", length: "200" }) | ||
| description: string |
There was a problem hiding this comment.
3. Album entity breaks tests 🐞 Bug ⛯ Reliability
The shared schema-builder Album test entity was changed to @Entity() (synchronize defaults on) and different columns, which can cause other schema-builder suites to auto-create/drop album during reloadTestingDatabases() and break test isolation.
Agent Prompt
### Issue description
A shared schema-builder test entity (`test/functional/schema-builder/entity/Album.ts`) was modified to be synchronized by default and to change its columns. Other test suites import this same entity and call `reloadTestingDatabases()`, which runs `synchronize(true)` and can now unexpectedly create/drop tables.
### Issue Context
This can lead to cross-test pollution or failures in `custom-db-and-schema-sync` (and any suite importing this entity).
### Fix Focus Areas
- test/functional/schema-builder/entity/Album.ts[1-15]
- test/functional/schema-builder/custom-db-and-schema-sync.test.ts[14-25]
- test/utils/test-utils.ts[504-509]
- test/functional/schema-builder/alter-column-type.test.ts[1-27]
### Implementation notes
- Preferred: create a new entity file used only by `alter-column-type.test.ts` (e.g. `test/functional/schema-builder/alter-column-type/entity/Album.ts` or similar) and revert the shared `schema-builder/entity/Album.ts` to its prior behavior.
- Alternative: keep shared `Album` as `synchronize:false` and adjust the new test to explicitly create schema/tables (e.g., set metadata.synchronize=true then `connection.synchronize(true)` in the test setup).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
You're right — modifying the shared Album entity is a bad idea since it affects other test suites. I'll create a dedicated entity file for the alter-column-type test and revert the shared Album entity to its original state.
There was a problem hiding this comment.
Code Implementation 🛠️
Implementation: Prevent the shared schema-builder Album test entity from being auto-synchronized by default by disabling synchronization at the entity level, avoiding cross-suite schema changes during reloadTestingDatabases().
Suggested change
| import { PrimaryGeneratedColumn } from "../../../../src/decorator/columns/PrimaryGeneratedColumn" | |
| @Entity({ synchronize: false }) | |
| @Entity() | |
| export class Album { | |
| @PrimaryColumn() | |
| @PrimaryGeneratedColumn() | |
| id: number | |
| @Column() | |
| name: string | |
| @Column({ type: "varchar", length: "50" }) | |
| title: string | |
| @Column({ type: "varchar", length: "200" }) | |
| description: string | |
| import { PrimaryGeneratedColumn } from "../../../../src/decorator/columns/PrimaryGeneratedColumn" | |
| @Entity({ synchronize: false }) | |
| export class Album { | |
| @PrimaryGeneratedColumn() | |
| id: number | |
| @Column({ type: "varchar", length: "50" }) | |
| title: string | |
| @Column({ type: "varchar", length: "200" }) | |
| description: string |
See review comment here
|
Persistent review updated to latest commit 78f5e5e |
Comment on lines
1107
to
1109
| if (newColumn.name !== oldColumn.name) { | ||
| // We don't change any column properties, just rename it. | ||
| upQueries.push( |
There was a problem hiding this comment.
1. Mysql rename+type change reverts type 🐞 Bug ✓ Correctness
When a column's type/length AND name change simultaneously, the new MODIFY step correctly applies the new type, but the subsequent CHANGE (rename) step calls buildCreateColumnSql(oldColumn, ...) which carries the original type/length definition. MySQL's CHANGE syntax rewrites the full column definition, so the rename step silently reverts the type change made by MODIFY.
Agent Prompt
## Issue description
When a column's type/length AND name change simultaneously in MySQL/AuroraMySQL, the new MODIFY step correctly applies the new type, but the subsequent CHANGE (rename) step uses `buildCreateColumnSql(oldColumn, ...)` which carries the original type/length. MySQL's CHANGE syntax rewrites the full column definition, so the rename step silently reverts the type change.
## Issue Context
MySQL's `CHANGE \`old\` \`new\` <full_col_def>` syntax requires a complete column definition. The current code passes `oldColumn` (original type/length) to `buildCreateColumnSql`, overwriting the MODIFY that preceded it.
## Fix Focus Areas
- src/driver/mysql/MysqlQueryRunner.ts[1107-1130]
- src/driver/aurora-mysql/AuroraMysqlQueryRunner.ts[889-910]
In both files, when the rename block fires after a type-change MODIFY, the CHANGE up-query should use `typeChangeColumn` (already constructed with `newColumn`'s type and `oldColumn.name`) instead of `oldColumn`. The down-query should use `oldColumn` (which is correct for reverting). Specifically, replace `buildCreateColumnSql(oldColumn, true, true)` in the up-query of the rename block with `buildCreateColumnSql(typeChangeColumn, true, true)` — but note `typeChangeColumn` must be in scope (move it to the outer `else` block if needed).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Acknowledged — same MySQL rename+type issue. Will be addressed as part of the CHANGE consolidation fix.
Comment on lines
1323
to
1370
| if ( | ||
| newColumn.type !== oldColumn.type || | ||
| newColumn.length !== oldColumn.length | ||
| ) { | ||
| // Use ALTER COLUMN instead of DROP+ADD to preserve existing data. | ||
| // Use oldColumn.name in the up-query because the column has not | ||
| // been renamed yet at this point in the query sequence; the RENAME | ||
| // statement is emitted afterwards. | ||
| const typeChangeColumn = newColumn.clone() | ||
| typeChangeColumn.name = oldColumn.name | ||
| upQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath( | ||
| table, | ||
| )} ALTER COLUMN ${this.buildCreateColumnSql( | ||
| table, | ||
| typeChangeColumn, | ||
| true, | ||
| false, | ||
| true, | ||
| )}`, | ||
| ), | ||
| ) | ||
| downQueries.push( | ||
| new Query( | ||
| `ALTER TABLE ${this.escapePath( | ||
| table, | ||
| )} ALTER COLUMN ${this.buildCreateColumnSql( | ||
| table, | ||
| oldColumn, | ||
| true, | ||
| false, | ||
| true, | ||
| )}`, | ||
| ), | ||
| ) | ||
|
|
||
| // update cloned table column type/length | ||
| const clonedColumn = clonedTable.columns.find( | ||
| (column) => column.name === oldColumn.name, | ||
| ) | ||
| if (clonedColumn) { | ||
| clonedColumn.type = newColumn.type | ||
| clonedColumn.length = newColumn.length | ||
| clonedColumn.precision = newColumn.precision | ||
| clonedColumn.scale = newColumn.scale | ||
| } | ||
| } |
There was a problem hiding this comment.
2. Sql server type change fails on indexed columns 🐞 Bug ⛯ Reliability
SQL Server does not allow ALTER TABLE … ALTER COLUMN to change a column's data type when non-clustered indexes or statistics reference that column. The new type-change block emits the ALTER COLUMN directly without dropping and re-creating dependent indexes, causing a guaranteed runtime failure for any indexed column.
Agent Prompt
## Issue description
SQL Server does not allow `ALTER TABLE … ALTER COLUMN` to change a column's data type when non-clustered indexes or statistics reference that column. The new type-change block emits the ALTER COLUMN directly without managing dependent indexes, causing a runtime failure for any indexed column.
## Issue Context
SQL Server error: 'The object \'IX_...\' is dependent on column \'...\'. ALTER TABLE ALTER COLUMN ... failed because one or more objects access this column.'
The rename path in the same method already shows the correct pattern for iterating indexes via `clonedTable.findColumnIndices(oldColumn)`. For a type change, indexes must be dropped before and re-created after.
## Fix Focus Areas
- src/driver/sqlserver/SqlServerQueryRunner.ts[1323-1370]
Before the `ALTER COLUMN` up-query, iterate `clonedTable.findColumnIndices(oldColumn)` and emit `DROP INDEX` for each. After the `ALTER COLUMN`, emit `CREATE INDEX` to restore them. Do the same for unique constraints via `clonedTable.findColumnUniques(oldColumn)`. Mirror the down-queries in reverse order. Reference the existing index-drop/recreate logic already present in the `changeColumn` method for the nullability-change path.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Good catch — SQL Server does require indexes to be dropped before altering a column's type. I'll add the index drop/recreate logic similar to what already exists in the nullability-change path, using clonedTable.findColumnIndices() and clonedTable.findColumnUniques().
Contributor
Author
|
Thanks for the thorough review! I've responded to all the inline comments. Here's a summary of the fixes I'll be making:
Will push these fixes shortly. |
…rations When a column's type or length changes, the migration generator previously used DROP COLUMN followed by ADD COLUMN, which destroys all existing data in the column. This changes the behavior to use ALTER COLUMN TYPE (Postgres, CockroachDB), MODIFY COLUMN (MySQL/MariaDB), or ALTER COLUMN (SQL Server) instead, which preserves the data. DROP+ADD is still used for truly incompatible changes like array<->non-array conversions and generated column expression changes, where ALTER COLUMN cannot be used. Fixes: typeorm#3357
- Move type/length ALTER logic into main change flow so it no longer short-circuits rename, nullability, default, and constraint changes - Update clonedTable.columns after ALTER TYPE/MODIFY to prevent stale schema cache - Fall back to DROP+ADD for enum type conversions since createFullType() produces literal "enum" string - Move test from test/github-issues/3357/ to test/functional/schema-builder/ per project conventions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
78f5e5e to
e610d3d
Compare
Code Review by Qodo
1. MySQL rename+type broken
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of change
This has been bugging me for a while — when you change a column's length or type in your entity, TypeORM generates a migration that does
DROP COLUMN+ADD COLUMNinstead of just altering the column in place. That obviously nukes all the data in the column, which is pretty bad for production databases.The fix is straightforward: for type and length changes, use
ALTER COLUMN ... TYPE(Postgres/CockroachDB),MODIFY COLUMN(MySQL/MariaDB), orALTER COLUMN(SQL Server) instead of dropping and recreating.DROP+ADD is still kept for cases where ALTER genuinely can't work:
For Postgres specifically, the
USINGclause is included so the database can attempt implicit type casts when the type changes (e.g.varchar→text).Tested locally by stepping through the changeColumn logic with a few scenarios (length change, type change varchar→text) and verifying the generated SQL uses ALTER instead of DROP+ADD.
Fixes #3357
Pull-Request Checklist
masterbranchFixes #3357tests/**.test.ts)docs/docs/**.md) - N/A, no docs changes needed for a bugfix in query generation