fix(api): publish a validated dataset, and stop truncating search
Reload cleared the caches first and rebuilt afterwards, so any failure left the API serving nothing, and requests arriving mid-reload saw a half-swapped state. It now builds and validates the replacement frames, place registry, reverse index and sitemaps off the request loop, then publishes them in one synchronous step under a lock. A failed reload returns 503 and keeps the previous data. Sitemap regeneration takes the same path rather than clearing the live registry up front. Typesense search returned at most one page of hits and used an empty list for both "no matches" and "search is down", so a genuine empty result silently fell back to substring matching. It now pages through every candidate and returns None only on failure; the fallback matches literally, since a query containing regex metacharacters used to throw. Empty datasets answer 503 rather than 200-with-nothing or a misleading 404, so callers can tell an outage from an absent school. Adds /api/release, which reports the build identity baked into the image. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
38bc17cab3
commit
9b75f54206
4 files changed
+247
-41
No files matched your search
+77
-32
@@ -26,7 +26,8 @@ from starlette.middleware.base import BaseHTTPMiddleware
|
||||
import asyncio
|
||||
from .config import settings
|
||||
from .data_loader import (
|
||||
clear_cache,
|
||||
build_latest_school_data,
|
||||
load_school_data_as_dataframe,
|
||||
compute_benchmarks,
|
||||
load_school_data,
|
||||
load_latest_school_data,
|
||||
@@ -272,7 +273,7 @@ def _places_payload(urn: int) -> list[dict]:
|
||||
return payload
|
||||
|
||||
|
||||
def _place_sitemap_rows(kinds: tuple[str, ...]) -> list[str]:
|
||||
def _place_sitemap_rows(kinds: tuple[str, ...], registry=None) -> list[str]:
|
||||
"""A <url> per place, plus a phase variant wherever that phase clears the
|
||||
threshold on its own.
|
||||
|
||||
@@ -282,7 +283,9 @@ def _place_sitemap_rows(kinds: tuple[str, ...]) -> list[str]:
|
||||
linked from the place page either.
|
||||
"""
|
||||
rows: list[str] = []
|
||||
for p in sorted(get_place_registry().values(), key=lambda p: (p.kind, p.slug)):
|
||||
if registry is None:
|
||||
registry = get_place_registry()
|
||||
for p in sorted(registry.values(), key=lambda p: (p.kind, p.slug)):
|
||||
if p.kind not in kinds:
|
||||
continue
|
||||
rows.append(_url_element(BASE_URL + _place_url(p)))
|
||||
@@ -296,8 +299,9 @@ def _place_sitemap_rows(kinds: tuple[str, ...]) -> list[str]:
|
||||
return rows
|
||||
|
||||
|
||||
def build_sitemaps() -> dict[str, str]:
|
||||
def build_sitemaps(df=None, registry=None) -> dict[str, str]:
|
||||
"""Build the sitemap index and every child, keyed by name."""
|
||||
if df is None:
|
||||
df = load_school_data()
|
||||
|
||||
children: dict[str, str] = {
|
||||
@@ -318,7 +322,7 @@ def build_sitemaps() -> dict[str, str]:
|
||||
# measured apart from the school pages'.
|
||||
for label, kinds in (("places", ("town", "locality", "authority")),
|
||||
("outcodes", ("outcode",))):
|
||||
rows = _place_sitemap_rows(kinds)
|
||||
rows = _place_sitemap_rows(kinds, registry)
|
||||
chunks = [rows[i:i + SITEMAP_CHUNK_SIZE]
|
||||
for i in range(0, len(rows), SITEMAP_CHUNK_SIZE)] or [[]]
|
||||
for n, chunk in enumerate(chunks, start=1):
|
||||
@@ -700,6 +704,15 @@ async def get_config():
|
||||
}
|
||||
|
||||
|
||||
@app.get("/api/release")
|
||||
async def release_identity():
|
||||
import json
|
||||
from pathlib import Path
|
||||
path = Path(__file__).with_name("build-info.json")
|
||||
identity = json.loads(path.read_text()) if path.exists() else {"sha": "development", "build_id": "development"}
|
||||
return JSONResponse(identity, headers={"Cache-Control": "no-store"})
|
||||
|
||||
|
||||
@app.get("/api/schools")
|
||||
@limiter.limit(f"{settings.rate_limit_per_minute}/minute")
|
||||
async def get_schools(
|
||||
@@ -736,7 +749,7 @@ async def get_schools(
|
||||
df_latest = load_latest_school_data()
|
||||
|
||||
if df_latest.empty:
|
||||
return {"schools": [], "total": 0, "page": page, "page_size": 0}
|
||||
raise HTTPException(status_code=503, detail="School data temporarily unavailable")
|
||||
|
||||
# Use configured default if not specified
|
||||
if page_size is None:
|
||||
@@ -835,8 +848,8 @@ async def get_schools(
|
||||
|
||||
# Apply filters
|
||||
if search:
|
||||
ts_urns = search_schools_typesense(search)
|
||||
if ts_urns:
|
||||
ts_urns = await asyncio.to_thread(search_schools_typesense, search)
|
||||
if ts_urns is not None:
|
||||
urn_order = {urn: i for i, urn in enumerate(ts_urns)}
|
||||
schools_df = schools_df[schools_df["urn"].isin(set(ts_urns))].copy()
|
||||
schools_df["_ts_rank"] = schools_df["urn"].map(urn_order)
|
||||
@@ -844,9 +857,9 @@ async def get_schools(
|
||||
else:
|
||||
# Fallback: Typesense unavailable, use substring match
|
||||
search_lower = search.lower()
|
||||
mask = schools_df["school_name"].str.lower().str.contains(search_lower, na=False)
|
||||
mask = schools_df["school_name"].str.lower().str.contains(search_lower, na=False, regex=False)
|
||||
if "address" in schools_df.columns:
|
||||
mask = mask | schools_df["address"].str.lower().str.contains(search_lower, na=False)
|
||||
mask = mask | schools_df["address"].str.lower().str.contains(search_lower, na=False, regex=False)
|
||||
schools_df = schools_df[mask]
|
||||
|
||||
if local_authority:
|
||||
@@ -905,7 +918,7 @@ async def get_school_details(request: Request, urn: int):
|
||||
df = load_school_data()
|
||||
|
||||
if df.empty:
|
||||
raise HTTPException(status_code=404, detail="No data available")
|
||||
raise HTTPException(status_code=503, detail="School data temporarily unavailable")
|
||||
|
||||
school_data = df[df["urn"] == urn]
|
||||
|
||||
@@ -1542,20 +1555,51 @@ async def get_data_info(request: Request):
|
||||
}
|
||||
|
||||
|
||||
_publication_lock = asyncio.Lock()
|
||||
|
||||
|
||||
def _prepare_publication(df):
|
||||
if df.empty:
|
||||
raise ValueError("Refusing to publish an empty school dataset")
|
||||
if not {"urn", "year", "school_name"}.issubset(df.columns):
|
||||
raise ValueError("School dataset is missing required columns")
|
||||
if df["urn"].isna().any() or df.duplicated(["urn", "year"]).any():
|
||||
raise ValueError("School dataset has missing URNs or duplicate school years")
|
||||
latest = build_latest_school_data(df)
|
||||
registry = build_place_registry(df)
|
||||
index = build_place_index(registry)
|
||||
sitemaps = build_sitemaps(df, registry)
|
||||
return df, latest, registry, index, sitemaps
|
||||
|
||||
|
||||
def _publish(prepared):
|
||||
# Called on the event loop with no await: routes cannot observe half a swap.
|
||||
# The application currently runs one worker; replicas require coordination.
|
||||
from . import data_loader
|
||||
global _place_registry, _place_index, _place_index_source, _sitemaps
|
||||
df, latest, registry, index, sitemaps = prepared
|
||||
data_loader._df_cache = df
|
||||
data_loader._df_latest_cache = latest
|
||||
_place_registry = registry
|
||||
_place_index = index
|
||||
_place_index_source = registry
|
||||
_sitemaps = sitemaps
|
||||
|
||||
|
||||
@app.post("/api/admin/reload")
|
||||
@limiter.limit("5/minute")
|
||||
async def reload_data(
|
||||
request: Request,
|
||||
_: bool = Depends(verify_admin_api_key)
|
||||
):
|
||||
"""
|
||||
Admin endpoint to force data reload (useful after data updates).
|
||||
Requires X-API-Key header with valid admin API key.
|
||||
"""
|
||||
clear_cache()
|
||||
await asyncio.to_thread(load_school_data)
|
||||
await asyncio.to_thread(load_latest_school_data)
|
||||
return {"status": "reloaded"}
|
||||
async def reload_data(request: Request, _: bool = Depends(verify_admin_api_key)):
|
||||
"""Validate a complete replacement before publishing it; retain data on failure."""
|
||||
async with _publication_lock:
|
||||
try:
|
||||
df = await asyncio.to_thread(load_school_data_as_dataframe)
|
||||
prepared = await asyncio.to_thread(_prepare_publication, df)
|
||||
except Exception as exc:
|
||||
import logging
|
||||
logging.getLogger(__name__).exception("Dataset reload failed")
|
||||
raise HTTPException(status_code=503, detail="Dataset reload failed; previous data retained") from exc
|
||||
_publish(prepared)
|
||||
return {"status": "reloaded", "schools": len(prepared[1])}
|
||||
|
||||
|
||||
|
||||
@@ -1607,15 +1651,16 @@ async def regenerate_sitemap(
|
||||
request: Request,
|
||||
_: bool = Depends(verify_admin_api_key),
|
||||
):
|
||||
"""Rebuild and cache the sitemap from current school data. Called by Airflow after data updates."""
|
||||
global _sitemaps, _place_registry
|
||||
# Places and sitemap are rebuilt together — they read the same marts, and
|
||||
# letting them drift apart would submit URLs for places that no longer
|
||||
# exist.
|
||||
_place_registry = None
|
||||
_sitemaps = build_sitemaps()
|
||||
n = sum(x.count("<url>") for x in _sitemaps.values())
|
||||
return {"status": "ok", "urls": n, "sitemaps": len(_sitemaps)}
|
||||
"""Rebuild derived publication data without clearing the live registry."""
|
||||
async with _publication_lock:
|
||||
try:
|
||||
prepared = await asyncio.to_thread(_prepare_publication, load_school_data())
|
||||
except Exception as exc:
|
||||
raise HTTPException(status_code=503, detail="Sitemap rebuild failed; previous data retained") from exc
|
||||
_publish(prepared)
|
||||
n = sum(x.count("<url>") for x in prepared[4].values())
|
||||
return {"status": "ok", "urls": n, "sitemaps": len(prepared[4])}
|
||||
|
||||
|
||||
|
||||
# Mount static files directly (must be after all routes to avoid catching API calls)
|
||||
|
||||
+29
-9
@@ -84,21 +84,37 @@ def _get_typesense_client():
|
||||
return None
|
||||
|
||||
|
||||
def search_schools_typesense(query: str, limit: int = 250) -> List[int]:
|
||||
"""Search Typesense. Returns URNs in relevance order, or [] if unavailable."""
|
||||
def search_schools_typesense(query: str) -> Optional[List[int]]:
|
||||
"""Return all matching URNs in relevance order; None means unavailable.
|
||||
|
||||
Filtering and user pagination happen in the API after this search. Returning
|
||||
only the first search page would silently discard valid local matches.
|
||||
Never return a partial candidate set if a later page fails.
|
||||
"""
|
||||
client = _get_typesense_client()
|
||||
if client is None:
|
||||
return []
|
||||
return None
|
||||
urns = []
|
||||
try:
|
||||
page = 1
|
||||
while True:
|
||||
result = client.collections["schools"].documents.search({
|
||||
"q": query,
|
||||
"query_by": "school_name,local_authority,postcode",
|
||||
"per_page": min(limit, 250),
|
||||
"per_page": 250,
|
||||
"page": page,
|
||||
"typo_tokens_threshold": 1,
|
||||
})
|
||||
return [int(h["document"]["urn"]) for h in result.get("hits", [])]
|
||||
hits = result.get("hits", [])
|
||||
urns.extend(int(h["document"]["urn"]) for h in hits)
|
||||
if len(urns) >= result.get("found", len(urns)):
|
||||
return list(dict.fromkeys(urns))
|
||||
if not hits:
|
||||
raise ValueError("Search pagination ended before all matches arrived")
|
||||
page += 1
|
||||
except Exception:
|
||||
return []
|
||||
logging.getLogger(__name__).exception("School search unavailable")
|
||||
return None
|
||||
|
||||
|
||||
# The most a public endpoint will return in one response.
|
||||
@@ -502,7 +518,12 @@ def load_latest_school_data() -> pd.DataFrame:
|
||||
if _df_latest_cache is not None:
|
||||
return _df_latest_cache
|
||||
|
||||
df = load_school_data()
|
||||
_df_latest_cache = build_latest_school_data(load_school_data())
|
||||
return _df_latest_cache
|
||||
|
||||
|
||||
def build_latest_school_data(df: pd.DataFrame) -> pd.DataFrame:
|
||||
"""Build a replacement snapshot without mutating the published caches."""
|
||||
if df.empty:
|
||||
return df
|
||||
|
||||
@@ -535,8 +556,7 @@ def load_latest_school_data() -> pd.DataFrame:
|
||||
df_latest = pd.concat([df_latest, df_no_perf], ignore_index=True)
|
||||
|
||||
print(f"Latest-snapshot cache built: {len(df_latest)} schools")
|
||||
_df_latest_cache = df_latest
|
||||
return _df_latest_cache
|
||||
return df_latest
|
||||
|
||||
|
||||
def clear_cache():
|
||||
|
||||
@@ -0,0 +1,68 @@
|
||||
"""Publication must preserve the current dataset until every replacement is ready."""
|
||||
import asyncio
|
||||
import pandas as pd
|
||||
import pytest
|
||||
from fastapi.testclient import TestClient
|
||||
from backend import app as api, data_loader
|
||||
from backend.tests.test_sixth_form_flag import _schools_df
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def client(monkeypatch):
|
||||
old = _schools_df()
|
||||
monkeypatch.setattr(data_loader, '_df_cache', old)
|
||||
monkeypatch.setattr(data_loader, '_df_latest_cache', old)
|
||||
monkeypatch.setattr(api, '_place_registry', {'old': 'registry'})
|
||||
monkeypatch.setattr(api, '_place_index', {'old': 'index'})
|
||||
monkeypatch.setattr(api, '_place_index_source', api._place_registry)
|
||||
monkeypatch.setattr(api, '_sitemaps', {'old.xml': 'old sitemap'})
|
||||
monkeypatch.setattr(api, '_publication_lock', asyncio.Lock())
|
||||
monkeypatch.setattr(api.limiter, 'enabled', False)
|
||||
api.app.dependency_overrides[api.verify_admin_api_key] = lambda: True
|
||||
yield TestClient(api.app, raise_server_exceptions=False)
|
||||
api.app.dependency_overrides.clear()
|
||||
|
||||
|
||||
def state():
|
||||
return (data_loader._df_cache, data_loader._df_latest_cache, api._place_registry,
|
||||
api._place_index, api._place_index_source, api._sitemaps)
|
||||
|
||||
|
||||
@pytest.mark.parametrize('failure', ['empty', 'database', 'sitemap', 'duplicate'])
|
||||
def test_failed_reload_preserves_every_published_object(client, monkeypatch, failure):
|
||||
before = state()
|
||||
df = _schools_df()
|
||||
if failure == 'empty':
|
||||
df = pd.DataFrame()
|
||||
if failure == 'duplicate':
|
||||
df = pd.concat([df, df.iloc[:1]], ignore_index=True)
|
||||
def load():
|
||||
if failure == 'database':
|
||||
raise RuntimeError('database unavailable')
|
||||
return df
|
||||
monkeypatch.setattr(api, 'load_school_data_as_dataframe', load)
|
||||
if failure == 'sitemap':
|
||||
monkeypatch.setattr(api, 'build_sitemaps', lambda *args: (_ for _ in ()).throw(RuntimeError('bad XML')))
|
||||
response = client.post('/api/admin/reload')
|
||||
assert response.status_code == 503
|
||||
assert all(a is b for a, b in zip(before, state()))
|
||||
|
||||
|
||||
def test_success_publishes_school_data_places_and_sitemaps(client, monkeypatch):
|
||||
df = _schools_df()
|
||||
df.loc[0, 'school_name'] = 'Replacement School'
|
||||
monkeypatch.setattr(api, 'load_school_data_as_dataframe', lambda: df)
|
||||
response = client.post('/api/admin/reload')
|
||||
assert response.status_code == 200
|
||||
assert data_loader.load_school_data() is df
|
||||
assert data_loader.load_latest_school_data().iloc[0].school_name == 'Replacement School'
|
||||
assert api._place_index_source is api._place_registry
|
||||
assert 'old.xml' not in api._sitemaps
|
||||
assert 'replacement-school' in api._sitemaps['schools-1.xml']
|
||||
|
||||
|
||||
def test_failed_sitemap_regeneration_keeps_existing_publication(client, monkeypatch):
|
||||
before = state()
|
||||
monkeypatch.setattr(api, 'build_sitemaps', lambda *args: (_ for _ in ()).throw(RuntimeError('bad XML')))
|
||||
assert client.post('/api/admin/regenerate-sitemap').status_code == 503
|
||||
assert all(a is b for a, b in zip(before, state()))
|
||||
@@ -0,0 +1,73 @@
|
||||
from types import SimpleNamespace
|
||||
import pytest
|
||||
from fastapi.testclient import TestClient
|
||||
from backend import app as api, data_loader
|
||||
from backend.tests.test_sixth_form_flag import _schools_df
|
||||
|
||||
|
||||
def client_for(monkeypatch, search):
|
||||
client = SimpleNamespace(collections={'schools': SimpleNamespace(documents=SimpleNamespace(search=search))})
|
||||
monkeypatch.setattr(data_loader, '_get_typesense_client', lambda: client)
|
||||
|
||||
|
||||
def test_search_returns_matches_beyond_first_page(monkeypatch):
|
||||
pages = []
|
||||
def search(params):
|
||||
pages.append(params['page'])
|
||||
urns = range(100000, 100250) if params['page'] == 1 else [100999]
|
||||
return {'found': 251, 'hits': [{'document': {'urn': u}} for u in urns]}
|
||||
client_for(monkeypatch, search)
|
||||
result = data_loader.search_schools_typesense('academy')
|
||||
assert len(result) == 251
|
||||
assert result[-1] == 100999
|
||||
assert pages == [1, 2]
|
||||
|
||||
|
||||
def test_later_page_failure_does_not_return_partial_results(monkeypatch):
|
||||
def search(params):
|
||||
if params['page'] == 2:
|
||||
raise RuntimeError('timeout')
|
||||
return {'found': 251, 'hits': [{'document': {'urn': u}} for u in range(100000, 100250)]}
|
||||
client_for(monkeypatch, search)
|
||||
assert data_loader.search_schools_typesense('academy') is None
|
||||
|
||||
|
||||
def test_zero_matches_are_distinct_from_unavailable(monkeypatch):
|
||||
client_for(monkeypatch, lambda _: {'found': 0, 'hits': []})
|
||||
assert data_loader.search_schools_typesense('academy') == []
|
||||
monkeypatch.setattr(data_loader, '_get_typesense_client', lambda: None)
|
||||
assert data_loader.search_schools_typesense('academy') is None
|
||||
|
||||
|
||||
@pytest.mark.parametrize('matches, expected', [([], []), (None, [100001])])
|
||||
def test_fallback_only_on_dependency_failure(monkeypatch, matches, expected):
|
||||
monkeypatch.setattr(api.limiter, 'enabled', False)
|
||||
monkeypatch.setattr(api, 'load_latest_school_data', _schools_df)
|
||||
monkeypatch.setattr(api, 'search_schools_typesense', lambda _: matches)
|
||||
response = TestClient(api.app).get('/api/schools?search=Alpha')
|
||||
assert response.status_code == 200
|
||||
assert [s['urn'] for s in response.json()['schools']] == expected
|
||||
|
||||
|
||||
def test_filtered_api_keeps_match_from_second_search_page(monkeypatch):
|
||||
monkeypatch.setattr(api.limiter, 'enabled', False)
|
||||
df = _schools_df()
|
||||
monkeypatch.setattr(api, 'load_latest_school_data', lambda: df)
|
||||
def search(params):
|
||||
urns = range(200000, 200250) if params['page'] == 1 else [100001]
|
||||
return {'found': 251, 'hits': [{'document': {'urn': u}} for u in urns]}
|
||||
client_for(monkeypatch, search)
|
||||
response = TestClient(api.app).get('/api/schools?search=Alpha&local_authority=Testshire')
|
||||
assert response.status_code == 200
|
||||
assert response.json()['total'] == 1
|
||||
assert response.json()['schools'][0]['urn'] == 100001
|
||||
|
||||
|
||||
def test_unavailable_dataset_is_not_a_missing_school_or_empty_search(monkeypatch):
|
||||
import pandas as pd
|
||||
monkeypatch.setattr(api.limiter, 'enabled', False)
|
||||
monkeypatch.setattr(api, 'load_school_data', lambda: pd.DataFrame())
|
||||
monkeypatch.setattr(api, 'load_latest_school_data', lambda: pd.DataFrame())
|
||||
client = TestClient(api.app)
|
||||
assert client.get('/api/schools/100001').status_code == 503
|
||||
assert client.get('/api/schools?search=school').status_code == 503
|
||||
Reference in new issue
Block a user