ref(backup): Filter options on comparison rather than export

Author: azaslavsky

src/sentry/backup/comparators.py

@@ -211,6 +211,42 @@ def compare(self, on: InstanceID, left: JSONData, right: JSONData) -> list[Compa
         return findings
 
 
+class KeyValueComparator(JSONScrubbingComparator):
+    """
+    Models of the kind `sentry.*option` store key-value pairs of options. Some of these keys and/or
+    values need special-handling to be compared properly. Because these key-value pairs have no
+    schema, we more or less have to resort to listing keys we want to skip.
+    """
+
+    def __init__(self, *, skip_keys: list[str]):
+        super().__init__("key", "value")
+        self.skip_keys = skip_keys
+
+    def compare(self, on: InstanceID, left: JSONData, right: JSONData) -> list[ComparatorFinding]:
+        # Keys must be equal, since we use them as part of the `InstanceID`.
+        assert left["fields"]["key"] == right["fields"]["key"]
+
+        findings: list[ComparatorFinding] = []
+        key = left["fields"]["key"]
+        if key in self.skip_keys:
+            return findings
+
+        left_value = left["fields"]["value"]
+        right_value = right["fields"]["value"]
+        if left_value != right_value:
+            findings.append(
+                ComparatorFinding(
+                    kind=self.get_kind(),
+                    on=on,
+                    left_pk=left["pk"],
+                    right_pk=right["pk"],
+                    reason=f"""the left value ({left_value}) of a key-value entry with key `{key}` was not equal to the right({right_value})""",
+                )
+            )
+
+        return findings
+
+
 class DateUpdatedComparator(JSONScrubbingComparator):
     """
     Comparator that ensures that the specified fields' value on the right input is an ISO-8601
@@ -866,6 +902,16 @@ def get_default_comparators() -> dict[str, list[JSONScrubbingComparator]]:
             ],
             "sentry.dashboardwidgetqueryondemand": [DateUpdatedComparator("date_modified")],
             "sentry.dashboardwidgetquery": [DateUpdatedComparator("date_modified")],
+            "sentry.option": [
+                KeyValueComparator(
+                    skip_keys=[
+                        "sentry:install-id",  # Only used on self-hosted.
+                        "sentry:latest_version",  # Should not necessarily invalidate comparison.
+                        "sentry:last_worker_ping",  # Changes very frequently.
+                        "sentry:last_worker_version",  # Changes very frequently.
+                    ]
+                ),
+            ],
             "sentry.organization": [AutoSuffixComparator("slug")],
             "sentry.organizationintegration": [DateUpdatedComparator("date_updated")],
             "sentry.organizationmember": [

src/sentry/backup/findings.py

@@ -100,6 +100,13 @@ class ComparatorFindingKind(FindingKind):
     # Failed to compare an ignored field.
     IgnoredComparator = auto()
 
+    # A key-value pair was unequal
+    KeyValueComparator = auto()
+
+    # Failed to compare a key-value pair because one side of the comparison had either the `key` or
+    # `value` field missing.
+    KeyValueComparatorExistenceCheck = auto()
+
     # Secret token fields did not match their regex specification.
     SecretHexComparator = auto()
 

src/sentry/models/options/option.py

@@ -5,7 +5,6 @@
 from django.db import models
 from django.utils import timezone
 
-from sentry.backup.dependencies import PrimaryKeyMap
 from sentry.backup.mixins import OverwritableConfigMixin
 from sentry.backup.scopes import RelocationScope
 from sentry.db.models import (
@@ -47,19 +46,6 @@ class Meta:
 
     __repr__ = sane_repr("key", "value")
 
-    @classmethod
-    def query_for_relocation_export(cls, q: models.Q, pk_map: PrimaryKeyMap) -> models.Q:
-        # These ping options change too frequently, or necessarily with each install, to be useful
-        # in exports. More broadly, we don't really care about comparing them for accuracy.
-        return q & ~models.Q(
-            key__in={
-                "sentry:install-id",  # Only used on self-hosted
-                "sentry:latest_version",  # Auto-generated periodically, which defeats comparison
-                "sentry:last_worker_ping",  # Changes very frequently
-                "sentry:last_worker_version",  # Changes very frequently
-            }
-        )
-
 
 @region_silo_only_model
 class Option(BaseOption):

tests/sentry/backup/snapshots/test_comparators/test_default_comparators.pysnap

@@ -882,6 +882,13 @@ source: tests/sentry/backup/test_comparators.py
     - user
   model_name: sentry.notificationsettingprovider
 - comparators:
+  - class: DateUpdatedComparator
+    fields:
+    - last_updated
+  - class: KeyValueComparator
+    fields:
+    - key
+    - value
   - class: ForeignKeyComparator
     fields: []
   model_name: sentry.option

tests/sentry/backup/test_comparators.py

@@ -11,6 +11,7 @@
     ForeignKeyComparator,
     HashObfuscatingComparator,
     IgnoredComparator,
+    KeyValueComparator,
     ScrubbedData,
     SecretHexComparator,
     SubscriptionIDComparator,
@@ -2146,6 +2147,143 @@ def test_good_user_password_obfuscating_comparator_scrubbed_short():
     assert right["scrubbed"]["UserPasswordObfuscatingComparator::password"] == ["..."]
 
 
+def test_good_key_value_comparator():
+    cmp = KeyValueComparator(skip_keys=[])
+    id = InstanceID("sentry.test", 0)
+    model = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "key": "foo",
+            "value": 1,
+        },
+    }
+    assert not cmp.compare(id, model, model)
+
+
+def test_good_key_value_skip_comparator():
+    cmp = KeyValueComparator(skip_keys=["foo"])
+    id = InstanceID("sentry.test", 0)
+    left: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "key": "foo",
+            "value": 1,
+        },
+    }
+    right: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "key": "foo",
+            "value": 2,
+        },
+    }
+    assert not cmp.compare(id, left, right)
+
+
+def test_bad_key_value_comparator():
+    cmp = KeyValueComparator(skip_keys=["bar"])
+    id = InstanceID("sentry.test", 0)
+    left: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "key": "foo",
+            "value": 1,
+        },
+    }
+    right: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "key": "foo",
+            "value": 2,
+        },
+    }
+    res = cmp.compare(id, left, right)
+    assert res
+    assert len(res) == 1
+
+    assert res[0]
+    assert res[0].kind == ComparatorFindingKind.KeyValueComparator
+    assert res[0].on == id
+    assert res[0].left_pk == 1
+    assert res[0].right_pk == 1
+    assert "`foo`" in res[0].reason
+    assert "1" in res[0].reason
+    assert "2" in res[0].reason
+
+
+def test_good_key_value_comparator_existence():
+    cmp = KeyValueComparator(skip_keys=["bar"])
+    id = InstanceID("sentry.test", 0)
+    present: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "key": "foo",
+            "value": 1,
+        },
+    }
+    missing: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "key": "foo",
+        },
+    }
+    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.KeyValueComparatorExistenceCheck
+    assert res[0].left_pk == 1
+    assert res[0].right_pk == 1
+    assert "left" in res[0].reason
+    assert "`value`" in res[0].reason
+
+
+def test_good_key_value_comparator_scrubbed():
+    cmp = KeyValueComparator(skip_keys=["bar"])
+    left: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "key": "foo",
+            "value": 1,
+        },
+    }
+    right: JSONData = {
+        "model": "test",
+        "ordinal": 1,
+        "pk": 1,
+        "fields": {
+            "key": "foo",
+            "value": 2,
+        },
+    }
+    cmp.scrub(left, right)
+    assert left["scrubbed"]
+    assert left["scrubbed"]["KeyValueComparator::key"] is ScrubbedData()
+    assert left["scrubbed"]["KeyValueComparator::value"] is ScrubbedData()
+
+    assert right["scrubbed"]
+    assert right["scrubbed"]["KeyValueComparator::key"] is ScrubbedData()
+    assert right["scrubbed"]["KeyValueComparator::value"] is ScrubbedData()
+
+
 def test_default_comparators(insta_snapshot):
     serialized = []
     defs = get_default_comparators()

tests/sentry/backup/test_exports.py

@@ -11,7 +11,6 @@
 from sentry.backup.validate import validate
 from sentry.db import models
 from sentry.models.email import Email
-from sentry.models.options.option import Option
 from sentry.models.organization import Organization
 from sentry.models.orgauthtoken import OrgAuthToken
 from sentry.models.user import User
@@ -315,36 +314,3 @@ def test_export_keep_only_admin_users_in_config_scope(self):
             assert self.count(data, UserRole) == 1
             assert self.count(data, UserRoleUser) == 1
             assert self.count(data, UserPermission) == 1
-
-
-class QueryTests(ExportTestCase):
-    """
-    Some models have custom export logic that requires bespoke testing.
-    """
-
-    def test_export_query_for_option_model(self):
-        # There are a number of options we specifically exclude by name, for various reasons
-        # enumerated in that model's definition file.
-        Option.objects.create(key="sentry:install-id", value='"excluded"')
-        Option.objects.create(key="sentry:latest_version", value='"excluded"')
-        Option.objects.create(key="sentry:last_worker_ping", value='"excluded"')
-        Option.objects.create(key="sentry:last_worker_version", value='"excluded"')
-
-        Option.objects.create(key="sentry:test-unfiltered", value="included")
-        Option.objects.create(key="foo:bar", value='"included"')
-
-        with tempfile.TemporaryDirectory() as tmp_dir:
-            data = self.export(
-                tmp_dir,
-                scope=ExportScope.Config,
-            )
-
-            assert self.count(data, Option) == 2
-            assert not self.exists(data, Option, "key", "sentry:install-id")
-            assert not self.exists(data, Option, "key", "sentry:last_version")
-            assert not self.exists(data, Option, "key", "sentry:last_worker_ping")
-            assert not self.exists(data, Option, "key", "sentry:last_worker_version")
-            assert not self.exists(data, Option, "value", '"excluded"')
-            assert self.exists(data, Option, "key", "sentry:test-unfiltered")
-            assert self.exists(data, Option, "key", "foo:bar")
-            assert self.exists(data, Option, "value", '"included"')