Fix: Restore Meta.indexes when altering fields (#486, #491)#498
Fix: Restore Meta.indexes when altering fields (#486, #491)#498robberwick wants to merge 2 commits intomicrosoft:devfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes SQL Server migration behavior in mssql/schema.py so that indexes dropped during ALTER COLUMN operations are properly restored, including indexes defined via Meta.indexes, and avoids duplicate index creation in combined migrations (CreateModel + AlterField).
Changes:
- Refactors
_alter_field()index/constraint restoration to includeMeta.indexesand to restore all indexes for AutoField/BigAutoField alterations. - Adds deduplication against
deferred_sqlandpost_actionsto prevent creating the same index twice in combined migrations. - Adds an extensive regression test suite covering type/nullability changes, renames, FK alterations, multi-column indexes, and combined vs split migration contexts.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
mssql/schema.py |
Refactors index restoration in _alter_field() to restore Meta.indexes and deduplicate creation, including special handling for AutoField/BigAutoField changes. |
testapp/tests/test_indexes.py |
Adds TransactionTestCase regression coverage for index retention/restoration across many migration scenarios and execution contexts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
testapp/tests/test_indexes.py
Outdated
| Runs with both split and combined migrations | ||
| """ | ||
| for use_single_migration in [False, True]: | ||
| context_desc = "combined context" if use_single_migration else "split contexts" |
There was a problem hiding this comment.
context_desc is assigned but never used. This will be flagged by flake8/pyflakes (F841) and can break CI. Remove the assignment or use it in the subTest message/error output.
| context_desc = "combined context" if use_single_migration else "split contexts" |
There was a problem hiding this comment.
superfluous assignment removed and rebased into 4e5e36a
testapp/tests/test_indexes.py
Outdated
| context_desc = "combined context" if use_single_migration else "split contexts" | ||
| with self.subTest(single_migration=use_single_migration): |
There was a problem hiding this comment.
context_desc is assigned but never used. This will be flagged by flake8/pyflakes (F841) and can break CI. Remove the assignment or use it in the subTest message/error output.
There was a problem hiding this comment.
superfluous assignment removed and rebased into 4e5e36a
| # -------------------------------------------------------------------------------- | ||
| for index in model._meta.indexes: | ||
| # Get the field objects for this index | ||
| index_fields = [model._meta.get_field(field_name) for field_name in index.fields] |
There was a problem hiding this comment.
Index restoration uses model._meta.get_field(field_name) for each entry in index.fields. In Django, Index(fields=...) can include ordering prefixes like '-created_at', which would raise FieldDoesNotExist here. Consider normalizing field names (strip leading '-' / '+') or using index.fields_orders (when available) to resolve actual model fields before computing index_columns_list.
| index_fields = [model._meta.get_field(field_name) for field_name in index.fields] | |
| # Index.fields may include ordering prefixes like '-created_at'. | |
| # Prefer fields_orders when available to resolve actual field names. | |
| if hasattr(index, "fields_orders") and index.fields_orders: | |
| index_field_names = [field_name for field_name, _order in index.fields_orders] | |
| else: | |
| index_field_names = list(index.fields) | |
| index_fields = [ | |
| model._meta.get_field(field_name.lstrip("+-")) | |
| for field_name in index_field_names | |
| ] |
There was a problem hiding this comment.
This may be an issue, but if it is, I think it already exists elsewhere e.g. in the [delete_indexes method](https://github.com/microsoft/mssql-django/blob/b1b5f3cc125038ba6b4d56b7acff93e8a278faf2/mssql/schema.py#L961C1-L964C46).
Given that this is an established pattern for resolving field names, I'd suggest punting this out as a separate issue instead of attempting to address it here.
|
|
||
| if ( | ||
| new_field.get_internal_type() not in ("JSONField", "TextField") and | ||
| (old_field.db_index or not new_field.db_index) and | ||
| new_field.db_index or | ||
| ((indexes_dropped and sorted(indexes_dropped) == sorted([index.name for index in model._meta.indexes])) or | ||
| (indexes_dropped and sorted(indexes_dropped) == sorted(auto_index_names))) | ||
| ): | ||
| create_index_sql_statement = self._create_index_sql(model, [new_field]) | ||
| if create_index_sql_statement.__str__() not in [sql.__str__() for sql in self.deferred_sql]: |
There was a problem hiding this comment.
Note: the restoration step here is deleted, as it was not correctly restoring Meta.indexes. The restoration is now handled in the column alteration cleanup phase below.
|
@microsoft-github-policy-service agree company="HP" |
232164c to
6eb4b68
Compare
6eb4b68 to
6953139
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Fixes #486 — Indexes defined via
Meta.indexesare dropped and not recreated when altering an indexed column (e.g., changingmax_lengthor nullability).Fixes #491 — Duplicate index error (
ProgrammingError) when changingAutoFieldtoBigAutoFieldin a combined migration where another field hasdb_index=True.Issue #486: Meta.indexes not restored after field alteration
When SQL Server needs to alter a column's type or nullability,
_alter_fieldinschema.pydrops all indexes on the affected column first (SQL Server requires this). However, the restoration logic that runs afterward only handled:db_index=Truesingle-field indexesindex_togetherindexesIndexes defined via
Meta.indexeswere never restored. This meant any index defined inMeta.indexeswould be silently dropped whenever a participating column was altered.Issue #491: Duplicate index on AutoField → BigAutoField change
When
AutoFieldis changed toBigAutoField, the restoration logic unconditionally recreated alldb_index=Trueindexes viaself.execute(). In a combined migration (whereCreateModelandAlterFieldare in the same migration file), the index already existed indeferred_sql(queued duringCreateModelbut not yet executed). This caused the same index to be created twice, resulting in aProgrammingError.Fix
The index restoration phase in
_alter_fieldhas been refactored to:Restore
Meta.indexes: After column alteration, indexes frommodel._meta.indexesinvolving the altered field are now restored usingindex.create_sql().Handle AutoField/BigAutoField comprehensively: When an
AutoField/BigAutoFieldis altered, ALL indexes are restored (db_index, index_together, and Meta.indexes), since SQL Server drops all indexes on the table for IDENTITY column changes.Deduplicate index creation: Before executing any index creation, the SQL is checked against both
deferred_sqlandpost_actions. This prevents the duplicate creation that caused Duplicate index error when changing AutoField to BigAutoField in combined migration with db_index=True field #491.Remove the broken nullability-path restoration: The old nullability change path had ad-hoc index restoration logic. This has been removed in favor of the unified restoration block that handles all cases.
Changes
mssql/schema.py: Refactored_alter_fieldindex restoration to handleMeta.indexes, AutoField/BigAutoField changes, and deduplication in a unified block.Testing
testapp/tests/test_indexes.py: Added new test methods inTestMetaIndexesRetainedcovering:db_index=Truecombined withMeta.indexesunique_togethercoexistence withMeta.indexesindex_togetherretention (Django < 5.1)Each test runs in both "split migration" and "combined migration" modes because Django's
schema_editoruses adeferred_sqlqueue that changes the timing of index creation, and each mode exercises different code paths:Split mode (two separate
schema_editorcontexts): Simulates two separate migration files. The first context creates the table and its indexes, then exits, at which pointdeferred_sqlis flushed and all indexes physically exist in the database. The second context then runs theAlterFieldoperation, which queries the database, finds and drops the indexes, alters the column, and must restore them.Combined mode (single
schema_editorcontext): Simulates a single migration file containing bothCreateModelandAlterField. Here,CreateModeladds index creation SQL todeferred_sqlbut it has not been executed yet whenAlterFieldruns. The indexes don't physically exist in the database, so the DROP phase finds nothing. However, the RESTORE phase must not blindly create indexes that are already queued indeferred_sql, or they will be created twice when the context exits and flushes the queue.Known existing bugs not fixed by this PR (documented with
@expectedFailuretests)RenameFieldAND altered in the same migration,_delete_indexes()fails because it looks up the field by the old name which no longer exists in model state.Indexes from Meta.indexes not restored when RenameField and AlterField (type or nullability change) occur in the same migration #499
unique_together+unique=Trueon same field: When a field has bothunique=Trueand participates inunique_together, only the single-field unique constraint is restored (theunique_togetherrestoration is in anelsebranch).unique_togetherconstraint not restored after field alteration when field hasunique=True#494