ref(backup): Remove ManyToMany dependencies (#54480)

The `sorted_dependencies` function has, since time immemorial, tried to
track all `ManyToManyField` dependencies without a `through` model set,
since there is otherwise no way to deduce the dependency relationship
for the shadow junction tables Django creates under the hood. Except...
we actually don't have any `ManyToManyField` definitions without an
explicit `through` argument. Maybe we did when this code was written,
but today we no longer do.

The removed bits of code were thus never really executed, since they
were checking for a state of affairs that wasn't really a problem in our
actual code base. They were also buggy: the comment said they were
checking for `through`-ness, but there was no code to this effect,
resulting in double dependencies.

This change removes the check altogether, and adds an invariant test to
ensure that all `ManyToManyField`s defined in the future carry a
`through` argument.

Issue: getsentry/team-ospo#171

fixtures/backup/model_dependencies/flat.json

@@ -9,7 +9,6 @@
   "sentry.AlertRule": [
     "sentry.Actor",
     "sentry.Organization",
-    "sentry.Project",
     "sentry.SnubaQuery"
   ],
   "sentry.AlertRuleActivity": [
@@ -20,8 +19,7 @@
     "sentry.Project"
   ],
   "sentry.AlertRuleTrigger": [
-    "sentry.AlertRule",
-    "sentry.Incident"
+    "sentry.AlertRule"
   ],
   "sentry.AlertRuleTriggerAction": [
     "sentry.AlertRuleTrigger"
@@ -86,9 +84,7 @@
   "sentry.CommitFileChange": [
     "sentry.Commit"
   ],
-  "sentry.ControlFile": [
-    "sentry.ControlFileBlob"
-  ],
+  "sentry.ControlFile": [],
   "sentry.ControlFileBlob": [],
   "sentry.ControlFileBlobIndex": [
     "sentry.ControlFile",
@@ -104,8 +100,7 @@
     "sentry.Project"
   ],
   "sentry.Dashboard": [
-    "sentry.Organization",
-    "sentry.Project"
+    "sentry.Organization"
   ],
   "sentry.DashboardProject": [
     "sentry.Dashboard",
@@ -130,8 +125,7 @@
     "sentry.Release"
   ],
   "sentry.DiscoverSavedQuery": [
-    "sentry.Organization",
-    "sentry.Project"
+    "sentry.Organization"
   ],
   "sentry.DiscoverSavedQueryProject": [
     "sentry.DiscoverSavedQuery",
@@ -145,9 +139,7 @@
     "sentry.DocIntegration"
   ],
   "sentry.Email": [],
-  "sentry.Environment": [
-    "sentry.Project"
-  ],
+  "sentry.Environment": [],
   "sentry.EnvironmentProject": [
     "sentry.Environment",
     "sentry.Project"
@@ -277,8 +269,7 @@
   "sentry.IdentityProvider": [],
   "sentry.Incident": [
     "sentry.AlertRule",
-    "sentry.Organization",
-    "sentry.Project"
+    "sentry.Organization"
   ],
   "sentry.IncidentActivity": [
     "sentry.Incident"
@@ -324,8 +315,7 @@
   ],
   "sentry.MonitorLocation": [],
   "sentry.NotificationAction": [
-    "sentry.Organization",
-    "sentry.Project"
+    "sentry.Organization"
   ],
   "sentry.NotificationActionProject": [
     "sentry.NotificationAction",
@@ -351,8 +341,7 @@
   ],
   "sentry.OrganizationMapping": [],
   "sentry.OrganizationMember": [
-    "sentry.Organization",
-    "sentry.Team"
+    "sentry.Organization"
   ],
   "sentry.OrganizationMemberMapping": [
     "sentry.User"
@@ -386,8 +375,7 @@
     "sentry.ProjectDebugFile"
   ],
   "sentry.Project": [
-    "sentry.Organization",
-    "sentry.Team"
+    "sentry.Organization"
   ],
   "sentry.ProjectArtifactBundle": [
     "sentry.ArtifactBundle"
@@ -461,8 +449,7 @@
   "sentry.Relay": [],
   "sentry.RelayUsage": [],
   "sentry.Release": [
-    "sentry.Organization",
-    "sentry.Project"
+    "sentry.Organization"
   ],
   "sentry.ReleaseActivity": [
     "sentry.Release"
@@ -588,9 +575,7 @@
     "sentry.User"
   ],
   "sentry.UserReport": [],
-  "sentry.UserRole": [
-    "sentry.User"
-  ],
+  "sentry.UserRole": [],
   "sentry.UserRoleUser": [
     "sentry.User",
     "sentry.UserRole"

fixtures/backup/model_dependencies/sorted.json

@@ -21,12 +21,14 @@
   "sentry.DeletedProject",
   "sentry.DeletedTeam",
   "sentry.Email",
+  "sentry.Environment",
   "sentry.EventAttachment",
   "sentry.EventUser",
   "sentry.GroupCommitResolution",
   "sentry.GroupRedirect",
   "sentry.GroupRelease",
   "sentry.Organization",
+  "sentry.Release",
   "sentry.IdentityProvider",
   "sentry.DocIntegration",
   "sentry.ExternalActor",
@@ -40,15 +42,29 @@
   "sentry.OrganizationMapping",
   "sentry.OrganizationMemberMapping",
   "sentry.OrgAuthToken",
+  "sentry.SnubaQuery",
+  "sentry.SnubaQueryEventType",
+  "sentry.Project",
+  "sentry.ProjectBookmark",
+  "sentry.ProjectKey",
+  "sentry.ProjectOwnership",
   "sentry.ProjectPlatform",
+  "sentry.ProjectRedirect",
   "sentry.PromptsActivity",
   "sentry.PullRequest",
   "sentry.PullRequestComment",
+  "sentry.RawEvent",
   "sentry.RecentSearch",
   "sentry.RelayUsage",
   "sentry.Relay",
+  "sentry.ReleaseActivity",
+  "sentry.ReleaseEnvironment",
   "sentry.ReleaseFile",
+  "sentry.ReleaseProjectEnvironment",
   "sentry.Repository",
+  "sentry.ReprocessingReport",
+  "sentry.Rule",
+  "sentry.RuleActivity",
   "sentry.SavedSearch",
   "sentry.ScheduledDeletion",
   "sentry.RegionScheduledDeletion",
@@ -56,15 +72,24 @@
   "sentry.ServiceHook",
   "sentry.RegionTombstone",
   "sentry.ControlTombstone",
+  "sentry.ProjectTransactionThresholdOverride",
+  "sentry.ProjectTransactionThreshold",
   "sentry.UserEmail",
   "sentry.UserIP",
   "sentry.UserPermission",
   "sentry.UserReport",
   "sentry.UserRole",
   "sentry.UserRoleUser",
+  "sentry.NotificationAction",
   "sentry.TimeSeriesSnapshot",
+  "sentry.AlertRule",
+  "sentry.AlertRuleTrigger",
+  "sentry.AlertRuleTriggerAction",
+  "sentry.AlertRuleActivity",
+  "sentry.DiscoverSavedQuery",
   "sentry.Monitor",
   "sentry.MonitorLocation",
+  "sentry.MonitorEnvironment",
   "sentry.MetricsKeyIndexer",
   "sentry.StringIndexer",
   "sentry.PerfStringIndexer",
@@ -73,17 +98,44 @@
   "nodestore.Node",
   "replays.ReplayRecordingSegment",
   "social_auth.UserSocialAuth",
+  "sentry.MonitorCheckIn",
+  "sentry.DiscoverSavedQueryProject",
+  "sentry.AlertRuleExcludedProjects",
+  "sentry.Incident",
+  "sentry.IncidentSeen",
+  "sentry.IncidentProject",
+  "sentry.NotificationActionProject",
   "sentry.ServiceHookProject",
+  "sentry.RuleSnooze",
+  "sentry.QuerySubscription",
+  "sentry.ProcessingIssue",
+  "sentry.OrganizationOnboardingTask",
   "sentry.LostPasswordHash",
+  "sentry.LatestAppConnectBuildsCheck",
+  "sentry.RepositoryProjectPathConfig",
+  "sentry.ProjectIntegration",
   "sentry.OrganizationIntegration",
   "sentry.Identity",
+  "sentry.GroupTombstone",
+  "sentry.ReleaseProject",
+  "sentry.OrganizationMember",
   "sentry.Team",
+  "sentry.OrganizationMemberTeam",
+  "sentry.Group",
+  "sentry.GroupHistory",
   "sentry.FeatureAdoption",
+  "sentry.EnvironmentProject",
+  "sentry.Distribution",
+  "sentry.Deploy",
   "sentry.DashboardTombstone",
+  "sentry.Dashboard",
+  "sentry.DashboardProject",
+  "sentry.Counter",
   "sentry.Commit",
   "sentry.BroadcastSeen",
   "sentry.UserAvatar",
   "sentry.TeamAvatar",
+  "sentry.ProjectAvatar",
   "sentry.OrganizationAvatar",
   "sentry.DocIntegrationAvatar",
   "sentry.FileBlobIndex",
@@ -96,9 +148,12 @@
   "sentry.AssistantActivity",
   "sentry.ArtifactBundleFlatFileIndex",
   "sentry.ArtifactBundle",
+  "sentry.AppConnectBuild",
   "sentry.ApiApplication",
   "sentry.UserOption",
+  "sentry.ProjectOption",
   "sentry.OrganizationOption",
+  "sentry.Activity",
   "sentry.ApiAuthorization",
   "sentry.ApiGrant",
   "sentry.ApiToken",
@@ -108,54 +163,6 @@
   "sentry.DebugIdArtifactBundle",
   "sentry.ProjectArtifactBundle",
   "sentry.CommitFileChange",
-  "sentry.OrganizationMember",
-  "sentry.SentryApp",
-  "sentry.PagerDutyService",
-  "sentry.SentryAppComponent",
-  "sentry.SentryAppInstallation",
-  "sentry.SentryAppInstallationForProvider",
-  "sentry.SentryAppInstallationToken",
-  "sentry.OrganizationAccessRequest",
-  "sentry.Project",
-  "sentry.ProjectBookmark",
-  "sentry.ProjectKey",
-  "sentry.ProjectOwnership",
-  "sentry.ProjectRedirect",
-  "sentry.PullRequestCommit",
-  "sentry.RawEvent",
-  "sentry.ReprocessingReport",
-  "sentry.Rule",
-  "sentry.RuleActivity",
-  "sentry.ProjectTransactionThresholdOverride",
-  "sentry.ProjectTransactionThreshold",
-  "sentry.NotificationAction",
-  "sentry.DiscoverSavedQuery",
-  "sentry.DiscoverSavedQueryProject",
-  "sentry.NotificationActionProject",
-  "sentry.ProjectTeam",
-  "sentry.ProcessingIssue",
-  "sentry.OrganizationOnboardingTask",
-  "sentry.LatestAppConnectBuildsCheck",
-  "sentry.RepositoryProjectPathConfig",
-  "sentry.ProjectIntegration",
-  "sentry.GroupTombstone",
-  "sentry.Release",
-  "sentry.ReleaseProject",
-  "sentry.OrganizationMemberTeam",
-  "sentry.Group",
-  "sentry.GroupHistory",
-  "sentry.Environment",
-  "sentry.EnvironmentProject",
-  "sentry.Distribution",
-  "sentry.Deploy",
-  "sentry.Dashboard",
-  "sentry.DashboardProject",
-  "sentry.Counter",
-  "sentry.SentryAppAvatar",
-  "sentry.ProjectAvatar",
-  "sentry.AppConnectBuild",
-  "sentry.ProjectOption",
-  "sentry.Activity",
   "sentry.DashboardWidget",
   "sentry.GroupOwner",
   "sentry.GroupAssignee",
@@ -172,35 +179,28 @@
   "sentry.GroupShare",
   "sentry.GroupSnooze",
   "sentry.GroupSubscription",
+  "sentry.SentryApp",
+  "sentry.PagerDutyService",
+  "sentry.SentryAppComponent",
+  "sentry.SentryAppInstallation",
+  "sentry.SentryAppInstallationForProvider",
+  "sentry.SentryAppInstallationToken",
+  "sentry.OrganizationAccessRequest",
   "sentry.PlatformExternalIssue",
   "sentry.EventProcessingIssue",
-  "sentry.SnubaQuery",
-  "sentry.SnubaQueryEventType",
-  "sentry.QuerySubscription",
+  "sentry.ProjectTeam",
   "sentry.ProjectCodeOwners",
-  "sentry.ReleaseActivity",
+  "sentry.PullRequestCommit",
   "sentry.ReleaseCommit",
-  "sentry.ReleaseEnvironment",
   "sentry.ReleaseHeadCommit",
-  "sentry.ReleaseProjectEnvironment",
   "sentry.RuleFireHistory",
-  "sentry.AlertRule",
-  "sentry.AlertRuleActivity",
-  "sentry.TeamKeyTransaction",
-  "sentry.MonitorEnvironment",
-  "sentry.MonitorCheckIn",
-  "sentry.AlertRuleExcludedProjects",
-  "sentry.Incident",
-  "sentry.IncidentSeen",
-  "sentry.IncidentProject",
-  "sentry.RuleSnooze",
-  "sentry.DashboardWidgetQuery",
   "sentry.PendingIncidentSnapshot",
   "sentry.IncidentSnapshot",
   "sentry.IncidentActivity",
   "sentry.IncidentSubscription",
-  "sentry.AlertRuleTrigger",
+  "sentry.IncidentTrigger",
   "sentry.AlertRuleTriggerExclusion",
-  "sentry.AlertRuleTriggerAction",
-  "sentry.IncidentTrigger"
+  "sentry.TeamKeyTransaction",
+  "sentry.DashboardWidgetQuery",
+  "sentry.SentryAppAvatar"
 ]

src/sentry/backup/dependencies.py

@@ -4,7 +4,7 @@ from enum import Enum, auto, unique
 from typing import NamedTuple
 
 from django.db import models
-from django.db.models.fields.related import ForeignKey, ManyToManyField, OneToOneField
+from django.db.models.fields.related import ForeignKey, OneToOneField
 
 from sentry.backup.helpers import EXCLUDED_APPS
 from sentry.silo import SiloMode
@@ -24,9 +24,6 @@ class ForeignFieldKind(Enum):
     # Uses our `OneToOneCascadeDeletes` wrapper.
     OneToOneCascadeDeletes = auto()
 
-    # A naked usage of Django's `ManyToManyField`.
-    ManyToManyField = auto()
-
     # A naked usage of Django's `ForeignKey`.
     DefaultForeignKey = auto()
 
@@ -45,13 +42,13 @@ class ModelRelations(NamedTuple):
     """What other models does this model depend on, and how?"""
 
     model: models.base.ModelBase
-    relations: dict[str, ForeignField]
+    foreign_keys: dict[str, ForeignField]
     silos: list[SiloMode]
 
     def flatten(self) -> set[models.base.ModelBase]:
         """Returns a flat list of all related models, omitting the kind of relation they have."""
 
-        return {ff.model for ff in self.relations.values()}
+        return {ff.model for ff in self.foreign_keys.values()}
 
 
 def normalize_model_name(model):
@@ -95,7 +92,7 @@ def dependencies() -> dict[str, ModelRelations]:
         model_iterator = app_config.get_models()
 
         for model in model_iterator:
-            relations: dict[str, ForeignField] = dict()
+            foreign_keys: dict[str, ForeignField] = dict()
 
             # Now add a dependency for any FK relation with a model that defines a natural key.
             for field in model._meta.fields:
@@ -108,34 +105,22 @@ def dependencies() -> dict[str, ModelRelations]:
                         continue
 
                     if isinstance(field, FlexibleForeignKey):
-                        relations[field.name] = ForeignField(
+                        foreign_keys[field.name] = ForeignField(
                             model=rel_model,
                             kind=ForeignFieldKind.FlexibleForeignKey,
                         )
                     elif isinstance(field, HybridCloudForeignKey):
-                        relations[field.name] = ForeignField(
+                        foreign_keys[field.name] = ForeignField(
                             model=rel_model,
                             kind=ForeignFieldKind.HybridCloudForeignKey,
                         )
                     elif isinstance(field, ForeignKey):
-                        relations[field.name] = ForeignField(
+                        foreign_keys[field.name] = ForeignField(
                             model=rel_model,
                             kind=ForeignFieldKind.DefaultForeignKey,
                         )
 
-            # Also add a dependency for any simple M2M relation.
-            many_to_many_fields = [
-                field for field in model._meta.get_fields() if isinstance(field, ManyToManyField)
-            ]
-            for field in many_to_many_fields:
-                rel_model = getattr(field.remote_field, "model", None)
-                if rel_model is not None and rel_model != model:
-                    relations[field.name] = ForeignField(
-                        model=rel_model,
-                        kind=ForeignFieldKind.ManyToManyField,
-                    )
-
-            # Finally, get all simple O2O relations as well.
+            # Get all simple O2O relations as well.
             one_to_one_fields = [
                 field for field in model._meta.get_fields() if isinstance(field, OneToOneField)
             ]
@@ -143,12 +128,12 @@ def dependencies() -> dict[str, ModelRelations]:
                 rel_model = getattr(field.remote_field, "model", None)
                 if rel_model is not None and rel_model != model:
                     if isinstance(field, OneToOneCascadeDeletes):
-                        relations[field.name] = ForeignField(
+                        foreign_keys[field.name] = ForeignField(
                             model=rel_model,
                             kind=ForeignFieldKind.OneToOneCascadeDeletes,
                         )
                     elif isinstance(field, OneToOneField):
-                        relations[field.name] = ForeignField(
+                        foreign_keys[field.name] = ForeignField(
                             model=rel_model,
                             kind=ForeignFieldKind.DefaultOneToOneField,
                         )
@@ -157,7 +142,7 @@ def dependencies() -> dict[str, ModelRelations]:
 
             model_dependencies_list[normalize_model_name(model)] = ModelRelations(
                 model=model,
-                relations=relations,
+                foreign_keys=foreign_keys,
                 silos=list(
                     getattr(model._meta, "silo_limit", ModelSiloLimit(SiloMode.MONOLITH)).modes
                 ),

tests/sentry/backup/test_coverage.py

@@ -1,6 +1,6 @@
 from __future__ import annotations
 
-from sentry.backup.helpers import get_exportable_final_derivations_of, get_final_derivations_of
+from sentry.backup.helpers import get_exportable_final_derivations_of
 from sentry.db.models import BaseModel
 from tests.sentry.backup.test_models import UNIT_TESTED_MODELS
 from tests.sentry.backup.test_releases import RELEASE_TESTED_MODELS
@@ -8,18 +8,6 @@ from tests.sentry.backup.test_releases import RELEASE_TESTED_MODELS
 ALL_EXPORTABLE_MODELS = {c.__name__ for c in get_exportable_final_derivations_of(BaseModel)}
 
 
-# Note: this gets checked at runtime, but better to avoid possible runtime errors and catch it early
-# in CI.
-def test_all_final_derivations_of_django_model_set_included_in_export():
-    missing = set(
-        filter(
-            lambda c: not hasattr(c, "__include_in_export__"),
-            get_final_derivations_of(BaseModel),
-        )
-    )
-    assert not missing
-
-
 def test_exportable_final_derivations_of_django_model_are_unit_tested():
     untested = ALL_EXPORTABLE_MODELS - UNIT_TESTED_MODELS
     assert not untested

tests/sentry/backup/test_invariants.py

@@ -0,0 +1,38 @@
+from __future__ import annotations
+
+from django.db.models.fields.related import ManyToManyField
+
+from sentry.backup.helpers import get_exportable_final_derivations_of, get_final_derivations_of
+from sentry.db.models import BaseModel
+
+
+# Note: this gets checked at runtime, but better to avoid possible runtime errors and catch it early
+# in CI.
+def test_all_final_derivations_of_django_model_set_included_in_export():
+    missing = set(
+        filter(
+            lambda c: not hasattr(c, "__include_in_export__"),
+            get_final_derivations_of(BaseModel),
+        )
+    )
+    assert not missing
+
+
+def test_all_many_to_many_fields_explicitly_set_through_attribute():
+    # Make sure we are visiting the field definitions correctly.
+    visited = 0
+
+    for model in get_exportable_final_derivations_of(BaseModel):
+        many_to_many_fields = [
+            field for field in model._meta.get_fields() if isinstance(field, ManyToManyField)
+        ]
+        for field in many_to_many_fields:
+            if field.remote_field.through is not None and field.remote_field.through._meta:
+                if field.remote_field.through._meta.auto_created:
+                    raise AssertionError(
+                        f"""{model!r} model has a `ManyToManyField` field, "{field.name}", that does not set an explicit `through=...` junction model."""
+                    )
+                else:
+                    visited += 1
+
+    assert visited > 0