Leverage newest features of kinto-http.py (#1515)

* Use collection_diff

* Use get_changeset()

* Lint

Author: leplatrem

.secrets.baseline

@@ -90,6 +90,10 @@
     {
       "path": "detect_secrets.filters.allowlist.is_line_allowlisted"
     },
+    {
+      "path": "detect_secrets.filters.common.is_baseline_file",
+      "filename": ".secrets.baseline"
+    },
     {
       "path": "detect_secrets.filters.common.is_ignored_due_to_verification_policies",
       "min_level": 2
@@ -138,14 +142,14 @@
         "filename": "tests/checks/remotesettings/test_attachments_integrity.py",
         "hashed_secret": "263e1869fb57deeb75844cf85d55f0d03a6019fc",
         "is_verified": false,
-        "line_number": 31
+        "line_number": 32
       },
       {
         "type": "Hex High Entropy String",
         "filename": "tests/checks/remotesettings/test_attachments_integrity.py",
         "hashed_secret": "fdf34abe05071170ce2ec082a18aa762ec454ae3",
         "is_verified": false,
-        "line_number": 39
+        "line_number": 40
       }
     ],
     "tests/checks/remotesettings/test_public_suffix_list.py": [
@@ -158,5 +162,5 @@
       }
     ]
   },
-  "generated_at": "2024-10-18T09:21:04Z"
+  "generated_at": "2024-12-03T11:11:17Z"
 }

checks/remotesettings/backported_records.py

@@ -9,9 +9,11 @@
 from typing import Any, Dict
 from urllib.parse import parse_qs
 
+from kinto_http.utils import collection_diff
+
 from telescope.typings import CheckResult
 
-from .utils import KintoClient, compare_collections, human_diff
+from .utils import KintoClient, human_diff
 
 
 EXPOSED_PARAMETERS = ["server", "max_lag_seconds"]
