Skip ALTER COLUMN for computed columns when only CLR type changes#38252
Skip ALTER COLUMN for computed columns when only CLR type changes#38252m-x-shokhzod wants to merge 2 commits into
Conversation
Have you verified whether this is a SQL Server-specific thing, or universal to all databases? I'm pretty sure it's the former, in which case you're introducing a SQL Server-specific change into ModelDiffer, which is provider-independent. |
The migration model differ produced an AlterColumnOperation when the CLR
type of a property mapped to a computed column changed (e.g. int → long
for a column with .HasComputedColumnSql("DATALENGTH(...)")). The
generated ALTER TABLE ... ALTER COLUMN then failed at runtime with
"Cannot alter column ... because it is 'COMPUTED'".
A computed column's store type and collation are derived from the
expression; they aren't user-configurable. CLR-type-only changes are
metadata on the EF side and require no database-side change. SQL Server
specifically rejects ALTER COLUMN on computed columns altogether.
The fix lives in SqlServerMigrationsSqlGenerator (provider-specific):
when both source and target are computed, set alterStatementNeeded to
false. This suppresses the ALTER COLUMN emission while letting other
separately-emitted facets (notably the comment block at line 488 via
sp_addextendedproperty) still apply. The drop+add path for expression
changes earlier in the method is unchanged.
MigrationsModelDiffer remains provider-independent: it still produces
AlterColumnOperation as before; other providers (MySQL with MODIFY
COLUMN, PostgreSQL, etc.) decide for themselves how to handle it.
Tests:
- Unit: AlterColumnOperation_computed_column_with_only_clr_type_change_is_noop
asserts the generator produces empty SQL for the bug case.
- Unit: AlterColumnOperation_computed_column_with_changed_expression_drops_and_adds
guards the existing drop+add path.
- Functional: Alter_computed_column_clr_type_only_change_is_noop runs the
scenario end-to-end against real SQL Server in the CI matrix; verified
locally against Azure SQL Edge on Apple Silicon.
Fixes dotnet#33425
ad662f8 to
cdc292a
Compare
|
Good point — you're right. The constraint is SQL Server specific: PostgreSQL and SQLite have similar but narrower restrictions on generated columns, and MySQL handles type changes on generated columns fine via Reworked (force-pushed
|
There was a problem hiding this comment.
Pull request overview
This PR addresses SQL Server migrations failing when EF scaffolds an ALTER COLUMN for a computed column whose CLR type changed (e.g. int → long) while the computed expression is unchanged. Since SQL Server rejects ALTER COLUMN on computed columns, the generator is updated to avoid emitting that SQL and new regression tests are added.
Changes:
- Suppress
ALTER TABLE ... ALTER COLUMNSQL generation for computed columns when both old/new are computed (leaving expression-change behavior intact via the existing drop+add path). - Add SQL generator regression coverage for “CLR type only” changes (no SQL) vs expression changes (drop+add).
- Add end-to-end migrations functional coverage ensuring the migration completes without emitting SQL for the CLR-type-only scenario.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs |
Avoids emitting ALTER COLUMN for computed columns (when both old/new are computed). |
test/EFCore.SqlServer.FunctionalTests/Migrations/SqlServerMigrationsSqlGeneratorTest.cs |
Adds generator-level regression tests for computed columns (no-op vs drop+add). |
test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs |
Adds functional regression test ensuring no SQL is emitted for CLR-type-only computed column changes. |
| // SQL Server cannot ALTER COLUMN on a computed column (the type is derived from the | ||
| // expression, not user-configurable). When source and target are both computed with the | ||
| // same expression and persistence (the drop+add path above didn't trigger), suppress | ||
| // the ALTER statement — type/precision/scale/nullability/collation/annotation diffs | ||
| // either don't apply or cannot be applied this way; emitting ALTER COLUMN would fail | ||
| // with "Cannot alter column ... because it is 'COMPUTED'". Comment changes are handled | ||
| // separately via sp_addextendedproperty below and still apply. See #33425. | ||
| if (operation.ComputedColumnSql != null && operation.OldColumn.ComputedColumnSql != null) | ||
| { | ||
| alterStatementNeeded = false; | ||
| } |
| // SQL Server cannot ALTER COLUMN on a computed column (the type is derived from the | ||
| // expression, not user-configurable). When source and target are both computed with the | ||
| // same expression and persistence (the drop+add path above didn't trigger), suppress | ||
| // the ALTER statement — type/precision/scale/nullability/collation/annotation diffs | ||
| // either don't apply or cannot be applied this way; emitting ALTER COLUMN would fail | ||
| // with "Cannot alter column ... because it is 'COMPUTED'". Comment changes are handled | ||
| // separately via sp_addextendedproperty below and still apply. See #33425. |
Addresses Copilot review on dotnet#38252: the previous fix suppressed `ALTER COLUMN` for CLR-type-only changes on computed columns, but the `narrowed` path still dropped and recreated every index on the column because `columnType != oldType`. Hoist the computed-column-no-op detection earlier and reuse it to gate both the index rebuild and the `ALTER COLUMN` emission. Add an end-to-end regression test that creates an index on the computed column, changes only the CLR type, and asserts no SQL is emitted — would have caught the original issue.
Migrations for properties mapped to a computed column failed at runtime when only the CLR type changed (e.g.
int→longfor a column with.HasComputedColumnSql("DATALENGTH(...)")). EF emittedALTER TABLE ... ALTER COLUMN, which SQL Server rejected with:A computed column's store type is derived from the expression, so a CLR-type-only change is metadata on the EF side and requires no database-side change.
Fix
In
SqlServerMigrationsSqlGenerator.Generate(AlterColumnOperation), detect the no-op case once and use it to short-circuit both the index drop/recreate path and theALTER COLUMNemission:The flag is computed once and gates two sites:
narrowedblock at the top — preventsGetIndexesToRebuild/DropIndexes/CreateIndexesfrom running for a column the DB won't see modified.alterStatementNeededreset — suppresses the actualALTER COLUMNSQL.The provider-independent
MigrationsModelDifferis untouched — the constraint is SQL Server-specific (PostgreSQL/SQLite generated-column rules differ, MySQL allowsMODIFY COLUMNon generated columns), so the suppression belongs in the SQL Server generator.Comment changes via
sp_addextendedpropertyfurther down still run — this is intentional and covered by the existingAlter_computed_column_add_commentregression test.Behavior
ALTER COLUMN→ SQL Server rejectsALTER COLUMNsp_addextendedpropertyTests
Generator-level (
SqlServerMigrationsSqlGeneratorTest):AlterColumnOperation_computed_column_with_only_clr_type_change_is_noop— asserts empty SQL for the bug caseAlterColumnOperation_computed_column_with_changed_expression_drops_and_adds— guards the existing drop+add pathEnd-to-end (
MigrationsSqlServerTest):Alter_computed_column_clr_type_only_change_is_noop— full migration against real SQL Server, zero SQLAlter_computed_column_clr_type_only_change_does_not_rebuild_indexes— same scenario with an index on the computed column, asserts noDROP INDEX/CREATE INDEX(would have caught the original Copilot-flagged regression)CI matrix (SqlServer 2019/2022/2025) covers end-to-end execution.
Fixes #33425