feat(backup): Use model save (#55432)

Prior to this change, we used the `save()` method from the Django
serializer. But this is a "raw" save - it performs no validaion, and
triggers no signals. Instead, we now use the re-hydrated model's own
save method, which does perform these actions.

Because of this, a few tests have had to change to account for signal
triggering. In particular, `User`, `Organization`, and `Project` writes
now auto-create some models, so we account for this in tests as well.

src/sentry/backup/comparators.py

@@ -451,7 +451,11 @@ def get_default_comparators():
             "sentry.querysubscription": [DateUpdatedComparator("date_updated")],
             "sentry.relay": [HashObfuscatingComparator("relay_id", "public_key")],
             "sentry.relayusage": [HashObfuscatingComparator("relay_id", "public_key")],
-            "sentry.sentryapp": [EmailObfuscatingComparator("creator_label")],
+            "sentry.sentryapp": [
+                DateUpdatedComparator("date_updated"),
+                EmailObfuscatingComparator("creator_label"),
+            ],
+            "sentry.sentryappinstallation": [DateUpdatedComparator("date_updated")],
             "sentry.servicehook": [HashObfuscatingComparator("secret")],
             "sentry.user": [HashObfuscatingComparator("password")],
             "sentry.useremail": [

src/sentry/backup/imports.py

@@ -149,7 +149,7 @@ def _import(
                             if f.model == type(o) and getattr(o, f.field, None) not in f.values:
                                 break
                         else:
-                            written = o.write_relocation_import(pk_map, obj, scope)
+                            written = o.write_relocation_import(pk_map, scope)
                             if written is not None:
                                 old_pk, new_pk = written
                                 pk_map.insert(model_name, old_pk, new_pk)

src/sentry/backup/mixins.py

@@ -2,8 +2,6 @@
 
 from typing import Optional, Tuple
 
-from django.core.serializers.base import DeserializedObject
-
 from sentry.backup.dependencies import PrimaryKeyMap
 from sentry.backup.scopes import ImportScope
 
@@ -19,9 +17,9 @@ class SanitizeUserImportsMixin:
     """
 
     def write_relocation_import(
-        self, pk_map: PrimaryKeyMap, obj: DeserializedObject, scope: ImportScope
+        self, pk_map: PrimaryKeyMap, scope: ImportScope
     ) -> Optional[Tuple[int, int]]:
         if scope != ImportScope.Global:
             return None
 
-        return super().write_relocation_import(pk_map, obj, scope)  # type: ignore[misc]
+        return super().write_relocation_import(pk_map, scope)  # type: ignore[misc]

src/sentry/db/models/base.py

@@ -3,7 +3,6 @@
 from typing import Any, Callable, Iterable, Mapping, Optional, Tuple, Type, TypeVar
 
 from django.apps.config import AppConfig
-from django.core.serializers.base import DeserializedObject
 from django.db import models
 from django.db.models import signals
 from django.utils import timezone
@@ -139,7 +138,7 @@ def _normalize_before_relocation_import(
         return old_pk
 
     def write_relocation_import(
-        self, pk_map: PrimaryKeyMap, obj: DeserializedObject, scope: ImportScope
+        self, pk_map: PrimaryKeyMap, scope: ImportScope
     ) -> Optional[Tuple[int, int]]:
         """
         Writes a deserialized model to the database. If this write is successful, this method will return a tuple of the old and new `pk`s.
@@ -149,7 +148,7 @@ def write_relocation_import(
         if old_pk is None:
             return
 
-        obj.save(force_insert=True)
+        self.save(force_insert=True)
         return (old_pk, self.pk)
 
 

src/sentry/models/options/project_option.py

@@ -1,11 +1,12 @@
 from __future__ import annotations
 
-from typing import TYPE_CHECKING, Any, Mapping, Sequence
+from typing import TYPE_CHECKING, Any, Mapping, Optional, Sequence, Tuple
 
 from django.db import models
 
 from sentry import projectoptions
-from sentry.backup.scopes import RelocationScope
+from sentry.backup.dependencies import PrimaryKeyMap
+from sentry.backup.scopes import ImportScope, RelocationScope
 from sentry.db.models import FlexibleForeignKey, Model, region_silo_only_model, sane_repr
 from sentry.db.models.fields import PickledObjectField
 from sentry.db.models.manager import OptionManager, ValidateFunction, Value
@@ -159,3 +160,19 @@ class Meta:
         unique_together = (("project", "key"),)
 
     __repr__ = sane_repr("project_id", "key", "value")
+
+    def write_relocation_import(
+        self, pk_map: PrimaryKeyMap, scope: ImportScope
+    ) -> Optional[Tuple[int, int]]:
+        old_pk = super()._normalize_before_relocation_import(pk_map, scope)
+        if old_pk is None:
+            return None
+
+        (key, _) = self.__class__.objects.get_or_create(
+            project=self.project, key=self.key, defaults={"value": self.value}
+        )
+        if key:
+            self.pk = key.pk
+            self.save()
+
+        return (old_pk, self.pk)

src/sentry/models/projectkey.py

@@ -2,19 +2,21 @@
 
 import re
 import secrets
-from typing import Any
+from typing import Any, Optional, Tuple
 from urllib.parse import urlparse
 
 import petname
 from django.conf import settings
 from django.db import ProgrammingError, models
+from django.forms import model_to_dict
 from django.urls import reverse
 from django.utils import timezone
 from django.utils.translation import gettext_lazy as _
 
 from bitfield import TypedClassBitField
 from sentry import features, options
-from sentry.backup.scopes import RelocationScope
+from sentry.backup.dependencies import PrimaryKeyMap
+from sentry.backup.scopes import ImportScope, RelocationScope
 from sentry.db.models import (
     BaseManager,
     BoundedPositiveIntegerField,
@@ -276,3 +278,19 @@ def get_audit_log_data(self):
 
     def get_scopes(self):
         return self.scopes
+
+    def write_relocation_import(
+        self, pk_map: PrimaryKeyMap, scope: ImportScope
+    ) -> Optional[Tuple[int, int]]:
+        old_pk = super()._normalize_before_relocation_import(pk_map, scope)
+        if old_pk is None:
+            return None
+
+        (key, _) = self.__class__.objects.get_or_create(
+            project=self.project, defaults=model_to_dict(self)
+        )
+        if key:
+            self.pk = key.pk
+            self.save()
+
+        return (old_pk, self.pk)

src/sentry/models/team.py

@@ -5,7 +5,6 @@
 from typing import TYPE_CHECKING, Literal, Optional, Sequence, Tuple, Union, overload
 
 from django.conf import settings
-from django.core.serializers.base import DeserializedObject
 from django.db import IntegrityError, connections, models, router, transaction
 from django.utils import timezone
 from django.utils.translation import gettext_lazy as _
@@ -369,9 +368,9 @@ def get_member_actor_ids(self):
 
     # TODO(hybrid-cloud): actor refactor. Remove this method when done.
     def write_relocation_import(
-        self, pk_map: PrimaryKeyMap, obj: DeserializedObject, scope: ImportScope
+        self, pk_map: PrimaryKeyMap, scope: ImportScope
     ) -> Optional[Tuple[int, int]]:
-        written = super().write_relocation_import(pk_map, obj, scope)
+        written = super().write_relocation_import(pk_map, scope)
         if written is not None:
             (_, new_pk) = written
 

src/sentry/models/useremail.py

@@ -5,7 +5,6 @@
 from typing import TYPE_CHECKING, Iterable, Mapping, Optional, Tuple
 
 from django.conf import settings
-from django.core.serializers.base import DeserializedObject
 from django.db import models
 from django.forms import model_to_dict
 from django.utils import timezone
@@ -88,7 +87,7 @@ def get_primary_email(cls, user: User) -> UserEmail:
     # with `sentry.user` simultaneously? Will need to make more robust user handling logic, and to
     # test what happens when a UserEmail already exists.
     def write_relocation_import(
-        self, pk_map: PrimaryKeyMap, obj: DeserializedObject, scope: ImportScope
+        self, pk_map: PrimaryKeyMap, scope: ImportScope
     ) -> Optional[Tuple[int, int]]:
         old_pk = super()._normalize_before_relocation_import(pk_map, scope)
         if old_pk is None:

src/sentry/testutils/fixtures.py

@@ -140,7 +140,8 @@ def create_environment(self, project=None, **kwargs):
         return Factories.create_environment(project=project, **kwargs)
 
     def create_project(self, **kwargs):
-        kwargs.setdefault("teams", [self.team])
+        if "teams" not in kwargs:
+            kwargs["teams"] = [self.team]
         return Factories.create_project(**kwargs)
 
     def create_project_bookmark(self, project=None, *args, **kwargs):

src/sentry/testutils/helpers/backups.py

@@ -46,7 +46,6 @@
     DashboardWidgetQuery,
     DashboardWidgetTypes,
 )
-from sentry.models.environment import EnvironmentProject
 from sentry.models.integrations.integration import Integration
 from sentry.models.integrations.organization_integration import OrganizationIntegration
 from sentry.models.integrations.project_integration import ProjectIntegration
@@ -133,8 +132,8 @@ def clear_database(*, reset_pks: bool = False):
         with transaction.atomic(using="default"):
             reversed = reversed_dependencies()
             for model in reversed:
-                # For some reason, the tables for `SentryApp*` models don't get deleted properly here
-                # when using `model.objects.all().delete()`, so we have to call out to Postgres
+                # For some reason, the tables for `SentryApp*` models don't get deleted properly
+                # here when using `model.objects.all().delete()`, so we have to call out to Postgres
                 # manually.
                 connection = connections[router.db_for_write(SentryApp)]
                 with connection.cursor() as cursor:
@@ -285,21 +284,20 @@ def create_exhaustive_organization(
             },
         )
 
+        # Team
+        team = self.create_team(name=f"test_team_in_{slug}", organization=org)
+        self.create_team_membership(user=owner, team=team)
+        OrganizationAccessRequest.objects.create(member=invited, team=team)
+
         # Project*
-        project = self.create_project()
+        project = self.create_project(name=f"project-{slug}", teams=[team])
         self.create_project_key(project)
         self.create_project_bookmark(project=project, user=owner)
-        project = self.create_project()
         ProjectOwnership.objects.create(
             project=project, raw='{"hello":"hello"}', schema={"hello": "hello"}
         )
         ProjectRedirect.record(project, f"project_slug_in_{slug}")
 
-        # Team
-        team = self.create_team(name=f"test_team_in_{slug}", organization=org)
-        self.create_team_membership(user=owner, team=team)
-        OrganizationAccessRequest.objects.create(member=invited, team=team)
-
         # Integration*
         integration = Integration.objects.create(
             provider="slack", name=f"Slack for {slug}", external_id=f"slack:{slug}"
@@ -318,8 +316,7 @@ def create_exhaustive_organization(
         self.snooze_rule(user_id=owner_id, owner_id=owner_id, rule=rule)
 
         # Environment*
-        env = self.create_environment()
-        EnvironmentProject.objects.create(project=project, environment=env, is_hidden=False)
+        self.create_environment(project=project)
 
         # Monitor
         Monitor.objects.create(
@@ -330,12 +327,18 @@ def create_exhaustive_organization(
         )
 
         # AlertRule*
-        alert = self.create_alert_rule(include_all_projects=True, excluded_projects=[project])
-        trigger = self.create_alert_rule_trigger(alert_rule=alert, excluded_projects=[self.project])
+        other_project = self.create_project(name=f"other-project-{slug}", teams=[team])
+        alert = self.create_alert_rule(
+            organization=org,
+            projects=[project],
+            include_all_projects=True,
+            excluded_projects=[other_project],
+        )
+        trigger = self.create_alert_rule_trigger(alert_rule=alert, excluded_projects=[project])
         self.create_alert_rule_trigger_action(alert_rule_trigger=trigger)
 
         # Incident*
-        incident = self.create_incident()
+        incident = self.create_incident(org, [project])
         IncidentActivity.objects.create(
             incident=incident,
             type=1,
@@ -395,7 +398,7 @@ def create_exhaustive_organization(
         # misc
         Counter.increment(project, 1)
         self.create_repo(
-            project=self.project,
+            project=project,
             name="getsentry/getsentry",
             provider="integrations:github",
             integration_id=integration.id,

tests/sentry/backup/test_imports.py

@@ -17,8 +17,14 @@
 from sentry.backup.scopes import ExportScope, RelocationScope
 from sentry.models.authenticator import Authenticator
 from sentry.models.email import Email
+from sentry.models.options.project_option import ProjectOption
 from sentry.models.organization import Organization
+from sentry.models.organizationmapping import OrganizationMapping
+from sentry.models.organizationmember import OrganizationMember
+from sentry.models.organizationmembermapping import OrganizationMemberMapping
 from sentry.models.orgauthtoken import OrgAuthToken
+from sentry.models.project import Project
+from sentry.models.projectkey import ProjectKey
 from sentry.models.user import User
 from sentry.models.useremail import UserEmail
 from sentry.models.userip import UserIP
@@ -163,6 +169,61 @@ def export_to_tmp_file_and_clear_database(self, tmp_dir) -> Path:
         return tmp_path
 
 
+@run_backup_tests_only_on_single_db
+class SignalingTests(ImportTestCase):
+    """
+    Some models are automatically created via signals and similar automagic from related models. We test that behavior here.
+    """
+
+    def test_import_signaling_user(self):
+        self.create_exhaustive_user("user", email="[email protected]")
+
+        with tempfile.TemporaryDirectory() as tmp_dir:
+            tmp_path = self.export_to_tmp_file_and_clear_database(tmp_dir)
+            with open(tmp_path) as tmp_file:
+                import_in_user_scope(tmp_file, printer=NOOP_PRINTER)
+
+        assert User.objects.count() == 1
+        assert User.objects.filter(email="[email protected]").exists()
+
+        assert UserEmail.objects.count() == 1
+        assert UserEmail.objects.filter(email="[email protected]").exists()
+
+        assert Email.objects.count() == 1
+        assert Email.objects.filter(email="[email protected]").exists()
+
+    def test_import_signaling_organization(self):
+        owner = self.create_exhaustive_user("owner")
+        invited = self.create_exhaustive_user("invited")
+        member = self.create_exhaustive_user("member")
+        self.create_exhaustive_organization("some-org", owner, invited, [member])
+
+        with tempfile.TemporaryDirectory() as tmp_dir:
+            tmp_path = self.export_to_tmp_file_and_clear_database(tmp_dir)
+            with open(tmp_path) as tmp_file:
+                import_in_organization_scope(tmp_file, printer=NOOP_PRINTER)
+
+        assert Organization.objects.count() == 1
+        assert Organization.objects.filter(slug="some-org").exists()
+
+        assert OrganizationMapping.objects.count() == 1
+        assert OrganizationMapping.objects.filter(slug="some-org").exists()
+
+        assert OrganizationMember.objects.count() == 3
+        assert OrganizationMemberMapping.objects.count() == 3
+
+        # The exhaustive org has 2 projects which automatically get 1 key and 3 options each.
+        assert Project.objects.count() == 2
+        assert Project.objects.filter(name="project-some-org").exists()
+        assert Project.objects.filter(name="other-project-some-org").exists()
+
+        assert ProjectKey.objects.count() == 2
+        assert ProjectOption.objects.count() == 6
+        assert ProjectOption.objects.filter(key="sentry:relay-rev").exists()
+        assert ProjectOption.objects.filter(key="sentry:relay-rev-lastchange").exists()
+        assert ProjectOption.objects.filter(key="sentry:option-epoch").exists()
+
+
 @run_backup_tests_only_on_single_db
 class ScopingTests(ImportTestCase):
     """

tests/sentry/backup/test_roundtrip.py

@@ -30,19 +30,10 @@ def test_bad_unequal_json(tmp_path):
         import_export_from_fixture_then_validate(tmp_path, "fresh-install.json")
     findings = execinfo.value.info.findings
 
-    assert len(findings) == 3
+    assert len(findings) >= 3
     assert findings[0].kind == ComparatorFindingKind.UnequalJSON
-    assert findings[0].on == InstanceID("sentry.useremail", 1)
-    assert findings[0].left_pk == 1
-    assert findings[0].right_pk == 1
     assert findings[1].kind == ComparatorFindingKind.UnequalJSON
-    assert findings[1].on == InstanceID("sentry.userrole", 1)
-    assert findings[1].left_pk == 1
-    assert findings[1].right_pk == 1
     assert findings[2].kind == ComparatorFindingKind.UnequalJSON
-    assert findings[2].on == InstanceID("sentry.userroleuser", 1)
-    assert findings[2].left_pk == 1
-    assert findings[2].right_pk == 1
 
 
 @run_backup_tests_only_on_single_db