diff --git a/backend/app.py b/backend/app.py index b935af8..eccd542 100644 --- a/backend/app.py +++ b/backend/app.py @@ -30,6 +30,7 @@ from .data_loader import ( load_latest_school_data, geocode_single_postcode, get_supplementary_data, + get_supplementary_data_batch, search_schools_typesense, ) from .data_loader import get_data_info as get_db_info @@ -679,8 +680,10 @@ async def compare_schools( db = None try: db = database.SessionLocal() + # One query per table for all schools, not ~5 queries per school. + batch = get_supplementary_data_batch(db, urn_list) for urn in urn_list: - supp = get_supplementary_data(db, urn) + supp = batch.get(urn, {}) supplementary_by_urn[urn] = { key: supp.get(key, default) for key, default in _EMPTY_SUPPLEMENTARY.items() diff --git a/backend/data_loader.py b/backend/data_loader.py index 7d73205..e7a8cd1 100644 --- a/backend/data_loader.py +++ b/backend/data_loader.py @@ -662,92 +662,150 @@ def _admissions_row_dict(a) -> dict: } -def get_supplementary_data(db: Session, urn: int) -> dict: - """Fetch all supplementary data for a single school URN.""" - result = {} +def _census_dict(pc) -> dict: + return { + "year": pc.year, + "total_pupils": pc.total_pupils, + "female_pupils": pc.female_pupils, + "male_pupils": pc.male_pupils, + "fsm_pct": pc.fsm_pct, + "eal_pct": pc.eal_pct, + } - def safe_query(model, pk_field, latest_field=None): + +def _deprivation_dict(d) -> dict: + return { + "lsoa_code": d.lsoa_code, + "idaci_score": d.idaci_score, + "idaci_decile": d.idaci_decile, + } + + +def _finance_dict(f) -> dict: + return { + "year": f.year, + "per_pupil_spend": f.per_pupil_spend, + "staff_cost_pct": f.staff_cost_pct, + "teacher_cost_pct": f.teacher_cost_pct, + "support_staff_cost_pct": f.support_staff_cost_pct, + "premises_cost_pct": f.premises_cost_pct, + } + + +def _empty_supplementary() -> dict: + return { + "ofsted": None, + "census": None, + "admissions": None, + "admissions_history": [], + "sen_detail": None, + "phonics": None, + "deprivation": None, + "finance": None, + } + + +def get_supplementary_data_batch(db: Session, urns: list[int]) -> dict: + """Fetch supplementary data for many URNs with one query per table + (WHERE urn IN (...)) instead of ~5 queries per school, collapsing the + per-request round-trips from 5*N to a constant 5. Returns {urn: block} + with the same shape get_supplementary_data produces per URN. + + Each table is queried independently and failures degrade that table to + empty for every URN — a missing mart never blanks the others. + """ + urns = [int(u) for u in urns] + result = {urn: _empty_supplementary() for urn in urns} + if not urns: + return result + + def _safe(fn): try: - q = db.query(model).filter(getattr(model, pk_field) == urn) - if latest_field: - q = q.order_by(getattr(model, latest_field).desc()) - return q.first() + fn() except Exception as e: import logging - logging.getLogger(__name__).error("safe_query failed for %s: %s", model.__name__, e) + logging.getLogger(__name__).error("batch supplementary query failed: %s", e) db.rollback() - return None - # Latest Ofsted inspection - o = safe_query(FactOfstedInspection, "urn", "inspection_date") - result["ofsted"] = _ofsted_block(o, urn) if o else None - - # Census (latest year of fact_pupil_characteristics) - pc = safe_query(FactPupilCharacteristics, "urn", "year") - result["census"] = ( - { - "year": pc.year, - "total_pupils": pc.total_pupils, - "female_pupils": pc.female_pupils, - "male_pupils": pc.male_pupils, - "fsm_pct": pc.fsm_pct, - "eal_pct": pc.eal_pct, - } - if pc - else None - ) - - # Admissions — all years, oldest first (for the multi-year trend view). - try: - admissions_rows = ( - db.query(FactAdmissions) - .filter(FactAdmissions.urn == urn) - .order_by(FactAdmissions.year.asc()) + # Ofsted — latest inspection per URN. Ordered so the first row seen per + # URN is the most recent. + def _ofsted(): + rows = ( + db.query(FactOfstedInspection) + .filter(FactOfstedInspection.urn.in_(urns)) + .order_by(FactOfstedInspection.urn, FactOfstedInspection.inspection_date.desc()) .all() ) - except Exception as e: - import logging - logging.getLogger(__name__).error("admissions history query failed: %s", e) - db.rollback() - admissions_rows = [] + seen = set() + for o in rows: + if o.urn in seen: + continue + seen.add(o.urn) + result[o.urn]["ofsted"] = _ofsted_block(o, o.urn) + _safe(_ofsted) - history = [_admissions_row_dict(a) for a in admissions_rows] - result["admissions_history"] = history - # Keep the single latest-year object for backwards-compatible consumers - # (hero chips, etc.). - result["admissions"] = history[-1] if history else None + # Census — latest year per URN. + def _census(): + rows = ( + db.query(FactPupilCharacteristics) + .filter(FactPupilCharacteristics.urn.in_(urns)) + .order_by(FactPupilCharacteristics.urn, FactPupilCharacteristics.year.desc()) + .all() + ) + seen = set() + for pc in rows: + if pc.urn in seen: + continue + seen.add(pc.urn) + result[pc.urn]["census"] = _census_dict(pc) + _safe(_census) - # SEN detail — not available in current marts - result["sen_detail"] = None + # Admissions — all years per URN, oldest first (multi-year trend view). + def _admissions(): + rows = ( + db.query(FactAdmissions) + .filter(FactAdmissions.urn.in_(urns)) + .order_by(FactAdmissions.urn, FactAdmissions.year.asc()) + .all() + ) + history: dict = {urn: [] for urn in urns} + for a in rows: + history[a.urn].append(_admissions_row_dict(a)) + for urn, rows_for_urn in history.items(): + result[urn]["admissions_history"] = rows_for_urn + result[urn]["admissions"] = rows_for_urn[-1] if rows_for_urn else None + _safe(_admissions) - # Phonics — no school-level data on EES - result["phonics"] = None + # Deprivation — one row per URN. + def _deprivation(): + rows = ( + db.query(FactDeprivation) + .filter(FactDeprivation.urn.in_(urns)) + .all() + ) + for d in rows: + result[d.urn]["deprivation"] = _deprivation_dict(d) + _safe(_deprivation) - # Deprivation - d = safe_query(FactDeprivation, "urn") - result["deprivation"] = ( - { - "lsoa_code": d.lsoa_code, - "idaci_score": d.idaci_score, - "idaci_decile": d.idaci_decile, - } - if d - else None - ) - - # Finance (latest year) - f = safe_query(FactFinance, "urn", "year") - result["finance"] = ( - { - "year": f.year, - "per_pupil_spend": f.per_pupil_spend, - "staff_cost_pct": f.staff_cost_pct, - "teacher_cost_pct": f.teacher_cost_pct, - "support_staff_cost_pct": f.support_staff_cost_pct, - "premises_cost_pct": f.premises_cost_pct, - } - if f - else None - ) + # Finance — latest year per URN. + def _finance(): + rows = ( + db.query(FactFinance) + .filter(FactFinance.urn.in_(urns)) + .order_by(FactFinance.urn, FactFinance.year.desc()) + .all() + ) + seen = set() + for f in rows: + if f.urn in seen: + continue + seen.add(f.urn) + result[f.urn]["finance"] = _finance_dict(f) + _safe(_finance) return result + + +def get_supplementary_data(db: Session, urn: int) -> dict: + """Supplementary data for a single URN (thin wrapper over the batch).""" + return get_supplementary_data_batch(db, [urn])[int(urn)] diff --git a/backend/tests/test_compare_enrichment.py b/backend/tests/test_compare_enrichment.py index 072ddca..0ff671f 100644 --- a/backend/tests/test_compare_enrichment.py +++ b/backend/tests/test_compare_enrichment.py @@ -67,7 +67,9 @@ def client(monkeypatch): monkeypatch.setattr(app_module, "load_school_data", _two_primary_schools_df) monkeypatch.setattr( - app_module, "get_supplementary_data", lambda db, urn: dict(CANNED_SUPPLEMENTARY) + app_module, + "get_supplementary_data_batch", + lambda db, urns: {int(u): dict(CANNED_SUPPLEMENTARY) for u in urns}, ) monkeypatch.setattr(database_module, "SessionLocal", _StubSession) return TestClient(app_module.app, raise_server_exceptions=False) @@ -102,10 +104,10 @@ def test_top_level_national_averages_and_benchmarks(client): def test_supplementary_failure_degrades_not_500(client, monkeypatch): from backend import app as app_module - def _boom(db, urn): + def _boom(db, urns): raise RuntimeError("marts unavailable") - monkeypatch.setattr(app_module, "get_supplementary_data", _boom) + monkeypatch.setattr(app_module, "get_supplementary_data_batch", _boom) resp = client.get("/api/compare?urns=100140") assert resp.status_code == 200 school = resp.json()["comparison"]["100140"] diff --git a/backend/tests/test_supplementary_batch.py b/backend/tests/test_supplementary_batch.py new file mode 100644 index 0000000..0593034 --- /dev/null +++ b/backend/tests/test_supplementary_batch.py @@ -0,0 +1,111 @@ +"""get_supplementary_data_batch fetches one query per table for all URNs +(not ~5 per school) and returns the same per-URN block shape as the +single-URN function, picking the latest row per URN where relevant.""" + +import types + +from backend import data_loader +from backend.data_loader import get_supplementary_data_batch + + +class _FakeQuery: + """Records that a query ran and serves canned rows filtered by an in-list.""" + + def __init__(self, recorder, model_name, rows): + self._rec = recorder + self._model = model_name + self._rows = rows + + def filter(self, *args, **kwargs): + return self + + def order_by(self, *args, **kwargs): + return self + + def all(self): + self._rec.append(self._model) + return self._rows + + def first(self): + self._rec.append(self._model) + return self._rows[0] if self._rows else None + + +class _FakeSession: + def __init__(self, rows_by_model): + self.rows_by_model = rows_by_model + self.queries: list[str] = [] + + def query(self, model): + name = model.__name__ + return _FakeQuery(self.queries, name, self.rows_by_model.get(name, [])) + + def rollback(self): + pass + + +def _ofsted_row(urn, date, oe): + base = {f: None for f in ( + "framework", "inspection_type", "quality_of_education", "behaviour_attitudes", + "personal_development", "leadership_management", "early_years_provision", + "sixth_form_provision", "ungraded_outcome", "ungraded_grade", + "rc_safeguarding_met", "rc_inclusion", "rc_curriculum_teaching", "rc_achievement", + "rc_attendance_behaviour", "rc_personal_development", "rc_leadership_governance", + "rc_early_years", "rc_sixth_form", "report_url", + )} + base.update(urn=urn, inspection_date=types.SimpleNamespace(isoformat=lambda: date), + overall_effectiveness=oe, grade_source=None) + return types.SimpleNamespace(**base) + + +def _adm_row(urn, year): + return types.SimpleNamespace( + urn=urn, year=year, school_phase="Primary", places_offered=100, + total_applications=200, first_preference_applications=150, + first_preference_offers=140, first_preference_offer_pct=93.3, + oversubscription_ratio=1.5, oversubscribed=True, + total_offers=100, second_preference_offers=5, third_preference_offers=2, + cross_la_applications=10, cross_la_offers=3, + ) + + +def test_one_query_per_table_and_latest_row_per_urn(): + rows = { + # URN 1 has two Ofsted rows; the batch must keep the most recent (2023). + "FactOfstedInspection": [ + _ofsted_row(1, "2023-01-01", 2), + _ofsted_row(1, "2019-01-01", 3), + _ofsted_row(2, "2021-06-01", 1), + ], + "FactAdmissions": [_adm_row(1, 202526), _adm_row(1, 202627), _adm_row(2, 202627)], + "FactPupilCharacteristics": [], + "FactDeprivation": [], + "FactFinance": [], + } + session = _FakeSession(rows) + out = get_supplementary_data_batch(session, [1, 2]) + + # Exactly one query per table — five total, regardless of two URNs. + assert sorted(session.queries) == [ + "FactAdmissions", "FactDeprivation", "FactFinance", + "FactOfstedInspection", "FactPupilCharacteristics", + ] + + # Latest Ofsted kept per URN + assert out[1]["ofsted"]["overall_effectiveness"] == 2 + assert out[2]["ofsted"]["overall_effectiveness"] == 1 + + # Admissions history grouped per URN, latest exposed as `admissions` + assert [r["year"] for r in out[1]["admissions_history"]] == [202526, 202627] + assert out[1]["admissions"]["year"] == 202627 + assert out[2]["admissions_history"] == [{**out[2]["admissions_history"][0]}] + + # Empty tables degrade to the null block, not a crash + assert out[1]["census"] is None and out[1]["deprivation"] is None + + +def test_single_wrapper_matches_batch(monkeypatch): + session = _FakeSession({"FactOfstedInspection": [_ofsted_row(5, "2022-01-01", 2)]}) + single = data_loader.get_supplementary_data(session, 5) + assert single["ofsted"]["overall_effectiveness"] == 2 + assert single["admissions_history"] == []