From 932d6540fc81d3a25bdf68366fa74d2e87e68646 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Wed, 20 Nov 2024 16:46:00 -0800 Subject: [PATCH 1/3] feat(migrations): Add in `SafeDeleteColumn` migration operation This builds on https://github.com/getsentry/sentry/pull/81063 This adds in `SafeDeleteColumn`, which works the same way as `SafeDeleteModel`, except on database columns. It performs checks that the column doesn't have a db constraint set, and also that it is either nullable or has a db_default set. Similarly to `SafeDeleteModel` we still need to be careful to make sure that the pending deletion merges and deploys first, and then the real deletion. --- .../__init__.py | 0 .../migrations/0001_initial.py | 27 ++++ .../migrations/0002_delete_pending.py | 18 +++ .../migrations/0003_double_pending.py | 18 +++ .../migrations/__init__.py | 0 .../models.py | 5 + .../__init__.py | 0 .../migrations/0001_initial.py | 45 ++++++ .../migrations/0002_delete_without_pending.py | 19 +++ .../migrations/__init__.py | 0 .../models.py | 12 ++ .../__init__.py | 0 .../migrations/0001_initial.py | 25 +++ .../migrations/0002_delete_without_pending.py | 17 +++ .../migrations/__init__.py | 0 .../models.py | 5 + .../__init__.py | 0 .../migrations/0001_initial.py | 27 ++++ .../migrations/0002_delete_without_pending.py | 18 +++ .../migrations/__init__.py | 0 .../models.py | 5 + .../__init__.py | 0 .../migrations/0001_initial.py | 47 ++++++ .../0002_remove_constraints_and_pending.py | 34 +++++ .../migrations/0003_delete.py | 20 +++ .../migrations/__init__.py | 0 .../models.py | 12 ++ .../__init__.py | 0 .../migrations/0001_initial.py | 37 +++++ .../0002_remove_not_null_and_pending.py | 24 +++ .../migrations/0003_delete.py | 17 +++ .../migrations/__init__.py | 0 .../models.py | 5 + .../__init__.py | 0 .../migrations/0001_initial.py | 23 +++ .../migrations/0002_set_pending.py | 17 +++ .../migrations/0003_delete.py | 17 +++ .../migrations/__init__.py | 0 .../models.py | 5 + src/sentry/db/postgres/schema.py | 12 +- src/sentry/new_migrations/monkey/fields.py | 54 +++++++ src/sentry/new_migrations/monkey/state.py | 36 +++++ .../integration/test_migrations.py | 142 +++++++++++++++++- 43 files changed, 733 insertions(+), 10 deletions(-) create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/__init__.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/migrations/0001_initial.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/migrations/0002_delete_pending.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/migrations/0003_double_pending.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/migrations/__init__.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/models.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_fk_constraint_app/__init__.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_fk_constraint_app/migrations/0001_initial.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_fk_constraint_app/migrations/0002_delete_without_pending.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_fk_constraint_app/migrations/__init__.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_fk_constraint_app/models.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_not_null_app/__init__.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_not_null_app/migrations/0001_initial.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_not_null_app/migrations/0002_delete_without_pending.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_not_null_app/migrations/__init__.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_not_null_app/models.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_without_pending_app/__init__.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_without_pending_app/migrations/0001_initial.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_without_pending_app/migrations/0002_delete_without_pending.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_without_pending_app/migrations/__init__.py create mode 100644 fixtures/safe_migrations_apps/bad_flow_delete_field_without_pending_app/models.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/__init__.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/migrations/0001_initial.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/migrations/0002_remove_constraints_and_pending.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/migrations/0003_delete.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/migrations/__init__.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/models.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/__init__.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/migrations/0001_initial.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/migrations/0002_remove_not_null_and_pending.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/migrations/0003_delete.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/migrations/__init__.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/models.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/__init__.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/migrations/0001_initial.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/migrations/0002_set_pending.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/migrations/0003_delete.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/migrations/__init__.py create mode 100644 fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/models.py diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/__init__.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/migrations/0001_initial.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/migrations/0001_initial.py new file mode 100644 index 00000000000000..27b7cdf8c1b768 --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/migrations/0001_initial.py @@ -0,0 +1,27 @@ +# 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" + ), + ), + ("field", models.IntegerField(null=True)), + ], + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/migrations/0002_delete_pending.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/migrations/0002_delete_pending.py new file mode 100644 index 00000000000000..c4933d32cfcede --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/migrations/0002_delete_pending.py @@ -0,0 +1,18 @@ +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.fields import SafeRemoveField +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + + dependencies = [ + ("bad_flow_delete_field_double_pending_app", "0001_initial"), + ] + + operations = [ + SafeRemoveField( + model_name="testtable", + name="field", + deletion_action=DeletionAction.MOVE_TO_PENDING, + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/migrations/0003_double_pending.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/migrations/0003_double_pending.py new file mode 100644 index 00000000000000..ce220395342ff9 --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/migrations/0003_double_pending.py @@ -0,0 +1,18 @@ +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.fields import SafeRemoveField +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + + dependencies = [ + ("bad_flow_delete_field_double_pending_app", "0002_delete_pending"), + ] + + operations = [ + SafeRemoveField( + model_name="testtable", + name="field", + deletion_action=DeletionAction.MOVE_TO_PENDING, + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/migrations/__init__.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/migrations/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/models.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_double_pending_app/models.py new file mode 100644 index 00000000000000..770fa149c355ce --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_field_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_field_pending_with_fk_constraint_app/__init__.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_fk_constraint_app/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_fk_constraint_app/migrations/0001_initial.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_fk_constraint_app/migrations/0001_initial.py new file mode 100644 index 00000000000000..4446837fa33671 --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_fk_constraint_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_field_pending_with_fk_constraint_app.fktable", + db_index=False, + ), + ), + ], + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_fk_constraint_app/migrations/0002_delete_without_pending.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_fk_constraint_app/migrations/0002_delete_without_pending.py new file mode 100644 index 00000000000000..f036396f10a590 --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_fk_constraint_app/migrations/0002_delete_without_pending.py @@ -0,0 +1,19 @@ +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.fields import SafeRemoveField +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + atomic = False + + dependencies = [ + ("bad_flow_delete_field_pending_with_fk_constraint_app", "0001_initial"), + ] + + operations = [ + SafeRemoveField( + model_name="testtable", + name="fk_table", + deletion_action=DeletionAction.MOVE_TO_PENDING, + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_fk_constraint_app/migrations/__init__.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_fk_constraint_app/migrations/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_fk_constraint_app/models.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_fk_constraint_app/models.py new file mode 100644 index 00000000000000..d936d5039213f7 --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_fk_constraint_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/bad_flow_delete_field_pending_with_not_null_app/__init__.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_not_null_app/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_not_null_app/migrations/0001_initial.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_not_null_app/migrations/0001_initial.py new file mode 100644 index 00000000000000..fd1e8c68b7800c --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_not_null_app/migrations/0001_initial.py @@ -0,0 +1,25 @@ +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" + ), + ), + ("field", models.IntegerField()), + ], + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_not_null_app/migrations/0002_delete_without_pending.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_not_null_app/migrations/0002_delete_without_pending.py new file mode 100644 index 00000000000000..6502d91d97cd39 --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_not_null_app/migrations/0002_delete_without_pending.py @@ -0,0 +1,17 @@ +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.fields import SafeRemoveField +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + dependencies = [ + ("bad_flow_delete_field_pending_with_not_null_app", "0001_initial"), + ] + + operations = [ + SafeRemoveField( + model_name="testtable", + name="field", + deletion_action=DeletionAction.MOVE_TO_PENDING, + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_not_null_app/migrations/__init__.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_not_null_app/migrations/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_not_null_app/models.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_not_null_app/models.py new file mode 100644 index 00000000000000..770fa149c355ce --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_field_pending_with_not_null_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_field_without_pending_app/__init__.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_without_pending_app/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_field_without_pending_app/migrations/0001_initial.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_without_pending_app/migrations/0001_initial.py new file mode 100644 index 00000000000000..482ebcd0ab28f4 --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_field_without_pending_app/migrations/0001_initial.py @@ -0,0 +1,27 @@ +# 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" + ), + ), + ("field", models.IntegerField()), + ], + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_field_without_pending_app/migrations/0002_delete_without_pending.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_without_pending_app/migrations/0002_delete_without_pending.py new file mode 100644 index 00000000000000..b4cad5043fc589 --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_field_without_pending_app/migrations/0002_delete_without_pending.py @@ -0,0 +1,18 @@ +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.fields import SafeRemoveField +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + + dependencies = [ + ("bad_flow_delete_field_without_pending_app", "0001_initial"), + ] + + operations = [ + SafeRemoveField( + model_name="testtable", + name="field", + deletion_action=DeletionAction.DELETE, + ), + ] diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_field_without_pending_app/migrations/__init__.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_without_pending_app/migrations/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/bad_flow_delete_field_without_pending_app/models.py b/fixtures/safe_migrations_apps/bad_flow_delete_field_without_pending_app/models.py new file mode 100644 index 00000000000000..770fa149c355ce --- /dev/null +++ b/fixtures/safe_migrations_apps/bad_flow_delete_field_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/good_flow_delete_field_pending_with_fk_constraint_app/__init__.py b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/migrations/0001_initial.py b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/migrations/0001_initial.py new file mode 100644 index 00000000000000..85f25fffd727f5 --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/migrations/0001_initial.py @@ -0,0 +1,47 @@ +# 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_field_pending_with_fk_constraint_app.fktable", + db_index=False, + null=True, + ), + ), + ], + ), + ] diff --git a/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/migrations/0002_remove_constraints_and_pending.py b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/migrations/0002_remove_constraints_and_pending.py new file mode 100644 index 00000000000000..767c38e610f98b --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/migrations/0002_remove_constraints_and_pending.py @@ -0,0 +1,34 @@ +import django +from django.db import migrations + +import sentry +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.fields import SafeRemoveField +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + atomic = False + + dependencies = [ + ("good_flow_delete_field_pending_with_fk_constraint_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_field_pending_with_fk_constraint_app.fktable", + db_index=False, + db_constraint=False, + null=True, + ), + ), + SafeRemoveField( + model_name="testtable", + name="fk_table", + deletion_action=DeletionAction.MOVE_TO_PENDING, + ), + ] diff --git a/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/migrations/0003_delete.py b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/migrations/0003_delete.py new file mode 100644 index 00000000000000..adae7ffa190165 --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/migrations/0003_delete.py @@ -0,0 +1,20 @@ +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.fields import SafeRemoveField +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + dependencies = [ + ( + "good_flow_delete_field_pending_with_fk_constraint_app", + "0002_remove_constraints_and_pending", + ), + ] + + operations = [ + SafeRemoveField( + model_name="testtable", + name="fk_table", + deletion_action=DeletionAction.DELETE, + ), + ] diff --git a/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/migrations/__init__.py b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/migrations/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/models.py b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_app/models.py new file mode 100644 index 00000000000000..e78bdd99a9322d --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_fk_constraint_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, null=True, db_index=False) diff --git a/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/__init__.py b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/migrations/0001_initial.py b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/migrations/0001_initial.py new file mode 100644 index 00000000000000..1a0f0a785f5159 --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/migrations/0001_initial.py @@ -0,0 +1,37 @@ +# 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="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" + ), + ), + ("field", models.IntegerField(null=True)), + ], + ), + ] diff --git a/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/migrations/0002_remove_not_null_and_pending.py b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/migrations/0002_remove_not_null_and_pending.py new file mode 100644 index 00000000000000..e413d67d2a23b6 --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/migrations/0002_remove_not_null_and_pending.py @@ -0,0 +1,24 @@ +from django.db import migrations, models + +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.fields import SafeRemoveField +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + dependencies = [ + ("good_flow_delete_field_pending_with_not_null_app", "0001_initial"), + ] + + operations = [ + migrations.AlterField( + model_name="TestTable", + name="field", + field=models.IntegerField(null=True), + ), + SafeRemoveField( + model_name="testtable", + name="field", + deletion_action=DeletionAction.MOVE_TO_PENDING, + ), + ] diff --git a/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/migrations/0003_delete.py b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/migrations/0003_delete.py new file mode 100644 index 00000000000000..98ed7ffbe84c48 --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/migrations/0003_delete.py @@ -0,0 +1,17 @@ +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.fields import SafeRemoveField +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + dependencies = [ + ("good_flow_delete_field_pending_with_not_null_app", "0002_remove_not_null_and_pending"), + ] + + operations = [ + SafeRemoveField( + model_name="testtable", + name="field", + deletion_action=DeletionAction.DELETE, + ), + ] diff --git a/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/migrations/__init__.py b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/migrations/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/models.py b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_app/models.py new file mode 100644 index 00000000000000..770fa149c355ce --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_field_pending_with_not_null_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/good_flow_delete_field_simple_app/__init__.py b/fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/migrations/0001_initial.py b/fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/migrations/0001_initial.py new file mode 100644 index 00000000000000..2540b245ec0644 --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/migrations/0001_initial.py @@ -0,0 +1,23 @@ +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" + ), + ), + ("field", models.IntegerField(null=True)), + ], + ), + ] diff --git a/fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/migrations/0002_set_pending.py b/fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/migrations/0002_set_pending.py new file mode 100644 index 00000000000000..48b502df50c460 --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/migrations/0002_set_pending.py @@ -0,0 +1,17 @@ +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.fields import SafeRemoveField +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + dependencies = [ + ("good_flow_delete_field_simple_app", "0001_initial"), + ] + + operations = [ + SafeRemoveField( + model_name="testtable", + name="field", + deletion_action=DeletionAction.MOVE_TO_PENDING, + ), + ] diff --git a/fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/migrations/0003_delete.py b/fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/migrations/0003_delete.py new file mode 100644 index 00000000000000..e57524bf66d90d --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/migrations/0003_delete.py @@ -0,0 +1,17 @@ +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.fields import SafeRemoveField +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + dependencies = [ + ("good_flow_delete_field_simple_app", "0002_set_pending"), + ] + + operations = [ + SafeRemoveField( + model_name="testtable", + name="field", + deletion_action=DeletionAction.DELETE, + ), + ] diff --git a/fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/migrations/__init__.py b/fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/migrations/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/models.py b/fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/models.py new file mode 100644 index 00000000000000..f472e7d8e90506 --- /dev/null +++ b/fixtures/safe_migrations_apps/good_flow_delete_field_simple_app/models.py @@ -0,0 +1,5 @@ +from django.db import models + + +class TestTable(models.Model): + field = models.IntegerField(default=0, null=True) diff --git a/src/sentry/db/postgres/schema.py b/src/sentry/db/postgres/schema.py index 986396c22e7122..385feff6591a91 100644 --- a/src/sentry/db/postgres/schema.py +++ b/src/sentry/db/postgres/schema.py @@ -99,14 +99,16 @@ def delete_model(self, model, is_safe=False): ) super(DatabaseSchemaEditorMixin, self).delete_model(model) - def remove_field(self, model, field): + def remove_field(self, model, field, is_safe=False): """ It's never safe to remove a field using the standard migration process """ - raise UnsafeOperationException( - f"Removing the {model.__name__}.{field.name} field is unsafe.\n" - "More info here: https://develop.sentry.dev/database-migrations/#deleting-columns" - ) + if not is_safe: + raise UnsafeOperationException( + f"Removing the {model.__name__}.{field.name} field is unsafe.\n" + "More info here: https://develop.sentry.dev/database-migrations/#deleting-columns" + ) + super(DatabaseSchemaEditorMixin, self).remove_field(model, field) def execute(self, sql, params=()): if sql is DUMMY_SQL: diff --git a/src/sentry/new_migrations/monkey/fields.py b/src/sentry/new_migrations/monkey/fields.py index d45d426bbe6c0f..337d8fc4455371 100644 --- a/src/sentry/new_migrations/monkey/fields.py +++ b/src/sentry/new_migrations/monkey/fields.py @@ -1,4 +1,9 @@ +from django.db.migrations import RemoveField from django.db.models import Field +from django.db.models.fields import NOT_PROVIDED +from django_zero_downtime_migrations.backends.postgres.schema import UnsafeOperationException + +from sentry.new_migrations.monkey.state import DeletionAction IGNORED_ATTRS = ["verbose_name", "help_text", "choices"] original_deconstruct = Field.deconstruct @@ -14,3 +19,52 @@ def deconstruct(self): for attr in IGNORED_ATTRS: kwargs.pop(attr, None) return name, path, args, kwargs + + +class SafeRemoveField(RemoveField): + 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: + field = state.apps.get_model(app_label, self.model_name_lower)._meta.get_field( + self.name_lower + ) + if getattr(field, "db_constraint", False): + raise UnsafeOperationException( + f"Foreign key db constraint must be removed before dropping {app_label}.{self.model_name_lower}.{self.name}. " + "More info: https://develop.sentry.dev/api-server/application-domains/database-migrations/#deleting-columns" + ) + if not field.null and field.db_default is NOT_PROVIDED: + raise UnsafeOperationException( + f"Field {app_label}.{self.model_name_lower}.{self.name} must either be nullable or have a db_default before dropping. " + "More info: https://develop.sentry.dev/api-server/application-domains/database-migrations/#deleting-columns" + ) + + state.remove_field( + app_label, self.model_name_lower, 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 + + field = from_state.get_pending_deletion_field(app_label, self.model_name, self.name) + if self.allow_migrate_model(schema_editor.connection.alias, field.model): + schema_editor.remove_field(field.model, field, is_safe=True) + + def database_backwards(self, app_label, schema_editor, from_state, to_state): + if self.deletion_action == DeletionAction.MOVE_TO_PENDING: + return + field = to_state.get_pending_deletion_field(app_label, self.model_name, self.name) + if self.allow_migrate_model(schema_editor.connection.alias, field.model): + schema_editor.add_field(field.model, field) + + def describe(self): + if self.deletion_action == DeletionAction.MOVE_TO_PENDING: + return f"Moved {self.model_name}.{self.name} field 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 index a204ec81d13970..5c665583d65442 100644 --- a/src/sentry/new_migrations/monkey/state.py +++ b/src/sentry/new_migrations/monkey/state.py @@ -17,6 +17,7 @@ class SentryProjectState(ProjectState): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.pending_deletion_models: dict[tuple[str, str], type[Model]] = {} + self.pending_deletion_fields = {} def get_pending_deletion_model(self, app_label: str, model_name: str) -> type[Model]: model_key = (app_label.lower(), model_name.lower()) @@ -27,6 +28,15 @@ def get_pending_deletion_model(self, app_label: str, model_name: str) -> type[Mo ) return self.pending_deletion_models[model_key] + def get_pending_deletion_field(self, app_label: str, model_name: str, field_name: str): + field_key = (app_label.lower(), model_name.lower(), field_name.lower()) + if field_key not in self.pending_deletion_fields: + raise UnsafeOperationException( + "Field must be in the pending deletion state before full deletion. " + "More info: https://develop.sentry.dev/api-server/application-domains/database-migrations/#deleting-columns" + ) + return self.pending_deletion_fields[field_key] + def remove_model( self, app_label: str, model_name: str, deletion_action: DeletionAction | None = None ) -> None: @@ -48,7 +58,33 @@ def remove_model( self.pending_deletion_models[model_key] = self.apps.get_model(app_label, model_name) super().remove_model(app_label, model_name) + def remove_field( + self, app_label, model_name, name, deletion_action: DeletionAction | None = None + ): + field_key = app_label, model_name, name + if deletion_action == DeletionAction.DELETE: + if field_key not in self.pending_deletion_fields: + raise UnsafeOperationException( + "Field must be in the pending deletion state before full deletion. " + "More info: https://develop.sentry.dev/api-server/application-domains/database-migrations/#deleting-columns" + ) + del self.pending_deletion_fields[field_key] + return + + if deletion_action == DeletionAction.MOVE_TO_PENDING: + if field_key in self.pending_deletion_fields: + raise UnsafeOperationException( + f"{app_label}.{model_name}.{name} is already pending deletion. Use DeletionAction.DELETE to delete" + "More info: https://develop.sentry.dev/api-server/application-domains/database-migrations/#deleting-columns" + ) + self.pending_deletion_fields[field_key] = self.apps.get_model( + app_label, model_name + )._meta.get_field(name) + + super().remove_field(app_label, model_name, name) + def clone(self) -> SentryProjectState: new_state = super().clone() new_state.pending_deletion_models = deepcopy(self.pending_deletion_models) # type: ignore[attr-defined] + new_state.pending_deletion_fields = deepcopy(self.pending_deletion_fields) # type: ignore[attr-defined] return new_state # type: ignore[return-value] 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 0638959b6b9bca..dbbf62b2e187cd 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 @@ -1,4 +1,5 @@ import pytest +from django.core.exceptions import FieldDoesNotExist from django.db import connection from django.db.migrations.executor import MigrationExecutor from django.test import override_settings @@ -234,7 +235,7 @@ def test(self): ] -class DeletionBadDeleteModelWithoutPendingTest(BaseSafeMigrationTest): +class DeletionModelBadDeleteWithoutPendingTest(BaseSafeMigrationTest): app = "bad_flow_delete_model_without_pending_app" migrate_from = "0001" migrate_to = "0002" @@ -247,7 +248,7 @@ def test(self): self.run_migration() -class DeletionBadDeleteModelDoublePendingTest(BaseSafeMigrationTest): +class DeletionModelBadDeleteDoublePendingTest(BaseSafeMigrationTest): app = "bad_flow_delete_model_double_pending_app" migrate_from = "0001" migrate_to = "0003" @@ -260,7 +261,7 @@ def test(self): self.run_migration() -class DeletionBadDeletePendingWithFKConstraints(BaseSafeMigrationTest): +class DeletionModelBadDeletePendingWithFKConstraints(BaseSafeMigrationTest): app = "bad_flow_delete_pending_with_fk_constraints_app" migrate_from = "0001" migrate_to = "0002" @@ -275,7 +276,7 @@ def test(self): self.run_migration() -class DeletionGoodDeleteRemoveFKConstraints(BaseSafeMigrationTest): +class DeletionModelGoodDeleteRemoveFKConstraints(BaseSafeMigrationTest): app = "good_flow_delete_pending_with_fk_constraints_app" migrate_from = "0001" migrate_to = "0003" @@ -290,7 +291,7 @@ def test(self): assert f"{self.app}_testtable" not in connection.introspection.table_names() -class DeletionGoodDeleteSimple(BaseSafeMigrationTest): +class DeletionModelGoodDeleteSimple(BaseSafeMigrationTest): app = "good_flow_delete_simple_app" migrate_from = "0001" migrate_to = "0003" @@ -302,3 +303,134 @@ def test(self): 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 DeletionFieldBadDeleteWithoutPendingTest(BaseSafeMigrationTest): + app = "bad_flow_delete_field_without_pending_app" + migrate_from = "0001" + migrate_to = "0002" + + def test(self): + with pytest.raises( + UnsafeOperationException, + match="Field must be in the pending deletion state before full deletion", + ): + self.run_migration() + + +class DeletionFieldBadDeleteDoublePendingTest(BaseSafeMigrationTest): + app = "bad_flow_delete_field_double_pending_app" + migrate_from = "0001" + migrate_to = "0003" + + def test(self): + with pytest.raises( + FieldDoesNotExist, + match="TestTable has no field named 'field'", + ): + self.run_migration() + + +class DeletionFieldBadDeletePendingWithFKConstraint(BaseSafeMigrationTest): + app = "bad_flow_delete_field_pending_with_fk_constraint_app" + migrate_from = "0001" + migrate_to = "0002" + + def test(self): + with pytest.raises( + UnsafeOperationException, + match="Foreign key db constraint must be removed before dropping " + "bad_flow_delete_field_pending_with_fk_constraint_app.testtable.fk_table", + ): + self.run_migration() + + +class DeletionFieldBadDeletePendingWithNotNull(BaseSafeMigrationTest): + app = "bad_flow_delete_field_pending_with_not_null_app" + migrate_from = "0001" + migrate_to = "0002" + + def test(self): + with pytest.raises( + UnsafeOperationException, + match="Field bad_flow_delete_field_pending_with_not_null_app.testtable.field " + "must either be nullable or have a db_default before dropping", + ): + self.run_migration() + + +class ColExistsMixin: + app = "" + + def col_exists(self, col_name): + with connection.cursor() as cursor: + table_name = f"{self.app}_testtable" + columns = connection.introspection.get_table_description(cursor, table_name) + return any(c for c in columns if c.name == col_name) + + +class DeletionFieldGoodDeletePendingWithFKConstraint(BaseSafeMigrationTest, ColExistsMixin): + app = "good_flow_delete_field_pending_with_fk_constraint_app" + migrate_from = "0001" + migrate_to = "0003" + + def test(self): + self._run_migration(self.app, "0001_initial") + assert self.col_exists("fk_table_id") + self._run_migration(self.app, "0002_remove_constraints_and_pending") + assert self.col_exists("fk_table_id") + self._run_migration(self.app, "0003_delete") + assert not self.col_exists("fk_table_id") + + +class DeletionFieldGoodDeletePendingWithNotNull(BaseSafeMigrationTest, ColExistsMixin): + app = "good_flow_delete_field_pending_with_not_null_app" + migrate_from = "0001" + migrate_to = "0003" + + def test(self): + self._run_migration(self.app, "0001_initial") + assert self.col_exists("field") + self._run_migration(self.app, "0002_remove_not_null_and_pending") + assert self.col_exists("field") + self._run_migration(self.app, "0003_delete") + assert not self.col_exists("field") + + +class DeletionFieldGoodDeleteSimple(BaseSafeMigrationTest, ColExistsMixin): + app = "good_flow_delete_field_simple_app" + migrate_from = "0001" + migrate_to = "0003" + + def test(self): + self._run_migration(self.app, "0001_initial") + assert self.col_exists("field") + self._run_migration(self.app, "0002_set_pending") + assert self.col_exists("field") + self._run_migration(self.app, "0003_delete") + assert not self.col_exists("field") + + +class DeletionFieldGoodDeleteSimpleLockTimeoutTest(BaseSafeMigrationTest): + app = "good_flow_delete_field_simple_app" + migrate_from = "0001" + migrate_to = "0003" + + def test(self): + with override_settings( + ZERO_DOWNTIME_MIGRATIONS_LOCK_TIMEOUT="5s", + ZERO_DOWNTIME_MIGRATIONS_STATEMENT_TIMEOUT="5s", + ): + self._run_migration(self.app, "0001_initial") + migration_sql = self.sql_migrate(self.app, "0002_set_pending") + assert split_sql_queries(migration_sql) == [] + self._run_migration(self.app, "0002_set_pending") + migration_sql = self.sql_migrate(self.app, "0003_delete") + # This should set both the lock and statement timeouts + assert split_sql_queries(migration_sql) == [ + "SET statement_timeout TO '5s';", + "SET lock_timeout TO '5s';", + 'ALTER TABLE "good_flow_delete_field_simple_app_testtable" DROP COLUMN "field" CASCADE;', + "SET statement_timeout TO '0ms';", + "SET lock_timeout TO '0ms';", + ] From 27538293681b2ba48134cc1cbd5c3c33927a506c Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Thu, 21 Nov 2024 16:04:01 -0800 Subject: [PATCH 2/3] small fixes --- src/sentry/new_migrations/monkey/fields.py | 27 +++++++++++++++------- src/sentry/new_migrations/monkey/state.py | 16 +++++++++---- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/src/sentry/new_migrations/monkey/fields.py b/src/sentry/new_migrations/monkey/fields.py index 337d8fc4455371..119e64fb8dafcd 100644 --- a/src/sentry/new_migrations/monkey/fields.py +++ b/src/sentry/new_migrations/monkey/fields.py @@ -3,7 +3,8 @@ from django.db.models.fields import NOT_PROVIDED 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 IGNORED_ATTRS = ["verbose_name", "help_text", "choices"] original_deconstruct = Field.deconstruct @@ -22,13 +23,11 @@ def deconstruct(self): class SafeRemoveField(RemoveField): - 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: field = state.apps.get_model(app_label, self.model_name_lower)._meta.get_field( self.name_lower @@ -48,7 +47,13 @@ def state_forwards(self, app_label, state): app_label, self.model_name_lower, 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 @@ -56,14 +61,20 @@ def database_forwards(self, app_label, schema_editor, from_state, to_state): if self.allow_migrate_model(schema_editor.connection.alias, field.model): schema_editor.remove_field(field.model, field, 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 field = to_state.get_pending_deletion_field(app_label, self.model_name, self.name) if self.allow_migrate_model(schema_editor.connection.alias, field.model): schema_editor.add_field(field.model, field) - def describe(self): + def describe(self) -> str: if self.deletion_action == DeletionAction.MOVE_TO_PENDING: return f"Moved {self.model_name}.{self.name} field to pending deletion state" else: diff --git a/src/sentry/new_migrations/monkey/state.py b/src/sentry/new_migrations/monkey/state.py index 5c665583d65442..aa78cf1b3569a1 100644 --- a/src/sentry/new_migrations/monkey/state.py +++ b/src/sentry/new_migrations/monkey/state.py @@ -4,7 +4,7 @@ from enum import Enum from django.db.migrations.state import ProjectState -from django.db.models import Model +from django.db.models import Field, Model from django_zero_downtime_migrations.backends.postgres.schema import UnsafeOperationException @@ -17,7 +17,7 @@ class SentryProjectState(ProjectState): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.pending_deletion_models: dict[tuple[str, str], type[Model]] = {} - self.pending_deletion_fields = {} + self.pending_deletion_fields: dict[tuple[str, str, str], type[Field]] = {} def get_pending_deletion_model(self, app_label: str, model_name: str) -> type[Model]: model_key = (app_label.lower(), model_name.lower()) @@ -28,7 +28,9 @@ def get_pending_deletion_model(self, app_label: str, model_name: str) -> type[Mo ) return self.pending_deletion_models[model_key] - def get_pending_deletion_field(self, app_label: str, model_name: str, field_name: str): + def get_pending_deletion_field( + self, app_label: str, model_name: str, field_name: str + ) -> type[Field]: field_key = (app_label.lower(), model_name.lower(), field_name.lower()) if field_key not in self.pending_deletion_fields: raise UnsafeOperationException( @@ -59,9 +61,13 @@ def remove_model( super().remove_model(app_label, model_name) def remove_field( - self, app_label, model_name, name, deletion_action: DeletionAction | None = None + self, + app_label: str, + model_name: str, + name: str, + deletion_action: DeletionAction | None = None, ): - field_key = app_label, model_name, name + field_key = app_label.lower(), model_name.lower(), name.lower() if deletion_action == DeletionAction.DELETE: if field_key not in self.pending_deletion_fields: raise UnsafeOperationException( From b745fa415d40834652e9a77a57f0b6cb623228df Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 22 Nov 2024 10:27:30 -0800 Subject: [PATCH 3/3] fix tests --- src/sentry/new_migrations/monkey/__init__.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sentry/new_migrations/monkey/__init__.py b/src/sentry/new_migrations/monkey/__init__.py index d13eec497e52b1..1f581c9e0a6323 100644 --- a/src/sentry/new_migrations/monkey/__init__.py +++ b/src/sentry/new_migrations/monkey/__init__.py @@ -1,5 +1,4 @@ from django import VERSION -from django.db import models from sentry.new_migrations.monkey.executor import SentryMigrationExecutor from sentry.new_migrations.monkey.fields import deconstruct @@ -81,6 +80,8 @@ class Migration(CheckedMigration): def monkey_migrations(): + from django.db import models + # This import needs to be below the other imports for `executor` and `writer` so # that we can successfully monkeypatch them. from django.db.migrations import executor, migration, writer