From 7deeb22caea48f4f37fa9f4b71025c5c59c2f0e1 Mon Sep 17 00:00:00 2001 From: symroe Date: Fri, 21 Aug 2026 09:04:31 +0100 Subject: [PATCH 01/12] Scope storage per data type, and add replace/accumulate modes Storage sessions were scoped per council but not per data type, so a minutes run would delete a council's councillors output and vice versa. Sessions are now rooted at data///, matching the layout the GitHub backend already used. Alongside that, BaseStorage takes a storage_mode. REPLACE clears everything stored for the council and data type before writing, which is what councillors needs: the current set is the whole truth and someone who has left should disappear. ACCUMULATE adds to what is there, which is what any historical record needs, where last year's data is still a fact. REPLACE remains the default, so existing scrapers are unaffected. Co-Authored-By: Claude Opus 5 --- lgsf/storage/backends/__init__.py | 13 ++- lgsf/storage/backends/base.py | 34 +++++- lgsf/storage/backends/github.py | 16 ++- lgsf/storage/backends/local.py | 56 ++++++++-- lgsf/storage/tests/__init__.py | 0 lgsf/storage/tests/test_storage_modes.py | 136 +++++++++++++++++++++++ 6 files changed, 235 insertions(+), 20 deletions(-) create mode 100644 lgsf/storage/tests/__init__.py create mode 100644 lgsf/storage/tests/test_storage_modes.py diff --git a/lgsf/storage/backends/__init__.py b/lgsf/storage/backends/__init__.py index ba9f3c29..a6f00adb 100644 --- a/lgsf/storage/backends/__init__.py +++ b/lgsf/storage/backends/__init__.py @@ -1,7 +1,7 @@ import os from typing import Optional -from lgsf.storage.backends.base import BaseStorage +from lgsf.storage.backends.base import BaseStorage, StorageMode def detect_storage_backend_from_environment(options: dict) -> str: @@ -44,6 +44,8 @@ def get_storage_backend( options: Scraper options dictionary for backend detection. **kwargs: Additional backend-specific parameters: - scraper_object_type: For github backend, the type of scraper data + - storage_mode: StorageMode.REPLACE (default) or + StorageMode.ACCUMULATE - organization: For github backend, the organization name - github_token: For github backend, the authentication token @@ -61,10 +63,16 @@ def get_storage_backend( backend_type = backend_type.lower() + storage_mode = kwargs.get("storage_mode", StorageMode.REPLACE) + if backend_type == "local": from lgsf.storage.backends.local import LocalFilesystemStorage - return LocalFilesystemStorage(council_code=council_code) + return LocalFilesystemStorage( + council_code=council_code, + scraper_object_type=kwargs.get("scraper_object_type"), + storage_mode=storage_mode, + ) elif backend_type == "github": scraper_object_type = kwargs.get("scraper_object_type", "Data") from lgsf.storage.backends.github import GitHubStorage @@ -74,6 +82,7 @@ def get_storage_backend( scraper_object_type=scraper_object_type, organization=kwargs.get("organization"), github_token=kwargs.get("github_token"), + storage_mode=storage_mode, ) else: raise ValueError(f"Unsupported storage backend: {backend_type}") diff --git a/lgsf/storage/backends/base.py b/lgsf/storage/backends/base.py index 87d9e8cc..b464a151 100644 --- a/lgsf/storage/backends/base.py +++ b/lgsf/storage/backends/base.py @@ -1,9 +1,24 @@ import abc import contextlib +from enum import StrEnum from pathlib import Path from typing import Iterator, Literal, Optional, Union +class StorageMode(StrEnum): + """How a run should treat whatever previous runs left behind.""" + + #: Each run replaces everything previously stored for this council and data + #: type. Correct for data where the latest scrape is the whole truth (e.g. + #: councillors: someone who has left the council should disappear). + REPLACE = "replace" + + #: Each run adds to what is already stored, leaving untouched anything it + #: didn't scrape this time. Correct for append-only historical records (e.g. + #: minutes: a meeting from last year is still a fact). + ACCUMULATE = "accumulate" + + class StorageSession(abc.ABC): """ A storage session provides isolated, transactional operations on files. @@ -201,7 +216,9 @@ class BaseStorage(abc.ABC): - Session objects should not be shared across threads """ - def __init__(self, council_code: str): + def __init__( + self, council_code: str, storage_mode: StorageMode = StorageMode.REPLACE + ): """ Initialize storage backend for a specific council. @@ -209,14 +226,27 @@ def __init__(self, council_code: str): council_code: Identifier for the council/organization this storage instance will serve. Must be non-empty and contain only safe characters. + storage_mode: Either StorageMode.REPLACE (each run wipes what came + before) or StorageMode.ACCUMULATE (each run adds to + it). Plain strings are accepted and coerced. See + StorageMode for when each is appropriate. Raises: - ValueError: If council_code is invalid (empty, unsafe characters) + ValueError: If council_code or storage_mode is invalid """ if not council_code or not council_code.strip(): raise ValueError("council_code cannot be empty") + try: + storage_mode = StorageMode(storage_mode) + except ValueError: + raise ValueError( + f"Unknown storage_mode {storage_mode!r}, " + f"expected one of {', '.join(StorageMode)}" + ) from None + self.council_code = council_code.strip() + self.storage_mode = storage_mode # ---- Session lifecycle ---- def start_session(self, **kwargs) -> StorageSession: diff --git a/lgsf/storage/backends/github.py b/lgsf/storage/backends/github.py index a4d49ffa..70feb7ba 100644 --- a/lgsf/storage/backends/github.py +++ b/lgsf/storage/backends/github.py @@ -14,7 +14,7 @@ import jwt import requests -from lgsf.storage.backends.base import BaseStorage, StorageSession +from lgsf.storage.backends.base import BaseStorage, StorageMode, StorageSession logger = logging.getLogger(__name__) @@ -134,11 +134,13 @@ def __init__( council_code: str, scraper_object_type: str = "Data", run_id: str | None = None, + storage_mode: StorageMode = StorageMode.REPLACE, ): self.organization: str = organization self.github_token: str = github_token self.council_code: str = council_code self.scraper_object_type: str = scraper_object_type + self.storage_mode: str = storage_mode self.run_id: str = run_id or self._generate_run_id() # Repository configuration @@ -726,13 +728,15 @@ def _commit_files(self, commit_message: str) -> dict[str, Any]: # Create and checkout new branch self._create_and_checkout_branch() - # Delete all existing files in the scraper object type folder + # In REPLACE mode the latest scrape is the whole truth, so clear + # the folder first and let the commit record the deletions. In + # ACCUMULATE mode previous runs are history we're adding to, so + # keep them and let git diff pick up only what actually changed. scraper_dir = os.path.join(self.local_repo_path, self.scraper_object_type) - if os.path.exists(scraper_dir): + if self.storage_mode == StorageMode.REPLACE and os.path.exists(scraper_dir): logger.info(f"Deleting existing files in {self.scraper_object_type}/") shutil.rmtree(scraper_dir) - # Create directory os.makedirs(scraper_dir, exist_ok=True) # Write all staged files to disk @@ -897,6 +901,7 @@ def __init__( organization: str | None = None, github_token: str | None = None, auto_merge: bool = True, + storage_mode: StorageMode = StorageMode.REPLACE, ): """ Initialize GitHub storage backend. @@ -908,7 +913,7 @@ def __init__( github_token: GitHub authentication token (deprecated, ignored - GitHub App required) auto_merge: If True, automatically merge PRs after creation (always True in this version) """ - super().__init__(council_code) + super().__init__(council_code, storage_mode=storage_mode) self.scraper_object_type: str = scraper_object_type self.organization: str = organization or os.environ.get( @@ -959,6 +964,7 @@ def _start_session(self, **kwargs: Any) -> StorageSession: council_code=self.council_code, scraper_object_type=scraper_type, run_id=run_id, + storage_mode=self.storage_mode, ) self._active = session diff --git a/lgsf/storage/backends/local.py b/lgsf/storage/backends/local.py index 01e9000a..69bc11e4 100644 --- a/lgsf/storage/backends/local.py +++ b/lgsf/storage/backends/local.py @@ -5,7 +5,7 @@ from uuid import uuid4 from lgsf.conf import settings -from lgsf.storage.backends.base import BaseStorage, StorageSession +from lgsf.storage.backends.base import BaseStorage, StorageMode, StorageSession class _LocalPathlibSession(StorageSession): @@ -148,8 +148,13 @@ class LocalFilesystemStorage(BaseStorage): session.write(Path("output.csv"), processed) """ - def __init__(self, council_code: str): - super().__init__(council_code) + def __init__( + self, + council_code: str, + scraper_object_type: Optional[str] = None, + storage_mode: StorageMode = StorageMode.REPLACE, + ): + super().__init__(council_code, storage_mode=storage_mode) self.root = Path(settings.DATA_DIR_NAME) self.encoding = "utf8" self._active: Optional[_LocalPathlibSession] = None @@ -161,6 +166,32 @@ def __init__(self, council_code: str): self.safe_council_code = safe_council_code + # Scope sessions to a per-data-type subdirectory (matching the + # GitHub backend's layout) so that, for example, a minutes run + # can't wipe councillors data for the same council. + self.scraper_object_type = None + if scraper_object_type: + safe_object_type = "".join( + c for c in scraper_object_type if c.isalnum() or c in "_-" + ) + if not safe_object_type: + raise ValueError(f"Invalid scraper_object_type: {scraper_object_type}") + self.scraper_object_type = safe_object_type + + @property + def council_root(self) -> Path: + """ + Where this council's data for this scraper type lives. + + Exposed so callers can write alongside the scraped data - a run + log, say - without repeating the council code sanitising or + guessing the layout. + """ + root = self.root / self.safe_council_code + if self.scraper_object_type: + root = root / self.scraper_object_type + return root + def _start_session(self, **kwargs) -> StorageSession: """ Create a new local filesystem session for this instance's council. @@ -183,16 +214,16 @@ def _start_session(self, **kwargs) -> StorageSession: "A session is already active on this LocalFilesystemStorage instance." ) - # Clean and recreate council-specific subdirectory - council_root = self.root / self.safe_council_code + council_root = self.council_root try: - # Clean existing directory if it exists - if council_root.exists(): + # In REPLACE mode the latest scrape is the whole truth, so clear + # out anything from previous runs. In ACCUMULATE mode previous + # runs are history we're adding to, so leave them alone. + if self.storage_mode == StorageMode.REPLACE and council_root.exists(): import shutil shutil.rmtree(council_root) - # Create fresh directory council_root.mkdir(parents=True, exist_ok=True) except OSError as e: raise RuntimeError( @@ -286,9 +317,12 @@ def _end_session(self, session: StorageSession, commit_message: str, **kwargs): } try: - summary_data.update(run_log.as_json) - except (AttributeError, TypeError): - pass # run_log might not have as_json method or might not be serializable + # as_dict, not as_json: updating a dict with a JSON + # string raises, and the failure is swallowed far + # enough away that the summary file is never written. + summary_data.update(run_log.as_dict) + except (AttributeError, TypeError, ValueError): + pass # a run log that can't be serialised isn't fatal with summary_path.open("w", encoding="utf-8") as f: json.dump(summary_data, f, indent=2, default=str) diff --git a/lgsf/storage/tests/__init__.py b/lgsf/storage/tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/lgsf/storage/tests/test_storage_modes.py b/lgsf/storage/tests/test_storage_modes.py new file mode 100644 index 00000000..81f6a5cf --- /dev/null +++ b/lgsf/storage/tests/test_storage_modes.py @@ -0,0 +1,136 @@ +""" +Storage mode behaviour: StorageMode.REPLACE wipes what came before, +StorageMode.ACCUMULATE adds to it. + +This is the difference between councillors (the current set is the whole +truth) and minutes (an append-only historical record), so it is worth +pinning down explicitly for both. +""" + +from pathlib import Path + +import pytest + +from lgsf.conf import settings +from lgsf.storage.backends import get_storage_backend +from lgsf.storage.backends.base import StorageMode +from lgsf.storage.backends.local import LocalFilesystemStorage + + +@pytest.fixture(autouse=True) +def data_dir(tmp_path, monkeypatch): + """Point storage at a temporary directory rather than the repo's data/.""" + monkeypatch.setattr(settings, "DATA_DIR_NAME", str(tmp_path)) + return tmp_path + + +def stored_files(root): + return sorted( + p.relative_to(root).as_posix() for p in root.rglob("*") if p.is_file() + ) + + +def test_replace_mode_clears_previous_runs(data_dir): + storage = LocalFilesystemStorage( + council_code="ABC", + scraper_object_type="Councillors", + storage_mode=StorageMode.REPLACE, + ) + with storage.session("run 1") as session: + session.write(Path("json/departed-councillor.json"), "{}") + + with storage.session("run 2") as session: + session.write(Path("json/current-councillor.json"), "{}") + + root = data_dir / "ABC" / "Councillors" + assert stored_files(root) == ["json/current-councillor.json"] + + +def test_accumulate_mode_keeps_previous_runs(data_dir): + storage = LocalFilesystemStorage( + council_code="ABC", + scraper_object_type="Minutes", + storage_mode=StorageMode.ACCUMULATE, + ) + with storage.session("run 1") as session: + session.write(Path("json/2026-01-01-old-meeting.json"), "{}") + session.write_bytes(Path("raw/2026-01-01-old.xml"), b"") + + with storage.session("run 2") as session: + session.write(Path("json/2026-08-01-new-meeting.json"), "{}") + + root = data_dir / "ABC" / "Minutes" + assert stored_files(root) == [ + "json/2026-01-01-old-meeting.json", + "json/2026-08-01-new-meeting.json", + "raw/2026-01-01-old.xml", + ] + + +def test_accumulate_mode_still_overwrites_the_same_file(data_dir): + """Accumulating adds new files; it doesn't stop a meeting being updated.""" + storage = LocalFilesystemStorage( + council_code="ABC", + scraper_object_type="Minutes", + storage_mode=StorageMode.ACCUMULATE, + ) + with storage.session("run 1") as session: + session.write(Path("json/meeting.json"), '{"status": "Provisional"}') + + with storage.session("run 2") as session: + session.write(Path("json/meeting.json"), '{"status": "Confirmed"}') + + root = data_dir / "ABC" / "Minutes" + assert (root / "json/meeting.json").read_text() == '{"status": "Confirmed"}' + + +def test_accumulate_mode_can_read_back_earlier_runs(data_dir): + """A later run must be able to see what an earlier one stored.""" + storage = LocalFilesystemStorage( + council_code="ABC", + scraper_object_type="Minutes", + storage_mode=StorageMode.ACCUMULATE, + ) + with storage.session("run 1") as session: + session.write(Path("_index.json"), '{"version": 1}') + + session = storage.start_session() + assert session.open(Path("_index.json")) == '{"version": 1}' + storage.end_session(session, "run 2") + + +def test_replace_mode_cannot_read_back_earlier_runs(data_dir): + """The mirror image: replacing means last run's state is gone.""" + storage = LocalFilesystemStorage( + council_code="ABC", + scraper_object_type="Councillors", + storage_mode=StorageMode.REPLACE, + ) + with storage.session("run 1") as session: + session.write(Path("_index.json"), '{"version": 1}') + + session = storage.start_session() + with pytest.raises(FileNotFoundError): + session.open(Path("_index.json")) + storage.end_session(session, "run 2") + + +def test_default_mode_is_replace(data_dir): + assert ( + LocalFilesystemStorage(council_code="ABC").storage_mode == StorageMode.REPLACE + ) + + +def test_unknown_storage_mode_is_rejected(data_dir): + with pytest.raises(ValueError, match="Unknown storage_mode"): + LocalFilesystemStorage(council_code="ABC", storage_mode="append") + + +def test_factory_passes_storage_mode_through(data_dir): + storage = get_storage_backend( + council_code="ABC", + options={}, + scraper_object_type="Minutes", + storage_mode=StorageMode.ACCUMULATE, + ) + assert storage.storage_mode == StorageMode.ACCUMULATE From d92a3764f3df5e70bf78da5e7c5443718cc8ea68 Mon Sep 17 00:00:00 2001 From: symroe Date: Fri, 21 Aug 2026 09:04:31 +0100 Subject: [PATCH 02/12] Add pluggable storage for scraped document files Scraped documents are large binaries with different needs from the metadata that describes them: a meeting's JSON is a few KB of text that belongs in version control, while the agenda pack behind it can be hundreds of megabytes that belongs in an object store. lgsf/storage/documents/ stores those files, deliberately independent of the metadata backend so the two can be pointed at different places. The default LocalDocumentStorage writes to data//documents/, beside rather than inside the per-type metadata directories, since documents are shared across scraper types and excluded from version control. Each stored document reports its sha256 hash, size, storage key and backend name, which is what keeps metadata and file linked. The resolved URL is deliberately not part of that: for local storage it is an absolute filesystem path, so recording it would put a developer's home directory into metadata destined for git and give every person a different value for the same file. Backends rebuild it from the key via url_for(). Unlike the metadata backends this is not session based. Documents are immutable once written, so exists() is a cheap and reliable way for a scraper to skip work it has already done. Co-Authored-By: Claude Opus 5 --- lgsf/storage/documents/__init__.py | 75 ++++++++ lgsf/storage/documents/base.py | 147 ++++++++++++++ lgsf/storage/documents/local.py | 94 +++++++++ lgsf/storage/tests/test_document_storage.py | 202 ++++++++++++++++++++ 4 files changed, 518 insertions(+) create mode 100644 lgsf/storage/documents/__init__.py create mode 100644 lgsf/storage/documents/base.py create mode 100644 lgsf/storage/documents/local.py create mode 100644 lgsf/storage/tests/test_document_storage.py diff --git a/lgsf/storage/documents/__init__.py b/lgsf/storage/documents/__init__.py new file mode 100644 index 00000000..b74dec37 --- /dev/null +++ b/lgsf/storage/documents/__init__.py @@ -0,0 +1,75 @@ +"""Document storage backends and the factory used to select one.""" + +import os +from typing import Optional + +from lgsf.storage.documents.base import ( + BaseDocumentStorage, + StoredDocument, + hash_content, +) + +__all__ = [ + "BaseDocumentStorage", + "StoredDocument", + "hash_content", + "get_document_storage_backend", + "get_available_document_backends", +] + + +def detect_document_storage_backend_from_environment(options: dict) -> str: + """ + Work out which document backend to use, in priority order: an explicit + option, then the LGSF_DOCUMENT_STORAGE_BACKEND environment variable, + then local storage. + + Note this is deliberately independent of the metadata storage backend: + a Lambda run will typically write metadata to GitHub while putting the + documents themselves in an object store. + """ + if options and "document_storage_backend" in options: + return options["document_storage_backend"] + + backend_from_env = os.environ.get("LGSF_DOCUMENT_STORAGE_BACKEND") + if backend_from_env: + return backend_from_env.lower() + + return "local" + + +def get_document_storage_backend( + council_code: str, + backend_type: Optional[str] = None, + options: Optional[dict] = None, + **kwargs, +) -> BaseDocumentStorage: + """ + Get a document storage backend instance for a specific council. + + Args: + council_code: Council identifier this storage instance will serve. + backend_type: Backend to create. If None, detected from options and + environment. + options: Scraper options dictionary, used for detection. + **kwargs: Additional backend-specific parameters. + + Raises: + ValueError: If the backend_type is not supported. + """ + if backend_type is None: + backend_type = detect_document_storage_backend_from_environment(options or {}) + + backend_type = backend_type.lower() + + if backend_type == "local": + from lgsf.storage.documents.local import LocalDocumentStorage + + return LocalDocumentStorage(council_code=council_code) + + raise ValueError(f"Unsupported document storage backend: {backend_type}") + + +def get_available_document_backends() -> list[str]: + """Return a list of available document storage backend types.""" + return ["local"] diff --git a/lgsf/storage/documents/base.py b/lgsf/storage/documents/base.py new file mode 100644 index 00000000..a02dec98 --- /dev/null +++ b/lgsf/storage/documents/base.py @@ -0,0 +1,147 @@ +""" +Pluggable storage for scraped document *files*. + +This is deliberately separate from ``lgsf.storage.backends``, which stores +scraper *metadata* (the JSON and raw pages that we want in git). Documents +are large binaries with very different storage needs: the metadata for a +meeting is a few KB of text that belongs in version control, while the +agenda pack behind it can be hundreds of megabytes of PDFs that probably +belongs in an object store. + +Keeping the two apart means the metadata can be committed to git exactly +like councillors data, while the documents it references are addressed by +key and can live wherever suits the deployment. The metadata records the +content hash and the storage key/URL, so the two halves stay linked. +""" + +from __future__ import annotations + +import abc +import hashlib +from dataclasses import dataclass + + +@dataclass(frozen=True) +class StoredDocument: + """ + The result of storing one document, recorded in the meeting metadata so + the file can be found again (and checked for changes) later. + """ + + #: Backend-scoped identifier for this document, e.g. a relative path or + #: an object store key. + key: str + + #: Where the document can be retrieved from right now. A ``file://`` URL + #: for local storage, an ``https://`` or ``s3://`` URL for an object + #: store. Deliberately *not* recorded in metadata - see ``as_dict()``. + url: str + + #: ``sha256:`` of the file contents. + content_hash: str + + #: Size of the stored file in bytes. + content_length: int + + #: Name of the backend that stored it, e.g. ``"local"``. + backend: str + + def as_dict(self) -> dict: + """ + What gets recorded in the scraped metadata. + + The key and backend name are stored, but the resolved URL is not: + it is specific to wherever the scrape happened to run. For local + storage that would bake a developer's home directory into metadata + destined for git, so the same document would produce a different + value for every person who scraped it. The key is the document's + path within its backend, and the backend turns that back into a + URL via ``url_for()`` whenever one is needed. + """ + return { + "storage_key": self.key, + "storage_backend": self.backend, + "content_hash": self.content_hash, + "content_length": self.content_length, + } + + +def hash_content(content: bytes) -> str: + """Return the ``sha256:`` hash recorded against a document.""" + return "sha256:{}".format(hashlib.sha256(content).hexdigest()) + + +class BaseDocumentStorage(abc.ABC): + """ + Abstract base class for document storage backends. + + Unlike :class:`lgsf.storage.backends.base.BaseStorage`, document storage + is not session based. Documents are immutable once written: a given key + either holds the file or it doesn't, and there is no batch to commit or + roll back. That makes ``exists()`` a cheap, reliable way for a scraper to + skip work it has already done. + + Each instance is bound to a single council, mirroring the metadata + backends. + """ + + #: Short name recorded in metadata as ``storage_backend``. + name: str = "" + + def __init__(self, council_code: str): + if not council_code or not council_code.strip(): + raise ValueError("council_code cannot be empty") + self.council_code = council_code.strip() + + @abc.abstractmethod + def exists(self, key: str) -> bool: + """Return True if a document is already stored under ``key``.""" + ... + + @abc.abstractmethod + def write(self, key: str, content: bytes) -> StoredDocument: + """ + Store ``content`` under ``key``, overwriting anything already there. + + Returns a :class:`StoredDocument` describing where it went. + """ + ... + + @abc.abstractmethod + def read(self, key: str) -> bytes: + """ + Return the stored document's contents. + + Raises FileNotFoundError if nothing is stored under ``key``. + """ + ... + + @abc.abstractmethod + def url_for(self, key: str) -> str: + """ + Return the retrieval URL for ``key``, whether or not it exists. + + Resolved on demand rather than stored, so that metadata stays + portable between machines and deployments. + """ + ... + + def describe(self, key: str) -> StoredDocument | None: + """ + Return a :class:`StoredDocument` for an already-stored document, or + None if nothing is stored under ``key``. + + Used to re-record metadata for a document that was skipped because we + already had it, so a skipped document's JSON looks identical to a + freshly downloaded one's. + """ + if not self.exists(key): + return None + content = self.read(key) + return StoredDocument( + key=key, + url=self.url_for(key), + content_hash=hash_content(content), + content_length=len(content), + backend=self.name, + ) diff --git a/lgsf/storage/documents/local.py b/lgsf/storage/documents/local.py new file mode 100644 index 00000000..3691c6fe --- /dev/null +++ b/lgsf/storage/documents/local.py @@ -0,0 +1,94 @@ +"""Local filesystem document storage: the default backend.""" + +from __future__ import annotations + +from pathlib import Path +from uuid import uuid4 + +from lgsf.conf import settings +from lgsf.storage.documents.base import ( + BaseDocumentStorage, + StoredDocument, + hash_content, +) + + +class LocalDocumentStorage(BaseDocumentStorage): + """ + Stores documents on the local filesystem under + ``data//documents/``. + + Sitting alongside (not inside) the per-data-type metadata directories + means the same document store is shared by every scraper type for a + council, and that ``data//documents/`` can be excluded from + version control while the metadata beside it is committed. + + Writes go via a temporary file and an atomic rename, so a crashed or + interrupted run can never leave a half-written document that a later + run would mistake for a complete one. + """ + + name = "local" + + def __init__(self, council_code: str): + super().__init__(council_code) + + safe_council_code = "".join( + c for c in self.council_code if c.isalnum() or c in "_-" + ) + if not safe_council_code: + raise ValueError(f"Invalid council_code: {council_code}") + + self.root = ( + Path(settings.DATA_DIR_NAME) / safe_council_code / "documents" + ).resolve() + + def _path(self, key: str) -> Path: + """Resolve ``key`` to a path, refusing anything outside the root.""" + if not key or not key.strip(): + raise ValueError("Document key cannot be empty") + + candidate = Path(key) + if candidate.is_absolute(): + raise ValueError(f"Absolute keys not allowed: {key}") + if ".." in candidate.parts: + raise ValueError(f"Path traversal not allowed: {key}") + + resolved = (self.root / candidate).resolve() + if not resolved.is_relative_to(self.root): + raise ValueError(f"Key {key} resolves outside {self.root}") + return resolved + + def exists(self, key: str) -> bool: + return self._path(key).is_file() + + def write(self, key: str, content: bytes) -> StoredDocument: + target = self._path(key) + target.parent.mkdir(parents=True, exist_ok=True) + + tmp = target.with_name(f".tmp-{uuid4().hex}-{target.name}") + try: + with tmp.open("xb") as f: + f.write(content) + f.flush() + tmp.replace(target) + except OSError: + tmp.unlink(missing_ok=True) + raise + + return StoredDocument( + key=key, + url=self.url_for(key), + content_hash=hash_content(content), + content_length=len(content), + backend=self.name, + ) + + def read(self, key: str) -> bytes: + path = self._path(key) + if not path.is_file(): + raise FileNotFoundError(str(path)) + return path.read_bytes() + + def url_for(self, key: str) -> str: + return self._path(key).as_uri() diff --git a/lgsf/storage/tests/test_document_storage.py b/lgsf/storage/tests/test_document_storage.py new file mode 100644 index 00000000..148d44bd --- /dev/null +++ b/lgsf/storage/tests/test_document_storage.py @@ -0,0 +1,202 @@ +"""Document storage backend behaviour.""" + +from pathlib import Path + +import pytest + +from lgsf.conf import settings +from lgsf.storage.documents import ( + get_available_document_backends, + get_document_storage_backend, + hash_content, +) +from lgsf.storage.documents.local import LocalDocumentStorage + + +@pytest.fixture(autouse=True) +def data_dir(tmp_path, monkeypatch): + monkeypatch.setattr(settings, "DATA_DIR_NAME", str(tmp_path)) + return tmp_path + + +def test_documents_are_stored_under_the_council_documents_dir(data_dir): + storage = LocalDocumentStorage(council_code="ABC") + stored = storage.write("2026-08-19-council-1234.pdf", b"%PDF-fake") + + expected = data_dir / "ABC" / "documents" / "2026-08-19-council-1234.pdf" + assert expected.is_file() + assert expected.read_bytes() == b"%PDF-fake" + assert stored.key == "2026-08-19-council-1234.pdf" + assert stored.url == expected.resolve().as_uri() + assert stored.backend == "local" + + +def test_stored_document_records_hash_and_length(data_dir): + storage = LocalDocumentStorage(council_code="ABC") + stored = storage.write("doc.pdf", b"%PDF-fake") + + assert stored.content_hash == hash_content(b"%PDF-fake") + assert stored.content_hash.startswith("sha256:") + assert stored.content_length == len(b"%PDF-fake") + + +def test_as_dict_is_what_gets_recorded_in_meeting_metadata(data_dir): + storage = LocalDocumentStorage(council_code="ABC") + stored = storage.write("doc.pdf", b"%PDF-fake") + + assert stored.as_dict() == { + "storage_key": "doc.pdf", + "storage_backend": "local", + "content_hash": stored.content_hash, + "content_length": 9, + } + + +def test_metadata_does_not_record_a_machine_specific_path(data_dir): + """ + The resolved URL contains an absolute filesystem path, which differs + for every developer. Metadata goes to git, so it must not carry one. + """ + storage = LocalDocumentStorage(council_code="ABC") + stored = storage.write("doc.pdf", b"content") + + assert str(data_dir) in stored.url + assert not any(str(data_dir) in str(v) for v in stored.as_dict().values()) + + +def test_the_backend_can_rebuild_the_url_from_the_stored_key(data_dir): + """The key is what gets recorded; the URL resolves from it on demand.""" + storage = LocalDocumentStorage(council_code="ABC") + stored = storage.write("doc.pdf", b"content") + + recorded = stored.as_dict() + assert storage.url_for(recorded["storage_key"]) == stored.url + + +def test_exists_and_read_round_trip(data_dir): + storage = LocalDocumentStorage(council_code="ABC") + assert storage.exists("doc.pdf") is False + + storage.write("doc.pdf", b"content") + assert storage.exists("doc.pdf") is True + assert storage.read("doc.pdf") == b"content" + + +def test_read_missing_document_raises(data_dir): + storage = LocalDocumentStorage(council_code="ABC") + with pytest.raises(FileNotFoundError): + storage.read("nope.pdf") + + +def test_describe_returns_none_for_missing_document(data_dir): + storage = LocalDocumentStorage(council_code="ABC") + assert storage.describe("nope.pdf") is None + + +def test_describe_rebuilds_metadata_for_a_stored_document(data_dir): + storage = LocalDocumentStorage(council_code="ABC") + written = storage.write("doc.pdf", b"content") + + described = storage.describe("doc.pdf") + assert described == written + + +def test_documents_are_isolated_per_council(data_dir): + LocalDocumentStorage(council_code="ABC").write("doc.pdf", b"abc") + LocalDocumentStorage(council_code="XYZ").write("doc.pdf", b"xyz") + + assert (data_dir / "ABC" / "documents" / "doc.pdf").read_bytes() == b"abc" + assert (data_dir / "XYZ" / "documents" / "doc.pdf").read_bytes() == b"xyz" + + +def test_documents_sit_beside_not_inside_the_metadata_dirs(data_dir): + """ + Documents are shared across scraper types and excluded from version + control, so they must not land inside data///. + """ + storage = LocalDocumentStorage(council_code="ABC") + storage.write("doc.pdf", b"x") + + assert (data_dir / "ABC" / "documents").is_dir() + assert not (data_dir / "ABC" / "Minutes").exists() + + +def test_writes_are_atomic_leaving_no_temp_files(data_dir): + storage = LocalDocumentStorage(council_code="ABC") + storage.write("nested/dir/doc.pdf", b"x") + + files = [p.name for p in (data_dir / "ABC" / "documents").rglob("*") if p.is_file()] + assert files == ["doc.pdf"] + + +def test_overwriting_a_key_replaces_the_content(data_dir): + storage = LocalDocumentStorage(council_code="ABC") + storage.write("doc.pdf", b"first") + stored = storage.write("doc.pdf", b"second") + + assert storage.read("doc.pdf") == b"second" + assert stored.content_hash == hash_content(b"second") + + +@pytest.mark.parametrize( + "key", ["../escape.pdf", "/absolute.pdf", "nested/../../escape.pdf", ""] +) +def test_unsafe_keys_are_rejected(data_dir, key): + storage = LocalDocumentStorage(council_code="ABC") + with pytest.raises(ValueError): + storage.write(key, b"x") + + +def test_factory_defaults_to_local(data_dir): + storage = get_document_storage_backend(council_code="ABC", options={}) + assert isinstance(storage, LocalDocumentStorage) + + +def test_factory_honours_an_explicit_option(data_dir): + storage = get_document_storage_backend( + council_code="ABC", options={"document_storage_backend": "local"} + ) + assert isinstance(storage, LocalDocumentStorage) + + +def test_factory_honours_the_environment_variable(data_dir, monkeypatch): + monkeypatch.setenv("LGSF_DOCUMENT_STORAGE_BACKEND", "local") + storage = get_document_storage_backend(council_code="ABC", options={}) + assert isinstance(storage, LocalDocumentStorage) + + +def test_factory_rejects_unknown_backends(data_dir): + with pytest.raises(ValueError, match="Unsupported document storage backend"): + get_document_storage_backend(council_code="ABC", backend_type="magic") + + +def test_document_backend_is_independent_of_metadata_backend(data_dir, monkeypatch): + """ + The whole point of the split: metadata can go to git while documents go + somewhere else, so selecting one must not select the other. + """ + monkeypatch.setenv("LGSF_STORAGE_BACKEND", "github") + storage = get_document_storage_backend(council_code="ABC", options={}) + assert isinstance(storage, LocalDocumentStorage) + + +def test_available_backends_listed(data_dir): + assert get_available_document_backends() == ["local"] + + +def test_hash_content_is_stable(data_dir): + assert hash_content(b"x") == hash_content(b"x") + assert hash_content(b"x") != hash_content(b"y") + + +def test_empty_council_code_rejected(data_dir): + with pytest.raises(ValueError): + LocalDocumentStorage(council_code=" ") + + +def test_url_for_works_without_the_file_existing(data_dir): + storage = LocalDocumentStorage(council_code="ABC") + url = storage.url_for("doc.pdf") + assert url.startswith("file://") + assert url.endswith("/ABC/documents/doc.pdf") + assert not Path(storage._path("doc.pdf")).exists() From e2914f7028f9920f78e07f06d99882af36619814 Mon Sep 17 00:00:00 2001 From: symroe Date: Fri, 21 Aug 2026 09:04:31 +0100 Subject: [PATCH 03/12] Give scrapers conditional requests and a document store Three additions to ScraperBase, all needed by scrapers that fetch documents or revisit the same pages on a schedule. get_conditional() sends If-None-Match and If-Modified-Since so a server can answer 304 instead of resending a body. response_status() and response_header() normalise the differences between the four supported HTTP clients, since wreq returns a StatusCode object and bytes-valued headers where the others return ints and strings. get() was also silently discarding extra_headers under wreq, which made conditional requests impossible; headers are now merged over the emulated browser set rather than dropped. document_storage is a lazily created document backend, so scrapers that store no documents never construct one. storage_mode declares whether this scraper's data replaces or accumulates. Co-Authored-By: Claude Opus 5 --- lgsf/scrapers/base.py | 92 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 90 insertions(+), 2 deletions(-) diff --git a/lgsf/scrapers/base.py b/lgsf/scrapers/base.py index 1c2fd076..e0af7a05 100644 --- a/lgsf/scrapers/base.py +++ b/lgsf/scrapers/base.py @@ -2,6 +2,7 @@ import datetime import os import traceback +from functools import cached_property from pathlib import Path import httpx @@ -11,6 +12,8 @@ from ..metadata.models import CouncilMetadata from ..storage.backends import get_storage_backend +from ..storage.backends.base import StorageMode +from ..storage.documents import get_document_storage_backend from .checks import ScraperChecker @@ -28,6 +31,12 @@ class ScraperBase(metaclass=abc.ABCMeta): scraper_object_type = None use_proxy = False + #: How this scraper's storage should treat previous runs. + #: StorageMode.REPLACE (the default) suits data where the latest scrape is + #: the whole truth; StorageMode.ACCUMULATE suits append-only historical + #: records. See lgsf.storage.backends.base. + storage_mode = StorageMode.REPLACE + def __init__(self, options, console): self.options = options self.console = console @@ -44,6 +53,7 @@ def __init__(self, options, console): council_code=self.council_id, options=self.options, scraper_object_type=self.scraper_object_type, + storage_mode=self.storage_mode, ) self.storage_session = self.storage_backend.start_session() @@ -98,12 +108,17 @@ def get(self, url, extra_headers=None): if self.options.get("verbose"): self.console.log(f"Scraping from {url}") - # Don't change headers for wreq, as it does it for us + # wreq builds a full set of browser-emulation headers for us, so we + # only pass extra_headers when there are some: anything we supply is + # merged on top of the emulated set rather than replacing it. if self.http_lib == "wreq": # See: https://github.com/0x676e67/wreq-python/issues/405 + wreq_kwargs = {"timeout": datetime.timedelta(seconds=self.timeout)} + if extra_headers: + wreq_kwargs["headers"] = extra_headers response = self.http_client.get( url.replace(" ", "%20"), - timeout=datetime.timedelta(seconds=self.timeout), + **wreq_kwargs, ) elif self.http_lib == "playwright": response = self.http_client.get( @@ -121,6 +136,60 @@ def get(self, url, extra_headers=None): response.raise_for_status() return response + def response_status(self, response) -> int: + """ + Return the HTTP status as an int. + + Each supported client models this differently: wreq exposes a + StatusCode object with .as_int(), requests and httpx use an int + .status_code. + """ + status = getattr(response, "status_code", None) + if isinstance(status, int): + return status + + status = getattr(response, "status", None) + if status is None: + raise ValueError(f"Can't read status from {type(response)} response") + if isinstance(status, int): + return status + if hasattr(status, "as_int"): + return status.as_int() + return int(status) + + def response_header(self, response, name: str): + """ + Return a response header as a string, or None if it isn't set. + + wreq's HeaderMap returns bytes; requests and httpx return str. + """ + headers = getattr(response, "headers", None) + if headers is None: + return None + value = headers.get(name) + if value is None: + return None + if isinstance(value, bytes): + return value.decode("utf-8", errors="replace") + return value + + def get_conditional(self, url, etag=None, last_modified=None, extra_headers=None): + """ + GET a URL, telling the server what we already have so it can answer + 304 Not Modified instead of resending the body. + + Returns the response either way; callers should check + ``self.response_status(response) == 304`` before using the body. + Servers that ignore conditional headers answer 200 as usual. + """ + headers = dict(extra_headers or {}) + if etag: + headers["If-None-Match"] = etag + if last_modified: + headers["If-Modified-Since"] = last_modified + + return self.get(url, extra_headers=headers or None) + def get_text(self, url, extra_headers=None): """ Wraps self.get and always returns the response text. @@ -134,6 +203,25 @@ def get_text(self, url, extra_headers=None): text = text() return text + @cached_property + def document_storage(self): + """ + Where this scraper's document *files* go, as opposed to its metadata. + + Lazily created, so scrapers that don't download documents never + construct one. Selected independently of the metadata backend: see + lgsf.storage.documents. + """ + return get_document_storage_backend( + council_code=self.council_id, + options=self.options, + ) + + @property + def report_items(self): + """Objects collected during the run, used by the --report flag.""" + return [] + def check(self): assert self.service_name, "Scrapers must set a service_name" assert self.scraper_object_type, "Scrapers must set a scraper object type" From b9855a0ed6a3ea5e3715d05d2f2734c085c1b630 Mon Sep 17 00:00:00 2001 From: symroe Date: Fri, 21 Aug 2026 09:04:31 +0100 Subject: [PATCH 04/12] Key council metadata services by name CouncilMetadata had one field per service, plus matching branches in from_file(), to_dict() and a SERVICE_NAMES tuple. Adding a service meant four coordinated edits, each with its own failure mode: missing the to_dict() one silently dropped that service's config from every council the next time metadata was saved, and missing SERVICE_NAMES made get_service_metadata() return None so every scraper saw a base_url of None. Services are now a dict keyed by name. A new service works as soon as a council's metadata.json declares it, with no change to the model and no list of permitted names to keep in sync. get_service_metadata() always returns a ServiceData rather than None for an unknown service, so callers can read .base_url without a null check and get None for "not configured" either way. get_summary() reports every configured service rather than singling councillors out: a council has a cms_type and base_url per service, and there is no such thing as "the" one. Verified behaviour-preserving by loading all 435 scrapers/*/metadata.json through both the old and new models and comparing serialised output: identical for every file. Co-Authored-By: Claude Opus 5 --- lgsf/metadata/models.py | 76 +++++-- lgsf/metadata/tests/__init__.py | 0 lgsf/metadata/tests/test_council_metadata.py | 199 +++++++++++++++++++ 3 files changed, 256 insertions(+), 19 deletions(-) create mode 100644 lgsf/metadata/tests/__init__.py create mode 100644 lgsf/metadata/tests/test_council_metadata.py diff --git a/lgsf/metadata/models.py b/lgsf/metadata/models.py index a9f09ef2..6ee6f8f3 100644 --- a/lgsf/metadata/models.py +++ b/lgsf/metadata/models.py @@ -68,7 +68,14 @@ class CouncilMetadata: """Council metadata with file I/O.""" everyelection_data: EveryElectionData = field(default_factory=EveryElectionData) - councillors: ServiceData = field(default_factory=ServiceData) + + #: Per-service metadata, keyed by service name ("councillors", + #: "minutes", ...). Keying them means adding a scraper type needs no + #: change to this class: a service exists as soon as a council's + #: metadata.json declares one, and every service loads and saves + #: through the same code path. + services: Dict[str, ServiceData] = field(default_factory=dict) + everyelection_data_last_updated: Optional[str] = None @classmethod @@ -89,8 +96,10 @@ def from_file(cls, file_path: Path) -> "CouncilMetadata": data["everyelectiion_data"] ) - councillors_data = data["services"].get("councillors", {}) - metadata.councillors = ServiceData.from_dict(councillors_data) + for service_name, service_data in (data["services"] or {}).items(): + metadata.services[service_name] = ServiceData.from_dict( + service_data or {} + ) metadata.everyelection_data_last_updated = data.get( "everyelectiion_data_last_updated" @@ -105,9 +114,10 @@ def to_dict(self) -> Dict[str, Any]: "services": {}, } - councillors_dict = self.councillors.to_dict() - if councillors_dict: - result["services"]["councillors"] = councillors_dict + for service_name, service in sorted(self.services.items()): + service_dict = service.to_dict() + if service_dict: + result["services"][service_name] = service_dict if self.everyelection_data_last_updated: result["everyelectiion_data_last_updated"] = ( @@ -131,15 +141,33 @@ def update_everyelection_data(self, new_data: Dict[str, Any]) -> None: self.everyelection_data_last_updated = datetime.now().isoformat() def update_service_data(self, service_name: str, **kwargs) -> None: - """Update service data fields.""" - if service_name == "councillors": - self.councillors.update_from_dict(kwargs) - - def get_service_metadata(self, service_name: str) -> Optional[ServiceData]: - """Get metadata for a specific service type.""" - if service_name == "councillors": - return self.councillors - return None + """ + Update one service's fields, creating the service if it's new. + + There is deliberately no list of permitted service names: the + services a council has are whatever its metadata.json declares. + """ + self.services.setdefault(service_name, ServiceData()).update_from_dict(kwargs) + + def get_service_metadata(self, service_name: str) -> ServiceData: + """ + Get metadata for one service. + + Always returns a ServiceData. A service this council has no + configuration for comes back empty rather than as None, so callers + can read `.base_url` without a null check and get None for + "not configured" either way. + """ + return self.services.get(service_name) or ServiceData() + + def configured_services(self) -> list: + """Names of the services this council actually has config for.""" + return sorted( + name for name, service in self.services.items() if service.to_dict() + ) + + def has_service(self, service_name: str) -> bool: + return bool(self.services.get(service_name, ServiceData()).to_dict()) @classmethod def for_council(cls, council_id: str) -> "CouncilMetadata": @@ -154,11 +182,21 @@ def for_council(cls, council_id: str) -> "CouncilMetadata": return metadata def get_summary(self) -> Dict[str, Any]: - """Get summary of key metadata fields.""" + """ + Summary of this council's metadata. + + Every configured service is reported: a council has a cms_type and + base_url *per service*, so there is no single "the" one. + """ return { "official_identifier": self.everyelection_data.official_identifier, "common_name": self.everyelection_data.common_name, - "cms_type": self.councillors.cms_type, - "base_url": self.councillors.base_url, - "services": ["councillors"] if self.councillors.to_dict() else [], + "services": { + name: { + "cms_type": service.cms_type, + "base_url": service.base_url, + } + for name, service in sorted(self.services.items()) + if service.to_dict() + }, } diff --git a/lgsf/metadata/tests/__init__.py b/lgsf/metadata/tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/lgsf/metadata/tests/test_council_metadata.py b/lgsf/metadata/tests/test_council_metadata.py new file mode 100644 index 00000000..82cdb983 --- /dev/null +++ b/lgsf/metadata/tests/test_council_metadata.py @@ -0,0 +1,199 @@ +""" +CouncilMetadata service handling. + +Services are keyed by name rather than being one field per service, so +adding a scraper type needs no change to the model. The round-trip tests +carry the most weight: a service has to survive load-and-save intact, +including one the model knows nothing specific about. +""" + +import json + +import pytest + +from lgsf.metadata.models import CouncilMetadata, ServiceData + + +@pytest.fixture +def metadata_file(tmp_path): + path = tmp_path / "metadata.json" + path.write_text( + json.dumps( + { + "everyelectiion_data": { + "official_identifier": "ABC", + "common_name": "Anytown", + }, + "services": { + "councillors": { + "base_url": "https://example.gov.uk/councillors", + "cms_type": "ModernGov", + }, + "minutes": { + "base_url": "https://example.gov.uk/minutes", + "cms_type": "CMIS", + }, + }, + } + ) + ) + return path + + +def test_services_load_from_file(metadata_file): + metadata = CouncilMetadata.from_file(metadata_file) + + assert metadata.get_service_metadata("councillors").cms_type == "ModernGov" + assert metadata.get_service_metadata("minutes").cms_type == "CMIS" + + +def test_an_unconfigured_service_reads_as_empty_not_none(metadata_file): + """Callers read .base_url directly, so this must not be None.""" + metadata = CouncilMetadata.from_file(metadata_file) + + service = metadata.get_service_metadata("decisions") + + assert isinstance(service, ServiceData) + assert service.base_url is None + + +def test_reading_an_unconfigured_service_does_not_create_it(metadata_file): + metadata = CouncilMetadata.from_file(metadata_file) + metadata.get_service_metadata("decisions") + + assert "decisions" not in metadata.to_dict()["services"] + + +def test_a_service_the_model_has_never_heard_of_survives_a_round_trip(tmp_path): + """ + The point of keying services by name: a type nobody has written model + code for still loads and saves intact. + """ + path = tmp_path / "metadata.json" + path.write_text( + json.dumps( + { + "everyelectiion_data": {"official_identifier": "ABC"}, + "services": { + "decisions": { + "base_url": "https://example.gov.uk/decisions", + "cms_type": "ModernGov", + } + }, + } + ) + ) + + metadata = CouncilMetadata.from_file(path) + metadata.save_to_file(path) + + reloaded = json.loads(path.read_text()) + assert reloaded["services"]["decisions"] == { + "base_url": "https://example.gov.uk/decisions", + "cms_type": "ModernGov", + } + + +def test_saving_preserves_every_service(metadata_file): + metadata = CouncilMetadata.from_file(metadata_file) + metadata.save_to_file(metadata_file) + + reloaded = json.loads(metadata_file.read_text()) + assert sorted(reloaded["services"]) == ["councillors", "minutes"] + + +def test_update_service_data_creates_a_new_service(tmp_path): + metadata = CouncilMetadata() + metadata.update_service_data("decisions", base_url="https://example.gov.uk") + + assert metadata.get_service_metadata("decisions").base_url == ( + "https://example.gov.uk" + ) + assert metadata.to_dict()["services"]["decisions"]["base_url"] == ( + "https://example.gov.uk" + ) + + +def test_update_service_data_updates_an_existing_service(metadata_file): + metadata = CouncilMetadata.from_file(metadata_file) + metadata.update_service_data("minutes", base_url="https://new.example.gov.uk") + + service = metadata.get_service_metadata("minutes") + assert service.base_url == "https://new.example.gov.uk" + # Fields not mentioned are left alone + assert service.cms_type == "CMIS" + + +def test_configured_services_lists_only_populated_ones(metadata_file): + metadata = CouncilMetadata.from_file(metadata_file) + metadata.services["decisions"] = ServiceData() + + assert metadata.configured_services() == ["councillors", "minutes"] + + +def test_has_service(metadata_file): + metadata = CouncilMetadata.from_file(metadata_file) + + assert metadata.has_service("minutes") is True + assert metadata.has_service("decisions") is False + + +def test_services_are_written_in_a_stable_order(tmp_path): + """Otherwise saving churns the diff for no reason.""" + metadata = CouncilMetadata() + for name in ("minutes", "councillors", "decisions"): + metadata.update_service_data(name, base_url=f"https://example.gov.uk/{name}") + + path = tmp_path / "metadata.json" + metadata.save_to_file(path) + + written = json.loads(path.read_text()) + assert list(written["services"]) == ["councillors", "decisions", "minutes"] + + +def test_empty_services_block_is_handled(tmp_path): + path = tmp_path / "metadata.json" + path.write_text(json.dumps({"everyelectiion_data": {}, "services": {}})) + + metadata = CouncilMetadata.from_file(path) + + assert metadata.configured_services() == [] + assert metadata.get_service_metadata("councillors").base_url is None + + +def test_missing_file_gives_empty_metadata(tmp_path): + metadata = CouncilMetadata.from_file(tmp_path / "nope.json") + + assert metadata.configured_services() == [] + + +def test_get_summary_reports_every_service_not_just_councillors(metadata_file): + """ + A council has a cms_type and base_url per service; there is no "the" + one. Reporting councillors' as though there were misleads any caller + for a council whose services differ. + """ + metadata = CouncilMetadata.from_file(metadata_file) + + summary = metadata.get_summary() + + assert summary["services"] == { + "councillors": { + "cms_type": "ModernGov", + "base_url": "https://example.gov.uk/councillors", + }, + "minutes": { + "cms_type": "CMIS", + "base_url": "https://example.gov.uk/minutes", + }, + } + assert "cms_type" not in summary + assert "base_url" not in summary + + +def test_get_summary_omits_unconfigured_services(tmp_path): + metadata = CouncilMetadata() + metadata.services["decisions"] = ServiceData() + metadata.update_service_data("minutes", base_url="https://example.gov.uk") + + assert list(metadata.get_summary()["services"]) == ["minutes"] From 5fe15940b00700d6bced94e89290d498194abe78 Mon Sep 17 00:00:00 2001 From: symroe Date: Fri, 21 Aug 2026 09:04:31 +0100 Subject: [PATCH 05/12] Fix metadata update, and make the command work for any service manage.py metadata update was unusable. Two calls named methods that do not exist, so updating any council raised AttributeError before writing anything: update_everyelectiion_data (the real one has one i, matching the method rather than the misspelled JSON key) and get_councillors_service. Separately, --council only searched the first page of results, but the EveryElection API ignores an official_identifier filter and returns all 502 organisations paginated at 100, so every council after the first hundred alphabetically reported "not found" - quietly, since that path prints rather than raises. Three places assumed councillors, which is fine with one scraper type and wrong with more. Validation looked for councillors.py and read services.councillors, so a missing or broken scraper for any other service was invisible; it now takes --service, reports councils that take no part in a service as not applicable rather than failing them, and treats a scraper file without metadata (or the reverse) as an error. Tag filtering loaded councillors scrapers whatever service was requested, so --service minutes --tags filtered on the wrong scrapers entirely. And base_url was only ever auto-filled from councillors.py, so a council could gain a service and never have its metadata completed - every scraper file is now considered, named after its service by convention. The CMS type check used a table of three councillor class names, so scrapers written against any other base class went unchecked. Base classes are named Scraper, so it now reads the CMS off the prefix and needs no entry per scraper type. That found four real metadata inaccuracies: three councils label a PagedHTML scraper as "Custom HTML", and ERY labels a JSON scraper the same way. Also derive report["valid"] from whether errors were recorded. The base_url check appended an error without clearing the flag, so a council could be reported as passing while listing errors. Co-Authored-By: Claude Opus 5 --- lgsf/metadata/commands.py | 191 ++++++++++------ lgsf/metadata/tests/test_update_command.py | 209 ++++++++++++++++++ .../tests/test_validation_services.py | 183 +++++++++++++++ lgsf/metadata/validation.py | 188 ++++++++++------ 4 files changed, 642 insertions(+), 129 deletions(-) create mode 100644 lgsf/metadata/tests/test_update_command.py create mode 100644 lgsf/metadata/tests/test_validation_services.py diff --git a/lgsf/metadata/commands.py b/lgsf/metadata/commands.py index 8997b7f6..4a069c28 100644 --- a/lgsf/metadata/commands.py +++ b/lgsf/metadata/commands.py @@ -1,3 +1,4 @@ +import ast import csv import json import sys @@ -13,6 +14,40 @@ from lgsf.path_utils import create_org_package, scraper_abs_path +def find_scraper_base_url(source: str) -> str | None: + """ + Return the string literal a scraper class assigns to base_url, if any. + + Reading the assignment out of the parsed module rather than pattern + matching the source means a commented-out URL, or one built at runtime + from an expression, is ignored rather than picked up. + """ + for class_node in ast.parse(source).body: + if not isinstance(class_node, ast.ClassDef): + continue + + for node in class_node.body: + if isinstance(node, ast.Assign): + targets = node.targets + elif isinstance(node, ast.AnnAssign): + targets = [node.target] + else: + continue + + if not any( + isinstance(target, ast.Name) and target.id == "base_url" + for target in targets + ): + continue + + if isinstance(node.value, ast.Constant) and isinstance( + node.value.value, str + ): + return node.value.value + + return None + + class Command(CouncilFilteringCommandBase): command_name = "metadata" @@ -85,29 +120,47 @@ def add_arguments(self, parser): validate_parser = subparsers.add_parser( "validate", help="Validate scrapers against their metadata" ) + validate_parser.add_argument( + "--service", + action="store", + default="councillors", + help="Service to validate (default: councillors)", + ) self._add_council_filtering_args(validate_parser) + @property + def service_name(self): + """ + The service this invocation is about. + + Subcommands that work per service take --service; the rest operate + on councillors. + """ + options = getattr(self, "options", None) or {} + return options.get("service") or "councillors" + def handle(self, options): self.options = options # Store options for use in filtering subcommand = options.get("subcommand") if subcommand == "list-cms": self.list_cms_types( - options.get("service", "councillors"), + self.service_name, csv_format=options.get("csv", False), json_format=options.get("json", False), ) return elif subcommand == "validate": + service_name = self.service_name if options.get("council"): council_ids = [c.strip() for c in options.get("council").split(",")] if len(council_ids) == 1: - self.validate_single_scraper(council_ids[0]) + self.validate_single_scraper(council_ids[0], service_name) else: - self.validate_multiple_scrapers(council_ids) + self.validate_multiple_scrapers(council_ids, service_name) else: - self.validate_all_scrapers() + self.validate_all_scrapers(service_name) return elif subcommand == "update": @@ -142,28 +195,27 @@ def update_all_councils(self): print(f"Updated EveryElection metadata for {councils_updated} councils") def update_single_council(self, council_id): - """Update EveryElection metadata for a single council.""" + """ + Update EveryElection metadata for a single council. + + The organisation detail endpoint answers for a known identifier + directly, so one council costs one request rather than a walk + through every page of the organisation list. An identifier that + doesn't exist comes back as an empty result set rather than a 404. + """ base_url = "https://elections.democracyclub.org.uk/" - url = f"{base_url}api/organisations/" + url = f"{base_url}api/organisations/local-authority/{council_id}/" - # Search for the specific council - params = {"official_identifier": council_id} - req = requests.get(url, params=params) - data = req.json() + req = requests.get(url) + req.raise_for_status() + results = req.json()["results"] - found = False - for org in data["results"]: - if ( - org["organisation_type"] == "local-authority" - and org["official_identifier"] == council_id - ): - self.update_council_metadata(org) - found = True - print(f"Updated EveryElection metadata for {council_id}") - break - - if not found: + if not results: print(f"Council {council_id} not found in EveryElection API") + return + + self.update_council_metadata(results[0]) + print(f"Updated EveryElection metadata for {council_id}") def update_council_metadata(self, org_data): """Update metadata for a single council, preserving manual data.""" @@ -184,7 +236,7 @@ def update_council_metadata(self, org_data): metadata = CouncilMetadata.from_file(metadata_file) # Update EveryElection data while preserving manual data - metadata.update_everyelectiion_data(org_data) + metadata.update_everyelection_data(org_data) # Auto-detect CMS and scraper info if not already set self.auto_update_scraper_info(metadata, scraper_path) @@ -198,36 +250,35 @@ def update_council_metadata(self, org_data): init_file.touch() def auto_update_scraper_info(self, metadata: CouncilMetadata, scraper_path: Path): - """Auto-detect and update scraper information if not manually set.""" - councillors_file = scraper_path / "councillors.py" + """ + Fill in any service's missing base_url from its scraper file. + + A council's scrapers are named after their service + (scrapers//.py), so every scraper file present is + considered, and any service without a base_url gets one. + """ + for scraper_file in sorted(scraper_path.glob("*.py")): + service_name = scraper_file.stem + if service_name.startswith("_"): + continue - councillors_service = metadata.get_councillors_service() + if metadata.get_service_metadata(service_name).base_url: + continue - if councillors_file.exists(): try: - # Read the scraper file to extract base_url if not set - with open(councillors_file, "r") as f: - content = f.read() - - import re - - updates = {} - - # Extract base_url if not set - if not councillors_service.base_url: - url_match = re.search( - r'base_url\s*=\s*["\']([^"\']+)["\']', content - ) - if url_match: - updates["base_url"] = url_match.group(1) + content = scraper_file.read_text() + except OSError as e: + print(f"Warning: could not read {scraper_file}: {e}") + continue - if updates: - metadata.update_service_data("councillors", **updates) + try: + base_url = find_scraper_base_url(content) + except SyntaxError as e: + print(f"Warning: could not parse {scraper_file}: {e}") + continue - except Exception as e: - print( - f"Warning: Could not auto-detect scraper info for {scraper_path.name}: {e}" - ) + if base_url: + metadata.update_service_data(service_name, base_url=base_url) def update_manual_metadata(self, options): """Update manual metadata fields for a specific council.""" @@ -274,12 +325,7 @@ def list_cms_types( try: metadata = CouncilMetadata.for_council(council.council_id) - # Track available services across all councils - if service_name == "councillors" and metadata.councillors.to_dict(): - available_services.add("councillors") - # Future services can be added here - # if service_name == "meetings" and metadata.meetings.to_dict(): - # available_services.add("meetings") + available_services.update(metadata.configured_services()) service_metadata = metadata.get_service_metadata(service_name) @@ -443,7 +489,10 @@ def _get_filtered_councils(self): filtered_councils = [] for council in self._safe_current_councils(): try: - scraper = load_scraper(council.council_id, "councillors") + # Tags belong to the scraper for the service being + # operated on, so --service decides which scrapers + # are consulted. + scraper = load_scraper(council.council_id, self.service_name) if scraper and hasattr(scraper, "tags"): if any(tag in scraper.tags for tag in tag_list): filtered_councils.append(council) @@ -467,8 +516,8 @@ def _safe_current_councils(self): safe_councils.append(council) return safe_councils - def validate_all_scrapers(self): - """Validate all scrapers and print a comprehensive report.""" + def validate_all_scrapers(self, service_name="councillors"): + """Validate all scrapers for a service and print a report.""" from lgsf.metadata.validation import ScraperValidator validator = ScraperValidator() @@ -477,13 +526,16 @@ def validate_all_scrapers(self): councils_to_validate = self._get_filtered_councils() with self.console.status( - f"[bold green]Running validation for {len(councils_to_validate)} scrapers..." + f"[bold green]Validating {service_name} scrapers for " + f"{len(councils_to_validate)} councils..." ): - report = validator.validate_filtered_scrapers(councils_to_validate) + report = validator.validate_filtered_scrapers( + councils_to_validate, service_name + ) validator.print_validation_report(report, console=self.console) - def validate_multiple_scrapers(self, council_ids): + def validate_multiple_scrapers(self, council_ids, service_name="councillors"): """Validate multiple scrapers and print detailed reports.""" from lgsf.metadata.validation import ScraperValidator @@ -493,20 +545,29 @@ def validate_multiple_scrapers(self, council_ids): self.console.print(f"\n[bold blue]Validating {council_id}...[/bold blue]") with self.console.status( - f"[bold green]Validating scraper for {council_id}..." + f"[bold green]Validating {service_name} scraper for {council_id}..." ): - report = validator.validate_council_scraper(council_id) + report = validator.validate_council_scraper(council_id, service_name) validator.print_single_council_report(report, console=self.console) - def validate_single_scraper(self, council_id): + def validate_single_scraper(self, council_id, service_name="councillors"): """Validate a single scraper and print detailed report.""" from lgsf.metadata.validation import ScraperValidator validator = ScraperValidator() - with self.console.status(f"[bold green]Validating scraper for {council_id}..."): - report = validator.validate_council_scraper(council_id) + with self.console.status( + f"[bold green]Validating {service_name} scraper for {council_id}..." + ): + report = validator.validate_council_scraper(council_id, service_name) + + if not report.get("applicable", True): + self.console.print( + f"[yellow]{council_id} has no {service_name} scraper or " + f"metadata - nothing to validate[/yellow]" + ) + return # Create a panel with the validation status if report["valid"]: diff --git a/lgsf/metadata/tests/test_update_command.py b/lgsf/metadata/tests/test_update_command.py new file mode 100644 index 00000000..16e71d57 --- /dev/null +++ b/lgsf/metadata/tests/test_update_command.py @@ -0,0 +1,209 @@ +""" +Tests for `manage.py metadata update`. + +A single council is fetched from the organisation detail endpoint, which +answers for a known identifier directly. That it stays a single request, +and that an unknown identifier is reported rather than passed on as an +update, are both pinned down here. +""" + +import io + +import pytest + +from lgsf.metadata.commands import Command + + +DETAIL_URL = ( + "https://elections.democracyclub.org.uk/api/organisations/local-authority/{}/" +) + + +class FakeResponse: + def __init__(self, payload): + self._payload = payload + + def raise_for_status(self): + return None + + def json(self): + return self._payload + + +def make_command(): + return Command(["metadata"], io.StringIO(), pretty=False) + + +def org(identifier, org_type="local-authority"): + return { + "official_identifier": identifier, + "organisation_type": org_type, + "slug": identifier.lower(), + } + + +@pytest.fixture +def detail_api(monkeypatch): + """The detail endpoint, answering for one known council.""" + responses = { + DETAIL_URL.format("KIR"): {"count": 1, "next": None, "results": [org("KIR")]}, + DETAIL_URL.format("ZZZ"): {"count": 0, "next": None, "results": []}, + } + requested = [] + + def fake_get(url, **kwargs): + requested.append((url, kwargs)) + return FakeResponse(responses[url]) + + monkeypatch.setattr("lgsf.metadata.commands.requests.get", fake_get) + return requested + + +def test_fetches_a_council_in_a_single_request(detail_api, monkeypatch): + command = make_command() + updated = [] + monkeypatch.setattr(command, "update_council_metadata", updated.append) + + command.update_single_council("KIR") + + assert [o["official_identifier"] for o in updated] == ["KIR"] + assert len(detail_api) == 1 + + +def test_asks_the_local_authority_detail_endpoint(detail_api, monkeypatch): + """The org type is scoped by the URL, not filtered out afterwards.""" + command = make_command() + monkeypatch.setattr(command, "update_council_metadata", lambda org: None) + + command.update_single_council("KIR") + + assert detail_api[0][0] == DETAIL_URL.format("KIR") + + +def test_reports_a_council_that_is_genuinely_absent(detail_api, monkeypatch, capsys): + """An unknown identifier is an empty result set, not a 404.""" + command = make_command() + monkeypatch.setattr( + command, "update_council_metadata", lambda org: pytest.fail("should not update") + ) + + command.update_single_council("ZZZ") + + assert "ZZZ not found" in capsys.readouterr().out + assert len(detail_api) == 1 + + +class TestAutoUpdateScraperInfo: + """ + base_url is filled in from a council's scraper files, for every + service that has one. + """ + + def scraper(self, base_url): + return ( + "from lgsf.x import Y\n\n\nclass Scraper(Y):\n" + f' base_url = "{base_url}"\n' + ) + + def test_fills_in_base_url_for_every_service_with_a_scraper(self, tmp_path): + from lgsf.metadata.models import CouncilMetadata + + (tmp_path / "councillors.py").write_text(self.scraper("https://a.gov.uk")) + (tmp_path / "minutes.py").write_text(self.scraper("https://b.gov.uk")) + (tmp_path / "__init__.py").write_text("") + metadata = CouncilMetadata() + + make_command().auto_update_scraper_info(metadata, tmp_path) + + assert metadata.get_service_metadata("councillors").base_url == ( + "https://a.gov.uk" + ) + assert metadata.get_service_metadata("minutes").base_url == "https://b.gov.uk" + + def test_works_for_a_service_the_framework_has_never_heard_of(self, tmp_path): + from lgsf.metadata.models import CouncilMetadata + + (tmp_path / "decisions.py").write_text(self.scraper("https://c.gov.uk")) + metadata = CouncilMetadata() + + make_command().auto_update_scraper_info(metadata, tmp_path) + + assert metadata.get_service_metadata("decisions").base_url == "https://c.gov.uk" + + def test_does_not_overwrite_a_base_url_already_recorded(self, tmp_path): + from lgsf.metadata.models import CouncilMetadata + + (tmp_path / "minutes.py").write_text(self.scraper("https://scraper.gov.uk")) + metadata = CouncilMetadata() + metadata.update_service_data("minutes", base_url="https://metadata.gov.uk") + + make_command().auto_update_scraper_info(metadata, tmp_path) + + assert metadata.get_service_metadata("minutes").base_url == ( + "https://metadata.gov.uk" + ) + + def test_ignores_dunder_files(self, tmp_path): + from lgsf.metadata.models import CouncilMetadata + + (tmp_path / "__init__.py").write_text(self.scraper("https://nope.gov.uk")) + metadata = CouncilMetadata() + + make_command().auto_update_scraper_info(metadata, tmp_path) + + assert metadata.configured_services() == [] + + def test_ignores_a_commented_out_base_url(self, tmp_path): + """Reading the parsed assignment, not the text, so comments don't count.""" + from lgsf.metadata.models import CouncilMetadata + + (tmp_path / "minutes.py").write_text( + "from lgsf.x import Y\n\n\nclass Scraper(Y):\n" + ' # base_url = "https://old.gov.uk"\n' + ' base_url = "https://current.gov.uk"\n' + ) + metadata = CouncilMetadata() + + make_command().auto_update_scraper_info(metadata, tmp_path) + + assert metadata.get_service_metadata("minutes").base_url == ( + "https://current.gov.uk" + ) + + def test_ignores_a_base_url_that_is_not_a_literal(self, tmp_path): + """A URL built at runtime can't be recorded, so it must not be guessed at.""" + from lgsf.metadata.models import CouncilMetadata + + (tmp_path / "minutes.py").write_text( + "from lgsf.x import Y\n\n\nclass Scraper(Y):\n" + " base_url = SOME_CONSTANT + '/path'\n" + ) + metadata = CouncilMetadata() + + make_command().auto_update_scraper_info(metadata, tmp_path) + + assert metadata.get_service_metadata("minutes").base_url is None + + def test_survives_a_scraper_that_does_not_parse(self, tmp_path, capsys): + """A half-written scraper warns rather than taking the command down.""" + from lgsf.metadata.models import CouncilMetadata + + (tmp_path / "minutes.py").write_text("class Scraper(:\n") + metadata = CouncilMetadata() + + make_command().auto_update_scraper_info(metadata, tmp_path) + + assert "could not parse" in capsys.readouterr().out + assert metadata.get_service_metadata("minutes").base_url is None + + +def test_service_name_defaults_to_councillors(): + command = make_command() + command.options = {} + assert command.service_name == "councillors" + + +def test_service_name_follows_the_service_option(): + command = make_command() + command.options = {"service": "minutes"} + assert command.service_name == "minutes" diff --git a/lgsf/metadata/tests/test_validation_services.py b/lgsf/metadata/tests/test_validation_services.py new file mode 100644 index 00000000..4ce42bfa --- /dev/null +++ b/lgsf/metadata/tests/test_validation_services.py @@ -0,0 +1,183 @@ +""" +Scraper validation across services. + +Validation takes a service name, so a whole data type can be checked in +one pass: that a council declaring a service has a matching scraper file, +that base_url is set, and that the scraper's base class agrees with the +recorded cms_type. +""" + +import json + +import pytest + +from lgsf.conf import settings +from lgsf.metadata.validation import ScraperValidator + + +@pytest.fixture +def council(tmp_path, monkeypatch): + """ + A council directory the validator will resolve against. + + path_utils._abs_path resolves SCRAPER_DIR_NAME relative to the working + directory and ignores BASE_PATH, so point it at an absolute temporary + path rather than trying to relocate the base. + """ + scrapers_dir = tmp_path / "scrapers" + path = scrapers_dir / "ZZQ-anytown" + path.mkdir(parents=True) + monkeypatch.setattr(settings, "SCRAPER_DIR_NAME", str(scrapers_dir)) + + def write(scrapers=None, services=None): + for name, body in (scrapers or {}).items(): + (path / f"{name}.py").write_text(body) + (path / "metadata.json").write_text( + json.dumps( + { + "everyelectiion_data": {"official_identifier": "ZZQ"}, + "services": services or {}, + } + ) + ) + return path + + return write + + +MODGOV_MINUTES = ( + "from lgsf.minutes.scrapers import ModGovMinutesScraper\n\n\n" + "class Scraper(ModGovMinutesScraper):\n pass\n" +) +MODGOV_COUNCILLORS = ( + "from lgsf.councillors.scrapers import ModGovCouncillorScraper\n\n\n" + "class Scraper(ModGovCouncillorScraper):\n pass\n" +) +SERVICE = {"base_url": "https://example.gov.uk", "cms_type": "ModernGov"} + + +def test_a_valid_minutes_scraper_passes(council): + council(scrapers={"minutes": MODGOV_MINUTES}, services={"minutes": SERVICE}) + + report = ScraperValidator().validate_council_scraper("ZZQ", "minutes") + + assert report["valid"] is True + assert report["errors"] == [] + assert report["service"] == "minutes" + + +def test_a_council_without_the_service_is_not_applicable(council): + """Most councils will never have every service; that isn't a failure.""" + council( + scrapers={"councillors": MODGOV_COUNCILLORS}, + services={"councillors": SERVICE}, + ) + + report = ScraperValidator().validate_council_scraper("ZZQ", "minutes") + + assert report["applicable"] is False + assert report["errors"] == [] + + +def test_metadata_without_a_scraper_file_is_an_error(council): + council(services={"minutes": SERVICE}) + + report = ScraperValidator().validate_council_scraper("ZZQ", "minutes") + + assert report["valid"] is False + assert "no minutes.py" in report["errors"][0] + + +def test_a_scraper_file_without_metadata_is_an_error(council): + council(scrapers={"minutes": MODGOV_MINUTES}, services={}) + + report = ScraperValidator().validate_council_scraper("ZZQ", "minutes") + + assert report["valid"] is False + assert "services.minutes" in report["errors"][0] + + +def test_a_missing_base_url_is_an_error_naming_the_right_service(council): + council( + scrapers={"minutes": MODGOV_MINUTES}, + services={"minutes": {"cms_type": "ModernGov"}}, + ) + + report = ScraperValidator().validate_council_scraper("ZZQ", "minutes") + + assert report["valid"] is False + assert any("services.minutes.base_url" in e for e in report["errors"]) + + +def test_services_are_validated_independently(council): + """A broken minutes scraper must not affect the councillors verdict.""" + council( + scrapers={"councillors": MODGOV_COUNCILLORS}, + services={"councillors": SERVICE, "minutes": SERVICE}, + ) + validator = ScraperValidator() + + assert validator.validate_council_scraper("ZZQ", "councillors")["valid"] is True + assert validator.validate_council_scraper("ZZQ", "minutes")["valid"] is False + + +def test_a_bare_cms_subclass_is_not_flagged_as_incomplete(council): + """ + Subclassing a CMS base class and adding nothing is the expected shape: + the base class does the work and the council supplies a base_url. + """ + council(scrapers={"minutes": MODGOV_MINUTES}, services={"minutes": SERVICE}) + + report = ScraperValidator().validate_council_scraper("ZZQ", "minutes") + + assert not any("No custom methods" in w for w in report["warnings"]) + + +def test_a_bare_non_cms_subclass_is_flagged(council): + council( + scrapers={ + "minutes": "class Scraper(BaseMinutesScraper):\n pass\n", + }, + services={"minutes": {"base_url": "https://example.gov.uk"}}, + ) + + report = ScraperValidator().validate_council_scraper("ZZQ", "minutes") + + assert any("No custom methods" in w for w in report["warnings"]) + + +@pytest.mark.parametrize( + "parent_class,expected", + [ + ("ModGovMinutesScraper", "ModernGov"), + ("ModGovCouncillorScraper", "ModernGov"), + ("CMISMinutesScraper", "CMIS"), + ("CMISCouncillorScraper", "CMIS"), + # PagedHTML must win over HTML - it is checked first + ("PagedHTMLCouncillorScraper", "Custom HTML (Paged)"), + ("HTMLCouncillorScraper", "Custom HTML"), + ("JSONCouncillorScraper", "JSON API"), + ("BaseCouncillorScraper", "Custom Base"), + ("SomethingElse", None), + (None, None), + ], +) +def test_expected_cms_type_is_read_from_the_base_class_name(parent_class, expected): + """ + Base classes are named Scraper, so matching the prefix means + a new scraper type needs no entry in the table. + """ + assert ScraperValidator()._expected_cms_type(parent_class) == expected + + +def test_cms_mismatch_is_reported(council): + council( + scrapers={"minutes": MODGOV_MINUTES}, + services={ + "minutes": {"base_url": "https://example.gov.uk", "cms_type": "CMIS"} + }, + ) + + report = ScraperValidator().validate_council_scraper("ZZQ", "minutes") + + assert any("CMS type mismatch" in w for w in report["warnings"]) diff --git a/lgsf/metadata/validation.py b/lgsf/metadata/validation.py index 290ed580..ac2342ec 100644 --- a/lgsf/metadata/validation.py +++ b/lgsf/metadata/validation.py @@ -20,15 +20,25 @@ class ScraperValidator: def __init__(self): self.warnings_issued = [] - def validate_council_scraper(self, council_id: str) -> Dict[str, Any]: + def validate_council_scraper( + self, council_id: str, service_name: str = "councillors" + ) -> Dict[str, Any]: """ - Validate a council's scraper against its metadata. + Validate a council's scraper for one service against its metadata. - Returns a validation report with any issues found. + A council is only checked for a service it actually takes part in: + one that has neither a scraper file nor service metadata is + reported as not applicable rather than as broken, since most + councils will never have every service. + + Having exactly one of the two is an error - a scraper with no + base_url can't run, and metadata with no scraper is never used. """ report = { "council_id": council_id, + "service": service_name, "valid": True, + "applicable": True, "warnings": [], "errors": [], "suggestions": [], @@ -40,43 +50,51 @@ def validate_council_scraper(self, council_id: str) -> Dict[str, Any]: # Check if scraper file exists scraper_path = scraper_abs_path(council_id) - councillors_file = scraper_path / "councillors.py" + scraper_file = scraper_path / f"{service_name}.py" + has_service_metadata = metadata.has_service(service_name) + + if not scraper_file.exists(): + if not has_service_metadata: + # This council doesn't do this service. + report["applicable"] = False + return report + report["errors"].append( + f"metadata.json declares services.{service_name} but there " + f"is no {service_name}.py" + ) + report["valid"] = False + return report - if not councillors_file.exists(): - report["errors"].append("No councillors.py file found") + if not has_service_metadata: + report["errors"].append( + f"{service_name}.py exists but metadata.json has no " + f"services.{service_name} block" + ) report["valid"] = False return report # Load and inspect the scraper - scraper_info = self._load_scraper_info(councillors_file) + scraper_info = self._load_scraper_info(scraper_file) if not scraper_info: report["errors"].append("Could not load scraper class information") report["valid"] = False return report - # Basic metadata checks - councillors_service = metadata.get_service_metadata("councillors") - if councillors_service and councillors_service.cms_type: - # Could add runtime CMS validation here if needed - pass + service = metadata.get_service_metadata(service_name) # Check for missing metadata fields - missing_fields = self._check_missing_metadata(metadata) + missing_fields = self._check_missing_metadata(metadata, service_name) if missing_fields: report["suggestions"].extend( [f"Consider setting {field}" for field in missing_fields] ) # Check scraper implementation quality - quality_issues = self._check_scraper_quality(scraper_info, councillors_file) + quality_issues = self._check_scraper_quality(scraper_info, scraper_file) report["warnings"].extend(quality_issues) - # Check base_url in metadata - councillors_service = metadata.get_service_metadata("councillors") - metadata_base_url = ( - councillors_service.base_url if councillors_service else None - ) + metadata_base_url = service.base_url is_disabled = scraper_info.get("disabled", False) # Skip base_url validation for disabled scrapers @@ -88,28 +106,25 @@ def validate_council_scraper(self, council_id: str) -> Dict[str, Any]: # No base_url in metadata report["errors"].append( "No base_url found in metadata. " - "Please set services.councillors.base_url in metadata.json" + f"Please set services.{service_name}.base_url in metadata.json" ) # Check CMS type consistency - if councillors_service and councillors_service.cms_type and scraper_info: + if service.cms_type and scraper_info: parent_class = scraper_info.get("parent_class") - expected_cms_mapping = { - "ModGovCouncillorScraper": "ModernGov", - "CMISCouncillorScraper": "CMIS", - "HTMLCouncillorScraper": "Custom HTML", - } - expected_cms = expected_cms_mapping.get(parent_class) - if expected_cms and expected_cms != councillors_service.cms_type: + expected_cms = self._expected_cms_type(parent_class) + if expected_cms and expected_cms != service.cms_type: report["warnings"].append( f"CMS type mismatch: scraper uses '{parent_class}' (suggests '{expected_cms}') " - f"but metadata has '{councillors_service.cms_type}'" + f"but metadata has '{service.cms_type}'" ) except Exception as e: report["errors"].append(f"Validation failed: {str(e)}") - report["valid"] = False + # Derived once at the end rather than maintained at each append, so + # a report can never claim to pass while listing errors. + report["valid"] = not report["errors"] return report def _load_scraper_info(self, scraper_file: Path) -> Optional[Dict[str, Any]]: @@ -157,19 +172,50 @@ def _load_scraper_info(self, scraper_file: Path) -> Optional[Dict[str, Any]]: except Exception: return None - def _check_missing_metadata(self, metadata: CouncilMetadata) -> List[str]: + #: Scraper base classes are named Scraper, so the CMS a + #: scraper is written against can be read off the prefix. Matching on + #: the prefix rather than a table of full class names means a new + #: scraper type needs no entry here. + CMS_PREFIXES = ( + ("ModGov", "ModernGov"), + ("CMIS", "CMIS"), + # PagedHTML before HTML: the first match wins and one is a prefix + # of the other's meaning, not its name. + ("PagedHTML", "Custom HTML (Paged)"), + ("CustomHTML", "Custom HTML"), + ("HTML", "Custom HTML"), + ("JSON", "JSON API"), + ("Base", "Custom Base"), + ) + + def _is_cms_base_class(self, parent_class: Optional[str]) -> bool: + """ + True for the framework's turnkey CMS scrapers, where subclassing + with nothing added is correct rather than suspicious. + """ + return self._expected_cms_type(parent_class) in ("ModernGov", "CMIS") + + def _expected_cms_type(self, parent_class: Optional[str]) -> Optional[str]: + """Infer the CMS a scraper is written against from its base class.""" + if not parent_class: + return None + for prefix, cms_type in self.CMS_PREFIXES: + if parent_class.startswith(prefix): + return cms_type + return None + + def _check_missing_metadata( + self, metadata: CouncilMetadata, service_name: str = "councillors" + ) -> List[str]: """Check for important missing metadata fields.""" missing = [] - councillors_service = metadata.get_service_metadata("councillors") - if not councillors_service: - missing.append("councillors service metadata") - return missing + service = metadata.get_service_metadata(service_name) - if not councillors_service.cms_type: + if not service.cms_type: missing.append("cms_type") - if not councillors_service.base_url: + if not service.base_url: missing.append("base_url") return missing @@ -191,21 +237,21 @@ def _check_scraper_quality( if scraper_info["disabled"]: issues.append("Scraper is marked as disabled") - # Check if it's just inheriting without customization - if ( + # Subclassing a CMS base class and adding nothing is the expected + # shape for ModernGov and CMIS councils - the base class does the + # work and the council only supplies a base_url. Anything else with + # no methods probably is incomplete. + if len(scraper_info["custom_methods"]) == 0 and not self._is_cms_base_class( scraper_info["parent_class"] - in ["CMISCouncillorScraper", "ModGovCouncillorScraper"] - and len(scraper_info["custom_methods"]) == 0 ): - # This is actually good - simple CMS scrapers should be minimal - pass - elif len(scraper_info["custom_methods"]) == 0: issues.append("No custom methods defined - scraper may be incomplete") return issues - def validate_all_scrapers(self) -> Dict[str, Any]: - """Validate all scrapers and return a comprehensive report.""" + def validate_all_scrapers( + self, service_name: str = "councillors" + ) -> Dict[str, Any]: + """Validate every council's scraper for a service.""" from pathlib import Path scrapers_dir = Path("scrapers") @@ -213,6 +259,8 @@ def validate_all_scrapers(self) -> Dict[str, Any]: return {"error": "Scrapers directory not found"} results = { + "service": service_name, + "not_applicable": 0, "total_councils": 0, "valid_scrapers": 0, "scrapers_with_warnings": 0, @@ -228,9 +276,14 @@ def validate_all_scrapers(self) -> Dict[str, Any]: if council_dir.is_dir() and not council_dir.name.startswith("."): council_id = self._extract_council_id(council_dir.name) if council_id: - results["total_councils"] += 1 + validation_result = self.validate_council_scraper( + council_id, service_name + ) + if not validation_result.get("applicable", True): + results["not_applicable"] += 1 + continue - validation_result = self.validate_council_scraper(council_id) + results["total_councils"] += 1 results["councils"].append(validation_result) if validation_result["valid"]: @@ -245,16 +298,12 @@ def validate_all_scrapers(self) -> Dict[str, Any]: # Update summary statistics try: metadata = CouncilMetadata.for_council(council_id) - councillors_service = metadata.get_service_metadata( - "councillors" - ) - - # CMS type distribution + # CMS type distribution for the service being + # validated, not whatever councillors happens to use. cms_type = ( - councillors_service.cms_type - if councillors_service - else None - ) or "Unknown" + metadata.get_service_metadata(service_name).cms_type + or "Unknown" + ) results["summary"]["cms_types"][cms_type] = ( results["summary"]["cms_types"].get(cms_type, 0) + 1 ) @@ -270,9 +319,13 @@ def validate_all_scrapers(self) -> Dict[str, Any]: return results - def validate_filtered_scrapers(self, councils) -> Dict[str, Any]: + def validate_filtered_scrapers( + self, councils, service_name: str = "councillors" + ) -> Dict[str, Any]: """Validate a filtered list of councils and return a comprehensive report.""" results = { + "service": service_name, + "not_applicable": 0, "total_councils": 0, "valid_scrapers": 0, "scrapers_with_warnings": 0, @@ -286,9 +339,16 @@ def validate_filtered_scrapers(self, councils) -> Dict[str, Any]: for council in councils: council_id = council.council_id - results["total_councils"] += 1 - validation_result = self.validate_council_scraper(council_id) + validation_result = self.validate_council_scraper(council_id, service_name) + + # Councils that don't do this service aren't failures, and + # counting them would drown the real results. + if not validation_result.get("applicable", True): + results["not_applicable"] += 1 + continue + + results["total_councils"] += 1 results["councils"].append(validation_result) if validation_result["valid"]: @@ -303,12 +363,12 @@ def validate_filtered_scrapers(self, councils) -> Dict[str, Any]: # Update summary statistics try: metadata = CouncilMetadata.for_council(council_id) - councillors_service = metadata.get_service_metadata("councillors") - # CMS type distribution + # CMS type distribution for the service being validated, + # not whatever councillors happens to use. cms_type = ( - councillors_service.cms_type if councillors_service else None - ) or "Unknown" + metadata.get_service_metadata(service_name).cms_type or "Unknown" + ) results["summary"]["cms_types"][cms_type] = ( results["summary"]["cms_types"].get(cms_type, 0) + 1 ) From 3d88ab5db22dea6e9071cefea3c687a48f74a657 Mon Sep 17 00:00:00 2001 From: symroe Date: Fri, 21 Aug 2026 09:04:31 +0100 Subject: [PATCH 06/12] Make --all-councils survive failures, and run councils at once Running every council was close to unusable. With -v the first broken council raised and ended the run; without it, every failure went into one of several hundred Rich tables scrolling past. Neither leaves you with a list of what is broken, which is the reason to run them all. Councils are independent sites, so they are now scraped concurrently (--workers, default 4) and one failing never stops the others. Each council's outcome is recorded in data///runlog.json, beside the data that run produced, keeping the most recent run and the scraper's console output for the ones that failed. --list-failing reads those, so after a full run it reports what is actually broken rather than what a production dashboard says about a different data type. Run logs are written straight to disk rather than through the storage session, because the runs most worth recording are the ones the session never commits: a failure resets it without writing, and a run that scrapes nothing skips the commit entirely. Running exactly one council still raises with -v: that is someone working on that scraper, or a scripted call that needs the failure to reach its exit code. One council is also never run "concurrently", so its output streams as it happens instead of being replayed at the end. Concurrency needed two things to stop being shared. run_council mutated self.options["council"], so parallel runs would overwrite each other's council and scrape the wrong site; each council now gets its own options. And they shared one recording console, which would interleave every council's log into every other council's run record; each now gets its own. Lambda is unaffected: it runs a single council through its own handler, which builds its own console and run log and never enters this loop. Run logs are skipped there, and for any backend that keeps its data elsewhere, since there is nowhere local to put them. Also generalise --report to collect from any scraper type, and show real council names in the listings - they read official_name from the top level of metadata.json, where the current format nests it under everyelectiion_data, so every council showed as "Unknown". Co-Authored-By: Claude Opus 5 --- lgsf/commands/base.py | 397 ++++++++++++++++++++++++++++------- lgsf/councillors/commands.py | 34 +++ lgsf/councillors/scrapers.py | 4 + lgsf/tests/test_run_loop.py | 246 ++++++++++++++++++++++ 4 files changed, 600 insertions(+), 81 deletions(-) create mode 100644 lgsf/tests/test_run_loop.py diff --git a/lgsf/commands/base.py b/lgsf/commands/base.py index 74d0165f..d741e312 100644 --- a/lgsf/commands/base.py +++ b/lgsf/commands/base.py @@ -1,10 +1,16 @@ import abc import argparse import datetime +import io +import json +import os +import threading import traceback +from concurrent.futures import ThreadPoolExecutor, as_completed +from pathlib import Path from dataclasses import dataclass, field from functools import cached_property -from typing import List +from typing import List, Optional import requests from dateutil.parser import parse @@ -77,6 +83,35 @@ def metadata(self): self._metadata_cache = load_council_info(self.council_id) return self._metadata_cache + @property + def name(self): + """ + The council's name for display. + + metadata.json keeps the name under everyelectiion_data. Some files + are in an older flat format with it at the top level, so both are + checked, falling back to the council code. + """ + everyelection = self.metadata.get("everyelectiion_data", {}) + return ( + everyelection.get("official_name") + or everyelection.get("common_name") + # Legacy flat format + or self.metadata.get("official_name") + or self.council_id + ) + + def service_metadata(self, service_name): + """ + This council's recorded config for one service, or an empty dict. + + Deliberately takes the service name rather than defaulting to + councillors: a listing for one data type showing another type's + config reads as though the data is there when it isn't. + """ + services = self.metadata.get("services") or {} + return services.get(service_name) or {} + @property def current(self): # Check for dates in everyelectiion_data first (new format) @@ -138,11 +173,9 @@ def disabled(self): for council in self.current_councils: scraper = load_scraper(council.council_id, self.command_name) if scraper and scraper.disabled: - council_info = { - "code": council.council_id, - "name": council.metadata.get("official_name", "Unknown"), - } - disabled_councils.append(council_info) + disabled_councils.append( + {"code": council.council_id, "name": council.name} + ) return sorted(disabled_councils, key=lambda d: d["code"]) @@ -153,7 +186,11 @@ class PerCouncilCommandBase(CouncilFilteringCommandBase): def __init__(self, argv, stdout, pretty=False): super().__init__(argv, stdout, pretty) - self.scraped_councillors = [] # Store councillors for reporting + self.scraped_items = [] # Store scraped objects for reporting + self._results = [] + self._lock = threading.Lock() + self._concurrent = False + self._council_count = 0 def create_parser(self): self.parser = argparse.ArgumentParser() @@ -205,6 +242,13 @@ def create_parser(self): action="store_true", help="Print failing councils", ) + self.parser.add_argument( + "--workers", + type=int, + default=4, + help="How many scrapers to run at once. Use 1 to watch a " + "single scraper's output.", + ) self.parser.add_argument( "--report", action="store_true", @@ -234,19 +278,20 @@ def missing(self): for council in self.current_councils: scraper = load_scraper(council.council_id, self.command_name) if not scraper: - council_info = { - "code": council.council_id, - "name": council.metadata.get("official_name", "Unknown"), - } - missing_councils.append(council_info) + missing_councils.append( + {"code": council.council_id, "name": council.name} + ) return sorted(missing_councils, key=lambda d: d["code"]) def output_missing(self): - table = Table(title=f"Councils missing '{self.command_name}' scraper") + missing = self.missing() + table = Table( + title=f"{len(missing)} councils missing a '{self.command_name}' scraper" + ) table.add_column("Code", style="magenta") table.add_column("Name", style="green") - for council in self.missing(): + for council in missing: table.add_row(council["code"], council["name"]) self.console.print(table) @@ -261,20 +306,89 @@ def output_disabled(self): self.console.print(table) + #: URL listing the scrapers failing in production, for command types + #: that have one. The dashboard reports a single scheduled job with no + #: service field, so it describes exactly one data type and cannot be + #: reused for the others. + failing_api_url = None + + def local_run_logs(self): + """ + The last recorded run for each council of this scraper type. + + Only councils that have been run on this machine appear, so an + empty result means "nothing has been run here", not "nothing is + failing". + """ + logs = {} + for council in self.current_councils: + scraper_cls = load_scraper(council.council_id, self.command_name) + if not scraper_cls or not scraper_cls.scraper_object_type: + continue + path = ( + Path(settings.DATA_DIR_NAME) + / council.council_id + / scraper_cls.scraper_object_type + / self.RUN_LOG_FILE_NAME + ) + if not path.is_file(): + continue + try: + logs[council.council_id] = json.loads(path.read_text()) + except ValueError: + continue + return logs + def failing(self): - req = requests.get( - "https://democracyclub.github.io/lgsf-dashboard/api/failing.json" - ) + if not self.failing_api_url: + return [] + req = requests.get(self.failing_api_url) return req.json() def output_failing(self): - table = Table(title=f"Councils with '{self.command_name}' failing") - table.add_column("Code", style="magenta") - table.add_column("Error", style="red") - for council in self.failing(): - if council["council_id"] in self.current_council_ids: - table.add_row(council["council_id"], council["latest_run"]["log_text"]) - self.console.print(table) + logs = self.local_run_logs() + if logs: + failed = {code: log for code, log in logs.items() if log.get("status_code")} + table = Table( + title=f"{self.command_name}: {len(failed)} of {len(logs)} councils " + f"failing in the last local run" + ) + table.add_column("Code", style="magenta") + table.add_column("When", style="cyan") + table.add_column("Error", style="red", overflow="fold") + for code, log in sorted(failed.items()): + error = (log.get("error") or "").strip().splitlines() + table.add_row( + code, + str(log.get("start", ""))[:19], + error[-1][:160] if error else "", + ) + self.console.print(table) + self.console.print( + f"[dim]From {len(logs)} councils run locally. Councils never run " + f"here do not appear.[/dim]" + ) + return + + if self.failing_api_url: + table = Table( + title=f"Councils with '{self.command_name}' failing in production" + ) + table.add_column("Code", style="magenta") + table.add_column("Error", style="red") + for council in self.failing(): + if council["council_id"] in self.current_council_ids: + table.add_row( + council["council_id"], council["latest_run"]["log_text"] + ) + self.console.print(table) + return + + self.console.print( + f"[yellow]No {self.command_name} run logs on this machine, and no " + f"production report for this type. Run the scrapers to find out what " + f"is failing: `manage.py {self.command_name} --all-councils`.[/yellow]" + ) def output_status(self): from rich.columns import Columns @@ -312,66 +426,209 @@ def councils_to_run(self): ] return councils + @property + def in_lambda(self): + """ + Lambda runs one council per invocation through its own handler and + keeps its logs in CloudWatch, so there is nothing to write locally. + """ + return bool( + self.options.get("aws_lambda") or os.environ.get("AWS_LAMBDA_FUNCTION_NAME") + ) + + #: File a council's last run of this scraper type is recorded in, + #: alongside the data that run produced. + RUN_LOG_FILE_NAME = "runlog.json" + + def run_log_path(self, scraper) -> Optional[Path]: + """ + Where to record this council's run, or None if there is nowhere. + + Run logs sit beside the data they describe, in + data///. Only backends that keep data on this + machine offer a place for them; the GitHub backend writes to a + clone elsewhere and reports through the dashboard instead. + """ + council_root = getattr(scraper.storage_backend, "council_root", None) + if council_root is None: + return None + return council_root / self.RUN_LOG_FILE_NAME + + def record_run_log(self, scraper, run_log): + """ + Record how this council's run went, replacing any previous one. + + Written straight to disk rather than through the storage session, + because the runs most worth a record are the ones the session + never commits: a failure resets the session without writing, and a + run that scrapes nothing skips the commit entirely. + + The scraper's console output is kept only for runs that failed. It + is what you need to diagnose one, and keeping it for every council + would bloat every data directory. + """ + if self.in_lambda: + return + + path = self.run_log_path(scraper) + if path is None: + return + + entry = { + "council": scraper.council_id, + "command": self.command_name, + "status_code": run_log.status_code, + "start": run_log.start, + "end": run_log.end, + "duration": run_log.duration, + "error": run_log.error, + } + if run_log.error and run_log.log: + entry["log"] = run_log.log + + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(entry, indent=2, default=str)) + def run_councils(self): - for council in self.councils_to_run: - self.run_council(council.council_id) + self._run_councils(progress=False) def run_councils_with_progress(self): + self._run_councils(progress=True) + + def _run_councils(self, progress=True): to_run = self.councils_to_run - with Progress( - "[progress.description]{task.description}", - BarColumn(), - "[progress.percentage]{task.percentage:>3.0f}%", - TimeElapsedColumn(), - console=self.console, - auto_refresh=False, - ) as progress: - total = progress.add_task(description="Total", total=len(to_run)) - while not progress.finished: - for council in to_run: - self.run_council(council.council_id) - progress.update(total, advance=1) - progress.refresh() - - def _run_single(self, scraper): + self._council_count = len(to_run) + workers = max(1, int(self.options.get("workers") or 1)) + # Nothing to parallelise, and one council should stream its output + # rather than being held back and replayed. + if self._council_count <= 1: + workers = 1 + self._concurrent = workers > 1 + + if not progress: + self._map(to_run, workers, None) + else: + with Progress( + "[progress.description]{task.description}", + BarColumn(), + "[progress.percentage]{task.percentage:>3.0f}%", + TimeElapsedColumn(), + console=self.console, + auto_refresh=False, + ) as bar: + task = bar.add_task(description="Total", total=self._council_count) + self._map(to_run, workers, (bar, task)) + + if self._concurrent: + self.output_run_summary() + + def _map(self, councils, workers, progress): + def done(): + if progress: + bar, task = progress + bar.update(task, advance=1) + bar.refresh() + + if workers == 1: + for council in councils: + self.run_council(council.council_id) + done() + return + + with ThreadPoolExecutor(workers) as pool: + futures = [ + pool.submit(self.run_council, council.council_id) + for council in councils + ] + for future in as_completed(futures): + future.result() + done() + + def output_run_summary(self): + """Per-council outcomes, for runs too large to read as they happen.""" + table = Table(title=f"{self.command_name}: {len(self._results)} councils run") + table.add_column("Council", style="magenta") + table.add_column("Status", style="green") + table.add_column("Seconds", style="cyan", justify="right") + table.add_column("Error", style="red", overflow="fold") + + for council_id, run_log in sorted(self._results): + failed = bool(run_log.error) + table.add_row( + council_id, + "[red]failed[/red]" if failed else "ok", + f"{run_log.duration.total_seconds():.1f}", + run_log.error.strip().splitlines()[-1][:120] if failed else "", + ) + self.console.print(table) + + failures = [c for c, log in self._results if log.error] + if failures: + self.console.print( + f"[red]{len(failures)} of {len(self._results)} failed:[/red] " + + " ".join(sorted(failures)) + ) + + def _run_single(self, scraper, console): run_log = settings.RUN_LOGGER(start=datetime.datetime.now(datetime.UTC)) try: scraper.run(run_log) - # Collect councillors for reporting if flag is set if self.options.get("report"): - self.scraped_councillors.extend(scraper.councillors) + with self._lock: + self.scraped_items.extend(scraper.report_items) except KeyboardInterrupt: raise except Exception: run_log.error = traceback.format_exc() - if self.options.get("verbose"): + if not run_log.log and hasattr(console, "export_text"): + run_log.log = console.export_text() + # One council failing must not end a run over many of them. + # A run of exactly one is someone working on that scraper, or a + # scripted call that wants the failure to surface. + if self._council_count <= 1 and self.options.get("verbose"): + run_log.finish() + self.record_run_log(scraper, run_log) raise run_log.finish() - self.console.print(run_log.as_rich_table) + with self._lock: + self._results.append((scraper.options["council"], run_log)) + self.record_run_log(scraper, run_log) + + if not self._concurrent: + console.print(run_log.as_rich_table) def run_council(self, council): - self.options["council"] = council - self.options["council_info"] = load_council_info(council) + # Each council gets its own options and console: sharing them means + # concurrent runs overwrite each other's council and interleave + # their logs into each other's run records. + options = dict(self.options) + options["council"] = council + options["council_info"] = load_council_info(council) + + console = self.console + if self._concurrent: + console = Console(file=io.StringIO(), record=True, width=120) + scraper_cls = load_scraper(council, self.command_name) if not scraper_cls: return - with scraper_cls(self.options, self.console) as scraper: + with scraper_cls(options, console) as scraper: should_run = True if scraper.disabled: should_run = False - if should_run and self.options["refresh"] and scraper.run_since(): + if should_run and options["refresh"] and scraper.run_since(): should_run = False - if should_run and self.options["tags"]: - required_tags = set(self.options["tags"].split(",")) + if should_run and options["tags"]: + required_tags = set(options["tags"].split(",")) scraper_tags = set(scraper.get_tags) if not required_tags.issubset(scraper_tags): should_run = False if should_run: # Clear console recording to exclude bootstrapping output from run log - if hasattr(self.console, "_record_buffer"): - self.console._record_buffer.clear() - self._run_single(scraper) + if hasattr(console, "_record_buffer"): + console._record_buffer.clear() + self._run_single(scraper, console) def normalise_codes(self): new_codes = [] @@ -384,32 +641,10 @@ def normalise_codes(self): return self.options def output_report(self): - """Display a Rich table report of scraped councillor data""" - table = Table(title="Scraped Councillor Data") - table.add_column("Name", style="cyan", no_wrap=False) - table.add_column("Ward", style="green", no_wrap=False) - table.add_column("Party", style="yellow", no_wrap=False) - table.add_column("Email", style="blue", no_wrap=False) - table.add_column("Photo", style="magenta", overflow="fold") - - # Use councillors collected during scraping (in memory) - for councillor in sorted(self.scraped_councillors, key=lambda c: c.name): - table.add_row( - councillor.name or "N/A", - councillor.division or "N/A", - councillor.party or "N/A", - getattr(councillor, "email", None) or "", - getattr(councillor, "photo_url", None) or "", - ) - - total_councillors = len(self.scraped_councillors) - if total_councillors > 0: - self.console.print( - f"\n[bold green]Total councillors scraped: {total_councillors}[/bold green]" - ) - self.console.print(table) - else: - self.console.print("[yellow]No councillor data found to report[/yellow]") + """Display a report of scraped data. Subclasses render their own table.""" + self.console.print( + f"\n[bold green]Total items scraped: {len(self.scraped_items)}[/bold green]" + ) def handle(self, options): self.options = options diff --git a/lgsf/councillors/commands.py b/lgsf/councillors/commands.py index 3f785b14..04bc3072 100644 --- a/lgsf/councillors/commands.py +++ b/lgsf/councillors/commands.py @@ -1,6 +1,40 @@ +from rich.table import Table + from lgsf.commands.aws_mixin import AWSInvokableMixin from lgsf.commands.base import PerCouncilCommandBase class Command(AWSInvokableMixin, PerCouncilCommandBase): command_name = "councillors" + + # The scheduled production job behind this dashboard runs councillors + # scrapers. + failing_api_url = "https://democracyclub.github.io/lgsf-dashboard/api/failing.json" + + def output_report(self): + """Display a Rich table report of scraped councillor data""" + table = Table(title="Scraped Councillor Data") + table.add_column("Name", style="cyan", no_wrap=False) + table.add_column("Ward", style="green", no_wrap=False) + table.add_column("Party", style="yellow", no_wrap=False) + table.add_column("Email", style="blue", no_wrap=False) + table.add_column("Photo", style="magenta", overflow="fold") + + # Use councillors collected during scraping (in memory) + for councillor in sorted(self.scraped_items, key=lambda c: c.name): + table.add_row( + councillor.name or "N/A", + councillor.division or "N/A", + councillor.party or "N/A", + getattr(councillor, "email", None) or "", + getattr(councillor, "photo_url", None) or "", + ) + + total_councillors = len(self.scraped_items) + if total_councillors > 0: + self.console.print( + f"\n[bold green]Total councillors scraped: {total_councillors}[/bold green]" + ) + self.console.print(table) + else: + self.console.print("[yellow]No councillor data found to report[/yellow]") diff --git a/lgsf/councillors/scrapers.py b/lgsf/councillors/scrapers.py index 320ebfba..dadbae44 100644 --- a/lgsf/councillors/scrapers.py +++ b/lgsf/councillors/scrapers.py @@ -53,6 +53,10 @@ def add_councillor( def get_tags(self): return self.tags + self.class_tags + @property + def report_items(self): + return list(self.councillors) + def run(self, run_log: RunLog): for councillor_html in self.get_councillors(): try: diff --git a/lgsf/tests/test_run_loop.py b/lgsf/tests/test_run_loop.py new file mode 100644 index 00000000..d0da65e3 --- /dev/null +++ b/lgsf/tests/test_run_loop.py @@ -0,0 +1,246 @@ +""" +How a per-council command runs many councils. + +Councils are independent sites, so they are scraped concurrently, and one +of them failing has to leave the rest of the run alone. +""" + +import io +import json + +import pytest + +from lgsf.commands.base import PerCouncilCommandBase +from lgsf.conf import settings + + +class FakeStorageBackend: + """Only the part the run log needs: where this council's data lives.""" + + def __init__(self, root, council_id): + self.council_root = root / council_id / "Fake" + + +class FakeScraper: + """Stands in for a council's scraper class.""" + + disabled = False + tags = [] + get_tags = [] + scraper_object_type = "Fake" + failing_councils = set() + data_root = None + ran = [] + + def __init__(self, options, console): + self.options = options + self.console = console + self.council_id = options["council"] + self.storage_backend = FakeStorageBackend( + FakeScraper.data_root, self.council_id + ) + + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + def run_since(self, hours=24): + return None + + def run(self, run_log): + council = self.options["council"] + FakeScraper.ran.append(council) + if council in FakeScraper.failing_councils: + raise RuntimeError(f"{council} is broken") + + +class Command(PerCouncilCommandBase): + command_name = "minutes" + + def output_report(self): + pass + + +@pytest.fixture +def command(tmp_path, monkeypatch): + monkeypatch.setattr(settings, "BASE_PATH", tmp_path) + monkeypatch.setattr( + "lgsf.commands.base.load_scraper", lambda code, cmd: FakeScraper + ) + monkeypatch.setattr("lgsf.commands.base.load_council_info", lambda code: {}) + FakeScraper.ran = [] + FakeScraper.failing_councils = set() + FakeScraper.data_root = tmp_path / "data" + + def make(councils, **options): + cmd = Command(["minutes", "--council", ",".join(councils)], io.StringIO()) + cmd.options = { + "council": ",".join(councils), + "all_councils": False, + "refresh": False, + "tags": None, + "exclude_missing": False, + "verbose": False, + "report": False, + "workers": 4, + **options, + } + return cmd + + return make + + +def run_logs(tmp_path): + """Every runlog.json written under the fake data directory.""" + return { + path.parent.parent.name: json.loads(path.read_text()) + for path in (tmp_path / "data").glob("*/Fake/runlog.json") + } + + +def test_every_council_runs_even_when_one_fails(command, tmp_path): + """The whole point: a broken council must not end the run.""" + cmd = command(["AAA", "BBB", "CCC"]) + FakeScraper.failing_councils = {"BBB"} + + cmd.run_councils() + + assert sorted(FakeScraper.ran) == ["AAA", "BBB", "CCC"] + + +def test_a_failure_does_not_abort_even_with_verbose(command): + """-v is for detail, not for stopping at the first problem.""" + cmd = command(["AAA", "BBB", "CCC"], verbose=True) + FakeScraper.failing_councils = {"AAA"} + + cmd.run_councils() + + assert sorted(FakeScraper.ran) == ["AAA", "BBB", "CCC"] + + +def test_a_single_council_still_raises_with_verbose(command): + """ + One council is someone working on that scraper, or a scripted call that + needs the failure to reach its exit code. + """ + cmd = command(["AAA"], verbose=True) + FakeScraper.failing_councils = {"AAA"} + + with pytest.raises(RuntimeError, match="AAA is broken"): + cmd.run_councils() + + +def test_a_single_council_without_verbose_does_not_raise(command): + cmd = command(["AAA"]) + FakeScraper.failing_councils = {"AAA"} + + cmd.run_councils() + + assert FakeScraper.ran == ["AAA"] + + +def test_each_council_gets_its_own_options(command): + """ + Sharing one options dict means concurrent runs overwrite each other's + council, and scrapers end up scraping the wrong site. + """ + seen = [] + original = FakeScraper.run + + def record(self, run_log): + seen.append(self.options["council"]) + return original(self, run_log) + + FakeScraper.run = record + try: + cmd = command(["AAA", "BBB", "CCC"]) + cmd.run_councils() + finally: + FakeScraper.run = original + + assert sorted(seen) == ["AAA", "BBB", "CCC"] + # The command's own options are left as the user gave them + assert cmd.options["council"] == "AAA,BBB,CCC" + + +def test_run_log_records_every_council(command, tmp_path): + cmd = command(["AAA", "BBB"]) + FakeScraper.failing_councils = {"BBB"} + + cmd.run_councils() + + entries = run_logs(tmp_path) + assert sorted(entries) == ["AAA", "BBB"] + assert entries["AAA"]["status_code"] == 0 + assert entries["BBB"]["status_code"] == 1 + assert "BBB is broken" in entries["BBB"]["error"] + assert entries["AAA"]["command"] == "minutes" + + +def test_run_log_is_not_written_in_lambda(command, tmp_path): + """Lambda runs one council per invocation and logs to CloudWatch.""" + cmd = command(["AAA"], aws_lambda=True) + + cmd.run_councils() + + assert run_logs(tmp_path) == {} + + +def test_run_log_sits_beside_the_data_it_describes(command, tmp_path): + cmd = command(["AAA"]) + + cmd.run_councils() + + assert (tmp_path / "data" / "AAA" / "Fake" / "runlog.json").is_file() + + +def test_only_the_most_recent_run_is_kept(command, tmp_path): + """One file per council and type, replaced each run.""" + command(["AAA"]).run_councils() + FakeScraper.failing_councils = {"AAA"} + command(["AAA"]).run_councils() + + entry = run_logs(tmp_path)["AAA"] + assert entry["status_code"] == 1 + assert "AAA is broken" in entry["error"] + + +def test_a_backend_with_nowhere_local_to_write_is_skipped(command, tmp_path): + """The GitHub backend keeps data elsewhere and reports via the dashboard.""" + + class Remote: + pass + + original = FakeScraper.__init__ + + def no_local_root(self, options, console): + original(self, options, console) + self.storage_backend = Remote() + + FakeScraper.__init__ = no_local_root + try: + command(["AAA"]).run_councils() + finally: + FakeScraper.__init__ = original + + assert run_logs(tmp_path) == {} + + +def test_workers_can_be_forced_to_one(command): + cmd = command(["AAA", "BBB"], workers=1) + + cmd.run_councils() + + assert cmd._concurrent is False + assert sorted(FakeScraper.ran) == ["AAA", "BBB"] + + +def test_a_single_council_is_never_run_concurrently(command): + """Its output should stream rather than be replayed at the end.""" + cmd = command(["AAA"], workers=8) + + cmd.run_councils() + + assert cmd._concurrent is False From 9ef8169a6f62f32a47ca25b7fbcd55dbcd458eea Mon Sep 17 00:00:00 2001 From: symroe Date: Fri, 21 Aug 2026 09:04:31 +0100 Subject: [PATCH 07/12] Add minutes as a scraper data type Ports the meeting-scraping half of poteris/council-scraper into the framework, mirroring the shape of the councillors vertical: MeetingBase, BaseMinutesScraper, ModernGov and CMIS implementations, and a command. Meetings are scraped over a rolling window that looks mostly backwards, because minutes are published days or weeks after a meeting and revisiting is how they get picked up. Minutes accumulate rather than replace, since a meeting that has dropped out of the window is still a fact. Documents go to the document store, not into raw/ beside the metadata, and a _index.json records where each one went along with its hash and any HTTP validators. Documents already in the store are skipped, so a second run over the same window costs almost nothing - without it a single council re-downloaded 267 files and 104MB every run. The index is treated as a cache rather than a source of truth: every entry is checked against the store before it is used to skip, so a deleted file costs a re-download rather than losing the document. The full agenda pack is kept; categorise_document only labels documents, it doesn't decide which are worth downloading. The CMIS scraper walks a month calendar per month in the window. Its list view renders only the server's current calendar month and ignores any date parameter, so a CMIS council could otherwise never return the same window as ModernGov. The month view is reachable only in DNN's path form, which needs the site's tabid as well as the module id; both are lifted from a real meeting link, and the list view remains as a fallback. --skip-documents records documents without fetching them, which is what you want while iterating on parsing, or when checking several hundred scrapers still work. Co-Authored-By: Claude Opus 5 --- lgsf/conf/__init__.py | 1 + lgsf/minutes/__init__.py | 2 + lgsf/minutes/commands.py | 46 + lgsf/minutes/exceptions.py | 10 + lgsf/minutes/models.py | 83 ++ lgsf/minutes/scrapers.py | 933 ++++++++++++++++++ lgsf/minutes/tests/__init__.py | 0 lgsf/minutes/tests/fixtures/cmis_base.html | 10 + .../tests/fixtures/cmis_calendar_month.html | 39 + .../fixtures/cmis_detail_no_minutes.html | 17 + .../fixtures/cmis_detail_with_minutes.html | 22 + .../minutes/tests/fixtures/cmis_listview.html | 29 + .../tests/fixtures/cmis_meeting_summary.html | 30 + lgsf/minutes/tests/fixtures/committees.xml | 25 + .../tests/fixtures/meeting_no_minutes.xml | 30 + .../tests/fixtures/meeting_with_minutes.xml | 151 +++ lgsf/minutes/tests/fixtures/meetings.xml | 26 + lgsf/minutes/tests/test_base_class.py | 23 + lgsf/minutes/tests/test_cmis_parsing.py | 455 +++++++++ lgsf/minutes/tests/test_document_scraping.py | 396 ++++++++ lgsf/minutes/tests/test_index_and_etags.py | 228 +++++ lgsf/minutes/tests/test_modgov_parsing.py | 133 +++ lgsf/tests/test_runner.py | 1 + 23 files changed, 2690 insertions(+) create mode 100644 lgsf/minutes/__init__.py create mode 100644 lgsf/minutes/commands.py create mode 100644 lgsf/minutes/exceptions.py create mode 100644 lgsf/minutes/models.py create mode 100644 lgsf/minutes/scrapers.py create mode 100644 lgsf/minutes/tests/__init__.py create mode 100644 lgsf/minutes/tests/fixtures/cmis_base.html create mode 100644 lgsf/minutes/tests/fixtures/cmis_calendar_month.html create mode 100644 lgsf/minutes/tests/fixtures/cmis_detail_no_minutes.html create mode 100644 lgsf/minutes/tests/fixtures/cmis_detail_with_minutes.html create mode 100644 lgsf/minutes/tests/fixtures/cmis_listview.html create mode 100644 lgsf/minutes/tests/fixtures/cmis_meeting_summary.html create mode 100644 lgsf/minutes/tests/fixtures/committees.xml create mode 100644 lgsf/minutes/tests/fixtures/meeting_no_minutes.xml create mode 100644 lgsf/minutes/tests/fixtures/meeting_with_minutes.xml create mode 100644 lgsf/minutes/tests/fixtures/meetings.xml create mode 100644 lgsf/minutes/tests/test_base_class.py create mode 100644 lgsf/minutes/tests/test_cmis_parsing.py create mode 100644 lgsf/minutes/tests/test_document_scraping.py create mode 100644 lgsf/minutes/tests/test_index_and_etags.py create mode 100644 lgsf/minutes/tests/test_modgov_parsing.py diff --git a/lgsf/conf/__init__.py b/lgsf/conf/__init__.py index 13b60e96..e1519fa2 100644 --- a/lgsf/conf/__init__.py +++ b/lgsf/conf/__init__.py @@ -26,6 +26,7 @@ def __init__(self): self.APPS = ( "councillors", + "minutes", "templates", "metadata", "sync", diff --git a/lgsf/minutes/__init__.py b/lgsf/minutes/__init__.py new file mode 100644 index 00000000..0e084218 --- /dev/null +++ b/lgsf/minutes/__init__.py @@ -0,0 +1,2 @@ +from .models import * # noqa +from .exceptions import * # noqa diff --git a/lgsf/minutes/commands.py b/lgsf/minutes/commands.py new file mode 100644 index 00000000..4bc7929d --- /dev/null +++ b/lgsf/minutes/commands.py @@ -0,0 +1,46 @@ +from rich.table import Table + +from lgsf.commands.base import PerCouncilCommandBase + + +class Command(PerCouncilCommandBase): + command_name = "minutes" + + def add_arguments(self, parser): + parser.add_argument( + "--skip-documents", + action="store_true", + help="Find and record documents but don't download them. Useful " + "for checking a scraper works without pulling every PDF.", + ) + + def output_report(self): + """Display a Rich table report of scraped meeting data""" + table = Table(title="Scraped Meetings") + table.add_column("Date", style="cyan", no_wrap=True) + table.add_column("Committee", style="green", no_wrap=False) + table.add_column("Status", style="yellow", no_wrap=False) + table.add_column("Minutes", style="blue", no_wrap=True) + table.add_column("Documents", style="magenta", no_wrap=True) + + for meeting in sorted(self.scraped_items, key=lambda m: m.date): + documents = getattr(meeting, "documents", []) or [] + minutes_docs = [d for d in documents if d.get("category") == "minutes"] + stored = [d for d in documents if d.get("storage_key")] + table.add_row( + meeting.date or "N/A", + meeting.committee or "N/A", + getattr(meeting, "status", None) or "", + "yes" if getattr(meeting, "minutes_published", False) else "", + f"{len(stored)} stored / {len(documents)} linked " + f"({len(minutes_docs)} minutes)", + ) + + total_meetings = len(self.scraped_items) + if total_meetings > 0: + self.console.print( + f"\n[bold green]Total meetings scraped: {total_meetings}[/bold green]" + ) + self.console.print(table) + else: + self.console.print("[yellow]No meeting data found to report[/yellow]") diff --git a/lgsf/minutes/exceptions.py b/lgsf/minutes/exceptions.py new file mode 100644 index 00000000..caf6d5bd --- /dev/null +++ b/lgsf/minutes/exceptions.py @@ -0,0 +1,10 @@ +class SkipMeetingException(Exception): + """Raised by a scraper to drop a meeting from the run entirely.""" + + +class MeetingNotModifiedException(Exception): + """ + Raised when the server answered 304 to a conditional request for a + meeting, meaning what we already have stored is still current and there + is nothing to re-scrape. + """ diff --git a/lgsf/minutes/models.py b/lgsf/minutes/models.py new file mode 100644 index 00000000..e01b9845 --- /dev/null +++ b/lgsf/minutes/models.py @@ -0,0 +1,83 @@ +import json +from dataclasses import dataclass, field +from pathlib import Path + +from slugify import slugify + + +@dataclass +class MeetingBase: + url: str + identifier: str + title: str + committee: str + date: str + committee_id: str = None + status: str = field(init=False, hash=False, compare=False) + location: str = field(init=False, hash=False, compare=False) + minutes_published: bool = field(init=False, hash=False, compare=False) + documents: list = field(init=False, hash=False, compare=False) + # Where this meeting's detail was fetched from, plus any HTTP validators + # the server gave us, so the next run can ask "has this changed?" + source: dict = field(init=False, hash=False, compare=False) + + def __repr__(self): + return "".format(self.committee, self.date) + + def __hash__(self): + return hash(self.identifier) + + def __eq__(self, other): + return ( + issubclass(type(other), MeetingBase) and self.identifier == other.identifier + ) + + def as_file_name(self): + return ( + f"{slugify(self.date)}-{slugify(self.identifier)}-{slugify(self.committee)}" + ) + + @classmethod + def from_storage(cls, filename: Path, session): + """Load meeting from storage session""" + data = json.loads(session.open(filename)) + + status = data.pop("status", None) + location = data.pop("location", None) + minutes_published = data.pop("minutes_published", None) + documents = data.pop("documents", None) + source = data.pop("source", None) + for k in list(data.keys()): + if k.startswith("raw_"): + data[k[4:]] = data.pop(k) + + meeting = cls(**data) + if status: + meeting.status = status + if location: + meeting.location = location + if minutes_published is not None: + meeting.minutes_published = minutes_published + if documents is not None: + meeting.documents = documents + if source is not None: + meeting.source = source + return meeting + + def as_dict(self): + out = { + "url": self.url, + "status": getattr(self, "status", None), + "location": getattr(self, "location", None), + "minutes_published": getattr(self, "minutes_published", None), + "documents": getattr(self, "documents", []), + "source": getattr(self, "source", {}), + } + RAW_FIELDS = ["identifier", "title", "committee", "committee_id", "date"] + for attr in RAW_FIELDS: + out["raw_{}".format(attr)] = getattr(self, attr) + + return out + + def as_json(self): + return json.dumps(self.as_dict(), indent=4, sort_keys=True) diff --git a/lgsf/minutes/scrapers.py b/lgsf/minutes/scrapers.py new file mode 100644 index 00000000..c768304d --- /dev/null +++ b/lgsf/minutes/scrapers.py @@ -0,0 +1,933 @@ +import abc +import datetime +import json +import re +from functools import cached_property +from urllib.parse import urljoin + +from bs4 import BeautifulSoup, Tag + +from lgsf.aws_lambda.run_log import RunLog +from lgsf.minutes import MeetingBase +from lgsf.minutes.exceptions import ( + MeetingNotModifiedException, + SkipMeetingException, +) +from lgsf.scrapers import ScraperBase +from lgsf.storage.backends.base import StorageMode + + +class BaseMinutesScraper(ScraperBase): + tags = [] + class_tags = [] + ext = "html" + scraper_object_type = "Minutes" + service_name = "minutes" + + # Council-run ModernGov installs are far more variable than the + # vendor-hosted ones: moderngov.co.uk sites answer well inside a + # second, while self-hosted ones can take up to a minute. The + # framework default of 30s is too short for the slowest of them. + timeout = 60 + + # Minutes are an append-only historical record: a meeting that has + # dropped out of the rolling scrape window is still a fact, so runs + # must add to what's stored rather than replacing it. + storage_mode = StorageMode.ACCUMULATE + + # Meetings are scraped over a rolling window around today + weeks_back = 4 + weeks_forward = 1 + + # When True, every document linked from a meeting is downloaded to the + # document store. The full agenda pack is kept, not just the minutes: + # categorisation labels documents, it doesn't filter them. + save_documents = True + + #: Where the run index lives inside the metadata store. + INDEX_FILE_NAME = "_index.json" + INDEX_VERSION = 1 + + def __init__(self, options, console): + super().__init__(options, console) + self.meetings = set() + self.new_data = True + self.unchanged_meetings = 0 + self.documents_downloaded = 0 + self.documents_skipped = 0 + self.documents_linked = 0 + + @property + def skip_documents(self): + """ + True when this run should find documents but not download them. + + Set by --skip-documents. A full run of every council downloads + tens of gigabytes, which is a lot of transfer to answer "does this + scraper still work?", so a sweep can leave the files alone and + still check that meetings and their documents are found. + """ + return bool(self.options.get("skip_documents")) + + @cached_property + def index(self): + """ + What previous runs already fetched, keyed by URL. + + The storage session API can read and write files but not list them, + so rather than trying to enumerate hundreds of stored meeting JSON + files we keep one index alongside them. It records the HTTP + validators for each meeting page and, for each document, where it + was stored and what its hash was. + + This is a cache, not a source of truth: every entry is verified + against the document store before it's used to skip work, so a + stale or hand-deleted index costs a re-download rather than losing + a document. + """ + empty = {"version": self.INDEX_VERSION, "meetings": {}, "documents": {}} + try: + stored = json.loads( + self.storage_session.open(self._file_name(self.INDEX_FILE_NAME)) + ) + except (FileNotFoundError, ValueError): + return empty + + if stored.get("version") != self.INDEX_VERSION: + # Index written by a different version of this scraper; start + # again rather than guessing at its shape. + return empty + + empty.update( + { + "meetings": stored.get("meetings") or {}, + "documents": stored.get("documents") or {}, + } + ) + return empty + + def save_index(self): + """Write the run index back out for the next run to read.""" + self.index["updated"] = datetime.datetime.now().isoformat() + self.storage_session.write( + self._file_name(self.INDEX_FILE_NAME), + json.dumps(self.index, indent=4, sort_keys=True), + ) + + def validators_for(self, url): + """Return (etag, last_modified) recorded for ``url``, if any.""" + entry = self.index["meetings"].get(url) or {} + return entry.get("etag"), entry.get("last_modified") + + def record_source(self, url, response): + """ + Note the validators a response came back with, and return the + ``source`` block stored on the meeting. + """ + source = { + "url": url, + "etag": self.response_header(response, "etag"), + "last_modified": self.response_header(response, "last-modified"), + } + self.index["meetings"][url] = { + "etag": source["etag"], + "last_modified": source["last_modified"], + } + return source + + @property + def date_range(self): + today = datetime.date.today() + start = today - datetime.timedelta(weeks=self.weeks_back) + end = today + datetime.timedelta(weeks=self.weeks_forward) + return start, end + + @abc.abstractmethod + def get_meetings(self): + pass + + @abc.abstractmethod + def get_single_meeting(self, meeting_data): + pass + + def add_meeting( + self, + url, + identifier: str, + title: str, + committee: str, + date: str, + committee_id: str = None, + ): + assert committee, f"No Committee for {url}" + assert title, f"No Title for {url}" + assert date, f"No Date for {url}" + assert identifier, f"No Identifier for {url}" + meeting = MeetingBase( + url, + identifier=identifier, + title=title, + committee=committee, + date=date, + committee_id=committee_id, + ) + self.meetings.add(meeting) + return meeting + + @property + def get_tags(self): + return self.tags + self.class_tags + + @property + def report_items(self): + return list(self.meetings) + + def run(self, run_log: RunLog): + for meeting_data in self.get_meetings(): + try: + meeting = self.get_single_meeting(meeting_data) + self.process_meeting(meeting, meeting_data) + except MeetingNotModifiedException: + self.unchanged_meetings += 1 + continue + except SkipMeetingException: + continue + + self.save_index() + + # Finalize storage with run log data + self.finalize_storage(run_log) + + self.report() + + def response_bytes(self, response): + """ + Return a response body as bytes, normalising across the supported + HTTP clients (requests/httpx expose .content, wreq a .bytes()). + """ + for attr in ("content", "bytes"): + body = getattr(response, attr, None) + if body is None: + continue + if callable(body): + body = body() + return body + raise ValueError(f"Can't read bytes from {type(response)} response") + + def get_bytes(self, url): + """Wraps self.get and always returns the response body as bytes.""" + return self.response_bytes(self.get(url)) + + def prettify_meeting_str(self, meeting_raw_str): + if isinstance(meeting_raw_str, dict): + return json.dumps(meeting_raw_str, indent=4) + if isinstance(meeting_raw_str, Tag): + return meeting_raw_str.prettify() + return None + + def process_meeting(self, meeting, meeting_raw_str): + formatted_meeting_raw_str = self.prettify_meeting_str(meeting_raw_str) + + self.documents_linked += len(getattr(meeting, "documents", []) or []) + + # Documents first: storing them adds the hash and storage key to each + # document dict, and the meeting JSON is serialised from those, so + # saving the meeting first would record documents with no location. + if self.save_documents and not self.skip_documents: + self.save_meeting_documents(meeting) + + self.save_meeting(formatted_meeting_raw_str, meeting) + + def save_meeting(self, raw_content, meeting_obj): + assert type(meeting_obj) is MeetingBase, "Scrapers must return a meeting object" + file_name = "{}.{}".format(meeting_obj.as_file_name(), self.ext) + self.save_raw(file_name, raw_content) + self.save_json(meeting_obj) + + def document_key(self, meeting_obj, document, index): + """ + Build the document store key for one of a meeting's documents. + + CMIS documents have no clean attachment id (their URLs are opaque + encrypted query strings), so fall back to a per-meeting index to + keep keys distinct. + """ + suffix = document.get("attachment_id") or index + extension = self.document_extension(document) + return "{}-{}{}".format(meeting_obj.as_file_name(), suffix, extension) + + def document_extension(self, document): + """ + Guess a file extension from the document URL. + + ModernGov and CMIS both serve documents through handler URLs with no + extension at all (mgConvert2PDF.aspx, Document.ashx), and those are + overwhelmingly PDFs, so that's the fallback. + """ + path = document.get("url", "").split("?")[0].lower() + for extension in (".pdf", ".docx", ".doc", ".xlsx", ".xls", ".rtf", ".txt"): + if path.endswith(extension): + return extension + return ".pdf" + + def stored_document_metadata(self, url): + """ + Return the recorded metadata for an already-stored document, or None. + + The index is only trusted as far as the document store agrees with + it: an entry whose file has gone is treated as never fetched. + """ + entry = self.index["documents"].get(url) + if not entry: + return None + + key = entry.get("storage_key") + if not key: + return None + + try: + if not self.document_storage.exists(key): + return None + except ValueError: + # Key written by a backend with a different key format + return None + + return entry + + def save_meeting_documents(self, meeting_obj): + """ + Store every document linked from this meeting, skipping any we + already hold, and record where each one went on the meeting's + metadata. + + Documents go to the document store rather than the metadata store: + the JSON that lands in git carries a hash and a storage key, and the + file itself lives wherever the document backend puts it. + + A failed download shouldn't fail the whole scrape. + """ + for index, document in enumerate( + getattr(meeting_obj, "documents", []), start=1 + ): + url = document.get("url") + if not url: + continue + + existing = self.stored_document_metadata(url) + if existing: + # Already downloaded by an earlier run and still in the + # store: reuse its metadata so a skipped document looks + # exactly like a freshly fetched one. + document.update(existing) + self.documents_skipped += 1 + continue + + etag, last_modified = (None, None) + stale = self.index["documents"].get(url) or {} + if stale: + etag = stale.get("etag") + last_modified = stale.get("last_modified") + + try: + response = self.get_conditional( + url, + etag=etag, + last_modified=last_modified, + extra_headers=self.extra_headers, + ) + if self.response_status(response) == 304: + # Unchanged, but the file isn't in the store (or we + # wouldn't be here), so fetch it unconditionally. + response = self.get(url, extra_headers=self.extra_headers) + content = self.response_bytes(response) + except Exception as e: + self.console.log(f"[yellow]Failed to download {url}: {e}[/yellow]") + continue + + key = self.document_key(meeting_obj, document, index) + stored = self.document_storage.write(key, content) + + entry = stored.as_dict() + entry["etag"] = self.response_header(response, "etag") + entry["last_modified"] = self.response_header(response, "last-modified") + entry["content_type"] = self.response_header(response, "content-type") + + document.update(entry) + self.index["documents"][url] = entry + self.documents_downloaded += 1 + + def report(self): + # Unlike councillors, zero meetings in the scrape window is + # legitimate, so don't treat a low count as an error. + self.console.log( + f"Found {len(self.meetings)} meetings " + f"({self.unchanged_meetings} unchanged since last run)" + ) + if self.skip_documents: + self.console.log( + f"Documents: {self.documents_linked} found, none downloaded " + "(--skip-documents)" + ) + else: + self.console.log( + f"Documents: {self.documents_downloaded} downloaded, " + f"{self.documents_skipped} already stored" + ) + + def categorise_document(self, title): + lowered = title.lower() + if "minute" in lowered: + return "minutes" + if "agenda" in lowered: + return "agenda" + return "other" + + +class CustomHTMLMinutesScraper(BaseMinutesScraper): + """ + Base for councils whose meetings are on neither ModernGov nor CMIS. + + Supplies page fetching and parsing. `get_meetings` and `get_single_meeting` + are still abstract, and are what the council's scraper implements. + """ + + class_tags = ["html"] + + def get_page(self, url): + """The URL fetched and parsed, ready to select against.""" + return BeautifulSoup( + self.get_text(url, extra_headers=self.extra_headers), "lxml" + ) + + +class ModGovMinutesScraper(BaseMinutesScraper): + class_tags = ["modgov"] + ext = "xml" + + DATE_FORMAT = "%d/%m/%Y" + + def format_committees_api_url(self): + return "{}/mgWebService.asmx/GetCommittees".format(self.base_url) + + def format_meetings_api_url(self, committee_id, start, end): + return ( + "{}/mgWebService.asmx/GetMeetings?lCommitteeId={}" + "&sFromDate={}&sToDate={}".format( + self.base_url, + committee_id, + start.strftime(self.DATE_FORMAT), + end.strftime(self.DATE_FORMAT), + ) + ) + + def format_meeting_api_url(self, meeting_id): + return "{}/mgWebService.asmx/GetMeeting?lMeetingId={}".format( + self.base_url, meeting_id + ) + + def run(self, run_log: RunLog): + """ + ModernGov needs a three-level fetch (committees -> meeting stubs for + each committee's date window -> full detail per meeting), so - like + ModGovCouncillorScraper - this overrides the base run() loop rather + than using the generic get_meetings()/get_single_meeting() pairing. + """ + for committee in self.get_meetings(): + committee_id = committee.find("committeeid").text.strip() + for meeting_stub in self.get_meeting_stubs(committee_id): + meeting_id_tag = meeting_stub.find("meetingid") + if not meeting_id_tag or not meeting_id_tag.text.strip(): + continue + try: + detail = self.get_meeting_detail(meeting_id_tag.text.strip()) + except MeetingNotModifiedException: + self.unchanged_meetings += 1 + continue + if detail is None: + continue + try: + meeting = self.get_single_meeting(committee, detail) + self.process_meeting(meeting, detail) + except SkipMeetingException: + continue + + self.save_index() + self.finalize_storage(run_log) + self.report() + + def get_meetings(self): + text = self.get_text( + self.format_committees_api_url(), extra_headers=self.extra_headers + ) + soup = BeautifulSoup(text, features="xml") + return soup.find_all("committee") + + def get_meeting_stubs(self, committee_id): + start, end = self.date_range + text = self.get_text( + self.format_meetings_api_url(committee_id, start, end), + extra_headers=self.extra_headers, + ) + soup = BeautifulSoup(text, features="xml") + return soup.find_all("meeting") + + def get_meeting_detail(self, meeting_id): + """ + Fetch one meeting's full detail, asking the server first whether + anything has changed since we last looked. + + Raises MeetingNotModifiedException on a 304 so the caller can leave + the stored copy alone. ModernGov installs vary in whether they send + validators at all; where they don't, this is an ordinary GET. + """ + url = self.format_meeting_api_url(meeting_id) + etag, last_modified = self.validators_for(url) + + response = self.get_conditional( + url, + etag=etag, + last_modified=last_modified, + extra_headers=self.extra_headers, + ) + if self.response_status(response) == 304: + raise MeetingNotModifiedException(url) + + self._pending_source = self.record_source(url, response) + + text = response.text + if callable(text): + text = text() + soup = BeautifulSoup(text, features="xml") + return soup.find("meeting") + + def get_single_meeting(self, committee, detail): + committee_id = committee.find("committeeid").text.strip() + committee_title = committee.find("committeetitle").text.strip() + + identifier = detail.find("meetingid").text.strip() + date = self.parse_date(detail.find("meetingdate").text.strip()) + url = "{}/ieListDocuments.aspx?CId={}&MId={}".format( + self.base_url, committee_id, identifier + ) + + meeting = self.add_meeting( + url, + identifier=identifier, + title=f"{committee_title} - {date}", + committee=committee_title, + date=date, + committee_id=committee_id, + ) + + status_tag = detail.find("meetingstatus") + meeting.status = status_tag.text.strip() if status_tag else None + location_tag = detail.find("meetinglocation") + meeting.location = location_tag.text.strip() if location_tag else None + minutes_published_tag = detail.find("minutepublished") + meeting.minutes_published = bool( + minutes_published_tag and minutes_published_tag.text.strip() == "True" + ) + meeting.source = getattr(self, "_pending_source", {}) + meeting.documents = self.get_documents(detail) + + self.save_minutes_html(meeting, detail) + + if self.exclude_meeting_hook(meeting): + raise SkipMeetingException + + return meeting + + def parse_date(self, date_str): + """ModernGov dates are dd/mm/yyyy; store ISO format.""" + return datetime.datetime.strptime(date_str, self.DATE_FORMAT).date().isoformat() + + def get_documents(self, detail): + documents = [] + seen = set() + for linked_doc in detail.find_all("linkeddoc"): + url_tag = linked_doc.find("url") + title_tag = linked_doc.find("title") + attachment_id_tag = linked_doc.find("attachmentid") + if not url_tag or not url_tag.text.strip(): + continue + url = url_tag.text.strip() + if url in seen: + continue + seen.add(url) + title = title_tag.text.strip() if title_tag else "" + documents.append( + { + "title": title, + "url": url, + "attachment_id": attachment_id_tag.text.strip() + if attachment_id_tag + else None, + "category": self.categorise_document(title), + } + ) + return documents + + def save_minutes_html(self, meeting, detail): + """ + ModernGov embeds the text of published minutes in each agenda item + (as escaped HTML in ). Save the combined + HTML alongside the raw XML. + """ + parts = [] + for item in detail.find_all("agendaitem"): + body = item.find("minutesnonemptyhtmlbody") + if body and body.text.strip(): + title_tag = item.find("agendaitemtitle") + if title_tag and title_tag.text.strip(): + parts.append(f"

{title_tag.text.strip()}

") + parts.append(body.text) + if parts: + file_name = f"{meeting.as_file_name()}-minutes.html" + self.save_raw(file_name, "\n".join(parts)) + + def exclude_meeting_hook(self, meeting: MeetingBase): + return False + + +class CMISMinutesScraper(BaseMinutesScraper): + """ + CMIS councils publish a "Meetings" calendar module (a DNN/Telerik web + part). Its default view is a JS calendar widget, but the same module + also serves plain server-rendered views that need no session, cookies + or JS. + + Two of those views matter here: + + ``ctl=MeetingCalendarListView`` renders a table of meetings, but only + ever for the server's *current* calendar month - it ignores any date + parameter, so it can't reach the rest of a scrape window. + + ``ctl/MeetingCalendarPublicNoJava/mid//Date//`` renders + a whole month's calendar for *any* date, as a plain GET. That's what + this scraper walks, one request per month spanned by the window, which + is what lets CMIS councils return the same rolling window as ModernGov. + + It has to be reached in DNN's path form rather than as a query string, + and that form needs the site's ``tabid`` as well as the module id. + Neither is published anywhere convenient, so both are recovered from a + real meeting link on the list view - see ``get_calendar_url_template``. + + The list view remains the fallback for installs where no such link can + be found (typically a month with no meetings at all). + """ + + class_tags = ["cmis"] + ext = "html" + + # Rendering a month of the no-JS calendar is slow - Birmingham takes + # around 40 seconds - and comfortably exceeds the framework default. + timeout = 120 + + # The module is named MeetingCalendarPublic on most installs but + # carries a suffix on some (Nottinghamshire: MeetingCalendarPublicList), + # so match the stem rather than the whole name. + MODULE_ID_PATTERN = re.compile(r"dnn\$ctr(\d+)\$MeetingCalendarPublic\w*\$") + ROW_LINK_TEXT_PATTERN = re.compile( + r"^(?P\d{1,2} \w+ \d{4} \d{2}:\d{2}) - (?P.+)$" + ) + ROW_URL_PATTERN = re.compile( + r"/Meeting/(?P\d+)/Committee/(?P\d+)/" + ) + LINK_TEXT_DATE_FORMAT = "%d %b %Y %H:%M" + + # A meeting link in DNN path form, e.g. + # .../Meetings/tabid/70/ctl/ViewMeetingPublic/mid/397/Meeting/15218/... + # Everything up to and including the module id is reusable as the + # prefix for any other view of the same module. + CALENDAR_LINK_PATTERN = re.compile( + r"(?P\S*/tabid/\d+)/ctl/\w+/(?Pmid/\d+)/", re.IGNORECASE + ) + + # A day heading in the month calendar, e.g. "Monday, 8 June". The year + # is not repeated in the cell, so it comes from the requested month. + CALENDAR_DAY_PATTERN = re.compile( + r"^\w+day,\s+(?P\d{1,2})\s+(?P\w+)(?:\s+(?P\d{4}))?$" + ) + + # A meeting entry within a day, e.g. "10:00 Licensing Sub-Committee A". + CALENDAR_ENTRY_PATTERN = re.compile( + r"^(?:(?P