fix(seo): crawl hygiene and a per-family sitemap index (W1) #110
No files matched your search
+88
-25
@@ -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([
|
||||
'<?xml version="1.0" encoding="UTF-8"?>',
|
||||
'<urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">',
|
||||
*rows,
|
||||
"</urlset>",
|
||||
])
|
||||
|
||||
|
||||
def build_sitemaps() -> dict[str, str]:
|
||||
"""Build the sitemap index and every child, keyed by name."""
|
||||
df = load_school_data()
|
||||
|
||||
lines = ['<?xml version="1.0" encoding="UTF-8"?>',
|
||||
'<urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">']
|
||||
lines.extend(_url_element(BASE_URL + path) for path in STATIC_SITEMAP_PATHS)
|
||||
lines.extend(_school_sitemap_rows(df))
|
||||
lines.append("</urlset>")
|
||||
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 <url>, where
|
||||
# it would be a claim about content we cannot support.
|
||||
generated = datetime.now(timezone.utc).date().isoformat()
|
||||
index_rows = [
|
||||
f" <sitemap><loc>{BASE_URL}{SITEMAP_CHILD_PREFIX}/{name}</loc>"
|
||||
f"<lastmod>{generated}</lastmod></sitemap>"
|
||||
for name in children
|
||||
]
|
||||
index = "\n".join([
|
||||
'<?xml version="1.0" encoding="UTF-8"?>',
|
||||
'<sitemapindex xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">',
|
||||
*index_rows,
|
||||
"</sitemapindex>",
|
||||
])
|
||||
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("<url>")
|
||||
print(f"Sitemap built: {n} URLs.")
|
||||
_sitemaps = build_sitemaps()
|
||||
n = sum(x.count("<url>") 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("<url>")
|
||||
return {"status": "ok", "urls": n}
|
||||
global _sitemaps
|
||||
_sitemaps = build_sitemaps()
|
||||
n = sum(x.count("<url>") 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)
|
||||
|
||||
@@ -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 <loc> 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 "<priority>" not in sitemap
|
||||
assert "<changefreq>" not in sitemap
|
||||
for name, xml in sitemaps.items():
|
||||
assert "<priority>" not in xml, name
|
||||
assert "<changefreq>" 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 "<lastmod>2024-03-14</lastmod>" 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 "<lastmod>" 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"<loc>https://www.schoolcompare.co.uk{path}</loc>" in sitemap
|
||||
assert f"<loc>https://www.schoolcompare.co.uk{path}</loc>" 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 "<sitemapindex" in index
|
||||
assert "https://www.schoolcompare.co.uk/sitemaps/static.xml" in index
|
||||
assert "https://www.schoolcompare.co.uk/sitemaps/schools-1.xml" in index
|
||||
|
||||
|
||||
def test_index_carries_no_url_elements(sitemaps):
|
||||
# A sitemap index holds <sitemap> entries only; mixing in <url> is invalid.
|
||||
assert "<url>" not in sitemaps["sitemap.xml"]
|
||||
|
||||
|
||||
def test_index_does_not_list_itself(sitemaps):
|
||||
assert "<loc>https://www.schoolcompare.co.uk/sitemap.xml</loc>" 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"<loc>https://www.schoolcompare.co.uk{path}</loc>" 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("<url>") == 10_000
|
||||
assert maps["schools-2.xml"].count("<url>") == 1
|
||||
|
||||
|
||||
def test_build_sitemap_still_returns_the_index(sitemap):
|
||||
# lifespan and the admin endpoint call build_sitemap(); keep it working.
|
||||
assert "<sitemapindex" in sitemap
|
||||
@@ -1587,18 +1587,53 @@ test('a Welsh school URL 404s while an English one still resolves', async ({ pag
|
||||
expect(welsh?.status(), 'a Welsh school should no longer resolve').toBe(404);
|
||||
});
|
||||
|
||||
test('the sitemap submits no Welsh or overseas school', async ({ page }) => {
|
||||
async function sitemapChildren(page: Page): Promise<string[]> {
|
||||
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('<sitemapindex');
|
||||
return [...index.matchAll(/<loc>([^<]+)<\/loc>/g)].map((m) => m[1]);
|
||||
}
|
||||
|
||||
const urlCount = (xml.match(/<url>/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 <sitemap> entries only; mixing in <url> is invalid.
|
||||
expect(index).not.toContain('<url>');
|
||||
|
||||
// 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('<urlset');
|
||||
}
|
||||
});
|
||||
|
||||
test('the sitemap submits no Welsh or overseas school', async ({ page }) => {
|
||||
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(/<url>/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('<priority>');
|
||||
expect(xml).not.toContain('<changefreq>');
|
||||
});
|
||||
|
||||
/*
|
||||
|
||||
@@ -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');
|
||||
}
|
||||
@@ -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}`);
|
||||
}
|
||||
@@ -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<NextResponse> {
|
||||
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' },
|
||||
});
|
||||
}
|
||||
Reference in new issue
Block a user