Skip to content

Commit 6953139

Browse files
robberwickrobberwick
committed
FIX: Restore Meta.indexes when altering fields (#486, #491)
1 parent 4e5e36a commit 6953139

File tree

1 file changed

+194
-46
lines changed

1 file changed

+194
-46
lines changed

‎mssql/schema.py‎

Lines changed: 194 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -406,6 +406,43 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type,
406406
old_db_params, new_db_params, strict=False):
407407
"""Actually perform a "physical" (non-ManyToMany) field update."""
408408

409+
# ============================================================================
410+
# SQL Server Index/Constraint Management During Column Alterations
411+
# ============================================================================
412+
#
413+
# OVERVIEW:
414+
# SQL Server requires explicit DROP and RESTORE of indexes/constraints when
415+
# altering column types or nullability. This method handles alterations in
416+
# four phases:
417+
#
418+
# 1. Constraint and special case handling
419+
# 2. Column alter preparation
420+
# 3. Column alteration
421+
# 4. Column alteration cleanup
422+
#
423+
#
424+
# KNOWN BUGS/LIMITATIONS:
425+
# 1. Rename + alter in same migration: When a field is renamed (via RenameField)
426+
# AND has a type or nullability change (via AlterField) in the same migration,
427+
# indexes from Meta.indexes are not restored. The _delete_indexes() method
428+
# fails with FieldDoesNotExist because it looks up the index field by the
429+
# old field name, but RenameField has already updated the model state.
430+
# Tests (marked @expectedFailure):
431+
# - test_index_from_meta_indexes_retained_after_rename_and_type_change
432+
# - test_index_from_meta_indexes_retained_after_rename_and_nullability_change
433+
#
434+
# 2. unique_together + unique=True: When a field has BOTH unique=True AND
435+
# participates in unique_together, only the single-field unique constraint
436+
# is restored after field alteration. The unique_together constraint is NOT
437+
# restored because the restoration code is in an 'else' block that only
438+
# executes when the field does NOT have unique=True.
439+
# Test (marked @expectedFailure):
440+
# - test_unique_together_retained_when_field_also_has_unique_true
441+
442+
# ============================================================================
443+
# 1. Constraint and special case handling
444+
# ============================================================================
445+
409446
# the backend doesn't support altering a column to/from AutoField as
410447
# SQL Server cannot alter columns to add and remove IDENTITY properties
411448
old_is_auto = False
@@ -569,11 +606,23 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type,
569606
if isinstance(sql, DjStatement):
570607
sql.rename_column_references(model._meta.db_table, old_field.column, new_field.column)
571608

