From 45c2984526d58d7ebd8799ab2f9337d89d65f7b8 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Tue, 29 Sep 2026 22:08:35 +0200 Subject: [PATCH] fix(port): the documented datastore contract omitted the method documents() calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #312. /code-review's finding 4 said the port documented itself as four-method when `documents` made it five. I fixed the module header and missed the thing that mattered: the CLASS docstring also enumerates the datastore methods a caller must supply, and `get_predictions_by_metadata` was absent from it. Measured rather than reasoned about — a double built to the documented contract: class DocumentedContract: def get_latest_file_id(self, filters): ... def get_file_metadata(self, fid): ... def download_prediction(self, fid): ... def upload_data(self, **kw): ... AttributeError: 'DocumentedContract' object has no attribute 'get_predictions_by_metadata' Mid-delivery, after the upload has happened — which is the one place this port exists to prevent a surprise. Found while verifying, for this repo rather than taking it on trust, that a pipeline-core fix to `get_latest_file_id`'s missing sort would actually reach us. It does: this repo vendors no copy of the Appwrite client, calls `search_files_by_metadata` nowhere, and the port takes any duck-typed datastore with `views_pipeline_core.modules.datastore.DatastoreModule` passed in production. So the ordering fix reaches us with a pin bump and no change here. faoapi and crafdapi each carry their own copy of that module and are not covered. The guard is derived from the source, not a hardcoded list: it collects every `self._dsm.(` the port calls and asserts each appears in the module or class docstring. Prose cannot drift from the code without failing, which is how this one rotted. Mutation-proven by removing the fifth name, which fails both partners. ADR-014 §2 — the guard also asserts it found calls at all, so a refactor that renames `_dsm` cannot leave it silently scanning nothing. 526 passing. Both partner packages unchanged at 690 and 699. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ANY1CCy9Xo7zjMY4XJ69v9 --- tests/test_store_port.py | 37 ++++++++++++++++++++++++ views_postprocessing/crafd/store_port.py | 2 +- views_postprocessing/unfao/store_port.py | 2 +- 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/tests/test_store_port.py b/tests/test_store_port.py index c81a934..a0e4d41 100644 --- a/tests/test_store_port.py +++ b/tests/test_store_port.py @@ -24,6 +24,8 @@ from dataclasses import dataclass +import ast +import re import pytest from pathlib import Path @@ -320,3 +322,38 @@ def test_documents_forwards_the_filters_to_the_store_unchanged(partner): "that it asks the store the same question views-faoapi asks" ) assert got == store.documents_result + + +@pytest.mark.parametrize("partner", PARTNER_PACKAGES) +def test_the_documented_datastore_contract_lists_every_method_the_port_calls(partner): + """A double satisfying the docstring must actually work. + + `/code-review` caught that the module header said "four-method port" when + `documents` made it five. Fixing the header missed the thing that matters: the + class docstring also enumerates the datastore methods a caller must supply, and + `get_predictions_by_metadata` was absent from it. A test double built to the + documented contract raised AttributeError — mid-delivery, after the upload. + + Derived from the source rather than listed here, so the assertion cannot rot the + way the prose did (ADR-014 §2: prove the guard's inputs are real). + """ + source = (_PKG / partner / "store_port.py").read_text() + called = set(re.findall(r"self\._dsm\.(\w+)\(", source)) + assert called, "found no datastore calls — this guard is scanning the wrong thing" + + tree = ast.parse(source) + doc = "\n".join( + [ast.get_docstring(tree) or ""] + + [ + ast.get_docstring(n) or "" + for n in ast.walk(tree) + if isinstance(n, ast.ClassDef) + ] + ) + documented = set(re.findall(r"``(\w+)``", doc)) + missing = sorted(called - documented) + assert not missing, ( + f"{partner}/store_port.py calls {missing} on the datastore but its class " + "docstring does not list them. A caller building to the documented contract " + "gets an AttributeError during a delivery, after the upload has happened." + ) diff --git a/views_postprocessing/crafd/store_port.py b/views_postprocessing/crafd/store_port.py index 596b0d2..a13e4cb 100644 --- a/views_postprocessing/crafd/store_port.py +++ b/views_postprocessing/crafd/store_port.py @@ -18,7 +18,7 @@ class _ContractStorePort: """Adapts a prediction-store client to the wire ports. ``datastore`` is any object exposing ``get_latest_file_id``, ``get_file_metadata``, - ``download_prediction`` and ``upload_data``. + ``download_prediction``, ``upload_data`` and ``get_predictions_by_metadata``. """ def __init__(self, datastore) -> None: diff --git a/views_postprocessing/unfao/store_port.py b/views_postprocessing/unfao/store_port.py index 596b0d2..a13e4cb 100644 --- a/views_postprocessing/unfao/store_port.py +++ b/views_postprocessing/unfao/store_port.py @@ -18,7 +18,7 @@ class _ContractStorePort: """Adapts a prediction-store client to the wire ports. ``datastore`` is any object exposing ``get_latest_file_id``, ``get_file_metadata``, - ``download_prediction`` and ``upload_data``. + ``download_prediction``, ``upload_data`` and ``get_predictions_by_metadata``. """ def __init__(self, datastore) -> None: