PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m3s
PR Checks / Backend Smoke (pull_request) Successful in 8s
PR Checks / Build Backend (no push) (pull_request) Successful in 18s
PR Checks / Build Frontend (no push) (pull_request) Successful in 45s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 10s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 10s
Code review, both findings valid. The design doc claimed Cloudflare "replaces the header, so a browser cannot forge it", and that only the X-Forwarded-For fallback was forgeable. That is true only for traffic that actually passed through Cloudflare, and nothing in this process can verify that it did. Reaching the origin directly, both headers are equally attacker-controlled — and rotating CF-Connecting-IP mints a fresh rate-limit bucket per request, defeating per-client limits on every endpoint including the DataFrame-heavy /api/schools. Against abuse that is worse than the shared bucket it replaced, which at least capped everyone together. So the ceiling comes back. I dropped it earlier arguing it belonged at Cloudflare; that argument assumed the keying was sound, and it is not. GlobalRateLimitMiddleware counts all /api/ traffic in a fixed window against a total, independent of client identity, outermost so it refuses before any work happens. Written by hand because slowapi cannot express a global cap: default_limits and application_limits are both keyed by key_func, and the latter needs middleware this app does not install. It does not make the header trustworthy — it makes trusting it survivable. The real fix is Authenticated Origin Pulls or an origin firewall, now documented in DEPLOY.md as the open gap it is. 127.0.0.1 is exempt: the healthcheck curls localhost from inside the container, and starving it would restart the container and turn a load spike into an outage loop. Keyed on the peer address, never the Host header, which the caller sets. Second finding: suggest_schools_typesense promised "never raises" while the parsing loop sat outside the try, so int(None) on a malformed document would have made a keystroke a 500. The loop now skips bad rows rather than dropping the whole list — and a hit with no document no longer becomes a suggestion pointing at /school/0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
158 lines
6.0 KiB
Python
158 lines
6.0 KiB
Python
"""Tests for school autosuggest (spec 2026-08-26)."""
|
|
|
|
from backend import data_loader
|
|
|
|
|
|
class _FakeDocs:
|
|
def __init__(self, hits, explode=False):
|
|
self._hits = hits
|
|
self._explode = explode
|
|
self.last_params = None
|
|
|
|
def search(self, params):
|
|
self.last_params = params
|
|
if self._explode:
|
|
raise RuntimeError("typesense is down")
|
|
return {"hits": [{"document": d} for d in self._hits]}
|
|
|
|
|
|
class _FakeClient:
|
|
def __init__(self, hits, explode=False):
|
|
self.docs = _FakeDocs(hits, explode)
|
|
self.collections = {"schools": type("C", (), {"documents": self.docs})()}
|
|
|
|
|
|
_HIT = {
|
|
"urn": 100010, "school_name": "Brecknock Primary School",
|
|
"local_authority": "Camden", "postcode": "NW1 1AA",
|
|
"phase": "Primary", "school_type": "Community school",
|
|
}
|
|
|
|
|
|
def _use(monkeypatch, client):
|
|
monkeypatch.setattr(data_loader, "_get_typesense_client", lambda: client)
|
|
|
|
|
|
def test_returns_the_fields_a_suggestion_needs(monkeypatch):
|
|
# Local authority is not decoration: there are many schools called
|
|
# "St Mary's", and a list without it cannot be chosen between.
|
|
_use(monkeypatch, _FakeClient([_HIT]))
|
|
out = data_loader.suggest_schools_typesense("breck")
|
|
assert out == [{
|
|
"urn": 100010, "school_name": "Brecknock Primary School",
|
|
"local_authority": "Camden", "postcode": "NW1 1AA",
|
|
"phase": "Primary", "school_type": "Community school",
|
|
}]
|
|
|
|
|
|
def test_a_missing_optional_field_becomes_an_empty_string(monkeypatch):
|
|
# phase and school_type are optional in the Typesense schema. A missing
|
|
# key must not KeyError in the keystroke path.
|
|
_use(monkeypatch, _FakeClient([{"urn": 1, "school_name": "X",
|
|
"local_authority": "Y", "postcode": "Z"}]))
|
|
out = data_loader.suggest_schools_typesense("x")
|
|
assert out[0]["phase"] == "" and out[0]["school_type"] == ""
|
|
|
|
|
|
def test_typesense_unavailable_gives_no_suggestions_rather_than_raising(monkeypatch):
|
|
_use(monkeypatch, None)
|
|
assert data_loader.suggest_schools_typesense("anything") == []
|
|
|
|
|
|
def test_a_typesense_error_gives_no_suggestions_rather_than_raising(monkeypatch):
|
|
_use(monkeypatch, _FakeClient([], explode=True))
|
|
assert data_loader.suggest_schools_typesense("anything") == []
|
|
|
|
|
|
def test_the_limit_is_passed_through_and_clamped(monkeypatch):
|
|
client = _FakeClient([])
|
|
_use(monkeypatch, client)
|
|
data_loader.suggest_schools_typesense("x", limit=500)
|
|
assert client.docs.last_params["per_page"] == 20
|
|
|
|
|
|
def _client(monkeypatch, rows, *, blow_up_dataframe=False):
|
|
from fastapi.testclient import TestClient
|
|
from backend import app as app_module
|
|
|
|
monkeypatch.setattr(app_module, "suggest_schools_typesense",
|
|
lambda q, limit=8: rows)
|
|
if blow_up_dataframe:
|
|
def _boom():
|
|
raise AssertionError("the suggest path must not load the DataFrame")
|
|
monkeypatch.setattr(app_module, "load_school_data", _boom)
|
|
monkeypatch.setattr(app_module, "load_latest_school_data", _boom)
|
|
return TestClient(app_module.app, raise_server_exceptions=False)
|
|
|
|
|
|
def test_the_endpoint_returns_suggestions(monkeypatch):
|
|
body = _client(monkeypatch, [_HIT]).get("/api/suggest?q=breck").json()
|
|
assert body["suggestions"][0]["school_name"] == "Brecknock Primary School"
|
|
|
|
|
|
def test_the_endpoint_never_touches_the_dataframe(monkeypatch):
|
|
"""The whole reason this is not a mode of /api/schools.
|
|
|
|
That endpoint filters and sorts 25,000 rows of pandas per query, holding
|
|
the GIL. Per keystroke, that is the cost this endpoint exists to avoid.
|
|
"""
|
|
res = _client(monkeypatch, [_HIT], blow_up_dataframe=True).get("/api/suggest?q=breck")
|
|
assert res.status_code == 200
|
|
assert res.json()["suggestions"]
|
|
|
|
|
|
def test_a_one_character_query_returns_nothing_and_does_not_error(monkeypatch):
|
|
# The keystroke path never errors on ordinary input.
|
|
res = _client(monkeypatch, [_HIT]).get("/api/suggest?q=b")
|
|
assert res.status_code == 200
|
|
assert res.json() == {"suggestions": []}
|
|
|
|
|
|
def test_a_blank_query_returns_nothing_and_does_not_error(monkeypatch):
|
|
res = _client(monkeypatch, [_HIT]).get("/api/suggest?q=")
|
|
assert res.status_code == 200
|
|
assert res.json() == {"suggestions": []}
|
|
|
|
|
|
def test_typesense_down_is_an_empty_list_not_a_500(monkeypatch):
|
|
res = _client(monkeypatch, []).get("/api/suggest?q=breck")
|
|
assert res.status_code == 200
|
|
assert res.json() == {"suggestions": []}
|
|
|
|
|
|
def test_the_response_is_cacheable(monkeypatch):
|
|
# Prefix queries repeat enormously across users, and school names change
|
|
# once a year. Without this the endpoint pays full price every keystroke.
|
|
res = _client(monkeypatch, [_HIT]).get("/api/suggest?q=breck")
|
|
assert "s-maxage" in res.headers.get("cache-control", "")
|
|
assert res.headers.get("etag")
|
|
|
|
|
|
def test_a_malformed_urn_does_not_raise(monkeypatch):
|
|
"""The docstring promises "never raises"; the parsing loop sat outside the
|
|
try, so int(None) or int("abc") would have turned a keystroke into a 500.
|
|
|
|
Typesense declares urn as int32, so this should be unreachable — but the
|
|
contract is what the caller relies on, and a search index is a separate
|
|
system that can be reindexed by something other than this code.
|
|
"""
|
|
_use(monkeypatch, _FakeClient([{"urn": None, "school_name": "X",
|
|
"local_authority": "Y", "postcode": "Z"}]))
|
|
assert data_loader.suggest_schools_typesense("x") == []
|
|
|
|
|
|
def test_a_malformed_row_does_not_discard_the_good_ones(monkeypatch):
|
|
# One bad document must not blank the whole dropdown.
|
|
_use(monkeypatch, _FakeClient([
|
|
{"urn": "not-a-number", "school_name": "Bad", "local_authority": "Y",
|
|
"postcode": "Z"},
|
|
_HIT,
|
|
]))
|
|
out = data_loader.suggest_schools_typesense("x")
|
|
assert [r["urn"] for r in out] == [100010]
|
|
|
|
|
|
def test_a_hit_with_no_document_does_not_raise(monkeypatch):
|
|
_use(monkeypatch, _FakeClient([{}]))
|
|
assert data_loader.suggest_schools_typesense("x") == []
|