Add more details about collection inconsistencies (#120)

* Add more details about collection inconsistencies

* Improve messages

checks/remotesettings/collections_consistency.py

@@ -1,9 +1,10 @@
 """
-Preview and final collections have consistent records and status.
+Source, preview and/or final collections should have consistent records and status.
 
 Some insights about the consistencies are returned for each concerned collection.
 """
 import logging
+from typing import Dict, List, Optional, Tuple
 
 from poucave.typings import CheckResult
 from poucave.utils import run_parallel
@@ -21,18 +22,54 @@ def records_equal(a, b):
     return ra == rb
 
 
-def compare_collections(a, b):
+def compare_collections(
+    a: List[Dict], b: List[Dict]
+) -> Optional[Tuple[List[str], List[str], List[str]]]:
     """Compare two lists of records. Returns empty list if equal."""
     b_by_id = {r["id"]: r for r in b}
-    diff = []
+    missing = []
+    differ = []
     for ra in a:
         rb = b_by_id.pop(ra["id"], None)
         if rb is None:
-            diff.append(ra)
+            missing.append(ra["id"])
         elif not records_equal(ra, rb):
-            diff.append(ra)
-    diff.extend(b_by_id.values())
-    return diff
+            differ.append(ra["id"])
+    extras = list(b_by_id.keys())
+
+    if missing or differ or extras:
+        return (missing, differ, extras)
+
+    return None
+
+
+def human_diff(
+    left: str,
+    right: str,
+    missing: List[str],
+    differ: List[str],
+    extras: List[str],
+    show_ids: int = 5,
+) -> str:
+    def ellipse(l):
+        return ", ".join(repr(r) for r in l[:show_ids]) + (
+            "..." if len(l) > show_ids else ""
+        )
+
+    details = []
+    if missing:
+        details.append(
+            f"{len(missing)} record{'s' if len(missing) > 1 else ''} present in {left} but missing in {right} ({ellipse(missing)})"
+        )
+    if differ:
+        details.append(
+            f"{len(differ)} record{'s' if len(differ) > 1 else ''} differ between {left} and {right} ({ellipse(differ)})"
+        )
+    if extras:
+        details.append(
+            f"{len(extras)} record{'s' if len(extras) > 1 else ''} present in {right} but missing in {left} ({ellipse(extras)})"
+        )
+    return ", ".join(details)
 
 
 async def has_inconsistencies(server_url, auth, resource):
@@ -50,14 +87,16 @@ async def has_inconsistencies(server_url, auth, resource):
     except KeyError:
         return '"status" attribute missing'
 
+    message = f"status: '{status}'. "
+
     # Collection status is reset on any modification, so if status is ``to-review``,
     # then records in the source should be exactly the same as the records in the preview
     if status == "to-review":
         source_records = await client.get_records(**source)
         preview_records = await client.get_records(**resource["preview"])
         diff = compare_collections(source_records, preview_records)
         if diff:
-            return "to-review: source and preview differ"
+            return message + human_diff("source", "preview", *diff)
 
     # And if status is ``signed``, then records in the source and preview should
     # all be the same as those in the destination.
@@ -67,15 +106,19 @@ async def has_inconsistencies(server_url, auth, resource):
         if "preview" in resource:
             # If preview is enabled, then compare source/preview and preview/dest
             preview_records = await client.get_records(**resource["preview"])
-            diff_source = compare_collections(source_records, preview_records)
+
             diff_preview = compare_collections(preview_records, dest_records)
+            if diff_preview:
+                return message + human_diff("preview", "destination", *diff_preview)
+
+            diff_source = compare_collections(source_records, preview_records)
+            if diff_source:
+                return message + human_diff("source", "preview", *diff_source)
         else:
             # Otherwise, just compare source/dest
             diff_source = compare_collections(source_records, dest_records)
-            diff_preview = []
-        # If difference detected, report it!
-        if diff_source or diff_preview:
-            return "signed: source, preview, and/or destination differ"
+            if diff_source:
+                return message + human_diff("source", "destination", *diff_source)
 
     elif status == "work-in-progress":
         # And if status is ``work-in-progress``, we can't really check anything.
@@ -85,7 +128,7 @@ async def has_inconsistencies(server_url, auth, resource):
 
     else:
         # Other statuses should never be encountered.
-        return f"unexpected status '{status}'"
+        return f"Unexpected status '{status}'"
 
     return None
 
@@ -97,7 +140,7 @@ async def run(server: str, auth: str) -> CheckResult:
     results = await run_parallel(*futures)
 
     inconsistent = {
-        "{bucket}/{collection}".format(**resource["destination"]): error_info
+        "{bucket}/{collection}".format(**resource["destination"]): error_info.strip()
         for resource, error_info in zip(resources, results)
         if error_info
     }

tests/checks/remotesettings/test_collections_consistency.py

@@ -73,10 +73,10 @@ async def test_has_inconsistencies_unsupported_status(mock_responses):
 
     result = await has_inconsistencies(server_url, FAKE_AUTH, RESOURCES[1])
 
-    assert "unexpected status" in result
+    assert "Unexpected status" in result
 
 
-async def test_has_inconsistencies_preview_differs(mock_responses):
+async def test_has_inconsistencies_to_review_preview_differs(mock_responses):
     server_url = "http://fake.local/v1"
     resource = {
         "source": {"bucket": "security-workspace", "collection": "blocklist"},
@@ -87,6 +87,7 @@ async def test_has_inconsistencies_preview_differs(mock_responses):
         {"id": "abc", "last_modified": 42},
         {"id": "def", "title": "a", "last_modified": 41},
         {"id": "ghi", "last_modified": 40},
+        {"id": "jkl", "last_modified": 39},
     ]
 
     collection_url = server_url + COLLECTION_URL.format(
@@ -101,13 +102,73 @@ async def test_has_inconsistencies_preview_differs(mock_responses):
     mock_responses.get(
         records_url,
         payload={
-            "data": records[:1] + [{"id": "def", "title": "b", "last_modified": 123}]
+            "data": records[:1]
+            + [
+                {"id": "def", "title": "b", "last_modified": 123},
+                {"id": "jkl", "title": "bam", "last_modified": 456},
+            ]
         },
     )
 
     result = await has_inconsistencies(server_url, FAKE_AUTH, resource)
 
-    assert "source and preview differ" in result
+    assert "1 record present in source but missing in preview ('ghi')" in result
+    assert "2 records differ between source and preview ('def', 'jkl')" in result
+
+
+async def test_has_inconsistencies_preview_differs(mock_responses):
+    server_url = "http://fake.local/v1"
+    resource = {
+        "source": {"bucket": "security-workspace", "collection": "blocklist"},
+        "preview": {"bucket": "security-preview", "collection": "blocklist"},
+        "destination": {"bucket": "security", "collection": "blocklist"},
+    }
+    records = [{"id": "abc", "last_modified": 42}, {"id": "def", "last_modified": 41}]
+
+    collection_url = server_url + COLLECTION_URL.format(
+        "security-workspace", "blocklist"
+    )
+    mock_responses.get(
+        collection_url, payload={"data": {"id": "blocklist", "status": "signed"}}
+    )
+    records_url = server_url + RECORDS_URL.format("security-workspace", "blocklist")
+    mock_responses.get(
+        records_url, payload={"data": records + [{"id": "xyz", "last_modified": 40}]}
+    )
+    records_url = server_url + RECORDS_URL.format("security-preview", "blocklist")
+    mock_responses.get(records_url, payload={"data": records})
+    records_url = server_url + RECORDS_URL.format("security", "blocklist")
+    mock_responses.get(records_url, payload={"data": records})
+
+    result = await has_inconsistencies(server_url, FAKE_AUTH, resource)
+
+    assert "1 record present in source but missing in preview ('xyz')" in result
+
+
+async def test_has_inconsistencies_no_preview_destination_differs(mock_responses):
+    server_url = "http://fake.local/v1"
+    resource = {
+        "source": {"bucket": "security-workspace", "collection": "blocklist"},
+        "destination": {"bucket": "security", "collection": "blocklist"},
+    }
+    records = [{"id": "abc", "last_modified": 42}, {"id": "def", "last_modified": 41}]
+
+    collection_url = server_url + COLLECTION_URL.format(
+        "security-workspace", "blocklist"
+    )
+    mock_responses.get(
+        collection_url, payload={"data": {"id": "blocklist", "status": "signed"}}
+    )
+    records_url = server_url + RECORDS_URL.format("security-workspace", "blocklist")
+    mock_responses.get(records_url, payload={"data": records})
+    records_url = server_url + RECORDS_URL.format("security", "blocklist")
+    mock_responses.get(
+        records_url, payload={"data": records + [{"id": "xyz", "last_modified": 40}]}
+    )
+
+    result = await has_inconsistencies(server_url, FAKE_AUTH, resource)
+
+    assert "1 record present in destination but missing in source ('xyz')" in result
 
 
 async def test_has_inconsistencies_destination_differs(mock_responses):
@@ -136,7 +197,7 @@ async def test_has_inconsistencies_destination_differs(mock_responses):
 
     result = await has_inconsistencies(server_url, FAKE_AUTH, resource)
 
-    assert "source, preview, and/or destination differ" in result
+    assert "1 record present in destination but missing in preview ('xyz')" in result
 
 
 async def test_positive(mock_responses):