From f4eb8e9d4c59c41b869cca492c54abffa8913eea Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Wed, 20 Nov 2024 11:15:37 -0800 Subject: [PATCH 1/2] feat(migrations): Add in `SafeDeleteModel` migration operation This adds in the `SafeDeleteModel` operation. Most of this pr is testing and monkeypatching Django migrations to be able to hook this in. `SafeDeleteModel` allows us to delete models in two steps without using custom sql. If accepts a deletion_action parameter, which must be passed, and can be either MOVE_TO_PENDING or DELETE. MOVE_TO_PENDING checks that there are no constraints on the table and removes the table from the migation state. We keep track of pending models separately, so that they can be deleted in the next step. DELETE runs the delete command on the database. It is able to still reference the model since we keep track of this in our patched state. If delete is run before MOVE_TO_PENDING then the migration will be rejected. Some care is still needed when using these - we still have to ensure that the PENDING migration runs and fully deploys before the DELETE does. But other than that this reduces the manual error prone sql work around this process a lot. --- .../__init__.py | 0 .../migrations/0001_initial.py | 26 ++++++ .../migrations/0002_delete_pending.py | 17 ++++ .../migrations/0003_double_pending.py | 17 ++++ .../migrations/__init__.py | 0 .../models.py | 5 ++ .../__init__.py | 0 .../migrations/0001_initial.py | 26 ++++++ .../migrations/0002_delete_without_pending.py | 17 ++++ .../migrations/__init__.py | 0 .../models.py | 5 ++ .../__init__.py | 0 .../migrations/0001_initial.py | 45 +++++++++++ .../migrations/0002_delete_without_pending.py | 18 +++++ .../migrations/__init__.py | 0 .../models.py | 12 +++ .../__init__.py | 0 .../migrations/0001_initial.py | 46 +++++++++++ .../0002_remove_constraints_and_pending.py | 32 ++++++++ .../migrations/0003_delete.py | 16 ++++ .../migrations/__init__.py | 0 .../models.py | 12 +++ .../good_flow_delete_simple_app/__init__.py | 0 .../migrations/0001_initial.py | 22 +++++ .../migrations/0002_set_pending.py | 16 ++++ .../migrations/0003_delete.py | 16 ++++ .../migrations/__init__.py | 0 .../good_flow_delete_simple_app/models.py | 5 ++ src/sentry/db/postgres/schema.py | 12 +-- src/sentry/new_migrations/monkey/__init__.py | 12 +++ src/sentry/new_migrations/monkey/models.py | 47 +++++++++++ src/sentry/new_migrations/monkey/state.py | 49 ++++++++++++ .../integration/test_migrations.py | 80 ++++++++++++++++++- 33 files changed, 544 insertions(+), 9 deletions(-) create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/__init__.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/migrations/0001_initial.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/migrations/0002_delete_pending.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/migrations/0003_double_pending.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/migrations/__init__.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/models.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_model_without_pending_app/__init__.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_model_without_pending_app/migrations/0001_initial.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_model_without_pending_app/migrations/0002_delete_without_pending.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_model_without_pending_app/migrations/__init__.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_model_without_pending_app/models.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_pending_with_fk_constraints_app/__init__.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_pending_with_fk_constraints_app/migrations/0001_initial.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_pending_with_fk_constraints_app/migrations/0002_delete_without_pending.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_pending_with_fk_constraints_app/migrations/__init__.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_pending_with_fk_constraints_app/models.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/__init__.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/migrations/0001_initial.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/migrations/0002_remove_constraints_and_pending.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/migrations/0003_delete.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/migrations/__init__.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/models.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_simple_app/__init__.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_simple_app/migrations/0001_initial.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_simple_app/migrations/0002_set_pending.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_simple_app/migrations/0003_delete.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_simple_app/migrations/__init__.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_simple_app/models.py create mode 100644 src/sentry/new_migrations/monkey/models.py create mode 100644 src/sentry/new_migrations/monkey/state.py diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/__init__.py b/fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/migrations/0001_initial.py b/fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/migrations/0001_initial.py new file mode 100644 index 00000000000000..69de29635772a4 --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/migrations/0001_initial.py @@ -0,0 +1,26 @@ +# Generated by Django 3.1 on 2019-09-22 21:47 + +from django.db import migrations, models + +from sentry.new_migrations.migrations import CheckedMigration + + +class Migration(CheckedMigration): + + initial = True + + dependencies = [] + + operations = [ + migrations.CreateModel( + name="TestTable", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name="ID" + ), + ), + ], + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/migrations/0002_delete_pending.py b/fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/migrations/0002_delete_pending.py new file mode 100644 index 00000000000000..917b83001e8a8d --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/migrations/0002_delete_pending.py @@ -0,0 +1,17 @@ +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.models import SafeDeleteModel +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + + dependencies = [ + ("bad_flow_delete_model_double_pending_app", "0001_initial"), + ] + + operations = [ + SafeDeleteModel( + name="TestTable", + deletion_action=DeletionAction.MOVE_TO_PENDING, + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/migrations/0003_double_pending.py b/fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/migrations/0003_double_pending.py new file mode 100644 index 00000000000000..e52a64272a2020 --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/migrations/0003_double_pending.py @@ -0,0 +1,17 @@ +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.models import SafeDeleteModel +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + + dependencies = [ + ("bad_flow_delete_model_double_pending_app", "0002_delete_pending"), + ] + + operations = [ + SafeDeleteModel( + name="TestTable", + deletion_action=DeletionAction.MOVE_TO_PENDING, + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/migrations/__init__.py b/fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/migrations/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/models.py b/fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/models.py new file mode 100644 index 00000000000000..770fa149c355ce --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_model_double_pending_app/models.py @@ -0,0 +1,5 @@ +from django.db import models + + +class TestTable(models.Model): + field = models.IntegerField(default=0, null=False) diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_model_without_pending_app/__init__.py b/fixtures/safe_migrations_apps/bad_flow_delete_model_without_pending_app/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_model_without_pending_app/migrations/0001_initial.py b/fixtures/safe_migrations_apps/bad_flow_delete_model_without_pending_app/migrations/0001_initial.py new file mode 100644 index 00000000000000..69de29635772a4 --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_model_without_pending_app/migrations/0001_initial.py @@ -0,0 +1,26 @@ +# Generated by Django 3.1 on 2019-09-22 21:47 + +from django.db import migrations, models + +from sentry.new_migrations.migrations import CheckedMigration + + +class Migration(CheckedMigration): + + initial = True + + dependencies = [] + + operations = [ + migrations.CreateModel( + name="TestTable", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name="ID" + ), + ), + ], + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_model_without_pending_app/migrations/0002_delete_without_pending.py b/fixtures/safe_migrations_apps/bad_flow_delete_model_without_pending_app/migrations/0002_delete_without_pending.py new file mode 100644 index 00000000000000..10d83c4d9c3e39 --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_model_without_pending_app/migrations/0002_delete_without_pending.py @@ -0,0 +1,17 @@ +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.models import SafeDeleteModel +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + + dependencies = [ + ("bad_flow_delete_model_without_pending_app", "0001_initial"), + ] + + operations = [ + SafeDeleteModel( + name="TestTable", + deletion_action=DeletionAction.DELETE, + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_model_without_pending_app/migrations/__init__.py b/fixtures/safe_migrations_apps/bad_flow_delete_model_without_pending_app/migrations/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_model_without_pending_app/models.py b/fixtures/safe_migrations_apps/bad_flow_delete_model_without_pending_app/models.py new file mode 100644 index 00000000000000..770fa149c355ce --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_model_without_pending_app/models.py @@ -0,0 +1,5 @@ +from django.db import models + + +class TestTable(models.Model): + field = models.IntegerField(default=0, null=False) diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_pending_with_fk_constraints_app/__init__.py b/fixtures/safe_migrations_apps/bad_flow_delete_pending_with_fk_constraints_app/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_pending_with_fk_constraints_app/migrations/0001_initial.py b/fixtures/safe_migrations_apps/bad_flow_delete_pending_with_fk_constraints_app/migrations/0001_initial.py new file mode 100644 index 00000000000000..2747f841d8a570 --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_pending_with_fk_constraints_app/migrations/0001_initial.py @@ -0,0 +1,45 @@ +import django +from django.db import migrations, models + +import sentry +from sentry.new_migrations.migrations import CheckedMigration + + +class Migration(CheckedMigration): + + initial = True + + dependencies = [] + + operations = [ + migrations.CreateModel( + name="FkTable", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name="ID" + ), + ), + ], + ), + migrations.CreateModel( + name="TestTable", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name="ID" + ), + ), + ( + "fk_table", + sentry.db.models.fields.foreignkey.FlexibleForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="bad_flow_delete_pending_with_fk_constraints_app.fktable", + db_index=False, + ), + ), + ], + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_pending_with_fk_constraints_app/migrations/0002_delete_without_pending.py b/fixtures/safe_migrations_apps/bad_flow_delete_pending_with_fk_constraints_app/migrations/0002_delete_without_pending.py new file mode 100644 index 00000000000000..721c22b2798a0e --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_pending_with_fk_constraints_app/migrations/0002_delete_without_pending.py @@ -0,0 +1,18 @@ +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.models import SafeDeleteModel +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + atomic = False + + dependencies = [ + ("bad_flow_delete_pending_with_fk_constraints_app", "0001_initial"), + ] + + operations = [ + SafeDeleteModel( + name="TestTable", + deletion_action=DeletionAction.MOVE_TO_PENDING, + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_pending_with_fk_constraints_app/migrations/__init__.py b/fixtures/safe_migrations_apps/bad_flow_delete_pending_with_fk_constraints_app/migrations/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_pending_with_fk_constraints_app/models.py b/fixtures/safe_migrations_apps/bad_flow_delete_pending_with_fk_constraints_app/models.py new file mode 100644 index 00000000000000..d936d5039213f7 --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_pending_with_fk_constraints_app/models.py @@ -0,0 +1,12 @@ +from django.db import models + +from sentry.db.models import FlexibleForeignKey + + +class FkTable(models.Model): + field = models.IntegerField(default=0, null=False) + + +class TestTable(models.Model): + field = models.IntegerField(default=0, null=False) + fk_table = FlexibleForeignKey(FkTable, db_index=False) diff --git a/fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/__init__.py b/fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/migrations/0001_initial.py b/fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/migrations/0001_initial.py new file mode 100644 index 00000000000000..6f55cccc2e683d --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/migrations/0001_initial.py @@ -0,0 +1,46 @@ +# Generated by Django 3.1 on 2019-09-22 21:47 +import django +from django.db import migrations, models + +import sentry +from sentry.new_migrations.migrations import CheckedMigration + + +class Migration(CheckedMigration): + + initial = True + + dependencies = [] + + operations = [ + migrations.CreateModel( + name="FkTable", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name="ID" + ), + ), + ], + ), + migrations.CreateModel( + name="TestTable", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name="ID" + ), + ), + ( + "fk_table", + sentry.db.models.fields.foreignkey.FlexibleForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="good_flow_delete_pending_with_fk_constraints_app.fktable", + db_index=False, + ), + ), + ], + ), + ] diff --git a/fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/migrations/0002_remove_constraints_and_pending.py b/fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/migrations/0002_remove_constraints_and_pending.py new file mode 100644 index 00000000000000..cdb0408e096d99 --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/migrations/0002_remove_constraints_and_pending.py @@ -0,0 +1,32 @@ +import django +from django.db import migrations + +import sentry +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.models import SafeDeleteModel +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + atomic = False + + dependencies = [ + ("good_flow_delete_pending_with_fk_constraints_app", "0001_initial"), + ] + + operations = [ + migrations.AlterField( + model_name="TestTable", + name="fk_table", + field=sentry.db.models.fields.foreignkey.FlexibleForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="good_flow_delete_pending_with_fk_constraints_app.fktable", + db_index=False, + db_constraint=False, + ), + ), + SafeDeleteModel( + name="TestTable", + deletion_action=DeletionAction.MOVE_TO_PENDING, + ), + ] diff --git a/fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/migrations/0003_delete.py b/fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/migrations/0003_delete.py new file mode 100644 index 00000000000000..ac2813a8d7f014 --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/migrations/0003_delete.py @@ -0,0 +1,16 @@ +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.models import SafeDeleteModel +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + dependencies = [ + ("good_flow_delete_pending_with_fk_constraints_app", "0002_remove_constraints_and_pending"), + ] + + operations = [ + SafeDeleteModel( + name="TestTable", + deletion_action=DeletionAction.DELETE, + ), + ] diff --git a/fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/migrations/__init__.py b/fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/migrations/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/models.py b/fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/models.py new file mode 100644 index 00000000000000..d936d5039213f7 --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_pending_with_fk_constraints_app/models.py @@ -0,0 +1,12 @@ +from django.db import models + +from sentry.db.models import FlexibleForeignKey + + +class FkTable(models.Model): + field = models.IntegerField(default=0, null=False) + + +class TestTable(models.Model): + field = models.IntegerField(default=0, null=False) + fk_table = FlexibleForeignKey(FkTable, db_index=False) diff --git a/fixtures/safe_migrations_apps/good_flow_delete_simple_app/__init__.py b/fixtures/safe_migrations_apps/good_flow_delete_simple_app/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/good_flow_delete_simple_app/migrations/0001_initial.py b/fixtures/safe_migrations_apps/good_flow_delete_simple_app/migrations/0001_initial.py new file mode 100644 index 00000000000000..2b6d293ee049e6 --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_simple_app/migrations/0001_initial.py @@ -0,0 +1,22 @@ +from django.db import migrations, models + +from sentry.new_migrations.migrations import CheckedMigration + + +class Migration(CheckedMigration): + initial = True + dependencies = [] + + operations = [ + migrations.CreateModel( + name="TestTable", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name="ID" + ), + ), + ], + ), + ] diff --git a/fixtures/safe_migrations_apps/good_flow_delete_simple_app/migrations/0002_set_pending.py b/fixtures/safe_migrations_apps/good_flow_delete_simple_app/migrations/0002_set_pending.py new file mode 100644 index 00000000000000..c7475e451e0bcd --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_simple_app/migrations/0002_set_pending.py @@ -0,0 +1,16 @@ +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.models import SafeDeleteModel +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + dependencies = [ + ("good_flow_delete_simple_app", "0001_initial"), + ] + + operations = [ + SafeDeleteModel( + name="TestTable", + deletion_action=DeletionAction.MOVE_TO_PENDING, + ), + ] diff --git a/fixtures/safe_migrations_apps/good_flow_delete_simple_app/migrations/0003_delete.py b/fixtures/safe_migrations_apps/good_flow_delete_simple_app/migrations/0003_delete.py new file mode 100644 index 00000000000000..796cf774758675 --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_simple_app/migrations/0003_delete.py @@ -0,0 +1,16 @@ +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.models import SafeDeleteModel +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + dependencies = [ + ("good_flow_delete_simple_app", "0002_set_pending"), + ] + + operations = [ + SafeDeleteModel( + name="TestTable", + deletion_action=DeletionAction.DELETE, + ), + ] diff --git a/fixtures/safe_migrations_apps/good_flow_delete_simple_app/migrations/__init__.py b/fixtures/safe_migrations_apps/good_flow_delete_simple_app/migrations/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/good_flow_delete_simple_app/models.py b/fixtures/safe_migrations_apps/good_flow_delete_simple_app/models.py new file mode 100644 index 00000000000000..770fa149c355ce --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_simple_app/models.py @@ -0,0 +1,5 @@ +from django.db import models + + +class TestTable(models.Model): + field = models.IntegerField(default=0, null=False) diff --git a/src/sentry/db/postgres/schema.py b/src/sentry/db/postgres/schema.py index e38c153cd77c70..986396c22e7122 100644 --- a/src/sentry/db/postgres/schema.py +++ b/src/sentry/db/postgres/schema.py @@ -88,14 +88,16 @@ def alter_db_table(self, model, old_db_table, new_db_table): "More info here: https://develop.sentry.dev/database-migrations/#renaming-tables" ) - def delete_model(self, model): + def delete_model(self, model, is_safe=False): """ It's never safe to delete a model using the standard migration process """ - raise UnsafeOperationException( - f"Deleting the {model.__name__} model is unsafe.\n" - "More info here: https://develop.sentry.dev/database-migrations/#deleting-tables" - ) + if not is_safe: + raise UnsafeOperationException( + f"Deleting the {model.__name__} model is unsafe.\n" + "More info here: https://develop.sentry.dev/database-migrations/#deleting-tables" + ) + super(DatabaseSchemaEditorMixin, self).delete_model(model) def remove_field(self, model, field): """ diff --git a/src/sentry/new_migrations/monkey/__init__.py b/src/sentry/new_migrations/monkey/__init__.py index a6b52294b510c1..d13eec497e52b1 100644 --- a/src/sentry/new_migrations/monkey/__init__.py +++ b/src/sentry/new_migrations/monkey/__init__.py @@ -19,6 +19,10 @@ is copied and modified from `Queryset.update()` to add `RETURNING ` to the update query. Verify that the `update` code hasn't significantly changed, and if it has update as needed. + - We monkeypatch `SentryProjectState` over `ProjectState` in a few places. Check where + Django is importing it and make sure that we're still patching correctly. + We also need to verify that the patched `SentryProjectState` isn't missing new + features added by Django. When you're happy that these changes are good to go, update `LAST_VERIFIED_DJANGO_VERSION` to the version of Django you're upgrading to. If the @@ -86,3 +90,11 @@ def monkey_migrations(): migration.Migration.initial = None writer.MIGRATION_TEMPLATE = SENTRY_MIGRATION_TEMPLATE models.Field.deconstruct = deconstruct # type: ignore[method-assign] + + from django.db.migrations import graph, state + + from sentry.new_migrations.monkey.state import SentryProjectState + + state.ProjectState = SentryProjectState # type: ignore[misc] + graph.ProjectState = SentryProjectState # type: ignore[attr-defined] + executor.ProjectState = SentryProjectState # type: ignore[attr-defined] diff --git a/src/sentry/new_migrations/monkey/models.py b/src/sentry/new_migrations/monkey/models.py new file mode 100644 index 00000000000000..f741abea5baf06 --- /dev/null +++ b/src/sentry/new_migrations/monkey/models.py @@ -0,0 +1,47 @@ +from django.db.migrations import DeleteModel +from django_zero_downtime_migrations.backends.postgres.schema import UnsafeOperationException + +from sentry.new_migrations.monkey.state import DeletionAction + + +class SafeDeleteModel(DeleteModel): + def __init__(self, *args, deletion_action: DeletionAction | None = None, **kwargs): + if deletion_action is None: + raise UnsafeOperationException("Deletion State is required") + super().__init__(*args, **kwargs) + self.deletion_action = deletion_action + + def state_forwards(self, app_label, state): + if self.deletion_action == DeletionAction.MOVE_TO_PENDING: + model = state.apps.get_model(app_label, self.name) + fields_with_constraints = [ + f.name for f in model._meta.fields if getattr(f, "db_constraint", False) + ] + if fields_with_constraints: + raise UnsafeOperationException( + "Foreign key db constraints must be removed before dropping " + f"{app_label}.{self.name}. Fields with constraints: {fields_with_constraints}" + "More info: https://develop.sentry.dev/api-server/application-domains/database-migrations/#deleting-tables" + ) + state.remove_model(app_label, self.name_lower, deletion_action=self.deletion_action) + + def database_forwards(self, app_label, schema_editor, from_state, to_state): + if self.deletion_action == DeletionAction.MOVE_TO_PENDING: + return + + model = from_state.get_pending_deletion_model(app_label, self.name) + if self.allow_migrate_model(schema_editor.connection.alias, model): + schema_editor.delete_model(model, is_safe=True) + + def database_backwards(self, app_label, schema_editor, from_state, to_state): + if self.deletion_action == DeletionAction.MOVE_TO_PENDING: + return + model = to_state.get_pending_deletion_model(app_label, self.name) + if self.allow_migrate_model(schema_editor.connection.alias, model): + schema_editor.create_model(model) + + def describe(self): + if self.deletion_action == DeletionAction.MOVE_TO_PENDING: + return f"Moved model {self.name} to pending deletion state" + else: + return super().describe() diff --git a/src/sentry/new_migrations/monkey/state.py b/src/sentry/new_migrations/monkey/state.py new file mode 100644 index 00000000000000..ee01b6218020d9 --- /dev/null +++ b/src/sentry/new_migrations/monkey/state.py @@ -0,0 +1,49 @@ +from copy import deepcopy +from enum import Enum + +from django.db.migrations.state import ProjectState +from django_zero_downtime_migrations.backends.postgres.schema import UnsafeOperationException + + +class DeletionAction(Enum): + MOVE_TO_PENDING = 0 + DELETE = 1 + + +class SentryProjectState(ProjectState): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.pending_deletion_models = {} + + def get_pending_deletion_model(self, app_label, model_name): + model_key = (app_label.lower(), model_name.lower()) + if model_key not in self.pending_deletion_models: + raise UnsafeOperationException( + "Model must be in the pending deletion state before full deletion. " + "More info: https://develop.sentry.dev/api-server/application-domains/database-migrations/#deleting-tables" + ) + return self.pending_deletion_models[model_key] + + def remove_model(self, app_label, model_name, deletion_action: DeletionAction | None = None): + model_key = (app_label, model_name) + if deletion_action == DeletionAction.DELETE: + if model_key not in self.pending_deletion_models: + raise UnsafeOperationException( + "Model must be in the pending deletion state before full deletion. " + "More info: https://develop.sentry.dev/api-server/application-domains/database-migrations/#deleting-tables" + ) + del self.pending_deletion_models[model_key] + return + if deletion_action == DeletionAction.MOVE_TO_PENDING: + if model_key in self.pending_deletion_models: + raise UnsafeOperationException( + f"{app_label}.{model_name} is already pending deletion. Use DeletionAction.DELETE to delete" + "More info: https://develop.sentry.dev/api-server/application-domains/database-migrations/#deleting-tables" + ) + self.pending_deletion_models[model_key] = self.apps.get_model(app_label, model_name) + super().remove_model(app_label, model_name) + + def clone(self): + new_state = super().clone() + new_state.pending_deletion_models = deepcopy(self.pending_deletion_models) # type: ignore[attr-defined] + return new_state diff --git a/tests/sentry/db/postgres/schema/safe_migrations/integration/test_migrations.py b/tests/sentry/db/postgres/schema/safe_migrations/integration/test_migrations.py index 955f10c09a1615..0638959b6b9bca 100644 --- a/tests/sentry/db/postgres/schema/safe_migrations/integration/test_migrations.py +++ b/tests/sentry/db/postgres/schema/safe_migrations/integration/test_migrations.py @@ -37,22 +37,24 @@ def run_migration(self): self._run_migration(self.app, self.migrate_from) self._run_migration(self.app, self.migrate_to) - def _run_migration(self, app, migration): + def _run_migration(self, app, migration_name): with override_settings( INSTALLED_APPS=(f"{self.BASE_PATH}.{self.app}",), MIGRATION_MODULES={} ): executor = MigrationExecutor(connection) + migration = executor.loader.get_migration_by_prefix(app, migration_name) executor.loader.build_graph() - target = [(app, migration)] + target = [(migration.app_label, migration.name)] executor.loader.project_state(target).apps executor.migrate(target) - def sql_migrate(self, app_label, migration_name): + def sql_migrate(self, app, migration_name): with override_settings( INSTALLED_APPS=(f"{self.BASE_PATH}.{self.app}",), MIGRATION_MODULES={} ): - target = (app_label, migration_name) executor = MigrationExecutor(connection) + migration = executor.loader.get_migration_by_prefix(app, migration_name) + target = (app, migration.name) plan = [(executor.loader.graph.nodes[target], None)] sql_statements = executor.loader.collect_sql(plan) # type: ignore[attr-defined] return "\n".join(sql_statements) @@ -230,3 +232,73 @@ def test(self): "SET statement_timeout TO '0ms';", "SET lock_timeout TO '0ms';", ] + + +class DeletionBadDeleteModelWithoutPendingTest(BaseSafeMigrationTest): + app = "bad_flow_delete_model_without_pending_app" + migrate_from = "0001" + migrate_to = "0002" + + def test(self): + with pytest.raises( + UnsafeOperationException, + match="Model must be in the pending deletion state before full deletion", + ): + self.run_migration() + + +class DeletionBadDeleteModelDoublePendingTest(BaseSafeMigrationTest): + app = "bad_flow_delete_model_double_pending_app" + migrate_from = "0001" + migrate_to = "0003" + + def test(self): + with pytest.raises( + LookupError, + match="App 'bad_flow_delete_model_double_pending_app' doesn't have a 'TestTable' model", + ): + self.run_migration() + + +class DeletionBadDeletePendingWithFKConstraints(BaseSafeMigrationTest): + app = "bad_flow_delete_pending_with_fk_constraints_app" + migrate_from = "0001" + migrate_to = "0002" + + def test(self): + with pytest.raises( + UnsafeOperationException, + match="Foreign key db constraints must be removed before dropping " + "bad_flow_delete_pending_with_fk_constraints_app.TestTable. " + "Fields with constraints: \\['fk_table'\\]", + ): + self.run_migration() + + +class DeletionGoodDeleteRemoveFKConstraints(BaseSafeMigrationTest): + app = "good_flow_delete_pending_with_fk_constraints_app" + migrate_from = "0001" + migrate_to = "0003" + + def test(self): + + self._run_migration(self.app, "0001_initial") + assert f"{self.app}_testtable" in connection.introspection.table_names() + self._run_migration(self.app, "0002_remove_constraints_and_pending") + assert f"{self.app}_testtable" in connection.introspection.table_names() + self._run_migration(self.app, "0003_delete") + assert f"{self.app}_testtable" not in connection.introspection.table_names() + + +class DeletionGoodDeleteSimple(BaseSafeMigrationTest): + app = "good_flow_delete_simple_app" + migrate_from = "0001" + migrate_to = "0003" + + def test(self): + self._run_migration(self.app, "0001_initial") + assert f"{self.app}_testtable" in connection.introspection.table_names() + self._run_migration(self.app, "0002_set_pending") + assert f"{self.app}_testtable" in connection.introspection.table_names() + self._run_migration(self.app, "0003_delete") + assert f"{self.app}_testtable" not in connection.introspection.table_names() From e187edf0af6aa51e17b3066bf1e4e37d9091b61b Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Thu, 21 Nov 2024 14:49:38 -0800 Subject: [PATCH 2/2] typing and other small fixes --- src/sentry/new_migrations/monkey/models.py | 27 +++++++++++++++------- src/sentry/new_migrations/monkey/state.py | 17 +++++++++----- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/src/sentry/new_migrations/monkey/models.py b/src/sentry/new_migrations/monkey/models.py index f741abea5baf06..e744e11b356e70 100644 --- a/src/sentry/new_migrations/monkey/models.py +++ b/src/sentry/new_migrations/monkey/models.py @@ -1,17 +1,16 @@ from django.db.migrations import DeleteModel from django_zero_downtime_migrations.backends.postgres.schema import UnsafeOperationException -from sentry.new_migrations.monkey.state import DeletionAction +from sentry.db.postgres.schema import SafePostgresDatabaseSchemaEditor +from sentry.new_migrations.monkey.state import DeletionAction, SentryProjectState class SafeDeleteModel(DeleteModel): - def __init__(self, *args, deletion_action: DeletionAction | None = None, **kwargs): - if deletion_action is None: - raise UnsafeOperationException("Deletion State is required") + def __init__(self, *args, deletion_action: DeletionAction, **kwargs): super().__init__(*args, **kwargs) self.deletion_action = deletion_action - def state_forwards(self, app_label, state): + def state_forwards(self, app_label: str, state: SentryProjectState) -> None: # type: ignore[override] if self.deletion_action == DeletionAction.MOVE_TO_PENDING: model = state.apps.get_model(app_label, self.name) fields_with_constraints = [ @@ -25,7 +24,13 @@ def state_forwards(self, app_label, state): ) state.remove_model(app_label, self.name_lower, deletion_action=self.deletion_action) - def database_forwards(self, app_label, schema_editor, from_state, to_state): + def database_forwards( + self, + app_label: str, + schema_editor: SafePostgresDatabaseSchemaEditor, # type: ignore[override] + from_state: SentryProjectState, # type: ignore[override] + to_state: SentryProjectState, # type: ignore[override] + ) -> None: if self.deletion_action == DeletionAction.MOVE_TO_PENDING: return @@ -33,14 +38,20 @@ def database_forwards(self, app_label, schema_editor, from_state, to_state): if self.allow_migrate_model(schema_editor.connection.alias, model): schema_editor.delete_model(model, is_safe=True) - def database_backwards(self, app_label, schema_editor, from_state, to_state): + def database_backwards( + self, + app_label: str, + schema_editor: SafePostgresDatabaseSchemaEditor, # type: ignore[override] + from_state: SentryProjectState, # type: ignore[override] + to_state: SentryProjectState, # type: ignore[override] + ) -> None: if self.deletion_action == DeletionAction.MOVE_TO_PENDING: return model = to_state.get_pending_deletion_model(app_label, self.name) if self.allow_migrate_model(schema_editor.connection.alias, model): schema_editor.create_model(model) - def describe(self): + def describe(self) -> str: if self.deletion_action == DeletionAction.MOVE_TO_PENDING: return f"Moved model {self.name} to pending deletion state" else: diff --git a/src/sentry/new_migrations/monkey/state.py b/src/sentry/new_migrations/monkey/state.py index ee01b6218020d9..a204ec81d13970 100644 --- a/src/sentry/new_migrations/monkey/state.py +++ b/src/sentry/new_migrations/monkey/state.py @@ -1,7 +1,10 @@ +from __future__ import annotations + from copy import deepcopy from enum import Enum from django.db.migrations.state import ProjectState +from django.db.models import Model from django_zero_downtime_migrations.backends.postgres.schema import UnsafeOperationException @@ -13,9 +16,9 @@ class DeletionAction(Enum): class SentryProjectState(ProjectState): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self.pending_deletion_models = {} + self.pending_deletion_models: dict[tuple[str, str], type[Model]] = {} - def get_pending_deletion_model(self, app_label, model_name): + def get_pending_deletion_model(self, app_label: str, model_name: str) -> type[Model]: model_key = (app_label.lower(), model_name.lower()) if model_key not in self.pending_deletion_models: raise UnsafeOperationException( @@ -24,8 +27,10 @@ def get_pending_deletion_model(self, app_label, model_name): ) return self.pending_deletion_models[model_key] - def remove_model(self, app_label, model_name, deletion_action: DeletionAction | None = None): - model_key = (app_label, model_name) + def remove_model( + self, app_label: str, model_name: str, deletion_action: DeletionAction | None = None + ) -> None: + model_key = (app_label.lower(), model_name.lower()) if deletion_action == DeletionAction.DELETE: if model_key not in self.pending_deletion_models: raise UnsafeOperationException( @@ -43,7 +48,7 @@ def remove_model(self, app_label, model_name, deletion_action: DeletionAction | self.pending_deletion_models[model_key] = self.apps.get_model(app_label, model_name) super().remove_model(app_label, model_name) - def clone(self): + def clone(self) -> SentryProjectState: new_state = super().clone() new_state.pending_deletion_models = deepcopy(self.pending_deletion_models) # type: ignore[attr-defined] - return new_state + return new_state # type: ignore[return-value]