diff --git a/alerts/migrations/0002_alter_messagingserviceconfig_project.py b/alerts/migrations/0002_alter_messagingserviceconfig_project.py new file mode 100644 index 0000000..dad1812 --- /dev/null +++ b/alerts/migrations/0002_alter_messagingserviceconfig_project.py @@ -0,0 +1,23 @@ +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + # Django came up with 0014, whatever the reason, I'm sure that 0013 is at least required (as per comments there) + ("projects", "0014_alter_projectmembership_project"), + ("alerts", "0001_initial"), + ] + + operations = [ + migrations.AlterField( + model_name="messagingserviceconfig", + name="project", + field=models.ForeignKey( + on_delete=django.db.models.deletion.DO_NOTHING, + related_name="service_configs", + to="projects.project", + ), + ), + ] diff --git a/alerts/models.py b/alerts/models.py index 2d452c5..e7eec32 100644 --- a/alerts/models.py +++ b/alerts/models.py @@ -5,7 +5,7 @@ from .service_backends.slack import SlackBackend class MessagingServiceConfig(models.Model): - project = models.ForeignKey(Project, on_delete=models.CASCADE, related_name="service_configs") + project = models.ForeignKey(Project, on_delete=models.DO_NOTHING, related_name="service_configs") display_name = models.CharField(max_length=100, blank=False, help_text='For display in the UI, e.g. "#general on company Slack"') diff --git a/events/migrations/0022_alter_event_project.py b/events/migrations/0022_alter_event_project.py new file mode 100644 index 0000000..86a04e9 --- /dev/null +++ b/events/migrations/0022_alter_event_project.py @@ -0,0 +1,21 @@ +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + # Django came up with 0014, whatever the reason, I'm sure that 0013 is at least required (as per comments there) + ("projects", "0014_alter_projectmembership_project"), + ("events", "0021_alter_do_nothing"), + ] + + operations = [ + migrations.AlterField( + model_name="event", + name="project", + field=models.ForeignKey( + on_delete=django.db.models.deletion.DO_NOTHING, to="projects.project" + ), + ), + ] diff --git a/events/models.py b/events/models.py index d24fbb6..680b2c5 100644 --- a/events/models.py +++ b/events/models.py @@ -82,7 +82,7 @@ class Event(models.Model): # uuid4 clientside". In any case, we just rely on the envelope's event_id (required per the envelope spec). # Not a primary key: events may be duplicated across projects event_id = models.UUIDField(primary_key=False, null=False, editable=False, help_text="As per the sent data") - project = models.ForeignKey(Project, blank=False, null=True, on_delete=models.SET_NULL) # SET_NULL: cleanup 'later' + project = models.ForeignKey(Project, blank=False, null=False, on_delete=models.DO_NOTHING) data = models.TextField(blank=False, null=False) diff --git a/issues/migrations/0025_alter_grouping_project_alter_issue_project.py b/issues/migrations/0025_alter_grouping_project_alter_issue_project.py new file mode 100644 index 0000000..5c49f63 --- /dev/null +++ b/issues/migrations/0025_alter_grouping_project_alter_issue_project.py @@ -0,0 +1,28 @@ +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + # Django came up with 0014, whatever the reason, I'm sure that 0013 is at least required (as per comments there) + ("projects", "0014_alter_projectmembership_project"), + ("issues", "0024_turningpoint_project_alter_not_null"), + ] + + operations = [ + migrations.AlterField( + model_name="grouping", + name="project", + field=models.ForeignKey( + on_delete=django.db.models.deletion.DO_NOTHING, to="projects.project" + ), + ), + migrations.AlterField( + model_name="issue", + name="project", + field=models.ForeignKey( + on_delete=django.db.models.deletion.DO_NOTHING, to="projects.project" + ), + ), + ] diff --git a/issues/models.py b/issues/models.py index 634fbe1..150e03f 100644 --- a/issues/models.py +++ b/issues/models.py @@ -35,7 +35,7 @@ class Issue(models.Model): id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False) project = models.ForeignKey( - "projects.Project", blank=False, null=True, on_delete=models.SET_NULL) # SET_NULL: cleanup 'later' + "projects.Project", blank=False, null=False, on_delete=models.DO_NOTHING) is_deleted = models.BooleanField(default=False) @@ -213,7 +213,7 @@ class Grouping(models.Model): into a single issue. (such manual merging is not yet implemented, but the data-model is already prepared for it) """ project = models.ForeignKey( - "projects.Project", blank=False, null=True, on_delete=models.SET_NULL) # SET_NULL: cleanup 'later' + "projects.Project", blank=False, null=False, on_delete=models.DO_NOTHING) grouping_key = models.TextField(blank=False, null=False) diff --git a/projects/migrations/0013_delete_objects_pointing_to_null_project.py b/projects/migrations/0013_delete_objects_pointing_to_null_project.py new file mode 100644 index 0000000..aee41be --- /dev/null +++ b/projects/migrations/0013_delete_objects_pointing_to_null_project.py @@ -0,0 +1,48 @@ +from django.db import migrations + + +def delete_objects_pointing_to_null_project(apps, schema_editor): + # Up until now, we have various models w/ .project=FK(null=True, on_delete=models.SET_NULL) + # Although it is "not expected" in the interface, project-deletion would have led to those + # objects with a null project. We're about to change that to .project=FK(null=False, ...) which + # would crash if we don't remove those objects first. Object-removal is "fine" though, because + # as per the meaning of the SET_NULL, these objects were "dangling" anyway. + + # We implement this as a _single_ cross-app migration so that reasoning about the order of deletions is easy (and + # we can just copy the correct order from the project/tasks.py `preferred` variable. This cross-appness does mean + # that we must specify all dependencies here, and all the set-null migrations (from various apps) must point at this + # migration as their dependency. + + # from tasks.py, but in "strings" form + preferred = [ + 'tags.EventTag', + 'tags.IssueTag', + 'tags.TagValue', + 'tags.TagKey', + # 'issues.TurningPoint', # not needed, .project is already not-null (we just added it) + 'events.Event', + 'issues.Grouping', + # 'alerts.MessagingServiceConfig', was CASCADE (not null), so no deletion needed + # 'projects.ProjectMembership', was CASCADE (not null), so no deletion needed + 'releases.Release', + 'issues.Issue', + ] + + for model_name in preferred: + model = apps.get_model(*model_name.split('.')) + model.objects.filter(project__isnull=True).delete() + + +class Migration(migrations.Migration): + + dependencies = [ + ("projects", "0012_project_is_deleted"), + ("issues", "0024_turningpoint_project_alter_not_null"), + ("tags", "0004_alter_do_nothing"), + ("releases", "0002_release_releases_re_sort_ep_5c07c8_idx"), + ("events", "0021_alter_do_nothing"), + ] + + operations = [ + migrations.RunPython(delete_objects_pointing_to_null_project, reverse_code=migrations.RunPython.noop), + ] diff --git a/projects/migrations/0014_alter_projectmembership_project.py b/projects/migrations/0014_alter_projectmembership_project.py new file mode 100644 index 0000000..cd13986 --- /dev/null +++ b/projects/migrations/0014_alter_projectmembership_project.py @@ -0,0 +1,19 @@ +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ("projects", "0013_delete_objects_pointing_to_null_project"), + ] + + operations = [ + migrations.AlterField( + model_name="projectmembership", + name="project", + field=models.ForeignKey( + on_delete=django.db.models.deletion.DO_NOTHING, to="projects.project" + ), + ), + ] diff --git a/projects/models.py b/projects/models.py index eb1f0df..ac8c367 100644 --- a/projects/models.py +++ b/projects/models.py @@ -175,7 +175,7 @@ class Project(models.Model): class ProjectMembership(models.Model): - project = models.ForeignKey(Project, on_delete=models.CASCADE) + project = models.ForeignKey(Project, on_delete=models.DO_NOTHING) user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.CASCADE) send_email_alerts = models.BooleanField(default=None, null=True) diff --git a/projects/tests.py b/projects/tests.py index f8b3ff5..182a0a0 100644 --- a/projects/tests.py +++ b/projects/tests.py @@ -66,7 +66,7 @@ class ProjectDeletionTestCase(TransactionTestCase): # correct for bugsink/transaction.py's select_for_update for non-sqlite databases correct_for_select_for_update = 1 if 'sqlite' not in settings.DATABASES['default']['ENGINE'] else 0 - with self.assertNumQueries(40 + correct_for_select_for_update): + with self.assertNumQueries(29 + correct_for_select_for_update): self.project.delete_deferred() # tests run w/ TASK_ALWAYS_EAGER, so in the below we can just check the database directly diff --git a/releases/migrations/0003_alter_release_project.py b/releases/migrations/0003_alter_release_project.py new file mode 100644 index 0000000..b2f3ed3 --- /dev/null +++ b/releases/migrations/0003_alter_release_project.py @@ -0,0 +1,21 @@ +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + # Django came up with 0014, whatever the reason, I'm sure that 0013 is at least required (as per comments there) + ("projects", "0014_alter_projectmembership_project"), + ("releases", "0002_release_releases_re_sort_ep_5c07c8_idx"), + ] + + operations = [ + migrations.AlterField( + model_name="release", + name="project", + field=models.ForeignKey( + on_delete=django.db.models.deletion.DO_NOTHING, to="projects.project" + ), + ), + ] diff --git a/releases/models.py b/releases/models.py index a103db8..5d57288 100644 --- a/releases/models.py +++ b/releases/models.py @@ -44,8 +44,7 @@ class Release(models.Model): # sentry does releases per-org; we don't follow that example. our belief is basically: [1] in reality releases are # per software package and a software package is basically a bugsink project and [2] any cross-project-per-org # analysis you might do is more likely to be in the realm of "transactions", something we don't want to support. - project = models.ForeignKey( - "projects.Project", blank=False, null=True, on_delete=models.SET_NULL) # SET_NULL: cleanup 'later' + project = models.ForeignKey("projects.Project", blank=False, null=False, on_delete=models.DO_NOTHING) # full version as provided by either implicit (per-event) or explicit (some API) means, including package name # max_length matches Even.release (which is deduced from Sentry) diff --git a/releases/tests.py b/releases/tests.py index 89bdc31..b91652e 100644 --- a/releases/tests.py +++ b/releases/tests.py @@ -1,13 +1,16 @@ from django.test import TestCase as DjangoTestCase from datetime import timedelta +from projects.models import Project from .models import Release, ordered_releases, RE_PACKAGE_VERSION class ReleaseTestCase(DjangoTestCase): def test_create_and_order(self): - r0 = Release.objects.create(version="e80f98923f7426a8087009f4c629d25a35565a6a") + project = Project.objects.create(name="Test Project") + + r0 = Release.objects.create(project=project, version="e80f98923f7426a8087009f4c629d25a35565a6a") self.assertFalse(r0.is_semver) self.assertEqual(0, r0.sort_epoch) @@ -17,6 +20,7 @@ class ReleaseTestCase(DjangoTestCase): # real usage too) # * it ensures that dates are ignored when comparing r1 and r2 (r2 has a smaller date than r1, but comes later) r1 = Release.objects.create( + project=project, version="2a678dbbbecd2978ccaa76c326a0fb2e70073582", date_released=r0.date_released + timedelta(seconds=10), ) @@ -24,17 +28,17 @@ class ReleaseTestCase(DjangoTestCase): self.assertEqual(0, r1.sort_epoch) # switch to semver, epoch 1 - r2 = Release.objects.create(version="1.0.0") + r2 = Release.objects.create(project=project, version="1.0.0") self.assertTrue(r2.is_semver) self.assertEqual(1, r2.sort_epoch) # stick with semver, but use a lower version - r3 = Release.objects.create(version="0.1.0") + r3 = Release.objects.create(project=project, version="0.1.0") self.assertTrue(r3.is_semver) self.assertEqual(1, r3.sort_epoch) # put in package name; this is basically ignored for ordering purposes - r4 = Release.objects.create(version="package@2.0.0") + r4 = Release.objects.create(project=project, version="package@2.0.0") self.assertTrue(r4.is_semver) self.assertEqual(ordered_releases(), [r0, r1, r3, r2, r4]) diff --git a/tags/migrations/0005_alter_eventtag_project_alter_issuetag_project_and_more.py b/tags/migrations/0005_alter_eventtag_project_alter_issuetag_project_and_more.py new file mode 100644 index 0000000..d04aba4 --- /dev/null +++ b/tags/migrations/0005_alter_eventtag_project_alter_issuetag_project_and_more.py @@ -0,0 +1,42 @@ +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + # Django came up with 0014, whatever the reason, I'm sure that 0013 is at least required (as per comments there) + ("projects", "0014_alter_projectmembership_project"), + ("tags", "0004_alter_do_nothing"), + ] + + operations = [ + migrations.AlterField( + model_name="eventtag", + name="project", + field=models.ForeignKey( + on_delete=django.db.models.deletion.DO_NOTHING, to="projects.project" + ), + ), + migrations.AlterField( + model_name="issuetag", + name="project", + field=models.ForeignKey( + on_delete=django.db.models.deletion.DO_NOTHING, to="projects.project" + ), + ), + migrations.AlterField( + model_name="tagkey", + name="project", + field=models.ForeignKey( + on_delete=django.db.models.deletion.DO_NOTHING, to="projects.project" + ), + ), + migrations.AlterField( + model_name="tagvalue", + name="project", + field=models.ForeignKey( + on_delete=django.db.models.deletion.DO_NOTHING, to="projects.project" + ), + ), + ] diff --git a/tags/models.py b/tags/models.py index ee195c6..97a4eb2 100644 --- a/tags/models.py +++ b/tags/models.py @@ -36,7 +36,7 @@ from tags.utils import deduce_tags, is_mostly_unique class TagKey(models.Model): - project = models.ForeignKey(Project, blank=False, null=True, on_delete=models.SET_NULL) # SET_NULL: cleanup 'later' + project = models.ForeignKey(Project, blank=False, null=False, on_delete=models.DO_NOTHING) key = models.CharField(max_length=32, blank=False, null=False) # Tags that are "mostly unique" are not displayed in the issue tag counts, because the distribution of values is @@ -54,7 +54,7 @@ class TagKey(models.Model): class TagValue(models.Model): - project = models.ForeignKey(Project, blank=False, null=True, on_delete=models.SET_NULL) # SET_NULL: cleanup 'later' + project = models.ForeignKey(Project, blank=False, null=False, on_delete=models.DO_NOTHING) key = models.ForeignKey(TagKey, blank=False, null=False, on_delete=models.DO_NOTHING) value = models.CharField(max_length=200, blank=False, null=False, db_index=True) @@ -69,7 +69,7 @@ class TagValue(models.Model): class EventTag(models.Model): - project = models.ForeignKey(Project, blank=False, null=True, on_delete=models.SET_NULL) # SET_NULL: cleanup 'later' + project = models.ForeignKey(Project, blank=False, null=False, on_delete=models.DO_NOTHING) # value already implies key in our current setup. value = models.ForeignKey(TagValue, blank=False, null=False, on_delete=models.DO_NOTHING) @@ -106,7 +106,7 @@ class EventTag(models.Model): class IssueTag(models.Model): - project = models.ForeignKey(Project, blank=False, null=True, on_delete=models.SET_NULL) # SET_NULL: cleanup 'later' + project = models.ForeignKey(Project, blank=False, null=False, on_delete=models.DO_NOTHING) # denormalization that allows for a single-table-index for efficient search. key = models.ForeignKey(TagKey, blank=False, null=False, on_delete=models.DO_NOTHING)