test(backup): Improve test coverage (#53937)

This change adds a few more validation tests (and fixes the bugs they
caught!).

Issue: getsentry/team-ospo#155

fixtures/backup/single-option.json

@@ -0,0 +1,20 @@
+[
+    {
+        "model": "sites.site",
+        "pk": 1,
+        "fields": {
+        "domain": "example.com",
+        "name": "example.com"
+        }
+    },
+    {
+        "model": "sentry.option",
+        "pk": 1,
+        "fields": {
+        "key": "sentry:latest_version",
+        "last_updated": "2023-06-22T00:00:00.000Z",
+        "last_updated_by": "unknown",
+        "value": "\"23.6.1\""
+        }
+    }
+]

src/sentry/backup/comparators.py

@@ -55,17 +55,17 @@ class JSONScrubbingComparator(ABC):
             if f not in left["fields"]:
                 findings.append(
                     ComparatorFinding(
-                        kind=self.get_kind(),
+                        kind="Unexecuted" + self.get_kind(),
                         on=on,
-                        reason=f"the left {f} value on `{on}` was missing",
+                        reason=f"the left `{f}` value was missing",
                     )
                 )
             if f not in right["fields"]:
                 findings.append(
                     ComparatorFinding(
-                        kind=self.get_kind(),
+                        kind="Unexecuted" + self.get_kind(),
                         on=on,
-                        reason=f"the right {f} value on `{on}` was missing",
+                        reason=f"the right `{f}` value was missing",
                     )
                 )
         return findings
@@ -214,11 +214,12 @@ class HashObfuscatingComparator(ObfuscatingComparator):
     def truncate(self, data: list[str]) -> list[str]:
         truncated = []
         for d in data:
-            if len(d) >= 16:
+            length = len(d)
+            if length >= 16:
                 truncated.append(f"{d[:3]}...{d[-3:]}")
-            elif len(d) >= 8:
+            elif length >= 8:
                 truncated.append(f"{d[:1]}...{d[-1:]}")
-            elif len(d):
+            else:
                 truncated.append("...")
         return truncated
 

src/sentry/backup/findings.py

@@ -35,6 +35,9 @@ class ComparatorFindings:
     def append(self, finding: ComparatorFinding) -> None:
         self.findings.append(finding)
 
+    def empty(self) -> bool:
+        return not self.findings
+
     def extend(self, findings: list[ComparatorFinding]) -> None:
         self.findings += findings
 

src/sentry/backup/validate.py

@@ -29,23 +29,38 @@ def validate(
     findings = ComparatorFindings([])
     exp_models = {}
     act_models = {}
-    for model in expect:
-        id = InstanceID(model["model"], model["pk"])
-        exp_models[id] = model
 
     # Because we may be scrubbing data from the objects as we compare them, we may (optionally) make
     # deep copies to start to avoid potentially mangling the input data.
     expect = deepcopy(expect)
     actual = deepcopy(actual)
 
-    # Ensure that the actual JSON contains no duplicates - we assume that the expected JSON did not.
+    # Ensure that the actual JSON contains no duplicates.
+    exp_dupes = set()
+    for model in expect:
+        id = InstanceID(model["model"], model["pk"])
+        if id in exp_models and id not in exp_dupes:
+            findings.append(
+                ComparatorFinding("DuplicateEntry", id, "Duplicate entry in expected output")
+            )
+            exp_dupes.add(id)
+        else:
+            exp_models[id] = model
+    act_dupes = set()
     for model in actual:
         id = InstanceID(model["model"], model["pk"])
-        if id in act_models:
-            findings.append(ComparatorFinding("DuplicateEntry", id))
+        if id in act_models and id not in act_dupes:
+            findings.append(
+                ComparatorFinding("DuplicateEntry", id, "Duplicate entry in actual output")
+            )
+            act_dupes.add(id)
         else:
             act_models[id] = model
 
+    # If there are duplicate entries, something is seriously wrong, so abort ASAP.
+    if not findings.empty():
+        return findings
+
     # Report unexpected and missing entries in the actual JSON.
     extra = sorted(act_models.keys() - exp_models.keys())
     missing = sorted(exp_models.keys() - act_models.keys())

tests/sentry/backup/test_comparators.py

@@ -50,7 +50,7 @@ def test_bad_comparator_only_one_side_existing():
     assert res
     assert res[0]
     assert res[0].on == id
-    assert res[0].kind == "DateUpdatedComparator"
+    assert res[0].kind == "UnexecutedDateUpdatedComparator"
     assert "left" in res[0].reason
     assert "my_date_field" in res[0].reason
 
@@ -58,7 +58,7 @@ def test_bad_comparator_only_one_side_existing():
     assert res
     assert res[0]
     assert res[0].on == id
-    assert res[0].kind == "DateUpdatedComparator"
+    assert res[0].kind == "UnexecutedDateUpdatedComparator"
     assert "right" in res[0].reason
     assert "my_date_field" in res[0].reason
 

tests/sentry/backup/test_correctness.py

@@ -1,30 +1,277 @@
+from copy import deepcopy
+
 import pytest
 
 from sentry.backup.comparators import DEFAULT_COMPARATORS
 from sentry.backup.findings import InstanceID
+from sentry.backup.validate import validate
+from sentry.testutils.factories import get_fixture_path
 from sentry.testutils.helpers.backups import (
     ValidationError,
     import_export_from_fixture_then_validate,
 )
 from sentry.testutils.pytest.fixtures import django_db_all
+from sentry.utils import json
 
 
 @django_db_all(transaction=True, reset_sequences=True)
-def test_good_fresh_install_validation(tmp_path):
+def test_good_fresh_install(tmp_path):
     import_export_from_fixture_then_validate(tmp_path, "fresh-install.json", DEFAULT_COMPARATORS)
 
 
 @django_db_all(transaction=True, reset_sequences=True)
-def test_bad_fresh_install_validation(tmp_path):
-
+def test_bad_unequal_json(tmp_path):
+    # Without the `DEFAULT_COMPARATORS`, the `date_updated` fields will not be compared correctly.
     with pytest.raises(ValidationError) as excinfo:
         import_export_from_fixture_then_validate(tmp_path, "fresh-install.json")
-    info = excinfo.value.info
-    assert len(info.findings) == 2
-    assert info.findings[0].kind == "UnequalJSON"
-    assert info.findings[0].on == InstanceID("sentry.userrole", 1)
-    assert info.findings[1].kind == "UnequalJSON"
-    assert info.findings[1].on == InstanceID("sentry.userroleuser", 1)
+    findings = excinfo.value.info.findings
+
+    assert len(findings) == 2
+    assert findings[0].kind == "UnequalJSON"
+    assert findings[0].on == InstanceID("sentry.userrole", 1)
+    assert findings[1].kind == "UnequalJSON"
+    assert findings[1].on == InstanceID("sentry.userroleuser", 1)
+
+
+@django_db_all(transaction=True, reset_sequences=True)
+def test_bad_duplicate_entry(tmp_path):
+    with open(get_fixture_path("backup", "single-option.json")) as backup_file:
+        test_json = json.load(backup_file)
+    dupe = json.loads(
+        """
+            {
+                "model": "sentry.option",
+                "pk": 1,
+                "fields": {
+                "key": "sentry:latest_version",
+                "last_updated": "2023-06-23T00:00:00.000Z",
+                "last_updated_by": "unknown",
+                "value": "\\"23.7.1\\""
+                }
+            }
+        """
+    )
+    test_json.append(dupe)
+    out = validate(test_json, test_json)
+    findings = out.findings
+
+    assert len(findings) == 2
+    assert findings[0].kind == "DuplicateEntry"
+    assert findings[0].on == InstanceID("sentry.option", 1)
+    assert "expected" in findings[0].reason
+    assert findings[1].kind == "DuplicateEntry"
+    assert findings[1].on == InstanceID("sentry.option", 1)
+    assert "actual" in findings[1].reason
+
+
+@django_db_all(transaction=True, reset_sequences=True)
+def test_bad_unexpected_entry(tmp_path):
+    with open(get_fixture_path("backup", "single-option.json")) as backup_file:
+        left = json.load(backup_file)
+    right = deepcopy(left)
+    unexpected = json.loads(
+        """
+            {
+                "model": "sentry.option",
+                "pk": 2,
+                "fields": {
+                "key": "sentry:last_worker_version",
+                "last_updated": "2023-06-23T00:00:00.000Z",
+                "last_updated_by": "unknown",
+                "value": "\\"23.7.1\\""
+                }
+            }
+        """
+    )
+    right.append(unexpected)
+    out = validate(left, right)
+    findings = out.findings
+
+    assert len(findings) == 1
+    assert findings[0].kind == "UnexpectedEntry"
+    assert findings[0].on == InstanceID("sentry.option", 2)
+
+
+@django_db_all(transaction=True, reset_sequences=True)
+def test_bad_missing_entry(tmp_path):
+    with open(get_fixture_path("backup", "single-option.json")) as backup_file:
+        left = json.load(backup_file)
+    right = deepcopy(left)
+    missing = json.loads(
+        """
+            {
+                "model": "sentry.option",
+                "pk": 2,
+                "fields": {
+                "key": "sentry:last_worker_version",
+                "last_updated": "2023-06-23T00:00:00.000Z",
+                "last_updated_by": "unknown",
+                "value": "\\"23.7.1\\""
+                }
+            }
+        """
+    )
+    left.append(missing)
+    out = validate(left, right)
+    findings = out.findings
+
+    assert len(findings) == 1
+    assert findings[0].kind == "MissingEntry"
+    assert findings[0].on == InstanceID("sentry.option", 2)
+
+
+@django_db_all(transaction=True, reset_sequences=True)
+def test_bad_failing_comparator_field(tmp_path):
+    with open(get_fixture_path("backup", "single-option.json")) as backup_file:
+        left = json.load(backup_file)
+    right = deepcopy(left)
+    newer = json.loads(
+        """
+            {
+                "model": "sentry.userrole",
+                "pk": 1,
+                "fields": {
+                    "date_updated": "2023-06-22T23:00:00.123Z",
+                    "date_added": "2023-06-22T23:00:00.000Z",
+                    "name": "Admin",
+                    "permissions": "['users.admin']"
+                }
+            }
+        """
+    )
+    older = json.loads(
+        """
+            {
+                "model": "sentry.userrole",
+                "pk": 1,
+                "fields": {
+                    "date_updated": "2023-06-22T23:00:00.456Z",
+                    "date_added": "2023-06-22T23:00:00.000Z",
+                    "name": "Admin",
+                    "permissions": "['users.admin']"
+                }
+            }
+        """
+    )
+
+    # Note that the "newer" version is being added to the left side, meaning that the
+    # DateUpdatedComparator will fail.
+    left.append(older)
+    right.append(newer)
+    out = validate(left, right)
+    findings = out.findings
+
+    assert len(findings) == 1
+    assert findings[0].kind == "DateUpdatedComparator"
+
+
+@django_db_all(transaction=True, reset_sequences=True)
+def test_good_both_sides_comparator_field_missing(tmp_path):
+    with open(get_fixture_path("backup", "single-option.json")) as backup_file:
+        test_json = json.load(backup_file)
+    userrole_without_date_updated = json.loads(
+        """
+            {
+                "model": "sentry.userrole",
+                "pk": 1,
+                "fields": {
+                    "date_added": "2023-06-22T23:00:00.000Z",
+                    "name": "Admin",
+                    "permissions": "['users.admin']"
+                }
+            }
+        """
+    )
+    test_json.append(userrole_without_date_updated)
+    out = validate(test_json, test_json)
+
+    assert out.empty()
+
+
+@django_db_all(transaction=True, reset_sequences=True)
+def test_bad_left_side_comparator_field_missing(tmp_path):
+    with open(get_fixture_path("backup", "single-option.json")) as backup_file:
+        left = json.load(backup_file)
+    right = deepcopy(left)
+    userrole_without_date_updated = json.loads(
+        """
+            {
+                "model": "sentry.userrole",
+                "pk": 1,
+                "fields": {
+                    "date_added": "2023-06-22T23:00:00.000Z",
+                    "name": "Admin",
+                    "permissions": "['users.admin']"
+                }
+            }
+        """
+    )
+    userrole_with_date_updated = json.loads(
+        """
+            {
+                "model": "sentry.userrole",
+                "pk": 1,
+                "fields": {
+                    "date_updated": "2023-06-22T23:00:00.123Z",
+                    "date_added": "2023-06-22T23:00:00.000Z",
+                    "name": "Admin",
+                    "permissions": "['users.admin']"
+                }
+            }
+        """
+    )
+    left.append(userrole_without_date_updated)
+    right.append(userrole_with_date_updated)
+    out = validate(left, right)
+    findings = out.findings
+
+    assert len(findings) == 1
+    assert findings[0].kind == "UnexecutedDateUpdatedComparator"
+    assert findings[0].on == InstanceID("sentry.userrole", 1)
+    assert "left `date_updated`" in findings[0].reason
+
+
+@django_db_all(transaction=True, reset_sequences=True)
+def test_bad_right_side_comparator_field_missing(tmp_path):
+    with open(get_fixture_path("backup", "single-option.json")) as backup_file:
+        left = json.load(backup_file)
+    right = deepcopy(left)
+    userrole_without_date_updated = json.loads(
+        """
+            {
+                "model": "sentry.userrole",
+                "pk": 1,
+                "fields": {
+                    "date_added": "2023-06-22T23:00:00.000Z",
+                    "name": "Admin",
+                    "permissions": "['users.admin']"
+                }
+            }
+        """
+    )
+    userrole_with_date_updated = json.loads(
+        """
+            {
+                "model": "sentry.userrole",
+                "pk": 1,
+                "fields": {
+                    "date_updated": "2023-06-22T23:00:00.123Z",
+                    "date_added": "2023-06-22T23:00:00.000Z",
+                    "name": "Admin",
+                    "permissions": "['users.admin']"
+                }
+            }
+        """
+    )
+    left.append(userrole_with_date_updated)
+    right.append(userrole_without_date_updated)
+    out = validate(left, right)
+    findings = out.findings
+
+    assert len(findings) == 1
+    assert findings[0].kind == "UnexecutedDateUpdatedComparator"
+    assert findings[0].on == InstanceID("sentry.userrole", 1)
+    assert "right `date_updated`" in findings[0].reason
 
 
 @django_db_all(transaction=True, reset_sequences=True)