diff --git a/backend/app.py b/backend/app.py index 678fabc..2b22d81 100644 --- a/backend/app.py +++ b/backend/app.py @@ -7,6 +7,7 @@ Uses real data from UK Government Compare School Performance downloads. import hashlib import re from contextlib import asynccontextmanager +from datetime import datetime, timezone from typing import Optional import numpy as np @@ -53,8 +54,9 @@ PHASE_GROUPS: dict[str, set[str]] = { BASE_URL = "https://www.schoolcompare.co.uk" MAX_SLUG_LENGTH = 60 -# In-memory sitemap cache -_sitemap_xml: str | None = None +# In-memory sitemap cache: name -> XML. Populated on startup and by the admin +# regenerate endpoint after a pipeline run. +_sitemaps: dict[str, str] | None = None def _slugify(text: str) -> str: @@ -135,16 +137,65 @@ def _school_sitemap_rows(df) -> list[str]: return rows -def build_sitemap() -> str: - """Generate sitemap XML from in-memory school data. Returns the XML string.""" +# Sitemaps cap at 50,000 URLs per file. 10,000 keeps a child small enough to +# scan by eye in Search Console, which is the point of splitting at all: +# coverage is reported per submitted sitemap, so one file per page family is +# what makes an indexation problem attributable to a family. +SITEMAP_CHUNK_SIZE = 10_000 + +# Children are served under /sitemaps/ because Next.js only treats a whole +# bracketed path segment as dynamic — a route folder named "sitemap-[...parts]" +# is read as a literal static segment and never matches. +SITEMAP_CHILD_PREFIX = "/sitemaps" + + +def _urlset(rows: list[str]) -> str: + return "\n".join([ + '', + '', + *rows, + "", + ]) + + +def build_sitemaps() -> dict[str, str]: + """Build the sitemap index and every child, keyed by name.""" df = load_school_data() - lines = ['', - ''] - lines.extend(_url_element(BASE_URL + path) for path in STATIC_SITEMAP_PATHS) - lines.extend(_school_sitemap_rows(df)) - lines.append("") - return "\n".join(lines) + children: dict[str, str] = { + "static.xml": _urlset( + [_url_element(BASE_URL + path) for path in STATIC_SITEMAP_PATHS]), + } + + school_rows = _school_sitemap_rows(df) + # Always emit at least one school child, so the index shape is stable even + # on an empty database. + chunks = [school_rows[i:i + SITEMAP_CHUNK_SIZE] + for i in range(0, len(school_rows), SITEMAP_CHUNK_SIZE)] or [[]] + for n, chunk in enumerate(chunks, start=1): + children[f"schools-{n}.xml"] = _urlset(chunk) + + # On a sitemap index, lastmod means "when this sitemap file last changed", + # so generation time is the correct value here — unlike on a , where + # it would be a claim about content we cannot support. + generated = datetime.now(timezone.utc).date().isoformat() + index_rows = [ + f" {BASE_URL}{SITEMAP_CHILD_PREFIX}/{name}" + f"{generated}" + for name in children + ] + index = "\n".join([ + '', + '', + *index_rows, + "", + ]) + return {**children, "sitemap.xml": index} + + +def build_sitemap() -> str: + """The sitemap index. Kept for `lifespan` and the admin endpoint.""" + return build_sitemaps()["sitemap.xml"] def clean_filter_values(series: pd.Series) -> list[str]: @@ -322,7 +373,7 @@ def validate_postcode(postcode: Optional[str]) -> Optional[str]: @asynccontextmanager async def lifespan(app: FastAPI): """Application lifespan - startup and shutdown events.""" - global _sitemap_xml + global _sitemaps print("Loading school data from marts...") df = load_school_data() if df.empty: @@ -332,9 +383,9 @@ async def lifespan(app: FastAPI): # Pre-compute the latest-year snapshot so the first search request is fast await asyncio.to_thread(load_latest_school_data) try: - _sitemap_xml = build_sitemap() - n = _sitemap_xml.count("") - print(f"Sitemap built: {n} URLs.") + _sitemaps = build_sitemaps() + n = sum(x.count("") for x in _sitemaps.values()) + print(f"Sitemaps built: {len(_sitemaps)} files, {n} URLs.") except Exception as e: print(f"Warning: sitemap build failed on startup: {e}") @@ -1125,16 +1176,28 @@ async def robots_txt(): return FileResponse(settings.frontend_dir / "robots.txt", media_type="text/plain") -@app.get("/sitemap.xml") -async def sitemap_xml(): - """Serve sitemap.xml for search engine indexing.""" - global _sitemap_xml - if _sitemap_xml is None: +def _serve_sitemap(name: str) -> Response: + global _sitemaps + if _sitemaps is None: try: - _sitemap_xml = build_sitemap() + _sitemaps = build_sitemaps() except Exception as e: raise HTTPException(status_code=503, detail=f"Sitemap unavailable: {e}") - return Response(content=_sitemap_xml, media_type="application/xml") + if name not in _sitemaps: + raise HTTPException(status_code=404, detail="No such sitemap") + return Response(content=_sitemaps[name], media_type="application/xml") + + +@app.get("/sitemap.xml") +async def sitemap_xml(): + """Serve the sitemap index.""" + return _serve_sitemap("sitemap.xml") + + +@app.get("/sitemaps/{name}") +async def sitemap_child(name: str): + """Serve a child sitemap (static.xml, or schools-N.xml).""" + return _serve_sitemap(name) @app.post("/api/admin/regenerate-sitemap") @@ -1144,10 +1207,10 @@ async def regenerate_sitemap( _: bool = Depends(verify_admin_api_key), ): """Rebuild and cache the sitemap from current school data. Called by Airflow after data updates.""" - global _sitemap_xml - _sitemap_xml = build_sitemap() - n = _sitemap_xml.count("") - return {"status": "ok", "urls": n} + global _sitemaps + _sitemaps = build_sitemaps() + n = sum(x.count("") for x in _sitemaps.values()) + return {"status": "ok", "urls": n, "sitemaps": len(_sitemaps)} # Mount static files directly (must be after all routes to avoid catching API calls) diff --git a/backend/tests/test_sitemap.py b/backend/tests/test_sitemap.py index 7564cb1..eb030b1 100644 --- a/backend/tests/test_sitemap.py +++ b/backend/tests/test_sitemap.py @@ -32,32 +32,56 @@ def _schools_df() -> pd.DataFrame: @pytest.fixture() def sitemap(monkeypatch) -> str: + """The sitemap index.""" from backend import app as app_module monkeypatch.setattr(app_module, "load_school_data", _schools_df) return app_module.build_sitemap() -def test_every_loc_uses_the_www_host(sitemap): +@pytest.fixture() +def schools_child(monkeypatch) -> str: + """The first school child sitemap, where school URLs actually live.""" + from backend import app as app_module + + monkeypatch.setattr(app_module, "load_school_data", _schools_df) + return app_module.build_sitemaps()["schools-1.xml"] + + +@pytest.fixture() +def static_child(monkeypatch) -> str: + from backend import app as app_module + + monkeypatch.setattr(app_module, "load_school_data", _schools_df) + return app_module.build_sitemaps()["static.xml"] + + +def test_every_loc_uses_the_www_host(sitemaps): # The apex 301s to www. A that redirects burns a crawl per URL. - assert "https://www.schoolcompare.co.uk" in sitemap - assert "https://schoolcompare.co.uk" not in sitemap + # Checked across every file, index included, not just one. + for name, xml in sitemaps.items(): + assert "https://www.schoolcompare.co.uk" in xml, name + assert "https://schoolcompare.co.uk" not in xml, name -def test_school_with_results_is_listed(sitemap): - assert "/school/100001-alpha-primary" in sitemap +def test_school_with_results_is_listed(schools_child): + assert "/school/100001-alpha-primary" in schools_child -def test_school_with_no_results_and_no_ofsted_is_omitted(sitemap): +def test_school_with_no_results_and_no_ofsted_is_omitted(schools_child): # Nothing for a search result to say about it. Submitting it spends crawl # budget and drags the corpus-wide quality signal down. - assert "/school/100002" not in sitemap + # + # Asserted against the child, not the index: the index carries no school + # URLs at all, so it would pass this trivially and prove nothing. + assert "/school/100002" not in schools_child -def test_no_invented_priority_or_changefreq(sitemap): +def test_no_invented_priority_or_changefreq(sitemaps): # Google ignores both. They were noise dressed as signal. - assert "" not in sitemap - assert "" not in sitemap + for name, xml in sitemaps.items(): + assert "" not in xml, name + assert "" not in xml, name def test_ofsted_date_becomes_lastmod(monkeypatch): @@ -70,7 +94,7 @@ def test_ofsted_date_becomes_lastmod(monkeypatch): return base monkeypatch.setattr(app_module, "load_school_data", _df) - xml = app_module.build_sitemap() + xml = app_module.build_sitemaps()["schools-1.xml"] assert "2024-03-14" in xml @@ -85,10 +109,70 @@ def test_no_lastmod_invented_when_date_unknown(monkeypatch): return df monkeypatch.setattr(app_module, "load_school_data", _df) - xml = app_module.build_sitemap() + # The child only. The index legitimately carries a lastmod, because there + # it means "when this sitemap file changed", which we do know. + xml = app_module.build_sitemaps()["schools-1.xml"] assert "" not in xml -def test_static_routes_are_listed(sitemap): +def test_static_routes_are_listed(static_child): for path in ("/", "/rankings", "/compare", "/admissions"): - assert f"https://www.schoolcompare.co.uk{path}" in sitemap + assert f"https://www.schoolcompare.co.uk{path}" in static_child + + +@pytest.fixture() +def sitemaps(monkeypatch) -> dict: + from backend import app as app_module + + monkeypatch.setattr(app_module, "load_school_data", _schools_df) + return app_module.build_sitemaps() + + +def test_index_lists_each_child(sitemaps): + index = sitemaps["sitemap.xml"] + assert " entries only; mixing in is invalid. + assert "" not in sitemaps["sitemap.xml"] + + +def test_index_does_not_list_itself(sitemaps): + assert "https://www.schoolcompare.co.uk/sitemap.xml" not in sitemaps["sitemap.xml"] + + +def test_static_child_holds_the_static_routes(sitemaps): + static = sitemaps["static.xml"] + for path in ("/", "/rankings", "/compare", "/admissions"): + assert f"https://www.schoolcompare.co.uk{path}" in static + + +def test_school_child_holds_the_schools(sitemaps): + assert "/school/100001-alpha-primary" in sitemaps["schools-1.xml"] + + +def test_children_are_chunked_under_the_limit(monkeypatch): + # Sitemaps cap at 50,000 URLs per file. Chunk at 10,000 so a child stays + # small enough to eyeball in Search Console. + from backend import app as app_module + import pandas as _pd + + rows = [ + {"urn": 200000 + i, "school_name": f"School {i}", "year": 202425, + "rwm_expected_pct": 60.0, "attainment_8_score": None, + "ofsted_grade": 2.0, "ofsted_date": None} + for i in range(10_001) + ] + monkeypatch.setattr(app_module, "load_school_data", lambda: _pd.DataFrame(rows)) + + maps = app_module.build_sitemaps() + assert maps["schools-1.xml"].count("") == 10_000 + assert maps["schools-2.xml"].count("") == 1 + + +def test_build_sitemap_still_returns_the_index(sitemap): + # lifespan and the admin endpoint call build_sitemap(); keep it working. + assert " { +async function sitemapChildren(page: Page): Promise { const res = await page.request.get('/sitemap.xml'); expect(res.ok()).toBeTruthy(); - const xml = await res.text(); + const index = await res.text(); + expect(index).toContain('([^<]+)<\/loc>/g)].map((m) => m[1]); +} - const urlCount = (xml.match(//g) ?? []).length; - expect(urlCount, 'sitemap looks empty or truncated').toBeGreaterThan(1000); +test('the sitemap index names children that all resolve', async ({ page }) => { + const index = await (await page.request.get('/sitemap.xml')).text(); + // An index holds entries only; mixing in is invalid. + expect(index).not.toContain(''); - // 401559 (Cardiff) and 402426 (ACT Schools, Cardiff) were both submitted - // before the England-only filter landed. - expect(xml).not.toContain('/school/401559'); - expect(xml).not.toContain('/school/402426'); + const locs = await sitemapChildren(page); + expect(locs.length).toBeGreaterThanOrEqual(2); + + for (const loc of locs) { + expect(loc.startsWith('https://www.schoolcompare.co.uk/sitemaps/')).toBeTruthy(); + const child = await page.request.get(new URL(loc).pathname); + expect(child.ok(), `${loc} should resolve`).toBeTruthy(); + expect(await child.text()).toContain(' { + const locs = await sitemapChildren(page); + + let total = 0; + for (const loc of locs) { + const xml = await (await page.request.get(new URL(loc).pathname)).text(); + total += (xml.match(//g) ?? []).length; + // 401559 (Adamsdown, Cardiff) and 402426 (ACT Schools, Cardiff) were both + // submitted before the England-only filter landed. + expect(xml).not.toContain('/school/401559'); + expect(xml).not.toContain('/school/402426'); + } + expect(total, 'sitemap looks empty or truncated').toBeGreaterThan(1000); +}); + +test('the sitemap invents no priority or changefreq', async ({ page }) => { + const [first] = await sitemapChildren(page); + expect(first).toBeTruthy(); + + const xml = await (await page.request.get(new URL(first).pathname)).text(); + // Google ignores both. They were noise dressed as signal. + expect(xml).not.toContain(''); + expect(xml).not.toContain(''); }); /* diff --git a/nextjs-app/app/sitemap.xml/route.ts b/nextjs-app/app/sitemap.xml/route.ts index acb504d..9cfab89 100644 --- a/nextjs-app/app/sitemap.xml/route.ts +++ b/nextjs-app/app/sitemap.xml/route.ts @@ -1,32 +1,8 @@ -/** - * Runtime proxy for /sitemap.xml → the FastAPI backend's generated sitemap. - * - * Like the /api/* proxy, this reads FASTAPI_URL at request time rather than - * baking the backend host into the build, so one image works in every - * environment. robots.ts points crawlers here. - */ - -import { NextResponse } from 'next/server'; +import { proxySitemap } from '@/lib/sitemapProxy'; export const dynamic = 'force-dynamic'; export const runtime = 'nodejs'; -function backendOrigin(): string { - const base = process.env.FASTAPI_URL || process.env.NEXT_PUBLIC_API_URL || 'http://localhost:8000/api'; - return base.replace(/\/api$/, ''); -} - export async function GET() { - let upstream: Response; - try { - upstream = await fetch(`${backendOrigin()}/sitemap.xml`, { cache: 'no-store' }); - } catch { - return new NextResponse('Sitemap temporarily unavailable', { status: 502 }); - } - - const body = await upstream.text(); - return new NextResponse(body, { - status: upstream.status, - headers: { 'content-type': upstream.headers.get('content-type') || 'application/xml' }, - }); + return proxySitemap('/sitemap.xml'); } diff --git a/nextjs-app/app/sitemaps/[...parts]/route.ts b/nextjs-app/app/sitemaps/[...parts]/route.ts new file mode 100644 index 0000000..c540b82 --- /dev/null +++ b/nextjs-app/app/sitemaps/[...parts]/route.ts @@ -0,0 +1,24 @@ +import { NextResponse } from 'next/server'; +import { proxySitemap } from '@/lib/sitemapProxy'; + +export const dynamic = 'force-dynamic'; +export const runtime = 'nodejs'; + +/** + * Children are /sitemaps/static.xml and /sitemaps/schools-{n}.xml. The name is + * validated here rather than passed through, so this route cannot be used to + * reach arbitrary backend paths. + */ +const CHILD = /^(static|schools-\d+)\.xml$/; + +export async function GET( + _request: Request, + { params }: { params: Promise<{ parts: string[] }> }, +) { + const { parts } = await params; + const name = parts.join('/'); + if (!CHILD.test(name)) { + return new NextResponse('Not found', { status: 404 }); + } + return proxySitemap(`/sitemaps/${name}`); +} diff --git a/nextjs-app/lib/sitemapProxy.ts b/nextjs-app/lib/sitemapProxy.ts new file mode 100644 index 0000000..dab6359 --- /dev/null +++ b/nextjs-app/lib/sitemapProxy.ts @@ -0,0 +1,29 @@ +/** + * Runtime proxy for the sitemap family → the FastAPI backend. + * + * Like the /api/* proxy, this reads FASTAPI_URL at request time rather than + * baking the backend host into the build, so one image works in every + * environment. robots.ts points crawlers at /sitemap.xml, which is the index; + * the index names children under /sitemaps/, which land on the same proxy. + */ +import { NextResponse } from 'next/server'; + +function backendOrigin(): string { + const base = process.env.FASTAPI_URL || process.env.NEXT_PUBLIC_API_URL || 'http://localhost:8000/api'; + return base.replace(/\/api$/, ''); +} + +export async function proxySitemap(path: string): Promise { + let upstream: Response; + try { + upstream = await fetch(`${backendOrigin()}${path}`, { cache: 'no-store' }); + } catch { + return new NextResponse('Sitemap temporarily unavailable', { status: 502 }); + } + + const body = await upstream.text(); + return new NextResponse(body, { + status: upstream.status, + headers: { 'content-type': upstream.headers.get('content-type') || 'application/xml' }, + }); +}