From ae0899be0d787fbfc5f5ab2b18c5a8219d822d2b Mon Sep 17 00:00:00 2001 From: Mariusz Felisiak Date: Thu, 22 Dec 2022 07:12:17 +0100 Subject: [PATCH] Fixed #34219 -- Preserved Char/TextField.db_collation when altering column type. This moves setting a database collation to the column type alteration as both must be set at the same time. This should also avoid another layer of the column type alteration when adding database comments support (#18468). --- .../contrib/gis/db/backends/postgis/schema.py | 9 ++- django/db/backends/base/schema.py | 72 ++++++++----------- django/db/backends/mysql/schema.py | 11 +-- django/db/backends/oracle/schema.py | 23 +++--- django/db/backends/postgresql/schema.py | 17 +++-- tests/schema/tests.py | 32 +++++++++ 6 files changed, 97 insertions(+), 67 deletions(-) diff --git a/django/contrib/gis/db/backends/postgis/schema.py b/django/contrib/gis/db/backends/postgis/schema.py index 77a9096ef4..5464c85cf6 100644 --- a/django/contrib/gis/db/backends/postgis/schema.py +++ b/django/contrib/gis/db/backends/postgis/schema.py @@ -50,12 +50,16 @@ class PostGISSchemaEditor(DatabaseSchemaEditor): expressions=expressions, ) - def _alter_column_type_sql(self, table, old_field, new_field, new_type): + def _alter_column_type_sql( + self, table, old_field, new_field, new_type, old_collation, new_collation + ): """ Special case when dimension changed. """ if not hasattr(old_field, "dim") or not hasattr(new_field, "dim"): - return super()._alter_column_type_sql(table, old_field, new_field, new_type) + return super()._alter_column_type_sql( + table, old_field, new_field, new_type, old_collation, new_collation + ) if old_field.dim == 2 and new_field.dim == 3: sql_alter = self.sql_alter_column_to_3d @@ -69,6 +73,7 @@ class PostGISSchemaEditor(DatabaseSchemaEditor): % { "column": self.quote_name(new_field.column), "type": new_type, + "collation": "", }, [], ), diff --git a/django/db/backends/base/schema.py b/django/db/backends/base/schema.py index fe31967ce2..6697848191 100644 --- a/django/db/backends/base/schema.py +++ b/django/db/backends/base/schema.py @@ -87,13 +87,12 @@ class BaseDatabaseSchemaEditor: sql_create_column = "ALTER TABLE %(table)s ADD COLUMN %(column)s %(definition)s" sql_alter_column = "ALTER TABLE %(table)s %(changes)s" - sql_alter_column_type = "ALTER COLUMN %(column)s TYPE %(type)s" + sql_alter_column_type = "ALTER COLUMN %(column)s TYPE %(type)s%(collation)s" sql_alter_column_null = "ALTER COLUMN %(column)s DROP NOT NULL" sql_alter_column_not_null = "ALTER COLUMN %(column)s SET NOT NULL" sql_alter_column_default = "ALTER COLUMN %(column)s SET DEFAULT %(default)s" sql_alter_column_no_default = "ALTER COLUMN %(column)s DROP DEFAULT" sql_alter_column_no_default_null = sql_alter_column_no_default - sql_alter_column_collate = "ALTER COLUMN %(column)s TYPE %(type)s%(collation)s" sql_delete_column = "ALTER TABLE %(table)s DROP COLUMN %(column)s CASCADE" sql_rename_column = ( "ALTER TABLE %(table)s RENAME COLUMN %(old_column)s TO %(new_column)s" @@ -950,17 +949,14 @@ class BaseDatabaseSchemaEditor: # Type suffix change? (e.g. auto increment). old_type_suffix = old_field.db_type_suffix(connection=self.connection) new_type_suffix = new_field.db_type_suffix(connection=self.connection) - # Collation change? - if old_collation != new_collation: - # Collation change handles also a type change. - fragment = self._alter_column_collation_sql( - model, new_field, new_type, new_collation, old_field - ) - actions.append(fragment) - # Type change? - elif (old_type, old_type_suffix) != (new_type, new_type_suffix): + # Type or collation change? + if ( + old_type != new_type + or old_type_suffix != new_type_suffix + or old_collation != new_collation + ): fragment, other_actions = self._alter_column_type_sql( - model, old_field, new_field, new_type + model, old_field, new_field, new_type, old_collation, new_collation ) actions.append(fragment) post_actions.extend(other_actions) @@ -1076,20 +1072,14 @@ class BaseDatabaseSchemaEditor: rel_collation = rel_db_params.get("collation") old_rel_db_params = old_rel.field.db_parameters(connection=self.connection) old_rel_collation = old_rel_db_params.get("collation") - if old_rel_collation != rel_collation: - # Collation change handles also a type change. - fragment = self._alter_column_collation_sql( - new_rel.related_model, - new_rel.field, - rel_type, - rel_collation, - old_rel.field, - ) - other_actions = [] - else: - fragment, other_actions = self._alter_column_type_sql( - new_rel.related_model, old_rel.field, new_rel.field, rel_type - ) + fragment, other_actions = self._alter_column_type_sql( + new_rel.related_model, + old_rel.field, + new_rel.field, + rel_type, + old_rel_collation, + rel_collation, + ) self.execute( self.sql_alter_column % { @@ -1209,7 +1199,9 @@ class BaseDatabaseSchemaEditor: params, ) - def _alter_column_type_sql(self, model, old_field, new_field, new_type): + def _alter_column_type_sql( + self, model, old_field, new_field, new_type, old_collation, new_collation + ): """ Hook to specialize column type alteration for different backends, for cases when a creation type is different to an alteration type @@ -1219,33 +1211,25 @@ class BaseDatabaseSchemaEditor: an ALTER TABLE statement and a list of extra (sql, params) tuples to run once the field is altered. """ + if collate_sql := self._collate_sql( + new_collation, old_collation, model._meta.db_table + ): + collate_sql = f" {collate_sql}" + else: + collate_sql = "" return ( ( self.sql_alter_column_type % { "column": self.quote_name(new_field.column), "type": new_type, + "collation": collate_sql, }, [], ), [], ) - def _alter_column_collation_sql( - self, model, new_field, new_type, new_collation, old_field - ): - return ( - self.sql_alter_column_collate - % { - "column": self.quote_name(new_field.column), - "type": new_type, - "collation": " " + self._collate_sql(new_collation) - if new_collation - else "", - }, - [], - ) - def _alter_many_to_many(self, model, old_field, new_field, strict): """Alter M2Ms to repoint their to= endpoints.""" # Rename the through table @@ -1745,8 +1729,8 @@ class BaseDatabaseSchemaEditor: def _delete_primary_key_sql(self, model, name): return self._delete_constraint_sql(self.sql_delete_pk, model, name) - def _collate_sql(self, collation): - return "COLLATE " + self.quote_name(collation) + def _collate_sql(self, collation, old_collation=None, table_name=None): + return "COLLATE " + self.quote_name(collation) if collation else "" def remove_procedure(self, procedure_name, param_types=()): sql = self.sql_delete_procedure % { diff --git a/django/db/backends/mysql/schema.py b/django/db/backends/mysql/schema.py index 821f4ddbce..6be755f8fb 100644 --- a/django/db/backends/mysql/schema.py +++ b/django/db/backends/mysql/schema.py @@ -9,8 +9,7 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): sql_alter_column_null = "MODIFY %(column)s %(type)s NULL" sql_alter_column_not_null = "MODIFY %(column)s %(type)s NOT NULL" - sql_alter_column_type = "MODIFY %(column)s %(type)s" - sql_alter_column_collate = "MODIFY %(column)s %(type)s%(collation)s" + sql_alter_column_type = "MODIFY %(column)s %(type)s%(collation)s" sql_alter_column_no_default_null = "ALTER COLUMN %(column)s SET DEFAULT NULL" # No 'CASCADE' which works as a no-op in MySQL but is undocumented @@ -218,9 +217,13 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): new_type += " NOT NULL" return new_type - def _alter_column_type_sql(self, model, old_field, new_field, new_type): + def _alter_column_type_sql( + self, model, old_field, new_field, new_type, old_collation, new_collation + ): new_type = self._set_field_new_type_null_status(old_field, new_type) - return super()._alter_column_type_sql(model, old_field, new_field, new_type) + return super()._alter_column_type_sql( + model, old_field, new_field, new_type, old_collation, new_collation + ) def _rename_field_sql(self, table, old_field, new_field, new_type): new_type = self._set_field_new_type_null_status(old_field, new_type) diff --git a/django/db/backends/oracle/schema.py b/django/db/backends/oracle/schema.py index 1bf12293e8..ec5c9f4142 100644 --- a/django/db/backends/oracle/schema.py +++ b/django/db/backends/oracle/schema.py @@ -13,13 +13,12 @@ from django.utils.duration import duration_iso_string class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): sql_create_column = "ALTER TABLE %(table)s ADD %(column)s %(definition)s" - sql_alter_column_type = "MODIFY %(column)s %(type)s" + sql_alter_column_type = "MODIFY %(column)s %(type)s%(collation)s" sql_alter_column_null = "MODIFY %(column)s NULL" sql_alter_column_not_null = "MODIFY %(column)s NOT NULL" sql_alter_column_default = "MODIFY %(column)s DEFAULT %(default)s" sql_alter_column_no_default = "MODIFY %(column)s DEFAULT NULL" sql_alter_column_no_default_null = sql_alter_column_no_default - sql_alter_column_collate = "MODIFY %(column)s %(type)s%(collation)s" sql_delete_column = "ALTER TABLE %(table)s DROP COLUMN %(column)s" sql_create_column_inline_fk = ( @@ -169,7 +168,9 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): self._create_fk_sql(rel.related_model, rel.field, "_fk") ) - def _alter_column_type_sql(self, model, old_field, new_field, new_type): + def _alter_column_type_sql( + self, model, old_field, new_field, new_type, old_collation, new_collation + ): auto_field_types = {"AutoField", "BigAutoField", "SmallAutoField"} # Drop the identity if migrating away from AutoField. if ( @@ -178,7 +179,9 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): and self._is_identity_column(model._meta.db_table, new_field.column) ): self._drop_identity(model._meta.db_table, new_field.column) - return super()._alter_column_type_sql(model, old_field, new_field, new_type) + return super()._alter_column_type_sql( + model, old_field, new_field, new_type, old_collation, new_collation + ) def normalize_name(self, name): """ @@ -242,11 +245,7 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): ) return cursor.fetchone()[0] - def _alter_column_collation_sql( - self, model, new_field, new_type, new_collation, old_field - ): - if new_collation is None: - new_collation = self._get_default_collation(model._meta.db_table) - return super()._alter_column_collation_sql( - model, new_field, new_type, new_collation, old_field - ) + def _collate_sql(self, collation, old_collation=None, table_name=None): + if collation is None and old_collation is not None: + collation = self._get_default_collation(table_name) + return super()._collate_sql(collation, old_collation, table_name) diff --git a/django/db/backends/postgresql/schema.py b/django/db/backends/postgresql/schema.py index 1bd72bc0cb..2887071254 100644 --- a/django/db/backends/postgresql/schema.py +++ b/django/db/backends/postgresql/schema.py @@ -140,7 +140,9 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): return sequence["name"] return None - def _alter_column_type_sql(self, model, old_field, new_field, new_type): + def _alter_column_type_sql( + self, model, old_field, new_field, new_type, old_collation, new_collation + ): # Drop indexes on varchar/text/citext columns that are changing to a # different type. old_db_params = old_field.db_parameters(connection=self.connection) @@ -155,7 +157,9 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): ) self.execute(self._delete_index_sql(model, index_name)) - self.sql_alter_column_type = "ALTER COLUMN %(column)s TYPE %(type)s" + self.sql_alter_column_type = ( + "ALTER COLUMN %(column)s TYPE %(type)s%(collation)s" + ) # Cast when data type changed. if using_sql := self._using_sql(new_field, old_field): self.sql_alter_column_type += using_sql @@ -178,6 +182,7 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): % { "column": self.quote_name(column), "type": new_type, + "collation": "", }, [], ), @@ -204,7 +209,7 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): ) column = strip_quotes(new_field.column) fragment, _ = super()._alter_column_type_sql( - model, old_field, new_field, new_type + model, old_field, new_field, new_type, old_collation, new_collation ) # Drop the sequence if exists (Django 4.1+ identity columns don't # have it). @@ -222,7 +227,7 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): return fragment, other_actions elif new_is_auto and old_is_auto and old_internal_type != new_internal_type: fragment, _ = super()._alter_column_type_sql( - model, old_field, new_field, new_type + model, old_field, new_field, new_type, old_collation, new_collation ) column = strip_quotes(new_field.column) db_types = { @@ -246,7 +251,9 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): ] return fragment, other_actions else: - return super()._alter_column_type_sql(model, old_field, new_field, new_type) + return super()._alter_column_type_sql( + model, old_field, new_field, new_type, old_collation, new_collation + ) def _alter_column_collation_sql( self, model, new_field, new_type, new_collation, old_field diff --git a/tests/schema/tests.py b/tests/schema/tests.py index fee5ca9d2b..9b2b512c1b 100644 --- a/tests/schema/tests.py +++ b/tests/schema/tests.py @@ -4950,6 +4950,38 @@ class SchemaTests(TransactionTestCase): editor.alter_field(Author, new_field, old_field, strict=True) self.assertIsNone(self.get_column_collation(Author._meta.db_table, "name")) + @skipUnlessDBFeature("supports_collation_on_charfield") + def test_alter_field_type_preserve_db_collation(self): + collation = connection.features.test_collations.get("non_default") + if not collation: + self.skipTest("Language collations are not supported.") + + with connection.schema_editor() as editor: + editor.create_model(Author) + + old_field = Author._meta.get_field("name") + new_field = CharField(max_length=255, db_collation=collation) + new_field.set_attributes_from_name("name") + new_field.model = Author + with connection.schema_editor() as editor: + editor.alter_field(Author, old_field, new_field, strict=True) + self.assertEqual( + self.get_column_collation(Author._meta.db_table, "name"), + collation, + ) + # Changing a field type should preserve the collation. + old_field = new_field + new_field = CharField(max_length=511, db_collation=collation) + new_field.set_attributes_from_name("name") + new_field.model = Author + with connection.schema_editor() as editor: + editor.alter_field(Author, new_field, old_field, strict=True) + # Collation is preserved. + self.assertEqual( + self.get_column_collation(Author._meta.db_table, "name"), + collation, + ) + @skipUnlessDBFeature("supports_collation_on_charfield") def test_alter_primary_key_db_collation(self): collation = connection.features.test_collations.get("non_default")