From 6cec44fa007404310d88652b9ac48a71cb035ca3 Mon Sep 17 00:00:00 2001 From: Chelsea Kelly-Reif <3114598+ChelseaKR@users.noreply.github.com> Date: Mon, 7 Sep 2026 19:49:32 -0700 Subject: [PATCH] A features value that is not a list is refused yield from over a mapping yields its keys and len() of it counts them, so a page whose features is an object made the walk emit the field names as though they were features and step its offset by the number of them. Nothing raised. Measured on the unmodified tree before the fix: a page carrying {"features": {"OBJECTID": 1, "YEAR_": 2020}} makes iter_features yield ['OBJECTID', 'YEAR_'] and stop, after one request. Found by the consuming project's own copy of this walk, which checks the type and would have had to keep the check to move onto this one. A compensation cannot be retired until the thing it compensates for is gone, so the check belongs here rather than downstream. --- CHANGELOG.md | 6 ++++++ src/perimeter/acquire.py | 9 +++++++++ tests/test_acquire.py | 27 +++++++++++++++++++++++++++ 3 files changed, 42 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e0ccbb..978fc5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,12 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this project a reason to expose the walk at all is that the offset rule stops being copied, and a non-default format reaching the same records by an untested path would leave the consumer with a second implementation of the subtle part after all. +- **A `features` value that is not a list is refused.** `yield from` over a mapping + yields its keys and `len()` of it counts them, so a page shaped that way made the walk + emit strings and step its offset by a number with nothing to do with any record, and + nothing raised. Found by the consuming project's own copy of this walk, which checked + the type: a compensation cannot be retired until the thing it compensates for is gone, + so the check belongs here. ### Added, a survey command that inventories a retrieval's candidate markers diff --git a/src/perimeter/acquire.py b/src/perimeter/acquire.py index 08180ea..730f576 100644 --- a/src/perimeter/acquire.py +++ b/src/perimeter/acquire.py @@ -287,6 +287,15 @@ def iter_features( "early and write a short file with a clean hash." ) features = payload["features"] + if not isinstance(features, list): + raise AcquisitionFailed( + f"{endpoint} answered a page whose 'features' is a " + f"{type(features).__name__} rather than a list, at offset {offset}. " + "The walk yields from it and steps its offset by its length, and both " + "of those do something plausible to a mapping: it would yield the " + "field names and step by the number of them. A page shaped like that " + "is not a page of features." + ) if not features: break yield from features diff --git a/tests/test_acquire.py b/tests/test_acquire.py index f0eec15..266eff9 100644 --- a/tests/test_acquire.py +++ b/tests/test_acquire.py @@ -774,6 +774,33 @@ def fake_get(url: str, **_: object) -> dict[str, Any]: assert "features" in str(raised.value) +def test_a_features_value_that_is_not_a_list_is_refused( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A mapping under `features` walks and produces nonsense, silently. + + `yield from` over a dict yields its keys, and `len()` of it is the number of + them, so a page shaped that way makes the walk emit strings and step its + offset by a number that has nothing to do with any record. Nothing raises. + + Found by a consuming project's own copy of this walk, which checked the type + and would have had to keep the check to move onto this one. That is the sort + of thing the audit in that repository exists to surface: a compensation + cannot be retired until the thing it compensates for is gone. + """ + + def fake_get(url: str, **_: object) -> dict[str, Any]: + return { + "features": {"OBJECTID": 1, "YEAR_": 2020}, + "exceededTransferLimit": False, + } + + monkeypatch.setattr(acquire_mod, "_get", fake_get) + with pytest.raises(AcquisitionFailed) as raised: + list(iter_features("https://example.invalid/query", ("OBJECTID",))) + assert "rather than a list" in str(raised.value) + + def test_a_layer_that_really_is_empty_is_still_walked_to_a_clean_stop( monkeypatch: pytest.MonkeyPatch, ) -> None: