Browse Source

feat(backup): Add EqualOrRemovedComparator (#62699)

Recently, we've added a couple of import handlers where we selectively
null out fields based on certain conditions (see: PRs #62665 and
#62526). This commit adds comparator support for this condition,
allowing us to define comparators that say "make sure these fields are
either equal or have been removed on the right side".
Alex Zaslavsky 1 year ago
parent
commit
57bbfd4459

+ 63 - 2
src/sentry/backup/comparators.py

@@ -519,6 +519,58 @@ class RegexComparator(JSONScrubbingComparator, ABC):
         return findings
 
 
+class EqualOrRemovedComparator(JSONScrubbingComparator):
+    """
+    A normal equality comparison, except that it allows the right-side value to be `None` or
+    missing.
+    """
+
+    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
+            if right["fields"].get(f) is None:
+                continue
+
+            lv = left["fields"][f]
+            rv = right["fields"][f]
+            if lv != 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 `{f}` was not equal to the right value ("{rv}")""",
+                    )
+                )
+
+        return findings
+
+    def existence(self, on: InstanceID, left: JSONData, right: JSONData) -> list[ComparatorFinding]:
+        """Ensure that all tracked fields on either both models or neither."""
+
+        findings = []
+        for f in self.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 missing_on_left:
+                findings.append(
+                    ComparatorFinding(
+                        kind=self.get_kind_existence_check(),
+                        on=on,
+                        left_pk=left["pk"],
+                        right_pk=right["pk"],
+                        reason=f"the left `{f}` value was missing",
+                    )
+                )
+        return findings
+
+
 class SecretHexComparator(RegexComparator):
     """Certain 16-byte hexadecimal API keys are regenerated during an import operation."""
 
@@ -712,7 +764,13 @@ def get_default_comparators():
             ],
             "sentry.apiapplication": [HashObfuscatingComparator("client_id", "client_secret")],
             "sentry.authidentity": [HashObfuscatingComparator("ident", "token")],
-            "sentry.alertrule": [DateUpdatedComparator("date_modified")],
+            "sentry.alertrule": [
+                DateUpdatedComparator("date_modified"),
+                # TODO(hybrid-cloud): actor refactor. Remove this check once we're sure we've
+                # migrated all remaining `owner_id`'s to also have `team_id` or `user_id`, which
+                # seems to not be the case today.
+                EqualOrRemovedComparator("owner", "team", "user_id"),
+            ],
             "sentry.incident": [UUID4Comparator("detection_uuid")],
             "sentry.incidentactivity": [UUID4Comparator("notification_uuid")],
             "sentry.incidenttrigger": [DateUpdatedComparator("date_modified")],
@@ -723,7 +781,10 @@ def get_default_comparators():
             ],
             "sentry.organization": [AutoSuffixComparator("slug")],
             "sentry.organizationintegration": [DateUpdatedComparator("date_updated")],
-            "sentry.organizationmember": [HashObfuscatingComparator("token")],
+            "sentry.organizationmember": [
+                HashObfuscatingComparator("token"),
+                EqualOrRemovedComparator("inviter_id"),
+            ],
             "sentry.projectkey": [
                 HashObfuscatingComparator("public_key", "secret_key"),
                 SecretHexComparator(16, "public_key", "secret_key"),

+ 6 - 0
src/sentry/backup/findings.py

@@ -75,6 +75,12 @@ class ComparatorFindingKind(FindingKind):
     # `None`.
     EmailObfuscatingComparatorExistenceCheck = auto()
 
+    # The fields were both present but unequal.
+    EqualOrRemovedComparator = auto()
+
+    # The left field does not exist.
+    EqualOrRemovedComparatorExistenceCheck = auto()
+
     # Hash equality comparison failed.
     HashObfuscatingComparator = auto()
 

+ 185 - 0
tests/sentry/backup/test_comparators.py

@@ -7,6 +7,7 @@ from sentry.backup.comparators import (
     DatetimeEqualityComparator,
     DateUpdatedComparator,
     EmailObfuscatingComparator,
+    EqualOrRemovedComparator,
     ForeignKeyComparator,
     HashObfuscatingComparator,
     IgnoredComparator,
@@ -546,6 +547,190 @@ def test_good_email_obfuscating_comparator_scrubbed():
     ]
 
 
+def test_good_equal_or_removed_comparator_equal():
+    cmp = EqualOrRemovedComparator("my_field")
+    id = InstanceID("sentry.test", 0)
+    present: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "my_field": "foo",
+        },
+    }
+
+    assert not cmp.existence(id, present, present)
+    assert not cmp.compare(id, present, present)
+
+
+def test_good_equal_or_removed_comparator_not_equal():
+    cmp = EqualOrRemovedComparator("my_field")
+    id = InstanceID("sentry.test", 0)
+    left: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "my_field": "foo",
+        },
+    }
+    right: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "my_field": "bar",
+        },
+    }
+
+    assert not cmp.existence(id, left, right)
+
+    res = cmp.compare(id, left, right)
+    assert res
+    assert len(res) == 1
+
+    assert res[0]
+    assert res[0].kind == ComparatorFindingKind.EqualOrRemovedComparator
+    assert res[0].on == id
+    assert res[0].left_pk == 1
+    assert res[0].right_pk == 1
+    assert "my_field" in res[0].reason
+    assert "foo" in res[0].reason
+    assert "bar" in res[0].reason
+
+
+def test_good_equal_or_removed_comparator_neither_side_existing():
+    cmp = EqualOrRemovedComparator("my_field")
+    id = InstanceID("sentry.test", 0)
+    missing: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {},
+    }
+    assert not cmp.existence(id, missing, missing)
+
+
+def test_good_equal_or_removed_comparator_only_right_side_missing():
+    cmp = EqualOrRemovedComparator("my_field")
+    id = InstanceID("sentry.test", 0)
+    present: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "my_field": "foo",
+        },
+    }
+    missing: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {},
+    }
+    assert not cmp.existence(id, present, missing)
+    assert not cmp.compare(id, present, missing)
+
+
+def test_bad_equal_or_removed_comparator_only_left_side_missing():
+    cmp = EqualOrRemovedComparator("my_field")
+    id = InstanceID("sentry.test", 0)
+    present: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "my_field": "foo",
+        },
+    }
+    missing: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {},
+    }
+    res = cmp.existence(id, missing, present)
+    assert res
+    assert len(res) == 1
+
+    assert res[0]
+    assert res[0].on == id
+    assert res[0].kind == ComparatorFindingKind.EqualOrRemovedComparatorExistenceCheck
+    assert res[0].left_pk == 1
+    assert res[0].right_pk == 1
+    assert "left" in res[0].reason
+    assert "my_field" in res[0].reason
+
+
+def test_good_equal_or_removed_comparator_both_sides_nulled():
+    cmp = EqualOrRemovedComparator("my_field")
+    id = InstanceID("sentry.test", 0)
+    nulled: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "my_field": None,
+        },
+    }
+    assert not cmp.existence(id, nulled, nulled)
+
+
+def test_good_equal_or_removed_comparator_only_right_side_nulled():
+    cmp = EqualOrRemovedComparator("my_field")
+    id = InstanceID("sentry.test", 0)
+    present: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "my_field": "foo",
+        },
+    }
+    missing: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "my_field": None,
+        },
+    }
+    assert not cmp.existence(id, present, missing)
+    assert not cmp.compare(id, present, missing)
+
+
+def test_bad_equal_or_removed_comparator_only_left_side_nulled():
+    cmp = EqualOrRemovedComparator("my_field")
+    id = InstanceID("sentry.test", 0)
+    present: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "my_field": "foo",
+        },
+    }
+    missing: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "my_field": None,
+        },
+    }
+    res = cmp.existence(id, missing, present)
+    assert res
+    assert len(res) == 1
+
+    assert res[0]
+    assert res[0].on == id
+    assert res[0].kind == ComparatorFindingKind.EqualOrRemovedComparatorExistenceCheck
+    assert res[0].left_pk == 1
+    assert res[0].right_pk == 1
+    assert "left" in res[0].reason
+    assert "my_field" in res[0].reason
+
+
 def test_good_hash_obfuscating_comparator():
     cmp = HashObfuscatingComparator("one_hash", "many_hashes")
     id = InstanceID("sentry.test", 0)