609+
# ===============================================================================
610+
# 2. Column alter preparation
611+
# ===============================================================================
612+
572613
# Next, start accumulating actions to do
573614
actions = []
574615
null_actions = []
575616
post_actions = []
576-
# Type or comment change?
617+
618+
# Column alter preparation: type change path
619+
# Triggers when: Column type changes (or db_comment changes in Django 4.2+)
620+
# Drops: Unique constraints + all indexes containing this field
621+
#
622+
# SQL Server requires indexes/constraints to be dropped before ALTER COLUMN
623+
# can change the column's data type. This path drops both unique constraints
624+
# and all indexes that include the altered field.
625+
577626
if old_type != new_type or (django_version >= (4, 2) and
578627
self.connection.features.supports_comments
579628
and old_field.db_comment != new_field.db_comment
@@ -626,29 +675,27 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type,
626675
needs_database_default = needs_database_default and new_field.db_default is NOT_PROVIDED
627676
if needs_database_default:
628677
actions.append(self._alter_column_default_sql(model, old_field, new_field))
629-
# Nullability change?
678+
679+
# Column alter preparation: nullability change path
680+
# Triggers when: Column nullability changes (NULL ↔ NOT NULL)
681+
# Drops: Unique constraints + all indexes containing this field
682+
#
683+
# SQL Server requires indexes/constraints to be dropped before ALTER COLUMN
684+
# can change the column's NULL/NOT NULL constraint.
685+
630686
if old_field.null != new_field.null:
631687
fragment = self._alter_column_null_sql(model, old_field, new_field)
632688
if fragment:
633689
null_actions.append(fragment)
634690
# Drop unique constraint, SQL Server requires explicit deletion
635691
self._delete_unique_constraints(model, old_field, new_field, strict)
636692
# Drop indexes, SQL Server requires explicit deletion
637-
indexes_dropped = self._delete_indexes(model, old_field, new_field)
638-
auto_index_names = []
639-
for index_from_meta in model._meta.indexes:
640-
auto_index_names.append(self._create_index_name(model._meta.db_table, index_from_meta.fields))
693+
self._delete_indexes(model, old_field, new_field)
694+
695+
# ================================================================================
696+
# 3. Column alteration
697+
# ================================================================================
641698

642-
if (
643-
new_field.get_internal_type() not in ("JSONField", "TextField") and
644-
(old_field.db_index or not new_field.db_index) and
645-
new_field.db_index or
646-
((indexes_dropped and sorted(indexes_dropped) == sorted([index.name for index in model._meta.indexes])) or
647-
(indexes_dropped and sorted(indexes_dropped) == sorted(auto_index_names)))
648-
):
649-
create_index_sql_statement = self._create_index_sql(model, [new_field])
650-
if create_index_sql_statement.__str__() not in [sql.__str__() for sql in self.deferred_sql]:
651-
post_actions.append((create_index_sql_statement, ()))
652699
# Only if we have a default and there is a change from NULL to NOT NULL
653700
four_way_default_alteration = (
654701
(new_field.has_default() or (django_version >= (5,0) and new_field.db_default is not NOT_PROVIDED)) and
@@ -731,14 +778,28 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type,
731778
if (not old_field.db_index or old_field.unique) and new_field.db_index and not new_field.unique:
732779
self.execute(self._create_index_sql(model, [new_field]))
733780

781+
# ================================================================================
782+
# 4. Column alteration cleanup
783+
# ================================================================================
784+
# WHEN THIS RUNS:
785+
# - Only if type changed OR nullability changed
786+
# - Only if column was NOT renamed (rename is handled separately)
787+
# Test:
788+
# - test_index_from_meta_indexes_retained_after_rename_and_type_change (@expectedFailure)
789+
# - test_index_from_meta_indexes_retained_after_rename_and_nullability_change (@expectedFailure)
790+
#
791+
734792
# Restore indexes & unique constraints deleted above, SQL Server requires explicit restoration
735793
if (old_type != new_type or (old_field.null != new_field.null)) and (
736794
old_field.column == new_field.column # column rename is handled separately above
737795
):
738-
# Restore unique constraints
739-
# Note: if nullable they are implemented via an explicit filtered UNIQUE INDEX (not CONSTRAINT)
740-
# in order to get ANSI-compliant NULL behaviour (i.e. NULL != NULL, multiple are allowed)
741-
# Note: Don't restore primary keys, we need to re-create those seperately
796+
# --------------------------------------------------------------------------------
797+
# restore single-field unique constraints
798+
# --------------------------------------------------------------------------------
799+
# If the field had unique=True and still does, recreate the constraint.
800+
# Note: Nullable unique constraints use filtered indexes (ANSI NULL behavior).
801+
# Note: Primary keys are restored separately below.
802+
# --------------------------------------------------------------------------------
742803
if old_field.unique and new_field.unique and not new_field.primary_key:
743804
if new_field.null:
744805
self.execute(
@@ -753,6 +814,15 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type,
753814
self.execute(self._create_unique_sql(model, columns=[old_field.column]))
754815
self._delete_deferred_unique_indexes_for_field(old_field)
755816
else:
817+
# --------------------------------------------------------------------------------
818+
# Restore unique_together constraints
819+
# --------------------------------------------------------------------------------
820+
# If the field is NOT unique itself but IS part of unique_together,
821+
# restore those multi-field unique constraints as filtered indexes.
822+
# The filter ensures ANSI NULL behavior (multiple NULLs allowed).
823+
# Test: test_unique_together_retained_when_field_also_has_unique_true
824+
# https://github.com/microsoft/mssql-django/issues/494
825+
# --------------------------------------------------------------------------------
756826
if django_version >= (4, 0):
757827
for field_names in model._meta.unique_together:
758828
columns = [model._meta.get_field(field).column for field in field_names]
@@ -766,7 +836,12 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type,
766836
if old_field.column in columns:
767837
condition = ' AND '.join(["[%s] IS NOT NULL" % col for col in columns])
768838
self.execute(self._create_unique_sql(model, columns, condition=condition))
769-
# Restore primary keys
839+
840+
# --------------------------------------------------------------------------------
841+
# Primary keys
842+
# --------------------------------------------------------------------------------
843+
# If the field was and still is a primary key, recreate the PK constraint.
844+
# --------------------------------------------------------------------------------
770845
if old_field.primary_key and new_field.primary_key:
771846
self.execute(
772847
self.sql_create_pk % {
@@ -777,8 +852,14 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type,
777852
"columns": self.quote_name(new_field.column),
778853
}
779854
)
780-
# Restore unqiue_together
781-
# If we have ALTERed an AutoField or BigAutoField we need to recreate all unique_together clauses
855+
856+
# --------------------------------------------------------------------------------
857+
# AutoField/BigAutoField special handling - unique_together
858+
# --------------------------------------------------------------------------------
859+
# When altering an AutoField or BigAutoField, restore ALL unique_together
860+
# constraints across the entire model (not just those containing this field).
861+
# This is necessary due to SQL Server's IDENTITY column restrictions.
862+
# --------------------------------------------------------------------------------
782863
for t in (AutoField, BigAutoField):
783864
if isinstance(old_field, t) or isinstance(new_field, t):
784865
for field_names in model._meta.unique_together:
@@ -797,37 +878,104 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type,
797878
)
798879
break
799880

800-
# Restore indexes
801-
# If we have ALTERed an AutoField or BigAutoField we need to recreate all indexes
802-
for t in (AutoField, BigAutoField):
803-
if isinstance(old_field, t) or isinstance(new_field, t):
804-
for field in model._meta.fields:
805-
if field.db_index:
806-
self.execute(
807-
self._create_index_sql(model, [field])
808-
)
809-
break
881+
# --------------------------------------------------------------------------------
882+
# db_index, index_together, and Meta.indexes
883+
# --------------------------------------------------------------------------------
884+
# Build lists of indexes to restore, then restore them with deduplication.
885+
#
886+
# Two lists are built:
887+
# - index_columns: Field lists for db_index and index_together
888+
# - indexes_to_restore: Index objects from Meta.indexes (preserves names)
889+
#
890+
# - Deduplication: Check against deferred_sql and post_actions to prevent
891+
# double creation when both DROP paths triggered
892+
#
893+
# AutoField/BigAutoField changes are special: SQL Server requires dropping ALL
894+
# indexes on the table to change an IDENTITY column, so we must restore ALL
895+
# indexes (not just those involving the altered field).
896+
# --------------------------------------------------------------------------------
810897
index_columns = []
811-
if old_field.db_index and new_field.db_index:
898+
indexes_to_restore = []
899+
900+
# Detect if this is an AutoField/BigAutoField type change
901+
is_autofield_change = (
902+
isinstance(old_field, (AutoField, BigAutoField)) or
903+
isinstance(new_field, (AutoField, BigAutoField))
904+
)
905+
906+
# ------------------------------------------------------------------------------------
907+
# Collect db_index=True indexes
908+
# ------------------------------------------------------------------------------------
909+
if is_autofield_change:
910+
# AutoField changes drop ALL indexes - restore ALL db_index=True fields
911+
for field in model._meta.fields:
912+
if field.db_index:
913+
index_columns.append([field])
914+
elif old_field.db_index and new_field.db_index:
812915
index_columns.append([old_field])
813-
else:
814-
# Handle index_together for only django version < 5.1
815-
if django_version < (5, 1):
816-
# Get the field objects for each field name in the index_together.
817-
for fields in model._meta.index_together:
818-
# If the old field's column is among the columns for this index,
819-
# add this set of columns to index_columns for later index recreation.
820-
columns = [model._meta.get_field(field) for field in fields]
821-
if old_field.column in [c.column for c in columns]:
822-
index_columns.append(columns)
916+
917+
# --------------------------------------------------------------------------------
918+
# index_together (Django < 5.1 only)
919+
# --------------------------------------------------------------------------------
920+
# Note: index_together is deprecated and removed in Django 5.1+.
921+
# For AutoField changes, restore ALL index_together indexes.
922+
# For other changes, only restore indexes involving the altered field.
923+
# --------------------------------------------------------------------------------
924+
if django_version < (5, 1):
925+
for fields in model._meta.index_together:
926+
columns = [model._meta.get_field(field) for field in fields]
927+
if is_autofield_change or old_field.column in [c.column for c in columns]:
928+
index_columns.append(columns)
929+
930+
# --------------------------------------------------------------------------------
931+
# Execute restoration: db_index and index_together
932+
# --------------------------------------------------------------------------------
933+
# Deduplication: Skip if already in deferred_sql (Django's queue) or
934+
# post_actions (other_actions from _alter_column_type_sql).
935+
# --------------------------------------------------------------------------------
823936
if index_columns:
824937
for columns in index_columns:
825938
create_index_sql_statement = self._create_index_sql(model, columns)
826-
if (create_index_sql_statement.__str__()
827-
not in [sql.__str__() for sql in self.deferred_sql] + [statement[0].__str__() for statement in post_actions]
939+
if (str(create_index_sql_statement)
940+
not in [str(sql) for sql in self.deferred_sql] + [str(statement[0]) for statement in post_actions]
828941
):
829942
self.execute(create_index_sql_statement)
830943

944+
# --------------------------------------------------------------------------------
945+
# Collect indexes defined in Meta.indexes
946+
# --------------------------------------------------------------------------------
947+
# Collect Index objects (not just field lists) to preserve explicit names
948+
# and other index attributes when calling index.create_sql().
949+
# For AutoField changes, restore ALL Meta.indexes.
950+
# For other changes, only restore indexes involving the altered field.
951+
# --------------------------------------------------------------------------------
952+
for index in model._meta.indexes:
953+
# Get the field objects for this index
954+
index_fields = [model._meta.get_field(field_name) for field_name in index.fields]
955+
index_columns_list = [field.column for field in index_fields]
956+
957+
# Restore if: AutoField change (all indexes dropped) OR field is in this index
958+
if is_autofield_change or old_field.column in index_columns_list:
959+
indexes_to_restore.append(index) # Store the Index object, not field list
960+
961+
# --------------------------------------------------------------------------------
962+
# Execute restoration: Meta.indexes
963+
# --------------------------------------------------------------------------------
964+
# Restore Index objects using index.create_sql() to preserve explicit names
965+
# and attributes.
966+
#
967+
# Deduplication: Skip if already in deferred_sql or post_actions
968+
# (which contains other_actions from _alter_column_type_sql).
969+
# This prevents duplicate index creation if the same index
970+
# was already scheduled elsewhere.
971+
# --------------------------------------------------------------------------------
972+
for index in indexes_to_restore:
973+
create_index_sql_statement = index.create_sql(model, self)
974+
if create_index_sql_statement and (str(create_index_sql_statement)
975+
not in [str(sql) for sql in self.deferred_sql] + [str(statement[0]) for statement in post_actions]
976+
):
977+
self.execute(create_index_sql_statement)
978+
831979
# Type alteration on primary key? Then we need to alter the column
832980
# referring to us.
833981
rels_to_update = []

0 commit comments

Comments
 (0)