diff --git a/backend/app.py b/backend/app.py index 6c4e0e8..caa3c21 100644 --- a/backend/app.py +++ b/backend/app.py @@ -38,7 +38,7 @@ from .data_loader import ( ) from .data_loader import get_data_info as get_db_info from . import flags -from .places import build_place_registry, places_for_urn +from .places import build_place_index, build_place_registry, places_for_urn from .schemas import METRIC_DEFINITIONS, RANKING_COLUMNS, SCHOOL_COLUMNS from .utils import clean_for_json, convert_to_native @@ -65,6 +65,10 @@ _sitemaps: dict[str, str] | None = None # Built from the same DataFrame the sitemap uses, so places and sitemap can # never describe different corpora. Reset by the same admin endpoint. _place_registry: dict | None = None +# Cached beside the registry, and invalidated by identity against it — see +# get_place_index. Never cleared independently. +_place_index: dict | None = None +_place_index_source: dict | None = None VALID_PLACE_KINDS = ("town", "locality", "authority", "outcode") @@ -188,6 +192,24 @@ def get_place_registry() -> dict: return _place_registry +def get_place_index() -> dict: + """URN → its published places, cached against the registry it came from. + + Invalidation is an identity check rather than a second flag to remember to + clear. Anything that drops `_place_registry` — the tests all do — gets a + fresh registry object here, which no longer matches the one the index was + built from, so the index rebuilds with it. A separate `_place_index = None` + would be one more thing to forget, and a stale reverse index is exactly the + bug that would put links to another dataset's places on a school page. + """ + global _place_index, _place_index_source + registry = get_place_registry() + if _place_index is None or _place_index_source is not registry: + _place_index = build_place_index(registry) + _place_index_source = registry + return _place_index + + def _urlset(rows: list[str]) -> str: return "\n".join([ '', @@ -229,7 +251,7 @@ def _places_payload(urn: int) -> list[dict]: so they report no phase links on their own. """ payload = [] - for place in places_for_urn(get_place_registry(), int(urn)): + for place in places_for_urn(get_place_index(), int(urn)): phases = [ { "phase": phase, diff --git a/backend/places.py b/backend/places.py index 6476c38..60055e8 100644 --- a/backend/places.py +++ b/backend/places.py @@ -296,21 +296,46 @@ def _locality_places(df, publishable: set[int], return out -def places_for_urn(registry: dict[str, Place], urn: int) -> tuple[Place, ...]: - """Every published place containing this school, largest kind first. +# Ordered authority → town/locality → outcode, widest first, because that is +# the order a breadcrumb reads. The link module re-sorts for its own purposes. +_PLACE_ORDER = {"authority": 0, "town": 1, "locality": 2, "outcode": 3} + + +def build_place_index(registry: dict[str, Place]) -> dict[int, tuple[Place, ...]]: + """URN → the published places containing it, built once per registry. The reverse of the registry, and the thing school pages link out through. - Derived from the registry rather than stored beside it, so the two cannot - disagree about which places exist: a link module must never offer a place - whose page does not exist, and a place below the publish threshold is - simply absent from the registry, so it is absent from here too. + Derived from the registry rather than maintained beside it, so the two + cannot disagree about which places exist: a place below the publish + threshold is absent from the registry, so it is absent from here too, and + a link is never offered for a page that does not exist. - Ordered authority → town/locality → outcode, widest first, because that is - the order a breadcrumb reads and the order the link module lists. + Built as an index rather than scanned per call because /api/schools/{urn} + is the site's highest-traffic endpoint. Scanning meant walking every place + and doing a tuple membership test against each — on the order of 10^5 + comparisons per request, repeated for every school page view. One pass at + registry-build time replaces all of it with a dict lookup. """ - order = {"authority": 0, "town": 1, "locality": 2, "outcode": 3} - found = [p for p in registry.values() if urn in p.urns] - return tuple(sorted(found, key=lambda p: (order.get(p.kind, 9), p.slug))) + grouped: dict[int, list[Place]] = {} + for place in registry.values(): + for urn in place.urns: + grouped.setdefault(int(urn), []).append(place) + + return { + urn: tuple(sorted(places, + key=lambda p: (_PLACE_ORDER.get(p.kind, 9), p.slug))) + for urn, places in grouped.items() + } + + +def places_for_urn(index: dict[int, tuple[Place, ...]], urn: int) -> tuple[Place, ...]: + """The published places containing this school, widest first. + + Empty is a real answer, not a failure: a school whose town and authority + both fall below the publish threshold has nowhere to link, and the page + renders without the module. + """ + return index.get(int(urn), ()) def build_place_registry(df) -> dict[str, Place]: diff --git a/backend/tests/test_places.py b/backend/tests/test_places.py index c041eda..2bb4ae3 100644 --- a/backend/tests/test_places.py +++ b/backend/tests/test_places.py @@ -8,7 +8,8 @@ import numpy as np import pandas as pd import pytest -from backend.places import MIN_SCHOOLS, build_place_registry, places_for_urn +from backend.places import (MIN_SCHOOLS, build_place_index, + build_place_registry, places_for_urn) def _df(rows: list[dict]) -> pd.DataFrame: @@ -428,7 +429,7 @@ def test_an_authority_still_publishes_phase_variants(): def test_a_school_resolves_to_every_published_place_containing_it(): reg = build_place_registry(_df(_town(MIN_SCHOOLS, "Brentwood", "Essex"))) - places = places_for_urn(reg, 100000) + places = places_for_urn(build_place_index(reg), 100000) kinds = {p.kind for p in places} assert "town" in kinds @@ -443,7 +444,7 @@ def test_a_school_in_an_unpublished_town_still_resolves_to_its_authority(): _town(MIN_SCHOOLS - 1, "Tinytown", "Essex") + _town(MIN_SCHOOLS, "Brentwood", "Essex", start=200000) )) - places = places_for_urn(reg, 100000) + places = places_for_urn(build_place_index(reg), 100000) # The town is below the threshold, so it has no page and must not be # offered as a link. The authority above it does, and is the right target. @@ -455,7 +456,7 @@ def test_an_unknown_urn_resolves_to_nothing_rather_than_raising(): # A school page renders for any URN the API knows; the link module is not # entitled to take the page down when it has nothing to say. reg = build_place_registry(_df(_town(MIN_SCHOOLS, "Brentwood", "Essex"))) - assert places_for_urn(reg, 999999) == () + assert places_for_urn(build_place_index(reg), 999999) == () def test_the_index_is_consistent_with_the_registry_it_was_built_from(): @@ -465,7 +466,32 @@ def test_the_index_is_consistent_with_the_registry_it_was_built_from(): _town(MIN_SCHOOLS, "Brentwood", "Essex") + _town(MIN_SCHOOLS, "Bedford", "Bedford", start=300000) )) + index = build_place_index(reg) for key, place in reg.items(): for urn in place.urns: - assert place in places_for_urn(reg, urn), ( + assert place in places_for_urn(index, urn), ( f"{urn} is in {key} but the index does not say so") + + +def test_the_index_holds_no_school_the_registry_does_not(): + # The reverse direction of the invariant above. An index entry for a URN + # no published place contains would put a link on a page for a place that + # does not list that school. + reg = build_place_registry(_df( + _town(MIN_SCHOOLS, "Brentwood", "Essex") + + _town(MIN_SCHOOLS - 1, "Tinytown", "Essex", start=400000) + )) + index = build_place_index(reg) + + for urn, places in index.items(): + for place in places: + assert urn in place.urns + assert place.key in reg + + +def test_the_index_preserves_the_widest_first_order(): + # The breadcrumb reads authority then town, and takes this order as given. + reg = build_place_registry(_df(_town(MIN_SCHOOLS, "Brentwood", "Essex"))) + kinds = [p.kind for p in places_for_urn(build_place_index(reg), 100000)] + + assert kinds.index("authority") < kinds.index("town") diff --git a/backend/tests/test_school_details.py b/backend/tests/test_school_details.py index 6b4ba55..c7fc398 100644 --- a/backend/tests/test_school_details.py +++ b/backend/tests/test_school_details.py @@ -56,6 +56,11 @@ def client(monkeypatch): monkeypatch.setattr( app_module, "get_supplementary_data", lambda db, urn: {} ) + # The place registry is a module-level cache, so without this the endpoint + # answers from whatever registry an earlier test happened to leave behind + # — and a `places == []` assertion is satisfied by a stale registry just + # as well as by this fixture's own data, which makes it prove nothing. + monkeypatch.setattr(app_module, "_place_registry", None) return TestClient(app_module.app, raise_server_exceptions=False) @@ -212,3 +217,31 @@ def test_a_school_absent_from_the_phase_page_is_not_linked_to_it(monkeypatch): # The town publishes a primary page, but this secondary school is not on # it, and there are too few secondaries for a secondary page. assert town["phases"] == [] + + +def test_the_place_index_rebuilds_when_the_registry_is_replaced(monkeypatch): + """The reverse index is cached; a stale one would put another dataset's + places on a school page. Invalidation is an identity check against the + registry rather than a second flag, so this asserts the check works.""" + from backend import app as app_module + + monkeypatch.setattr(app_module, "_place_registry", None) + monkeypatch.setattr(app_module, "_place_index", None) + monkeypatch.setattr(app_module, "_place_index_source", None) + monkeypatch.setattr(app_module, "load_school_data", _brentwood_df("Primary")) + + first = app_module.get_place_index() + assert 100000 in first + + # Same registry object, so the index is reused rather than rebuilt. + assert app_module.get_place_index() is first + + # Drop the registry the way every test that touches place data does. The + # index must follow it, not survive it. + app_module._place_registry = None + monkeypatch.setattr(app_module, "load_school_data", + _brentwood_df("Primary", n=0)) + + rebuilt = app_module.get_place_index() + assert rebuilt is not first + assert 100000 not in rebuilt, "the index outlived the registry it came from"