diff --git a/backend/app/routers/articles.py b/backend/app/routers/articles.py index 5f0a647..4416b38 100644 --- a/backend/app/routers/articles.py +++ b/backend/app/routers/articles.py @@ -401,6 +401,12 @@ def _plain_excerpt(article: Article, limit: int = 260) -> str: if not source.strip() and article.sections: source = (article.sections[0] or {}).get("content") or "" text = re.sub(r"!\[[^\]]*\]\([^)]*\)", " ", source) # images + # Our own cross-reference syntax, before the plain-link rule below gets at + # it: `[[288|eczema]]` and `[[Eczema|eczema]]` both mean a word, and the + # generic rule left "[[288 eczema]]" on the card. Which half is the word + # depends on which half is a number. + text = re.sub(r"\[\[(?:(\d+)\|([^\]]+)|([^\]|]+?)(?:\|[a-z0-9-]+)?)\]\]", + lambda m: (m.group(2) or m.group(3) or "").strip(), text) text = re.sub(r"\[([^\]]*)\]\([^)]*\)", r"\1", text) # links keep their words text = re.sub(r"^\s{0,3}#{1,6}\s*", "", text, flags=re.M) # headings text = re.sub(r"[*_`>|]|^\s*[-+]\s", " ", text, flags=re.M) diff --git a/backend/app/routers/uploads.py b/backend/app/routers/uploads.py index 38f5919..36e1227 100644 --- a/backend/app/routers/uploads.py +++ b/backend/app/routers/uploads.py @@ -6,7 +6,7 @@ from fastapi.responses import FileResponse, Response from sqlalchemy.orm import Session from app.database import get_db -from app.services import storage_service +from app.services import storage_service, thumbnails from app.utils.auth import get_current_user from app.utils.upload_access import ( LEGACY_LMS_PREFIXES, local_upload_path, upload_file, references, @@ -17,16 +17,32 @@ router = APIRouter() @router.api_route("/uploads/{path:path}", methods=["GET", "HEAD"]) -def read_upload(path: str, request: Request, attempt_id: int | None = None, db: Session = Depends(get_db)): +def read_upload(path: str, request: Request, attempt_id: int | None = None, + w: int | None = None, db: Session = Depends(get_db)): + """An upload, optionally at one of two smaller widths. + + `?w=256` and `?w=640` are the only sizes there are, and anything else is + refused rather than honoured — an endpoint that resizes to whatever the + query string asks for is a CPU sink anybody can point at. The authorisation + below is unchanged and runs first: a thumbnail of a file you may not read + is a file you may not read. + + These stay `private, no-store` like everything else here. They are behind + authentication, so there is nothing for a shared cache to do with them; the + win is that the bytes are a tenth the size. + """ headers = {"Cache-Control": "private, no-store", "Vary": "Cookie, Authorization", "X-Content-Type-Options": "nosniff"} + if w is not None and w not in thumbnails.WIDTHS: + raise HTTPException(400, f"Width must be one of {', '.join(map(str, thumbnails.WIDTHS))}", + headers=headers) try: - return _read_upload(path, request, attempt_id, db, headers) + return _read_upload(path, request, attempt_id, db, headers, w) except HTTPException as exc: exc.headers = {**(exc.headers or {}), **headers} raise -def _read_upload(path, request, attempt_id, db, headers): +def _read_upload(path, request, attempt_id, db, headers, width=None): try: path = local_upload_path(path) target = upload_file(path) @@ -47,6 +63,18 @@ def _read_upload(path, request, attempt_id, db, headers): if target.suffix.lower() == ".svg": headers["Content-Security-Policy"] = "sandbox; default-src 'none'; style-src 'unsafe-inline'" + # Asked for small, and small exists: the derivative is made on the first + # ask and stored beside the original. `None` back means there is nothing + # smaller worth serving — not an image, or already narrower than asked — + # so the original goes out, which is what the caller wanted anyway. + if width: + small = thumbnails.get(path, width) + if small is not None: + if request.method == "HEAD": + return Response(status_code=200, media_type="image/webp", + headers={**headers, "Content-Length": str(len(small))}) + return Response(content=small, media_type="image/webp", headers=headers) + # Serving must go through the storage service, or object storage would be # write-only: the bytes would be in the bucket and still read from disk. # The path is already authorised and confined to the upload root above. diff --git a/backend/app/services/thumbnails.py b/backend/app/services/thumbnails.py new file mode 100644 index 0000000..e9e96a3 --- /dev/null +++ b/backend/app/services/thumbnails.py @@ -0,0 +1,115 @@ +"""Smaller copies of an uploaded image, made once and kept. + +A question's stem image is 2–4 MB of scanned radiograph and a grid of them is +twenty of those, so a browse page was pulling forty megabytes to draw forty +postage stamps. This makes a 256px and a 640px WebP of each and serves those +where a small one will do. + +Three rules, all of them about not turning this into a way to spend the +server's afternoon: + + * **Allow-listed widths only.** 256 and 640, and any other `?w=` is refused + rather than honoured. An endpoint that resizes to whatever the query string + asks for is a CPU sink anybody can point at. + * **Never enlarged.** A 180px image asked for at 640 is served as it is. + Scaling up invents detail and costs bytes to do it. + * **Best effort.** A derivative that cannot be made is not an error — the + caller falls back to the original. A failed preview must never fail an + upload, and a corrupt file must never take a page down with it. + +Derivatives live in the same bucket under `thumbs/{width}/{key}`, so +credentials, lifecycle and backup are unchanged and nothing new has to be +configured for them to be backed up or thrown away. + +Pillow rather than sharp: sharp is Node, this backend is Python, and Pillow is +already here and does the same three things. +""" +import io +import logging +import mimetypes + +from app.services import storage_service + +logger = logging.getLogger(__name__) + +#: The only widths that exist. See the module docstring. +WIDTHS = (256, 640) + +#: What is worth resizing. A PDF or an SVG is not: one is not an image and the +#: other is already small and resolution-independent. +RESIZABLE = {"image/jpeg", "image/png", "image/webp", "image/gif", "image/bmp", "image/tiff"} + +QUALITY = 82 + + +def thumb_key(key: str, width: int) -> str: + return f"thumbs/{width}/{key}" + + +def is_resizable(key: str) -> bool: + return (mimetypes.guess_type(key)[0] or "") in RESIZABLE + + +def render(data: bytes, width: int) -> bytes | None: + """One derivative, or None if it should not or cannot be made.""" + try: + from PIL import Image, ImageOps + except ImportError: # pragma: no cover - Pillow is a hard dependency here + logger.warning("Pillow is not installed; serving originals") + return None + try: + image = Image.open(io.BytesIO(data)) + # Before anything measures it: a photograph from a phone is stored + # sideways with a rotation flag, and a thumbnail made without reading + # that flag is a sideways thumbnail of a portrait image. + image = ImageOps.exif_transpose(image) + if image.width <= width: + return None + height = max(1, round(image.height * width / image.width)) + image = image.convert("RGBA" if image.mode in ("RGBA", "LA", "P") else "RGB") + image = image.resize((width, height), Image.LANCZOS) + out = io.BytesIO() + image.save(out, format="WEBP", quality=QUALITY, method=4) + return out.getvalue() + except Exception: + # A file that is not the image its name claims, a truncated upload, a + # decompression bomb Pillow refuses to open. None of those is a reason + # to fail the request that asked for a preview. + logger.info("Could not make a %spx thumbnail", width, exc_info=True) + return None + + +def get(key: str, width: int) -> bytes | None: + """The derivative, making it on first ask. None means "serve the original". + + Not a cache that can be turned off: the second reader of a page gets the + stored copy, and the first pays for it once. + """ + if width not in WIDTHS or not is_resizable(key): + return None + derived = thumb_key(key, width) + existing = storage_service.load(derived) + if existing: + return existing + source = storage_service.load(key) + if not source: + return None + made = render(source, width) + if not made: + return None + try: + storage_service.save(derived, made, "image/webp") + except Exception: + # Serve it anyway. Storing is an optimisation for the next reader; this + # one already has the bytes in hand. + logger.info("Could not store the %spx thumbnail for %s", width, key, exc_info=True) + return made + + +def forget(key: str) -> None: + """Drop every derivative of a key, for when the original changes or goes.""" + for width in WIDTHS: + try: + storage_service.delete(thumb_key(key, width)) + except Exception: + logger.info("Could not remove the %spx thumbnail for %s", width, key, exc_info=True) diff --git a/backend/tests/test_articles_cards.py b/backend/tests/test_articles_cards.py index cb65200..d8208ff 100644 --- a/backend/tests/test_articles_cards.py +++ b/backend/tests/test_articles_cards.py @@ -280,13 +280,20 @@ class ArticleReadingTests(unittest.TestCase): def test_excerpt_reads_as_prose_not_markup(self): self.make([self.section('a', 'A')], slug='marked-up', summary='', - content='## Heading\n\n![scan](/uploads/x.png) See [the workup](/articles/workup) **now**.') + content='## Heading\n\n![scan](/uploads/x.png) See [the workup](/articles/workup) **now**, ' + 'stratified by [[288|eczema]] and [[Croup|croup]].') self.client.post('/articles/1/publish', json={'published': True}) excerpt = self.client.get('/articles/preview/marked-up').json()['excerpt'] self.assertNotIn('!', excerpt) self.assertNotIn('##', excerpt) self.assertNotIn('/uploads/', excerpt) self.assertIn('the workup', excerpt) # a link keeps its words + # And so does a cross-reference. The generic link rule does not know + # this syntax, so it used to leave "[[288 eczema]]" on the card. + self.assertNotIn('[[', excerpt) + self.assertNotIn('288', excerpt) + self.assertIn('eczema', excerpt) + self.assertIn('Croup', excerpt) def test_all_three_views_round_trip_through_a_save(self): payload = {"title": "Nested topic", "slug": "three-views", "summary": "S", "content": "I", diff --git a/backend/tests/test_thumbnails.py b/backend/tests/test_thumbnails.py new file mode 100644 index 0000000..b0835c6 --- /dev/null +++ b/backend/tests/test_thumbnails.py @@ -0,0 +1,112 @@ +"""Smaller copies of an uploaded image: which widths exist, and what refuses. + +Disposable temp directory for the upload root; Pillow does the real work, so +these are real images in and real WebP out rather than mocks agreeing with +themselves. +""" +import io +import os +import tempfile +import unittest + +os.environ["DATABASE_URL"] = "sqlite:///:memory:" + +from PIL import Image + +from app.config import settings +from app.services import storage_service, thumbnails + + +def png(width, height, colour=(200, 30, 40)): + buffer = io.BytesIO() + Image.new("RGB", (width, height), colour).save(buffer, format="PNG") + return buffer.getvalue() + + +class ThumbnailTests(unittest.TestCase): + def setUp(self): + self.root = tempfile.TemporaryDirectory() + self.previous = (settings.UPLOAD_DIR, settings.STORAGE_BACKEND) + settings.UPLOAD_DIR = self.root.name + # Pinned local, so the test is about resizing rather than about which + # bucket this container happens to be pointed at. + settings.STORAGE_BACKEND = "local" + + def tearDown(self): + settings.UPLOAD_DIR, settings.STORAGE_BACKEND = self.previous + self.root.cleanup() + + def put(self, key, data): + storage_service.save(key, data) + return key + + def test_a_wide_image_comes_back_narrower_and_as_webp(self): + key = self.put("questions/big.png", png(1600, 900)) + small = thumbnails.get(key, 256) + self.assertIsNotNone(small) + image = Image.open(io.BytesIO(small)) + self.assertEqual(image.width, 256) + self.assertEqual(image.format, "WEBP") + # The aspect ratio is kept rather than the height being guessed. + self.assertEqual(image.height, round(900 * 256 / 1600)) + self.assertLess(len(small), len(storage_service.load(key))) + + def test_it_is_made_once_and_kept_beside_the_original(self): + key = self.put("questions/kept.png", png(1200, 600)) + first = thumbnails.get(key, 640) + self.assertIsNotNone(first) + stored = storage_service.load(thumbnails.thumb_key(key, 640)) + self.assertEqual(stored, first) + # Removing the original must not change what a stored derivative + # returns: the second reader is served from the copy, not from a resize. + storage_service.delete(key) + self.assertEqual(thumbnails.get(key, 640), first) + + def test_a_small_image_is_never_enlarged(self): + key = self.put("questions/small.png", png(180, 120)) + # None means "there is nothing smaller worth serving", and the caller + # sends the original. Scaling up invents detail and costs bytes to do it. + self.assertIsNone(thumbnails.get(key, 256)) + self.assertIsNone(thumbnails.get(key, 640)) + + def test_only_the_two_widths_exist(self): + key = self.put("questions/widths.png", png(1600, 900)) + self.assertEqual(thumbnails.WIDTHS, (256, 640)) + for width in (1, 255, 512, 641, 4000): + self.assertIsNone(thumbnails.get(key, width)) + + def test_what_is_not_an_image_is_left_alone(self): + self.assertFalse(thumbnails.is_resizable("documents/handbook.pdf")) + self.assertFalse(thumbnails.is_resizable("figures/diagram.svg")) + key = self.put("documents/handbook.pdf", b"%PDF-1.4 not really") + self.assertIsNone(thumbnails.get(key, 256)) + + def test_a_file_that_is_not_the_image_it_claims_is_not_an_error(self): + key = self.put("questions/broken.png", b"this is not a PNG") + self.assertIsNone(thumbnails.get(key, 256)) + + def test_forgetting_removes_every_derivative(self): + key = self.put("questions/gone.png", png(1600, 900)) + for width in thumbnails.WIDTHS: + self.assertIsNotNone(thumbnails.get(key, width)) + thumbnails.forget(key) + for width in thumbnails.WIDTHS: + self.assertIsNone(storage_service.load(thumbnails.thumb_key(key, width))) + + def test_a_sideways_photograph_is_turned_the_right_way_up(self): + # Orientation 6 is "rotate 90° clockwise to view", which is how every + # phone stores a portrait photograph. A thumbnail made without reading + # it is a sideways thumbnail. + buffer = io.BytesIO() + image = Image.new("RGB", (1200, 600), (10, 20, 30)) + exif = image.getexif() + exif[274] = 6 + image.save(buffer, format="JPEG", exif=exif) + key = self.put("questions/sideways.jpg", buffer.getvalue()) + small = Image.open(io.BytesIO(thumbnails.get(key, 256))) + self.assertEqual(small.width, 256) + self.assertGreater(small.height, small.width) + + +if __name__ == "__main__": + unittest.main() diff --git a/docs/TODO.md b/docs/TODO.md index e3fd07d..b00e1c8 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -251,16 +251,22 @@ Captured so nothing is lost while the article writing runs. whether a code is valid before the account is made. - [x] **Settings, properly** — done 2026-09-11: People and AI models rebuilt natively, AdminPage deleted. - [x] **Comments backend removed** — done 2026-09-11: router, model and table dropped. -- [ ] **Image thumbnails.** The ped-ai design, read 2026-09-11: allow-listed - widths only (256 and 640 — any other `?w=` is refused, so the endpoint - cannot be turned into a resize-on-demand CPU sink), EXIF rotate, resize - without enlarging, WebP q82, derivatives stored in the same bucket under - a `thumbs/{id}/{width}` prefix so credentials, lifecycle and backup are - unchanged, generated best-effort so a failed preview never fails the - upload. **Pillow, not sharp** — sharp is Node and this backend is Python; - Pillow 12.3 is already installed and does the same three things. **No - Caddy caching**: these are behind auth, ped-ai serves its own - `private, no-store` for that reason, and the win is the 256px WebP. +- [x] **Image thumbnails** — done 2026-09-12. `?w=256` and `?w=640` on + `/uploads/...`, any other width refused with a 400 rather than honoured. + EXIF rotate, never enlarged, WebP q82, derivatives under `thumbs/{width}/{key}` + in the same bucket so credentials, lifecycle and backup are unchanged, and + best effort throughout — a file that will not decode serves its original + rather than failing. Pillow, not sharp. No Caddy caching: these are behind + authentication and stay `private, no-store`; the win is the byte count. + Asked for by the media grid, the figure strip's compact thumbnails, the + figure manager and the quiz editor. + **Loose end:** Pillow is not pinned in `backend/requirements.txt`. It + arrives transitively via PyMuPDF and is 12.3.0 in the image today. Adding + the pin invalidates the pip layer, and rebuilding that layer now fails + outright because `litellm==1.28.13` has been withdrawn from PyPI. Pinning + Pillow therefore means re-pinning litellm first, which is a deliberate + upgrade of the AI layer and not a side effect to slip into a thumbnail + change. - [ ] ~~Image thumbnails and caching~~ — you mentioned a tool from the ped-ai work that generates thumbnails and caches through Caddy so images load fast, click to open full size, same bucket, and no straightforward diff --git a/frontend/src/components/FigureManager.jsx b/frontend/src/components/FigureManager.jsx index 2341d2b..005ee2c 100644 --- a/frontend/src/components/FigureManager.jsx +++ b/frontend/src/components/FigureManager.jsx @@ -109,7 +109,7 @@ export default function FigureManager({ questionId }) {