@@ -40,8 +42,8 @@ async def run(
         dest_records = await client.get_records(
             bucket=dest_bid, collection=dest_cid, _expected="Foo"
         )
-        diff = compare_collections(source_records, dest_records)
-        if diff:
+        to_create, to_update, to_delete = collection_diff(source_records, dest_records)
+        if to_create or to_update or to_delete:
             source_timestamp = await client.get_records_timestamp(
                 bucket=source_bid, collection=source_cid
             )
@@ -50,8 +52,7 @@ async def run(
             )
             diff_millisecond = abs(int(source_timestamp) - int(dest_timestamp))
             if (diff_millisecond / 1000) > max_lag_seconds:
-                missing, differ, extras = diff
-                details = human_diff(source, dest, missing, differ, extras)
+                details = human_diff(source, dest, to_create, to_update, to_delete)
                 errors.append(details)
 
     return len(errors) == 0, errors

checks/remotesettings/collections_consistency.py

@@ -7,10 +7,12 @@
 
 import logging
 
+from kinto_http.utils import collection_diff
+
 from telescope.typings import CheckResult
 from telescope.utils import run_parallel
 
-from .utils import KintoClient, compare_collections, fetch_signed_resources, human_diff
+from .utils import KintoClient, fetch_signed_resources, human_diff
 
 
 EXPOSED_PARAMETERS = ["server"]
@@ -46,9 +48,13 @@ async def has_inconsistencies(server_url, auth, resource):
 
         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 message + human_diff("source", "preview", *diff)
+        to_create, to_update, to_delete = collection_diff(
+            source_records, preview_records
+        )
+        if to_create or to_update or to_delete:
+            return message + human_diff(
+                "source", "preview", to_create, to_update, to_delete
+            )
 
     # And if status is ``signed``, then records in the source and preview should
     # all be the same as those in the destination.
@@ -59,18 +65,30 @@ async def has_inconsistencies(server_url, auth, resource):
             # If preview is enabled, then compare source/preview and preview/dest
             preview_records = await client.get_records(**resource["preview"])
 
-            diff_preview = compare_collections(preview_records, dest_records)
-            if diff_preview:
-                return message + human_diff("preview", "destination", *diff_preview)
+            to_create, to_update, to_delete = collection_diff(
+                preview_records, dest_records
+            )
+            if to_create or to_update or to_delete:
+                return message + human_diff(
+                    "preview", "destination", to_create, to_update, to_delete
+                )
 
-            diff_source = compare_collections(source_records, preview_records)
-            if diff_source:
-                return message + human_diff("source", "preview", *diff_source)
+            to_create, to_update, to_delete = collection_diff(
+                source_records, preview_records
+            )
+            if to_create or to_update or to_delete:
+                return message + human_diff(
+                    "source", "preview", to_create, to_update, to_delete
+                )
         else:
             # Otherwise, just compare source/dest
-            diff_source = compare_collections(source_records, dest_records)
-            if diff_source:
-                return message + human_diff("source", "destination", *diff_source)
+            to_create, to_update, to_delete = collection_diff(
+                source_records, dest_records
+            )
+            if to_create or to_update or to_delete:
+                return message + human_diff(
+                    "source", "destination", to_create, to_update, to_delete
+                )
 
     elif status == "work-in-progress":
         # And if status is ``work-in-progress``, we can't really check anything.

checks/remotesettings/utils.py

@@ -1,8 +1,6 @@
-import asyncio
 import copy
-import random
 import re
-from typing import Dict, List, Optional, Tuple
+from typing import Any, Dict, List
 
 import backoff
 import kinto_http
@@ -49,23 +47,15 @@ async def get_records(self, *args, **kwargs) -> List[Dict]:
         return await self._client.get_records(*args, **kwargs)
 
     @retry_timeout
-    async def get_monitor_changes(self, bust_cache=False, **kwargs) -> List[Dict]:
-        if bust_cache:
-            if "_expected" in kwargs:
-                raise ValueError("Pick one of `bust_cache` and `_expected` parameters")
-            random_cache_bust = random.randint(999999000000, 999999999999)
-            kwargs["_expected"] = random_cache_bust
-        return await self.get_records(bucket="monitor", collection="changes", **kwargs)
+    async def get_monitor_changes(self, **kwargs) -> List[Dict]:
+        resp = await self.get_changeset(
+            bucket="monitor", collection="changes", **kwargs
+        )
+        return resp["changes"]
 
     @retry_timeout
-    async def get_changeset(self, bucket, collection, **kwargs) -> List[Dict]:
-        endpoint = f"/buckets/{bucket}/collections/{collection}/changeset"
-        kwargs.setdefault("_expected", random.randint(999999000000, 999999999999))
-        loop = asyncio.get_event_loop()
-        body, _ = await loop.run_in_executor(
-            None, lambda: self._client.session.request("get", endpoint, params=kwargs)
-        )
-        return body
+    async def get_changeset(self, *args, **kwargs) -> Dict[str, Any]:
+        return await self._client.get_changeset(*args, **kwargs)
 
     @retry_timeout
     async def get_record(self, *args, **kwargs) -> Dict:
@@ -110,9 +100,7 @@ async def fetch_signed_resources(server_url: str, auth: str) -> List[Dict[str, D
             preview_buckets.add(resource["preview"]["bucket"])
 
     resources = []
-    monitored = await client.get_records(
-        bucket="monitor", collection="changes", _sort="bucket,collection"
-    )
+    monitored = await client.get_monitor_changes(_sort="bucket,collection")
     for entry in monitored:
         bid = entry["bucket"]
         cid = entry["collection"]
@@ -138,60 +126,35 @@ async def fetch_signed_resources(server_url: str, auth: str) -> List[Dict[str, D
     return resources
 
 
-def records_equal(a, b):
-    """Compare records, ignoring timestamps."""
-    ignored_fields = ("last_modified", "schema")
-    ra = {k: v for k, v in a.items() if k not in ignored_fields}
-    rb = {k: v for k, v in b.items() if k not in ignored_fields}
-    return ra == rb
-
-
-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}
-    missing = []
-    differ = []
-    for ra in a:
-        rb = b_by_id.pop(ra["id"], None)
-        if rb is None:
-            missing.append(ra["id"])
-        elif not records_equal(ra, rb):
-            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],
+    missing: List[dict],
+    differ: List[tuple[dict, dict]],
+    extras: List[dict],
     show_ids: int = 5,
 ) -> str:
+    missing_ids = [r["id"] for r in missing]
+    differ_ids = [r["id"] for _, r in differ]
+    extras_ids = [r["id"] for r in extras]
+
     def ellipse(line):
         return ", ".join(repr(r) for r in line[:show_ids]) + (
             "..." if len(line) > show_ids else ""
         )
 
     details = []
-    if missing:
+    if missing_ids:
         details.append(
-            f"{len(missing)} record{'s' if len(missing) > 1 else ''} present in {left} but missing in {right} ({ellipse(missing)})"
+            f"{len(missing_ids)} record{'s' if len(missing_ids) > 1 else ''} present in {left} but missing in {right} ({ellipse(missing_ids)})"
         )
-    if differ:
+    if differ_ids:
         details.append(
-            f"{len(differ)} record{'s' if len(differ) > 1 else ''} differ between {left} and {right} ({ellipse(differ)})"
+            f"{len(differ_ids)} record{'s' if len(differ_ids) > 1 else ''} differ between {left} and {right} ({ellipse(differ_ids)})"
         )
-    if extras:
+    if extras_ids:
         details.append(
-            f"{len(extras)} record{'s' if len(extras) > 1 else ''} present in {right} but missing in {left} ({ellipse(extras)})"
+            f"{len(extras_ids)} record{'s' if len(extras_ids) > 1 else ''} present in {right} but missing in {left} ({ellipse(extras_ids)})"
         )
     return ", ".join(details)
 

checks/remotesettings/work_in_progress.py

@@ -9,10 +9,12 @@
 import sys
 from datetime import datetime
 
+from kinto_http.utils import collection_diff
+
 from telescope.typings import CheckResult
 from telescope.utils import run_parallel, utcnow
 
-from .utils import KintoClient, compare_collections, fetch_signed_resources
+from .utils import KintoClient, fetch_signed_resources
 
 
 logger = logging.getLogger(__name__)
@@ -62,7 +64,10 @@ async def run(server: str, auth: str, max_age: int) -> CheckResult:
             # These collections are worth introspecting.
             source_records = await client.get_records(**resource["source"])
             destination_records = await client.get_records(**resource["destination"])
-            if not compare_collections(source_records, destination_records):
+            to_create, to_update, to_delete = collection_diff(
+                source_records, destination_records
+            )
+            if not (to_create or to_update or to_delete):
                 continue
 
         # Fetch list of editors, if necessary to contact them.

tests/checks/remotesettings/test_attachments_availability.py

@@ -1,6 +1,7 @@
 from checks.remotesettings.attachments_availability import run
 
 
+CHANGESET_URL = "/buckets/{}/collections/{}/changeset"
 RECORDS_URL = "/buckets/{}/collections/{}/records"
 
 
@@ -10,11 +11,11 @@ async def test_positive(mock_responses, mock_aioresponses):
         server_url + "/",
         payload={"capabilities": {"attachments": {"base_url": "http://cdn/"}}},
     )
-    changes_url = server_url + RECORDS_URL.format("monitor", "changes")
+    changes_url = server_url + CHANGESET_URL.format("monitor", "changes")
     mock_responses.get(
         changes_url,
         payload={
-            "data": [
+            "changes": [
                 {"id": "abc", "bucket": "bid", "collection": "cid", "last_modified": 42}
             ]
         },
@@ -45,11 +46,11 @@ async def test_negative(mock_responses, mock_aioresponses):
         server_url + "/",
         payload={"capabilities": {"attachments": {"base_url": "http://cdn/"}}},
     )
-    changes_url = server_url + RECORDS_URL.format("monitor", "changes")
+    changes_url = server_url + CHANGESET_URL.format("monitor", "changes")
     mock_responses.get(
         changes_url,
         payload={
-            "data": [
+            "changes": [
                 {"id": "abc", "bucket": "bid", "collection": "cid", "last_modified": 42}
             ]
         },

tests/checks/remotesettings/test_attachments_bundles.py

@@ -41,11 +41,11 @@ async def test_negative(mock_responses, mock_aioresponses):
     may8_http = "Mon, 08 May 1982 00:01:01 GMT"
     may8_iso = "1982-05-08T00:01:01+00:00"
 
-    changes_url = server_url + RECORDS_URL.format("monitor", "changes")
+    changes_url = server_url + CHANGESET_URL.format("monitor", "changes")
     mock_responses.get(
         changes_url,
         payload={
-            "data": [
+            "changes": [
                 {
                     "id": "abc",
                     "bucket": "main",

tests/checks/remotesettings/test_attachments_integrity.py

@@ -1,6 +1,7 @@
 from checks.remotesettings.attachments_integrity import run
 
 
+CHANGESET_URL = "/buckets/{}/collections/{}/changeset"
 RECORDS_URL = "/buckets/{}/collections/{}/records"
 
 
@@ -10,11 +11,11 @@ async def test_positive(mock_responses, mock_aioresponses):
         server_url + "/",
         payload={"capabilities": {"attachments": {"base_url": "http://cdn/"}}},
     )
-    changes_url = server_url + RECORDS_URL.format("monitor", "changes")
+    changes_url = server_url + CHANGESET_URL.format("monitor", "changes")
     mock_responses.get(
         changes_url,
         payload={
-            "data": [
+            "changes": [
                 {"id": "abc", "bucket": "bid", "collection": "cid", "last_modified": 42}
             ]
         },
@@ -59,11 +60,11 @@ async def test_negative(mock_responses, mock_aioresponses):
         server_url + "/",
         payload={"capabilities": {"attachments": {"base_url": "http://cdn/"}}},
     )
-    changes_url = server_url + RECORDS_URL.format("monitor", "changes")
+    changes_url = server_url + CHANGESET_URL.format("monitor", "changes")
     mock_responses.get(
         changes_url,
         payload={
-            "data": [
+            "changes": [
                 {"id": "abc", "bucket": "bid", "collection": "cid", "last_modified": 42}
             ]
         },