diff --git a/backend/address2UPRN/main.py b/backend/address2UPRN/main.py index 6b66dfdbc..599cbaa6b 100644 --- a/backend/address2UPRN/main.py +++ b/backend/address2UPRN/main.py @@ -51,7 +51,7 @@ def get_uprn_from_historic_epc( historic addresses use a more verbose format that systematically depresses lexiscores. """ - repo = HistoricEpcS3Repository() + repo = HistoricEpcS3Repository.with_default_s3_client() return HistoricEpcResolver(repo).resolve_uprn(user_inputed_address, postcode) diff --git a/datatypes/epc/domain/historic_epc_matching.py b/datatypes/epc/domain/historic_epc_matching.py index aa64b9f54..20fa04997 100644 --- a/datatypes/epc/domain/historic_epc_matching.py +++ b/datatypes/epc/domain/historic_epc_matching.py @@ -110,5 +110,5 @@ def match_addresses_for_postcode( HistoricEpcS3Repository, ) - repo = HistoricEpcS3Repository(s3_root=s3_root) + repo = HistoricEpcS3Repository.with_default_s3_client(s3_root) return HistoricEpcResolver(repo).match(user_address, postcode) diff --git a/datatypes/epc/domain/tests/test_historic_epc_matching.py b/datatypes/epc/domain/tests/test_historic_epc_matching.py index 7fd05d6b3..eb982409b 100644 --- a/datatypes/epc/domain/tests/test_historic_epc_matching.py +++ b/datatypes/epc/domain/tests/test_historic_epc_matching.py @@ -11,7 +11,7 @@ from datatypes.epc.domain.historic_epc_matching import ( ScoredHistoricEpc, match_addresses_for_postcode, ) -from repositories.historic_epc import historic_epc_s3_repository as repo_mod +from infrastructure.s3.gzip_csv_s3_client import GzipCsvS3Client # Columns required by the HistoricEpc dataclass (lower-cased CSV columns). # The matcher only reads ADDRESS + UPRN to score; everything else is filled @@ -126,9 +126,12 @@ def _build_df(rows: list[dict]) -> pd.DataFrame: @pytest.fixture def patch_read(): - # match_addresses_for_postcode now reads via the historic-EPC repository, so - # the S3 read is patched where the repository binds it. - with patch.object(repo_mod, "read_csv_gz_from_s3") as m: + # match_addresses_for_postcode now reads through GzipCsvS3Client + # (infrastructure/s3) — not the old utils.s3 free function. Patch the + # client's read (it is called with the per-postcode key only; the bucket + # lives in the client) and stub boto3.client so the seam runs with no S3 and + # no AWS environment. + with patch("boto3.client"), patch.object(GzipCsvS3Client, "read_csv_gz") as m: yield m @@ -164,11 +167,11 @@ class TestMatchAddressesForPostcode: assert scores == sorted(scores, reverse=True) def test_s3_key_built_from_default_root(self, patch_read): + # The default root's prefix threads into the per-postcode key; the + # bucket parsing is covered by HistoricEpcS3Repository's factory test. patch_read.return_value = _build_df([_row("47 GORDON ROAD", "100")]) match_addresses_for_postcode("47 Gordon Road", "AB33 8AL") - patch_read.assert_called_once_with( - "retrofit-data-dev", "historical_epc/AB338AL/data.csv.gz" - ) + patch_read.assert_called_once_with("historical_epc/AB338AL/data.csv.gz") def test_s3_key_respects_custom_root_with_trailing_slash(self, patch_read): patch_read.return_value = _build_df([_row("47 GORDON ROAD", "100")]) @@ -177,9 +180,7 @@ class TestMatchAddressesForPostcode: "AB33 8AL", s3_root="s3://my-bucket/some/prefix/", ) - patch_read.assert_called_once_with( - "my-bucket", "some/prefix/AB338AL/data.csv.gz" - ) + patch_read.assert_called_once_with("some/prefix/AB338AL/data.csv.gz") def test_missing_postcode_object_yields_empty_matches(self, patch_read): # A valid postcode with no stored shard is a normal miss, not an error: diff --git a/infrastructure/s3/gzip_csv_s3_client.py b/infrastructure/s3/gzip_csv_s3_client.py new file mode 100644 index 000000000..acc3fd940 --- /dev/null +++ b/infrastructure/s3/gzip_csv_s3_client.py @@ -0,0 +1,22 @@ +from __future__ import annotations + +from io import BytesIO + +import pandas as pd + +from infrastructure.s3.s3_client import S3Client + + +class GzipCsvS3Client(S3Client): + """Reads a gzipped-CSV S3 object into a pandas DataFrame. + + The S3-facing half of the historic-EPC read: an :class:`S3Client` (injected + boto client + bucket) plus the gzip/CSV decode, so a Repo can depend on this + instead of the ``utils.s3`` free functions. ``low_memory=False`` so the wide, + mixed-type historic-EPC columns infer a dtype from the whole column rather + than per-chunk (which would otherwise split one column across object/float). + """ + + def read_csv_gz(self, key: str) -> pd.DataFrame: + raw = self.get_object(key) + return pd.read_csv(BytesIO(raw), compression="gzip", low_memory=False) diff --git a/repositories/historic_epc/historic_epc_s3_repository.py b/repositories/historic_epc/historic_epc_s3_repository.py index d2e17791d..fd4c857b1 100644 --- a/repositories/historic_epc/historic_epc_s3_repository.py +++ b/repositories/historic_epc/historic_epc_s3_repository.py @@ -1,50 +1,61 @@ from __future__ import annotations -from collections.abc import Callable -from typing import Optional +from typing import Any -import pandas as pd from botocore.exceptions import ClientError -from domain.postcode import Postcode from datatypes.epc.domain.historic_epc import HistoricEpc from datatypes.epc.domain.historic_epc_matching import ( map_historic_epc_pandas_row_to_domain, ) +from domain.postcode import Postcode +from infrastructure.s3.gzip_csv_s3_client import GzipCsvS3Client +from infrastructure.s3.s3_uri import parse_s3_uri from repositories.historic_epc.historic_epc_repository import ( HistoricEpcRepository, PostcodeNotFound, ) -from utils.s3 import parse_s3_uri, read_csv_gz_from_s3 DEFAULT_S3_ROOT = "s3://retrofit-data-dev/historical_epc" -# (bucket, key) -> DataFrame. Injected so the dataset is sourced from S3 in -# production or a fake in tests — the Repo holds no S3/HTTP code of its own -# (mirrors GeospatialS3Repository). -CsvGzReader = Callable[[str, str], pd.DataFrame] - class HistoricEpcS3Repository(HistoricEpcRepository): - """Reads per-postcode ``data.csv.gz`` shards of the historic EPC backup.""" + """Reads per-postcode ``data.csv.gz`` shards of the historic EPC backup. - def __init__( - self, - read_csv_gz: Optional[CsvGzReader] = None, - s3_root: str = DEFAULT_S3_ROOT, - ) -> None: - # Resolve the default reader at call time (not import time) so it stays - # patchable and the captured default can't go stale. - self._read_csv_gz: CsvGzReader = read_csv_gz or read_csv_gz_from_s3 - self._s3_root = s3_root + The bucket and the raw read live in the injected :class:`GzipCsvS3Client` + (``infrastructure/s3``); the Repo only builds the per-postcode key and maps + rows to domain records, so it holds no S3/HTTP code of its own — the same + shape as :class:`UnstandardisedAddressListCsvS3Repository`, and unlike the + old ``utils.s3`` free-function dependency. + """ + + def __init__(self, client: GzipCsvS3Client, root_prefix: str) -> None: + self._client = client + self._root_prefix = root_prefix + + @classmethod + def with_default_s3_client( + cls, s3_root: str = DEFAULT_S3_ROOT + ) -> "HistoricEpcS3Repository": + """Build a repository backed by a real S3 client for ``s3_root`` + (``s3://bucket/prefix``). + + The composition-root convenience constructor used by the lambda and the + ``match_addresses_for_postcode`` seam; tests inject a ``GzipCsvS3Client`` + over a moto-mocked boto client through ``__init__`` instead. + """ + import boto3 + + bucket, root_prefix = parse_s3_uri(s3_root) + boto_s3: Any = boto3.client("s3") # pyright: ignore[reportUnknownMemberType] + return cls(GzipCsvS3Client(boto_s3, bucket), root_prefix) def get_for_postcode(self, postcode: Postcode) -> list[HistoricEpc]: if not postcode.is_valid(): raise PostcodeNotFound(f"{postcode.value!r} is not a valid UK postcode") - bucket, root_prefix = parse_s3_uri(self._s3_root) - key = f"{root_prefix.rstrip('/')}/{postcode}/data.csv.gz" + key = f"{self._root_prefix.rstrip('/')}/{postcode}/data.csv.gz" try: - df = self._read_csv_gz(bucket, key) + df = self._client.read_csv_gz(key) except ClientError as e: if e.response.get("Error", {}).get("Code") in ("NoSuchKey", "404"): return [] diff --git a/tests/infrastructure/test_gzip_csv_s3_client.py b/tests/infrastructure/test_gzip_csv_s3_client.py new file mode 100644 index 000000000..7f2abcfb7 --- /dev/null +++ b/tests/infrastructure/test_gzip_csv_s3_client.py @@ -0,0 +1,45 @@ +import gzip +from collections.abc import Iterator + +import pytest +from botocore.exceptions import ClientError +from moto import mock_aws + +from infrastructure.s3.gzip_csv_s3_client import GzipCsvS3Client +from tests.infrastructure import make_boto_client + +BUCKET = "gzip-csv-bucket" + + +@pytest.fixture +def gzip_client() -> Iterator[GzipCsvS3Client]: + with mock_aws(): + boto_client = make_boto_client("s3") + boto_client.create_bucket(Bucket=BUCKET) + yield GzipCsvS3Client(boto_client, BUCKET) + + +def test_read_csv_gz_decodes_gzipped_csv_into_dataframe( + gzip_client: GzipCsvS3Client, +) -> None: + # arrange + csv = "ADDRESS,UPRN\n47 GORDON ROAD,100\n48 GORDON ROAD,200\n" + gzip_client.put_object("historical_epc/AB338AL/data.csv.gz", gzip.compress(csv.encode())) + + # act + df = gzip_client.read_csv_gz("historical_epc/AB338AL/data.csv.gz") + + # assert + assert list(df.columns) == ["ADDRESS", "UPRN"] + assert df.shape == (2, 2) + assert df["ADDRESS"].tolist() == ["47 GORDON ROAD", "48 GORDON ROAD"] + + +def test_read_csv_gz_raises_no_such_key_when_object_missing( + gzip_client: GzipCsvS3Client, +) -> None: + # act / assert — a missing object surfaces as a ClientError; translating that + # into a domain miss is the Repo's job, not the client's. + with pytest.raises(ClientError) as exc_info: + gzip_client.read_csv_gz("historical_epc/ZZ999ZZ/data.csv.gz") + assert exc_info.value.response["Error"]["Code"] == "NoSuchKey" diff --git a/tests/repositories/historic_epc/test_historic_epc_s3_repository.py b/tests/repositories/historic_epc/test_historic_epc_s3_repository.py index 73bf8aa01..030e39c12 100644 --- a/tests/repositories/historic_epc/test_historic_epc_s3_repository.py +++ b/tests/repositories/historic_epc/test_historic_epc_s3_repository.py @@ -2,21 +2,29 @@ A reference-data lookup, not a Fetcher (ADR-0011): no live EPC API call. The adapter takes a normalised ``Postcode`` and reads -``historical_epc/{POSTCODE}/data.csv.gz`` via an injected reader, so the tests -exercise mapping + key construction + absence against a fake reader with no -network. +``historical_epc/{POSTCODE}/data.csv.gz`` through an injected +``GzipCsvS3Client`` (infrastructure/s3) — never the ``utils.s3`` free functions. + +The client is wrapped over a tiny fake boto client (rather than moto) so the +tests can assert the *exact* S3 key the repo builds and inject a non-missing +``ClientError``; the real gzip/CSV decode in ``GzipCsvS3Client`` still runs. """ from __future__ import annotations +import csv import dataclasses +import gzip +from io import BytesIO, StringIO +from typing import Any, Optional +from unittest.mock import patch -import pandas as pd import pytest from botocore.exceptions import ClientError from datatypes.epc.domain.historic_epc import HistoricEpc from domain.postcode import Postcode +from infrastructure.s3.gzip_csv_s3_client import GzipCsvS3Client from repositories.historic_epc.historic_epc_repository import PostcodeNotFound from repositories.historic_epc.historic_epc_s3_repository import ( HistoricEpcS3Repository, @@ -25,23 +33,56 @@ from repositories.historic_epc.historic_epc_s3_repository import ( # HistoricEpc requires every CSV column; derive the (upper-cased) column list # straight from the dataclass so it can never drift from the domain type. _COLS = [f.name.upper() for f in dataclasses.fields(HistoricEpc)] +_ROOT_PREFIX = "historical_epc" -def _row(address: str, uprn: object) -> dict[str, object]: - row: dict[str, object] = {col: "" for col in _COLS} - row["ADDRESS"] = address - row["UPRN"] = uprn - return row +def _gzip_csv(rows: list[tuple[str, str]]) -> bytes: + """A gzipped CSV carrying every HistoricEpc column; only ADDRESS/UPRN set.""" + buffer = StringIO() + writer = csv.DictWriter(buffer, fieldnames=_COLS) + writer.writeheader() + for address, uprn in rows: + row = {col: "" for col in _COLS} + row["ADDRESS"] = address + row["UPRN"] = uprn + writer.writerow(row) + return gzip.compress(buffer.getvalue().encode()) -def _df(rows: list[dict[str, object]]) -> pd.DataFrame: - return pd.DataFrame(rows, columns=_COLS) +class _FakeBoto: + """A minimal stand-in for a boto3 S3 client: serves one canned object (or + raises a chosen ``ClientError``) and records the ``(Bucket, Key)`` of every + request, so the repo tests can assert the exact S3 location without a live + bucket.""" + + def __init__( + self, *, body: Optional[bytes] = None, error_code: Optional[str] = None + ) -> None: + self._body = body + self._error_code = error_code + self.calls: list[tuple[str, str]] = [] + + def get_object(self, *, Bucket: str, Key: str) -> dict[str, Any]: + self.calls.append((Bucket, Key)) + if self._error_code is not None: + raise ClientError( + {"Error": {"Code": self._error_code, "Message": "x"}}, "GetObject" + ) + return {"Body": BytesIO(self._body or b"")} + + @property + def requested_keys(self) -> list[str]: + return [key for _bucket, key in self.calls] -def test_get_for_postcode_maps_reader_rows_to_historic_epc_records(): +def _repo(boto: _FakeBoto) -> HistoricEpcS3Repository: + client = GzipCsvS3Client(boto, "retrofit-data-dev") + return HistoricEpcS3Repository(client, _ROOT_PREFIX) + + +def test_get_for_postcode_maps_shard_rows_to_historic_epc_records(): # Arrange - df = _df([_row("47 GORDON ROAD", "100")]) - repo = HistoricEpcS3Repository(read_csv_gz=lambda bucket, key: df) + repo = _repo(_FakeBoto(body=_gzip_csv([("47 GORDON ROAD", "100")]))) # Act records = repo.get_for_postcode(Postcode("AB33 8AL")) @@ -53,14 +94,21 @@ def test_get_for_postcode_maps_reader_rows_to_historic_epc_records(): assert records[0].uprn == "100" +def test_builds_s3_key_from_postcode_and_root_prefix(): + # Arrange + boto = _FakeBoto(body=_gzip_csv([("47 GORDON ROAD", "100")])) + repo = _repo(boto) + + # Act — the Postcode value object has already normalised casing/spacing. + repo.get_for_postcode(Postcode("ab33 8al")) + + # Assert + assert boto.requested_keys == ["historical_epc/AB338AL/data.csv.gz"] + + def test_non_empty_postcode_with_no_stored_object_returns_empty_list(): # Arrange — a postcode whose shard does not exist in S3. - def missing(bucket: str, key: str) -> pd.DataFrame: - raise ClientError( - {"Error": {"Code": "NoSuchKey", "Message": "missing"}}, "GetObject" - ) - - repo = HistoricEpcS3Repository(read_csv_gz=missing) + repo = _repo(_FakeBoto(error_code="NoSuchKey")) # Act records = repo.get_for_postcode(Postcode("AB33 8AL")) @@ -69,31 +117,9 @@ def test_non_empty_postcode_with_no_stored_object_returns_empty_list(): assert records == [] -def test_builds_s3_key_from_postcode_and_default_root(): - # Arrange - calls: list[tuple[str, str]] = [] - - def capture(bucket: str, key: str) -> pd.DataFrame: - calls.append((bucket, key)) - return _df([_row("47 GORDON ROAD", "100")]) - - repo = HistoricEpcS3Repository(read_csv_gz=capture) - - # Act — the Postcode value object has already normalised the casing/spacing. - repo.get_for_postcode(Postcode("ab33 8al")) - - # Assert - assert calls == [("retrofit-data-dev", "historical_epc/AB338AL/data.csv.gz")] - - def test_non_missing_read_error_propagates(): # Arrange — an error that is NOT a missing object must not be swallowed. - def denied(bucket: str, key: str) -> pd.DataFrame: - raise ClientError( - {"Error": {"Code": "AccessDenied", "Message": "nope"}}, "GetObject" - ) - - repo = HistoricEpcS3Repository(read_csv_gz=denied) + repo = _repo(_FakeBoto(error_code="AccessDenied")) # Act / Assert with pytest.raises(ClientError): @@ -102,7 +128,7 @@ def test_non_missing_read_error_propagates(): def test_empty_postcode_raises_postcode_not_found(): # Arrange — Postcode normalises whitespace away, leaving an empty key. - repo = HistoricEpcS3Repository(read_csv_gz=lambda bucket, key: _df([])) + repo = _repo(_FakeBoto()) # Act / Assert — an unusable key, distinct from a non-empty absent postcode. with pytest.raises(PostcodeNotFound): @@ -111,7 +137,7 @@ def test_empty_postcode_raises_postcode_not_found(): def test_malformed_postcode_raises_postcode_not_found(): # Arrange — a non-empty but malformed postcode can't key a real shard. - repo = HistoricEpcS3Repository(read_csv_gz=lambda bucket, key: _df([])) + repo = _repo(_FakeBoto()) # Act / Assert with pytest.raises(PostcodeNotFound): @@ -119,13 +145,29 @@ def test_malformed_postcode_raises_postcode_not_found(): def test_uprn_trailing_dot_zero_is_stripped(): - # Arrange — pandas reads an integer UPRN column as float, so the CSV cell - # arrives as "151020766.0"; the domain UPRN must be the bare integer string. - df = _df([_row("47 GORDON ROAD", "151020766.0")]) - repo = HistoricEpcS3Repository(read_csv_gz=lambda bucket, key: df) + # Arrange — pandas reads an integer UPRN column written with a decimal as + # float, so the cell stringifies to "151020766.0"; the domain UPRN must be + # the bare integer string. + repo = _repo(_FakeBoto(body=_gzip_csv([("47 GORDON ROAD", "151020766.0")]))) # Act records = repo.get_for_postcode(Postcode("AB33 8AL")) # Assert assert records[0].uprn == "151020766" + + +def test_with_default_s3_client_threads_bucket_and_key_from_s3_root(): + # Arrange — the factory parses ``s3://bucket/prefix`` into the client's + # bucket and the repo's root prefix, so a read lands at the right location. + boto = _FakeBoto(error_code="NoSuchKey") + with patch("boto3.client", return_value=boto): + repo = HistoricEpcS3Repository.with_default_s3_client( + "s3://my-bucket/some/prefix/" + ) + + # Act + repo.get_for_postcode(Postcode("ab33 8al")) + + # Assert — bucket from the URI authority, key from the URI path + postcode. + assert boto.calls == [("my-bucket", "some/prefix/AB338AL/data.csv.gz")]