Restore Meta.indexes after RenameField and AlterField

[Rob Berwick (robberwick)]

Fixes #499.
Fixes #619.

A migration that renames a field and then changes its type or nullability could permanently drop a Meta.indexes index or raise FieldDoesNotExist. Django updates the model field during RenameField, but structured index metadata can retain the old field name; the SQL Server schema editor then sees migration state that no longer matches the physical index.

This PR now addresses the full rename-sensitive structured-index path uncovered during review. It keeps Meta.indexes and Meta.constraints UniqueConstraint references aligned across migration state, deferred SQL, schema-editor drop/recreate operations, and optimizer-folded CreateModel operations.

The initial change addressed the two rename-plus-alter regressions in #499. Review correctly identified that resolving every unknown field name to the field currently being altered was unsafe: removed fields, reused names, multiple renames, filtered conditions, and covering indexes could otherwise fail or silently move an index to the wrong column.

Addressing those findings required preserving structured field references through migration state and deferred SQL rather than guessing from FieldDoesNotExist. The same machinery is used by conditional and covering UniqueConstraint definitions, particularly when a rename is reconstructed from migration state or folded into CreateModel. Review of that deferred constraint path then exposed inconsistent handling of unsupported OR/negated predicates between AddConstraint and CreateModel.

The constraint changes are therefore included deliberately. They share the state-rewrite, condition-rewrite, deferred-rendering, and drop/recreate paths needed for #499; splitting them now would require duplicating or temporarily undoing that shared machinery and would leave the optimizer/deferred cases divided across interdependent PRs. Issue #619 records the expanded behavior and acceptance criteria explicitly.

Changes

Preserve structured migration state

• Rewrite renamed field references in Meta.indexes and Meta.constraints UniqueConstraint definitions.
• Cover ProjectState.rename_field(), the Django 3.2 RenameField.state_forwards() path, and CreateModel.reduce() when migration optimization absorbs a rename.
• Clone definitions instead of mutating preserved historical state.

Reconcile and restore indexes safely

• Match stale Meta.indexes key and include entries to the existing named physical index instead of mapping every unresolved name to new_field.
• Fetch table index metadata in one catalog query and preserve explicit names, ordering, conditions, expressions, included columns, and other index options when rebuilding.
• Track indexes actually dropped by the current alteration, so removed indexes are not retargeted and indexes still queued in deferred_sql are not created twice.
• Handle multiple renames, reused field names, removed fields, combined column rename/type changes, and unbound replacement fields.

Keep filtered definitions structured

• Rewrite field references inside copied Q/expression trees, including nested and transformed F() references and tuple/list RHS values, without changing string literals.
• Resolve pk aliases and ForeignKey attnames when deciding whether a filtered index or constraint depends on the renamed field.
• Keep deferred filtered-index and conditional-unique predicates structured until SQL rendering so later renames update identifiers safely.

Preserve UniqueConstraint semantics

• Rebuild affected Meta.constraints UniqueConstraint objects through their structured definitions, preserving conditions and key-versus-INCLUDE semantics for covering constraints.
• Defer conditional constraints declared through CreateModel through the same structured predicate path used by later renames.
• Apply the backend's existing predicate limitation consistently to AddConstraint and CreateModel: nested OR and negated conditions raise NotImplementedError instead of reaching SQL Server as invalid filtered-index SQL.

Restore indexes dropped by AutoField changes

For AutoField/BigAutoField column-renaming paths, SQL Server requires all indexes on the table to be dropped. Restore the dropped db_index, index_together (Django < 5.1), unique_together, Meta.indexes, and Meta.constraints UniqueConstraint definitions while avoiding deferred-create collisions.

Testing

Regression coverage in testapp/tests/test_indexes.py and testapp/tests/test_constraints.py includes:

• rename plus type or nullability change, in split and combined migrations;
• removed/reused names and multiple renames;
• filtered, expression, covering, deferred, and condition-only indexes;
• migration-state reconstruction and optimizer-folded CreateModel;
• pk aliases and ForeignKey attname references;
• conditional and covering UniqueConstraint rename behavior;
• consistent rejection of top-level/nested OR and negated unique conditions;
• AutoField/BigAutoField index restoration across the supported index/constraint categories; and
• Django-version-specific state and index_together paths.

The separate pre-existing AutoField restoration gap for an unrelated null=True, unique=True field-level index is not fixed by this PR; that index is created by the nullable-unique field path and has db_index=False.

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

This PR fixes a Django migration edge case in the SQL Server backend where Meta.indexes entries could be dropped (or index handling could error) when a RenameField is followed by an AlterField (type/nullability) in the same migration, by making index drop/restore logic resilient to stale field names (issue #499).

Changes:

• Updates mssql/schema.py to resolve stale Index.fields names when determining which indexes to drop/restore and when generating CREATE INDEX SQL.
• Rebuilds Index instances with corrected field names before calling create_sql() so explicit names/options/ordering are preserved.
• Enables the existing regression tests by removing @expectedFailure and updating docstrings to reference #499.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
mssql/schema.py	Adjusts index deletion/restoration logic in _alter_field() / _delete_indexes() to handle stale index field names after rename+alter sequences.
testapp/tests/test_indexes.py	Turns previously expected-failure scenarios into active regressions for rename+type-change and rename+nullability-change.

Suppressed comments (1)

mssql/schema.py:999

• The stale-field detection checks model._meta.get_field(field_name) against raw values from index.fields. For ordered indexes, those raw values can start with '-' (e.g. '-a'), which will always raise FieldDoesNotExist even when the underlying field exists, causing unnecessary cloning/reconstruction.

                for field_name in index.fields:
                    try:
                        model._meta.get_field(field_name)
                    except FieldDoesNotExist:
                        has_stale_fields = True

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Gaurav Sharma (bewithgaurav)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

🔵 Needs a closer look

Multiple renames, filtered indexes, and covering indexes remain incorrectly handled.

Review details

Suppressed comments (5)

Previously missed (3) — in code that hasn't changed since the last review.

mssql/schema.py:1020

• The restoration path has the same many-to-one fallback: every stale field is replaced with the currently altered field. If more than one indexed field was renamed earlier, this can recreate the index with duplicate/wrong columns (for example ['aa', 'aa'] instead of ['aa', 'bb']). Use a per-field rename mapping here rather than replacing all unresolved names with new_field.name.

This issue also appears on line 1157 of the same file.
mssql/schema.py:1024

• Only fields is corrected; deconstruct() carries the stale condition forward unchanged. Django's rename state logic does not rewrite Meta.indexes, and Index.create_sql() resolves a condition against the current model, so a filtered index such as Index(fields=['a'], condition=Q(a__isnull=False)) still raises FieldError after a is renamed and altered. Rewrite field references in index conditions as part of the clone (and add a filtered-index regression case).

This issue also appears on line 1179 of the same file.
mssql/schema.py:431

• This entry still sits under KNOWN BUGS/LIMITATIONS and states in the present tense that these indexes are not restored and _delete_indexes() fails, even though this PR enables the tests as passing. Remove this resolved item (and renumber the remaining item) so the schema-editor documentation does not advertise a fixed defect as current behavior.

mssql/schema.py:1163

• Mapping every unresolved field to the field currently being altered breaks an index after multiple preceding renames. For example, after a -> aa and b -> bb, altering aa resolves stale Index(fields=['a', 'b']) as ['aa', 'aa']; _constraint_names() then cannot find the actual index on ['aa', 'bb'], leaving it in place for ALTER COLUMN. Preserve each old-to-new rename mapping or resolve the existing index's actual columns rather than collapsing all missing names to new_field.column.

        def _resolve_column(field_name):
            try:
                return model._meta.get_field(field_name).column
            except FieldDoesNotExist:
                # The field was renamed by a preceding RenameField; the old name
                # is stale. Use new_field.column since sp_rename has already run.
                return new_field.column

mssql/schema.py:1182

• Affected-index detection only examines key fields. Because this backend advertises covering-index support, Index(fields=['b'], include=['a']) is valid; after renaming and altering a, this loop does not drop that dependent index, and SQL Server can reject the ALTER COLUMN. Include index.include (and other index references such as conditions) when deciding which named index to drop and restore.

        for index in model._meta.indexes:
            columns = [_resolve_column(field) for field, _ in index.fields_orders]
            if old_field.column in columns or new_field.column in columns:
                index_columns.append(columns)

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 100%

🎯 Overall Coverage 87.07%

📈 Total Lines Covered: 2956 out of 3395 📁 Project: mssql-django

---

Diff Coverage

Diff: dev...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

---

📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)

- mssql/creation.py: 60.3%  (73 lines)
- mssql/operations.py: 79.7%  (399 lines)
- mssql/management/commands/inspectdb.py: 81.8%  (11 lines)
- mssql/compiler.py: 83.5%  (643 lines)
- mssql/functions.py: 87.1%  (427 lines)
- mssql/client.py: 87.5%  (40 lines)
- mssql/schema.py: 89.0%  (1027 lines)
- mssql/introspection.py: 90.9%  (132 lines)
- mssql/base.py: 93.5%  (555 lines)
- mssql/__init__.py: 100.0%  (1 lines)

---

🔗 Quick Links

⚙️ Build Summary	📋 Coverage Details
View Azure DevOps Build	Browse Full Coverage Report

[Gaurav Sharma (bewithgaurav)]

went through this against a local sql2022 container on django 6.1. the two tests it un-skips do pass and the full testapp suite stays at 214 green, so the rename plus alter path it targets is genuinely fixed.

the part I want to work through is the fallback at mssql/schema.py line 1020 and line 1163. when an index field name does not resolve, both spots assume the missing name was renamed into the field being altered. FieldDoesNotExist does not tell you that, and on the autofield path a wrong guess does not raise. it builds the index on the wrong column and reports success. repros are in the line comments.

one framing note so this lands in proportion: all of it needs hand written migrations. makemigrations puts RemoveIndex before the rename and AddIndex after, so generated ones are not exposed. but hand editing is exactly how you keep data across a rename plus type change, which is the case #499 is about.

[Rob Berwick (robberwick)]

Thanks for the review Gaurav Sharma (@bewithgaurav). I've addressed the comments in daa32ea and aa29f82 (two commits because the first one is regression tests for the issues identified, and everyone loves tests 😉)

1. daa32ea Test #499: cover stale Meta.indexes rebuilding

   • Adds split- and combined-migration regressions for stale key fields after an AutoField alteration, removed fields followed by an unrelated alteration, multiple renames, filtered indexes, and covering indexes.
   • Uses targeted sys.indexes / sys.index_columns assertions where Django constraint introspection does not expose filter definitions or included-column metadata.

2. aa29f82 Fix #499: reconcile stale Meta indexes

   • Replaces the unsafe FieldDoesNotExist -> new_field fallback.
   • Reconciles stale index references only from the existing physical named index, including key fields, included columns, and partial-index conditions.
   • Does not recreate an index when its named physical index no longer exists, avoiding retargeting a removed index to an unrelated field.
   • Removes the obsolete rename-and-alter known-bug note.

[Gaurav Sharma (bewithgaurav)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

🟡 Changes recommended

Index restoration currently skips ordinary indexes and can issue invalid drops, while filtered expression conditions may crash.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 3
• Review effort level: Balanced

[Gaurav Sharma (bewithgaurav)]

Rob Berwick (@robberwick) - thanks for iterating fast, the tests are failing and copilot suggestions look correct to me, requesting you to please make the changes
will re-review once done, let me know if you need any help

[Gaurav Sharma (bewithgaurav)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Copilot review overview

🟡 Changes recommended

Cloning can break migration-serializable custom index and constraint subclasses by changing their constructor arguments.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 2

Open (3)

• Preserve custom Index signatures during field renames · New
• Move changelog entry under the Unreleased Fixed section
• Document structured index migration fix in changelog

[Copilot]

Copilot review overview

🔵 Needs a closer look

Expression-based negation bypasses validation, and conditional custom constraint rendering is no longer polymorphic.

Review effort: Balanced
Findings: 2

Open (2)

• Move changelog entry under the Unreleased Fixed section
• Document structured index migration fix in changelog

Resolved since last review (1)

• Preserve custom Index signatures during field renames

Previously missed (1)

In code that hasn't changed since last review

Preserve custom constraint_sql handling for UniqueConstraint subclasses

mssql/​schema.py:2276

This isinstance() branch bypasses constraint.constraint_sql() for every conditional UniqueConstraint subclass. A custom subclass that overrides that documented schema hook previously controlled its CreateModel DDL, but now silently receives the backend's generic unique-index rendering instead. Preserve polymorphic constraint_sql() handling for subclasses that override it, while using the structured deferred path for the built-in implementation.

[Gaurav Sharma (bewithgaurav)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Gaurav Sharma (bewithgaurav)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Copilot review overview

🔵 Needs a closer look

Empty Q() conditions can now produce invalid deferred filtered-index SQL.

Review effort: Balanced
Findings: 2

Open (2)

• Move changelog entry under the Unreleased Fixed section
• Document structured index migration fix in changelog

Previously missed (1)

In code that hasn't changed since last review

Preserve empty Q as an unfiltered index condition

mssql/​schema.py:592

An empty Q() is a valid Index.condition and Django treats it as no filter because it is falsy. This branch instead wraps it in a truthy IndexCondition, so deferred rendering can emit a dangling WHERE (or fail while compiling the empty predicate) instead of creating the normal unfiltered index. Preserve the original falsy-condition behavior here.

This issue also appears on line 2278 of the same file.

[Copilot]

Copilot review overview

🟡 Changes recommended

Custom conditional indexes can retain stale predicate columns after a deferred rename.

Review effort: Balanced
Findings: 1 · 2

Open (3)

• Custom conditional Index retains stale predicate after field rename · New
• Move changelog entry under the Unreleased Fixed section
• Document structured index migration fix in changelog

[Gaurav Sharma (bewithgaurav)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).