fix(backup): Fix various small import and comparison bugs (#60593)

For this PR, we fix the following:

1. No longer crash when comparing `None` with a nonexistent field.
Previously, this behavior was correct when comparing either of a `None`
or missing field to itself, or a present field, but not when comparing
with one another.
2. Remove existence check for `IgnoredComparator`. This makes no sense:
if we are ignoring the field for comparison purposes, we should not
raise findings when one side is missing.
3. Ignore country and/or region code re-assignments. Since we are
accepting whatever codes are assigned by the geo IP handler on the
receiving side, we should not raise alerts if this modules comes to a
different result than the exporter. Instead, we should quietly accept
the new value, and ignore the fields during comparison.
4. Allow null `expires_at` and `refresh_token` for the `ApiToken` model.
Previously, we auto-assigned these fields. But we've since encountered
instances where these are left null on purpose, so we should not mutate
this apparently valid persisted state at import time.
5. Handle unordered list fields when comparing. This is relevant
specifically to the `scope_list` field of `ApiToken`, which seems to get
the order of members shuffled a bit.

Author: azaslavsky

src/sentry/backup/comparators.py

@@ -19,7 +19,8 @@
 
 
 class ScrubbedData:
-    """A singleton class used to indicate data has been scrubbed, without indicating what that data is. A unit type indicating "scrubbing was successful" only."""
+    """A singleton class used to indicate data has been scrubbed, without indicating what that data
+    is. A unit type indicating "scrubbing was successful" only."""
 
     instance: ScrubbedData
 
@@ -70,9 +71,11 @@ def existence(self, on: InstanceID, left: JSONData, right: JSONData) -> list[Com
 
         findings = []
         for f in self.fields:
-            if f not in left["fields"] and f not in right["fields"]:
+            missing_on_left = f not in left["fields"] or left["fields"][f] is None
+            missing_on_right = f not in right["fields"] or right["fields"][f] is None
+            if missing_on_left and missing_on_right:
                 continue
-            if f not in left["fields"]:
+            if missing_on_left:
                 findings.append(
                     ComparatorFinding(
                         kind=self.get_kind_existence_check(),
@@ -82,7 +85,7 @@ def existence(self, on: InstanceID, left: JSONData, right: JSONData) -> list[Com
                         reason=f"the left `{f}` value was missing",
                     )
                 )
-            if f not in right["fields"]:
+            if missing_on_right:
                 findings.append(
                     ComparatorFinding(
                         kind=self.get_kind_existence_check(),
@@ -461,13 +464,20 @@ def truncate(self, data: list[str]) -> list[str]:
 class IgnoredComparator(JSONScrubbingComparator):
     """Ensures that two fields are tested for mutual existence, and nothing else.
 
-    Using this class means that you are foregoing comparing the relevant field(s), so please make sure you are validating them some other way!"""
+    Using this class means that you are foregoing comparing the relevant field(s), so please make
+    sure you are validating them some other way!
+    """
 
-    def compare(self, on: InstanceID, left: JSONData, right: JSONData) -> list[ComparatorFinding]:
+    def compare(self, _o: InstanceID, _l: JSONData, _r: JSONData) -> list[ComparatorFinding]:
         """Noop - there is nothing to compare once we've checked for existence."""
 
         return []
 
+    def existence(self, _o: InstanceID, _l: JSONData, _r: JSONData) -> list[ComparatorFinding]:
+        """Noop - never compare existence for ignored fields, they're ignored after all."""
+
+        return []
+
 
 class RegexComparator(JSONScrubbingComparator, ABC):
     """Comparator that ensures that both sides match a certain regex."""
@@ -517,7 +527,8 @@ def __init__(self, bytes: int, *fields: str):
 
 
 class SubscriptionIDComparator(RegexComparator):
-    """Compare the basic format of `QuerySubscription` IDs, which is basically a UUID1 with a numeric prefix. Ensure that the two values are NOT equivalent."""
+    """Compare the basic format of `QuerySubscription` IDs, which is basically a UUID1 with a
+    numeric prefix. Ensure that the two values are NOT equivalent."""
 
     def __init__(self, *fields: str):
         super().__init__(re.compile("^\\d+/[0-9a-f]{32}$"), *fields)
@@ -549,11 +560,38 @@ def compare(self, on: InstanceID, left: JSONData, right: JSONData) -> list[Compa
         return findings
 
 
+class UnorderedListComparator(JSONScrubbingComparator):
+    """Comparator for fields that are lists of unordered elements, which simply orders them before
+    doing the comparison."""
+
+    def compare(self, on: InstanceID, left: JSONData, right: JSONData) -> list[ComparatorFinding]:
+        findings = []
+        fields = sorted(self.fields)
+        for f in fields:
+            if left["fields"].get(f) is None and right["fields"].get(f) is None:
+                continue
+
+            lv = left["fields"][f] or []
+            rv = right["fields"][f] or []
+            if sorted(lv) != sorted(rv):
+                findings.append(
+                    ComparatorFinding(
+                        kind=self.get_kind(),
+                        on=on,
+                        left_pk=left["pk"],
+                        right_pk=right["pk"],
+                        reason=f"""the left value ({lv}) of the unordered list field `{f}` was not equal to the right value ({rv})""",
+                    )
+                )
+        return findings
+
+
 # Note: we could also use the `uuid` Python uuid module for this, but it is finicky and accepts some
 # weird syntactic variations that are not very common and may cause weird failures when they are
 # rejected elsewhere.
 class UUID4Comparator(RegexComparator):
-    """UUIDs must be regenerated on import (otherwise they would not be unique...). This comparator ensures that they retain their validity, but are not equivalent."""
+    """UUIDs must be regenerated on import (otherwise they would not be unique...). This comparator
+    ensures that they retain their validity, but are not equivalent."""
 
     def __init__(self, *fields: str):
         super().__init__(
@@ -590,7 +628,8 @@ def compare(self, on: InstanceID, left: JSONData, right: JSONData) -> list[Compa
 
 
 def auto_assign_datetime_equality_comparators(comps: ComparatorMap) -> None:
-    """Automatically assigns the DateAddedComparator to any `DateTimeField` that is not already claimed by the `DateUpdatedComparator`."""
+    """Automatically assigns the DateAddedComparator to any `DateTimeField` that is not already
+    claimed by the `DateUpdatedComparator`."""
 
     exportable = get_exportable_sentry_models()
     for e in exportable:
@@ -616,7 +655,8 @@ def auto_assign_datetime_equality_comparators(comps: ComparatorMap) -> None:
 
 
 def auto_assign_email_obfuscating_comparators(comps: ComparatorMap) -> None:
-    """Automatically assigns the EmailObfuscatingComparator to any field that is an `EmailField` or has a foreign key into the `sentry.User` table."""
+    """Automatically assigns the EmailObfuscatingComparator to any field that is an `EmailField` or
+    has a foreign key into the `sentry.User` table."""
 
     exportable = get_exportable_sentry_models()
     for e in exportable:
@@ -667,7 +707,8 @@ def get_default_comparators():
         list,
         {
             "sentry.apitoken": [
-                HashObfuscatingComparator("refresh_token", "token", "token_last_characters")
+                HashObfuscatingComparator("refresh_token", "token", "token_last_characters"),
+                UnorderedListComparator("scope_list"),
             ],
             "sentry.apiapplication": [HashObfuscatingComparator("client_id", "client_secret")],
             "sentry.authidentity": [HashObfuscatingComparator("ident", "token")],
@@ -716,7 +757,13 @@ def get_default_comparators():
                 DateUpdatedComparator("date_hash_added"),
                 IgnoredComparator("validation_hash", "is_verified"),
             ],
-            "sentry.userip": [DateUpdatedComparator("first_seen", "last_seen")],
+            "sentry.userip": [
+                DateUpdatedComparator("first_seen", "last_seen"),
+                # Incorrect country and region codes may be updated during an import, so we don't
+                # want to compare them explicitly. This update is pulled from the geo IP service, so
+                # we only really want to compare the IP address itself.
+                IgnoredComparator("country_code", "region_code"),
+            ],
             "sentry.userrole": [DateUpdatedComparator("date_updated")],
             "sentry.userroleuser": [DateUpdatedComparator("date_updated")],
         },

src/sentry/backup/findings.py

@@ -92,10 +92,6 @@ class ComparatorFindingKind(FindingKind):
     # Failed to compare an ignored field.
     IgnoredComparator = auto()
 
-    # Failed to compare an ignored field because one of the fields being compared was not present or
-    # `None`.
-    IgnoredComparatorExistenceCheck = auto()
-
     # Secret token fields did not match their regex specification.
     SecretHexComparator = auto()
 
@@ -110,6 +106,13 @@ class ComparatorFindingKind(FindingKind):
     # present or `None`.
     SubscriptionIDComparatorExistenceCheck = auto()
 
+    # Unordered list fields did not match.
+    UnorderedListComparator = auto()
+
+    # Failed to compare a unordered list field because one of the fields being compared was not
+    # present or `None`.
+    UnorderedListComparatorExistenceCheck = auto()
+
     # UUID4 fields did not match their regex specification.
     UUID4Comparator = auto()
 

src/sentry/models/apitoken.py

@@ -120,9 +120,11 @@ def write_relocation_import(
         query = models.Q(token=self.token) | models.Q(refresh_token=self.refresh_token)
         existing = self.__class__.objects.filter(query).first()
         if existing:
-            self.expires_at = timezone.now() + DEFAULT_EXPIRATION
             self.token = generate_token()
-            self.refresh_token = generate_token()
+            if self.refresh_token is not None:
+                self.refresh_token = generate_token()
+            if self.expires_at is not None:
+                self.expires_at = timezone.now() + DEFAULT_EXPIRATION
 
         return super().write_relocation_import(scope, flags)
 

src/sentry/runner/commands/backup.py

@@ -172,7 +172,7 @@ def get_encryptor_from_flags(
 
 def write_findings(findings_file: TextIO | None, findings: Sequence[Finding], printer: Callable):
     for f in findings:
-        printer(f.pretty(), err=True)
+        printer(f"\n\n{f.pretty()}", err=True)
 
     if findings_file:
         findings_encoder = FindingJSONEncoder(
@@ -310,11 +310,11 @@ def load_data(
 
     res = validate(left_data, right_data, get_default_comparators())
     if res:
+        click.echo(f"\n\nDone, found {len(res.findings)} differences:")
         write_findings(findings_file, res.findings, click.echo)
-        click.echo(f"Done, found {len(res.findings)} differences:\n\n{res.pretty()}")
     else:
+        click.echo("\n\nDone, found 0 differences!")
         write_findings(findings_file, [], click.echo)
-        click.echo("Done, found 0 differences!")
 
 
 @backup.command(name="decrypt")