From 91f2d72945724370f99c8c758baeb45773c64780 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Thu, 27 Jul 2023 11:44:25 +0100 Subject: [PATCH 1/7] Add a section on removing nullable fields from models --- dev/README.md | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/dev/README.md b/dev/README.md index fd33bc4798..96dbf1d66f 100644 --- a/dev/README.md +++ b/dev/README.md @@ -427,3 +427,69 @@ backwards compatible See [django-migration-linter checklist](https://github.com/3YOURMIND/django-migration-linter/blob/main/docs/incompatibilities.md) for the common mistakes and best practices + +### Removing a nullable field from a model + +> This only works for nullable fields (fields with `null=True` in the field definition). +> +> DO NOT USE THIS APPROACH FOR NON-NULLABLE FIELDS, IT WILL BREAK THINGS! + +1. Remove all usages of the field you want to remove. Make sure the field is not used anywhere, including filtering, +querying, or explicit field referencing from views, models, forms, serializers, etc. +2. Remove the field from the model definition. +3. Generate migrations using `makemigrations` management command. +Django should create a new migration with a `RemoveField` operation in it. +4. Make it so that the generated migration does not actually remove the field from the database, but makes Django think +that it has been removed. To do this, manually edit the generated migration file to use `SeparateDatabaseAndState`: + + ```python + operations = [ + migrations.SeparateDatabaseAndState( + state_operations=[ + # Put the RemoveField operation generated by Django here + ], + database_operations=[], + ), + ] + ``` + + You can use + [this migration](https://github.com/grafana/oncall/blob/ad0ed6714ac9993582e78b50d855aaf69df17619/engine/apps/alerts/migrations/0027_remove_alertreceivechannel_restricted_at_from_state.py#L10-L20) + as an example. + +5. Release the changes (removal of the field & migration). Once released, Django will not be aware of this field +anymore, but the field will be still present in the database. This allows for a gradual migration, where the field +is no longer used in new code, but still exists in the database for backward compatibility with old code. +6. In any subsequent release, include a new migration that actually deletes the field: + + ```python + operations = [ + migrations.RunSQL( + sql="ALTER TABLE DROP COLUMN ", + reverse_sql="ALTER TABLE ADD COLUMN NULL", + ) + ] + ``` + + You can use + [this migration](https://github.com/grafana/oncall/commit/e15d18a6049f4b50ffcac30c604f8d7677ca5a2b#diff-466e204c4c967410172ba43a425e01cdff05a74fa1dc7447ec7aa4afee4dbc1bR13-R19) + as an example. + + To generate the `sql` statement automatically, find the migration that added the field and use the + [sqlmigrate](https://docs.djangoproject.com/en/4.2/ref/django-admin/#sqlmigrate) management command on it. + For the example above, the command would be: + + ```bash + pyhon manage.py sqlmigrate alerts 0014 # Field "restricted_at" was added in migration alerts.0014 + # Output: ALTER TABLE `alerts_alertreceivechannel` ADD COLUMN `restricted_at` datetime(6) NULL + ``` + + To generate the `reverse_sql` statement, use the same command, but with the `--backwards` flag: + + ```bash + pyhon manage.py sqlmigrate alerts 0014 --backwards + # Output: ALTER TABLE `alerts_alertreceivechannel` DROP COLUMN `restricted_at` + ``` + +7. After releasing and deploying the migration, the field will be removed both from the database and Django state, +without backward compatibility issues or downtime 🎉 From f7b7b776434875d84b446ba80d1386cb64c07413 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Thu, 27 Jul 2023 11:54:57 +0100 Subject: [PATCH 2/7] wording --- dev/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev/README.md b/dev/README.md index 96dbf1d66f..b245c33dbe 100644 --- a/dev/README.md +++ b/dev/README.md @@ -457,7 +457,7 @@ that it has been removed. To do this, manually edit the generated migration file [this migration](https://github.com/grafana/oncall/blob/ad0ed6714ac9993582e78b50d855aaf69df17619/engine/apps/alerts/migrations/0027_remove_alertreceivechannel_restricted_at_from_state.py#L10-L20) as an example. -5. Release the changes (removal of the field & migration). Once released, Django will not be aware of this field +5. Release the changes (removal of the field & migration). Once released and deployed, Django will not be aware of this field anymore, but the field will be still present in the database. This allows for a gradual migration, where the field is no longer used in new code, but still exists in the database for backward compatibility with old code. 6. In any subsequent release, include a new migration that actually deletes the field: From c1b685502d47e684bd333412a7cf017e3c19cbd2 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Thu, 27 Jul 2023 12:16:18 +0100 Subject: [PATCH 3/7] will -> can --- dev/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev/README.md b/dev/README.md index b245c33dbe..62b9f5ab5a 100644 --- a/dev/README.md +++ b/dev/README.md @@ -432,7 +432,7 @@ for the common mistakes and best practices > This only works for nullable fields (fields with `null=True` in the field definition). > -> DO NOT USE THIS APPROACH FOR NON-NULLABLE FIELDS, IT WILL BREAK THINGS! +> DO NOT USE THIS APPROACH FOR NON-NULLABLE FIELDS, IT CAN BREAK THINGS! 1. Remove all usages of the field you want to remove. Make sure the field is not used anywhere, including filtering, querying, or explicit field referencing from views, models, forms, serializers, etc. From ed0cb64434a4e5184717b25cc65ac11b8fd44833 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Thu, 27 Jul 2023 12:32:21 +0100 Subject: [PATCH 4/7] sql -> reverse_sql --- dev/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dev/README.md b/dev/README.md index 62b9f5ab5a..cd9f48b2b6 100644 --- a/dev/README.md +++ b/dev/README.md @@ -475,7 +475,7 @@ is no longer used in new code, but still exists in the database for backward com [this migration](https://github.com/grafana/oncall/commit/e15d18a6049f4b50ffcac30c604f8d7677ca5a2b#diff-466e204c4c967410172ba43a425e01cdff05a74fa1dc7447ec7aa4afee4dbc1bR13-R19) as an example. - To generate the `sql` statement automatically, find the migration that added the field and use the + To generate the `reverse_sql` statement automatically, find the migration that added the field and use the [sqlmigrate](https://docs.djangoproject.com/en/4.2/ref/django-admin/#sqlmigrate) management command on it. For the example above, the command would be: @@ -484,7 +484,7 @@ is no longer used in new code, but still exists in the database for backward com # Output: ALTER TABLE `alerts_alertreceivechannel` ADD COLUMN `restricted_at` datetime(6) NULL ``` - To generate the `reverse_sql` statement, use the same command, but with the `--backwards` flag: + To generate the `sql` statement, use the same command, but with the `--backwards` flag: ```bash pyhon manage.py sqlmigrate alerts 0014 --backwards From 93498dbac76691ee2cc8f754a20c6a01bfa470bf Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Mon, 7 Aug 2023 21:23:23 +0100 Subject: [PATCH 5/7] add custom operations and management command --- engine/common/migrations/__init__.py | 0 engine/common/migrations/remove_field.py | 79 +++++++++++++++++++ .../management/commands/remove_field.py | 64 +++++++++++++++ 3 files changed, 143 insertions(+) create mode 100644 engine/common/migrations/__init__.py create mode 100644 engine/common/migrations/remove_field.py create mode 100644 engine/engine/management/commands/remove_field.py diff --git a/engine/common/migrations/__init__.py b/engine/common/migrations/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/engine/common/migrations/remove_field.py b/engine/common/migrations/remove_field.py new file mode 100644 index 0000000000..a079bb54a6 --- /dev/null +++ b/engine/common/migrations/remove_field.py @@ -0,0 +1,79 @@ +from django.db import connection +from django.db.migrations import RemoveField +from django.db.migrations.loader import MigrationLoader + + +class RemoveFieldState(RemoveField): + """ + Remove field from Django's migration state, but not from the database. + This is essentially the same as RemoveField, but database_forwards and database_backwards methods are modified + to do nothing. + """ + + def database_forwards(self, app_label, schema_editor, from_state, to_state): + pass + + def database_backwards(self, app_label, schema_editor, from_state, to_state): + pass + + def describe(self): + return f"{super().describe()} (state)" + + @property + def migration_name_fragment(self): + return f"{super().migration_name_fragment}_state" + + +class RemoveFieldDB(RemoveField): + """ + Remove field from the database, but not from Django's migration state. + This is implemented as a custom operation, because Django's RemoveField operation does not support + removing fields from the database after it has been removed from the state. The workaround is to use the state + that was in effect before the field was removed from the state (i.e. just before the RemoveFieldState migration). + """ + + def __init__(self, model_name, name, remove_state_migration): + """ + Specifying "remove_state_migration" allows database operations to run against a particular historical state. + Example: remove_state_migration = ("alerts", "0014_alertreceivechannel_restricted_at") will "trick" Django + into thinking that the last applied migration in the "alerts" app is 0013. + """ + super().__init__(model_name, name) + self.remove_state_migration = remove_state_migration + + def deconstruct(self): + """Update serialized representation of the operation.""" + deconstructed = super().deconstruct() + return ( + deconstructed[0], + deconstructed[1], + deconstructed[2] | {"remove_state_migration": self.remove_state_migration} + ) + + def state_forwards(self, app_label, state): + """Skip any state changes.""" + pass + + def database_forwards(self, app_label, schema_editor, from_state, to_state): + # use historical state instead of what Django provides + from_state = self.state_before_remove_state_migration + + super().database_forwards(app_label, schema_editor, from_state, to_state) + + def database_backwards(self, app_label, schema_editor, from_state, to_state): + # use historical state instead of what Django provides + to_state = self.state_before_remove_state_migration + + super().database_backwards(app_label, schema_editor, from_state, to_state) + + def describe(self): + return f"{super().describe()} (db)" + + @property + def migration_name_fragment(self): + return f"{super().migration_name_fragment}_db" + + @property + def state_before_remove_state_migration(self): + """Get project state just before migration "remove_state_migration" was applied.""" + return MigrationLoader(connection).project_state(self.remove_state_migration, at_end=False) diff --git a/engine/engine/management/commands/remove_field.py b/engine/engine/management/commands/remove_field.py new file mode 100644 index 0000000000..fd7e2ef624 --- /dev/null +++ b/engine/engine/management/commands/remove_field.py @@ -0,0 +1,64 @@ +from django.core.management import BaseCommand +from django.db import connection +from django.db.migrations import Migration +from django.db.migrations.autodetector import MigrationAutodetector +from django.db.migrations.loader import MigrationLoader +from django.db.migrations.writer import MigrationWriter + +from common.migrations.remove_field import RemoveFieldDB, RemoveFieldState + + +class Command(BaseCommand): + """ + Generate two migrations that remove a field from the state and the database separately. + This allows removing a field in 2 separate releases and avoid downtime. + """ + + def add_arguments(self, parser): + parser.add_argument( + "args", nargs=3, help="app_label model_name field_name, example: alerts AlertReceiveChannel restricted_at" + ) + + def handle(self, *args, **options): + app_label, model_name, field_name = args + + # Check that the app, the model, and the field to be removed exist + project_state = MigrationLoader(connection).project_state() + model_state = project_state.apps.get_model(app_label, model_name) + model_state._meta.get_field(field_name) + + # Write migration that removes the field from the state + remove_state_migration = self.write_operation( + app_label, RemoveFieldState(model_name=model_name, name=field_name), project_state + ) + + # Write migration that removes the field from the database + self.write_operation( + app_label, + RemoveFieldDB( + model_name=model_name, name=field_name, remove_state_migration=(app_label, remove_state_migration.name) + ), + project_state, + ) + + @staticmethod + def write_operation(app_label, operation, project_state): + """ + Some Django magic to write a single-operation migration to a file, so it's similar to what Django would generate + when running the "makemigrations" command. + """ + + migration_class = type("Migration", (Migration,), {"operations": [operation]}) + + changes = MigrationAutodetector(project_state, project_state).arrange_for_graph( + changes={app_label: [migration_class(None, app_label)]}, + graph=MigrationLoader(connection).graph, + migration_name=operation.migration_name_fragment, + ) + + migration = changes[app_label][0] + writer = MigrationWriter(migration) + with open(writer.path, "w", encoding="utf-8") as file: + file.write(writer.as_string()) + + return migration From 8879839af93aa2e852445bc2c720be01b7bb96c0 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Mon, 7 Aug 2023 21:55:58 +0100 Subject: [PATCH 6/7] update instruction + assert that field is nullable --- dev/README.md | 61 ++++--------------- .../management/commands/remove_field.py | 5 +- 2 files changed, 16 insertions(+), 50 deletions(-) diff --git a/dev/README.md b/dev/README.md index 7ba81a3df5..a800b8c38c 100644 --- a/dev/README.md +++ b/dev/README.md @@ -437,59 +437,22 @@ for the common mistakes and best practices 1. Remove all usages of the field you want to remove. Make sure the field is not used anywhere, including filtering, querying, or explicit field referencing from views, models, forms, serializers, etc. 2. Remove the field from the model definition. -3. Generate migrations using `makemigrations` management command. -Django should create a new migration with a `RemoveField` operation in it. -4. Make it so that the generated migration does not actually remove the field from the database, but makes Django think -that it has been removed. To do this, manually edit the generated migration file to use `SeparateDatabaseAndState`: +3. Generate migrations using the following management command: ```python - operations = [ - migrations.SeparateDatabaseAndState( - state_operations=[ - # Put the RemoveField operation generated by Django here - ], - database_operations=[], - ), - ] + python manage.py remove_field ``` - You can use - [this migration](https://github.com/grafana/oncall/blob/ad0ed6714ac9993582e78b50d855aaf69df17619/engine/apps/alerts/migrations/0027_remove_alertreceivechannel_restricted_at_from_state.py#L10-L20) - as an example. + Example: `python manage.py remove_field alerts AlertReceiveChannel restricted_at` -5. Release the changes (removal of the field & migration). Once released and deployed, Django will not be aware of this field -anymore, but the field will be still present in the database. This allows for a gradual migration, where the field -is no longer used in new code, but still exists in the database for backward compatibility with old code. -6. In any subsequent release, include a new migration that actually deletes the field: + This command will generate two migrations that **MUST BE DEPLOYED IN TWO SEPARATE RELEASES**: + - Migration #1 will remove the field from Django's state, but not from the database. Release #1 must include + migration #1, and must not include migration #2. + - Migration #2 will remove the field from the database. Stash this migration for use in a future release. - ```python - operations = [ - migrations.RunSQL( - sql="ALTER TABLE DROP COLUMN ", - reverse_sql="ALTER TABLE ADD COLUMN NULL", - ) - ] - ``` - - You can use - [this migration](https://github.com/grafana/oncall/commit/e15d18a6049f4b50ffcac30c604f8d7677ca5a2b#diff-466e204c4c967410172ba43a425e01cdff05a74fa1dc7447ec7aa4afee4dbc1bR13-R19) - as an example. - - To generate the `reverse_sql` statement automatically, find the migration that added the field and use the - [sqlmigrate](https://docs.djangoproject.com/en/4.2/ref/django-admin/#sqlmigrate) management command on it. - For the example above, the command would be: - - ```bash - pyhon manage.py sqlmigrate alerts 0014 # Field "restricted_at" was added in migration alerts.0014 - # Output: ALTER TABLE `alerts_alertreceivechannel` ADD COLUMN `restricted_at` datetime(6) NULL - ``` - - To generate the `sql` statement, use the same command, but with the `--backwards` flag: - - ```bash - pyhon manage.py sqlmigrate alerts 0014 --backwards - # Output: ALTER TABLE `alerts_alertreceivechannel` DROP COLUMN `restricted_at` - ``` - -7. After releasing and deploying the migration, the field will be removed both from the database and Django state, +4. Make release #1 (removal of the field + migration #1). Once released and deployed, Django will not be +aware of this field anymore, but the field will be still present in the database. This allows for a gradual migration, +where the field is no longer used in new code, but still exists in the database for backward compatibility with old code. +5. In any subsequent release, include migration #2 (the one that removes the field from the database). +6. After releasing and deploying migration #2, the field will be removed both from the database and Django state, without backward compatibility issues or downtime 🎉 diff --git a/engine/engine/management/commands/remove_field.py b/engine/engine/management/commands/remove_field.py index fd7e2ef624..efb6aef3fc 100644 --- a/engine/engine/management/commands/remove_field.py +++ b/engine/engine/management/commands/remove_field.py @@ -25,7 +25,10 @@ def handle(self, *args, **options): # Check that the app, the model, and the field to be removed exist project_state = MigrationLoader(connection).project_state() model_state = project_state.apps.get_model(app_label, model_name) - model_state._meta.get_field(field_name) + field = model_state._meta.get_field(field_name) + + # This command only works for nullable fields for now + assert field.null is True, "Field must be nullable" # Write migration that removes the field from the state remove_state_migration = self.write_operation( From 560effddc87c0f7b24c8869d215d1e8e066f864e Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Mon, 7 Aug 2023 22:14:19 +0100 Subject: [PATCH 7/7] remove assert --- engine/engine/management/commands/remove_field.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/engine/engine/management/commands/remove_field.py b/engine/engine/management/commands/remove_field.py index efb6aef3fc..fd7e2ef624 100644 --- a/engine/engine/management/commands/remove_field.py +++ b/engine/engine/management/commands/remove_field.py @@ -25,10 +25,7 @@ def handle(self, *args, **options): # Check that the app, the model, and the field to be removed exist project_state = MigrationLoader(connection).project_state() model_state = project_state.apps.get_model(app_label, model_name) - field = model_state._meta.get_field(field_name) - - # This command only works for nullable fields for now - assert field.null is True, "Field must be nullable" + model_state._meta.get_field(field_name) # Write migration that removes the field from the state remove_state_migration = self.write_operation(