diff --git a/CLAUDE.md b/CLAUDE.md index f51844a6..67fe2eb1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -113,9 +113,14 @@ payload uses absolute URLs. Cross-origin requests are allowed on `/api/` only via the in-repo `ApiCorsMiddleware` (no `django-cors-headers` dependency). `Person.email` is intentionally not serialized. Projects are gated to `is_visible=True`; the people list is scoped to actual members (those with a -Position). When adding a resource, follow the existing viewset/serializer -pattern and keep `v1` fields additive-only (breaking changes → `v2`). Full -reference: `docs/API.md`. Tests: `website/tests/test_api.py`. +Position). Image fields follow one rule (#1432): `thumbnail` is a cropped, sized +derivative built by `website.utils.thumbnail_utils.get_cropped_thumbnail` (sizes +in `serializers.py`; keep each size's aspect ratio equal to the model's +`ImageRatioField`), `image_original` is the raw upload; `warm_api_thumbnails` +pre-generates the derivatives at container start. When adding a resource, follow +the existing viewset/serializer pattern and keep `v1` fields additive-only +(breaking changes → `v2`). Full reference: `docs/API.md`. Tests: +`website/tests/test_api.py`. ### Settings, config, and environment @@ -128,7 +133,7 @@ reference: `docs/API.md`. Tests: `website/tests/test_api.py`. ### Container startup side effects (`docker-entrypoint.sh`) -Every container start runs, in order: `collectstatic` → `makemigrations` → `migrate` → `makemigrations website` → `migrate website` → `delete_unused_files` → `thumbnail_cleanup` → `generate_slugs_for_old_news_items` → `auto_close_project_roles` → `remove_year_from_forum_name` → `fix_sortedm2m_columns` → `seed_sidewalk_participants` → `runserver 0.0.0.0:8000`. The repeated `makemigrations website` step is intentional (fixes first-run issues). If you add a one-shot data migration command under `website/management/commands/`, decide whether it belongs in this startup sequence. +Every container start runs, in order: `collectstatic` → `makemigrations` → `migrate` → `makemigrations website` → `migrate website` → `delete_unused_files` → `thumbnail_cleanup` → `generate_slugs_for_old_news_items` → `auto_close_project_roles` → `remove_year_from_forum_name` → `fix_sortedm2m_columns` → `seed_sidewalk_participants` → `warm_api_thumbnails` → `runserver 0.0.0.0:8000`. The repeated `makemigrations website` step is intentional (fixes first-run issues). If you add a one-shot data migration command under `website/management/commands/`, decide whether it belongs in this startup sequence. ### Image handling diff --git a/docker-entrypoint.sh b/docker-entrypoint.sh index 296a0d59..8c227c5b 100755 --- a/docker-entrypoint.sh +++ b/docker-entrypoint.sh @@ -185,6 +185,13 @@ echo "4.10d Running 'python manage.py seed_sidewalk_participants' to backfill Pr echo "******************************************" python manage.py seed_sidewalk_participants +echo "****************** STEP 4.10e/5: docker-entrypoint.sh ************************" +echo "4.10e Running 'python manage.py warm_api_thumbnails' to pre-generate the cropped thumbnails the public API serves (#1432)" +echo "******************************************" +# Idempotent: after the first run this is a couple of stat calls per row. Without +# it, the first API request after a deploy generates every derivative inline. +python manage.py warm_api_thumbnails + # echo "****************** STEP 4.3/5: docker-entrypoint.sh ************************" # echo "4.3 Running 'python manage.py rename_person_images' to rename person images" # echo "******************************************" diff --git a/docs/API.md b/docs/API.md index 4df07614..a86d08b6 100644 --- a/docs/API.md +++ b/docs/API.md @@ -67,7 +67,8 @@ Only **publicly visible** projects (`is_visible=True`). Detail and sub-resources are keyed by `short_name`: - `GET /api/v1/projects//` — summary, about, website, dates, - keywords, umbrellas, thumbnail. + keywords, umbrellas, `thumbnail` (cropped **1000×600**) and `image_original`. + See *Images* below. - `GET /api/v1/projects//publications/` — the project's pubs. - `GET /api/v1/projects//grants/` — grants funding the project. - `GET /api/v1/projects//people/` — everyone with a role on the @@ -100,8 +101,9 @@ Funding amounts are intentionally **not** exposed by the API. Actual lab members (people with at least one Position); external co-authors are not listed here even though they appear as publication `authors`. Detail by `url_name`: `GET /api/v1/people//` — name, `current_title`, -`current_school`, `current_department`, bio, thumbnail, and public social/web -links (ORCID, Google Scholar, GitHub, etc.). +`current_school`, `current_department`, bio, `thumbnail` (cropped **256×256**), +`image_original`, and public social/web links (ORCID, Google Scholar, GitHub, +etc.). See *Images* below. > **Note:** the `current_*` fields come from the person's *latest* Position, so > for an alum they describe their last lab position, not their present-day @@ -111,11 +113,38 @@ links (ORCID, Google Scholar, GitHub, etc.). > **Note:** `email` is intentionally **not** exposed by the API to avoid making > it an email-harvesting surface, even where it appears on a member page. +## Images + +Every payload with a picture follows one convention: + +| Field | What it is | +|------------------|---------------------------------------------------------------| +| `thumbnail` | A **cropped, sized derivative** — the same image the site itself renders, honoring the crop box an editor set in the admin. Use this. | +| `image_original` | The **raw upload**. Full resolution, uncropped, and sometimes tens of megabytes. Only reach for it if you truly need the source. | + +Sizes are **256×256** for a person and **1000×600** for a project — roughly 2× what +the site renders, so a 128 px avatar (or a 500 px-wide project card) stays sharp +on a HiDPI display. Both keep the aspect ratio of the corresponding crop box, so +nothing gets cropped a second time. + +Person image fields appear wherever a person does: `/people/`, publication +`authors`, and the nested `person` on `/projects//people/`. Both +fields are `null` when there's no image at all. + +Derivatives are generated server-side, so **don't hand-build a thumbnail URL** by +editing the size in one — a size the site has never rendered doesn't exist on +disk and will 404. Ask for a different size in an issue instead. + ## Stability contract - **`v1` fields are additive-only.** New fields may be added; existing field names and meanings will not change or be removed within `v1`. Breaking changes ship as `/api/v2/`. + - One exception has been taken, in **2.30.0** (#1432): `thumbnail` on people + and projects used to return the raw upload and now returns the cropped + derivative described above. A field named `thumbnail` handing out an 11 MB + original was a bug, not a contract — 13 headshots cost one consumer 33.5 MB. + The raw file is still available as `image_original`. - Don't hardcode pagination page sizes as a proxy for "all" — page through `next`, or set `page_size` explicitly (≤100). - URLs in responses (PDFs, thumbnails, page links) are absolute and safe to use @@ -130,6 +159,16 @@ Code lives in `website/api/` (`serializers.py`, `views.py`, `urls.py`, (`website.api.middleware.ApiCorsMiddleware`), scoped to `/api/`, rather than a third-party package. Tests: `website/tests/test_api.py`. +Thumbnails go through `website.utils.thumbnail_utils.get_cropped_thumbnail` — +the same easy-thumbnails options the site's templates pass, so the API shares +their cached files — with the sizes defined as `API_PERSON_THUMBNAIL_SIZE` / +`API_PROJECT_THUMBNAIL_SIZE` in `serializers.py`. **Keep any new size's aspect +ratio equal to the model's `ImageRatioField`**, or easy-thumbnails center-crops a +second time on top of the editor's box (#1424). The API's sizes aren't rendered +anywhere on the site, so `warm_api_thumbnails` (run from `docker-entrypoint.sh`) +pre-generates them at container start; without it the first request after a +deploy generates them all inline. + **Deliberately deferred** (add on the same pattern when needed): write endpoints, auth / API keys, request throttling, and Talks/Posters/Videos resources. diff --git a/makeabilitylab/settings.py b/makeabilitylab/settings.py index bd05873c..2e7011d4 100644 --- a/makeabilitylab/settings.py +++ b/makeabilitylab/settings.py @@ -86,8 +86,8 @@ SECURE_PROXY_SSL_HEADER = ('HTTP_X_FORWARDED_PROTO', 'https') # Makeability Lab Global Variables, including Makeability Lab version -ML_WEBSITE_VERSION = "2.29.1" # Keep this updated with each release and also change the short description below -ML_WEBSITE_VERSION_DESCRIPTION = "Performs the one-time rename adding the '_Talk'/'_Poster' filename suffix (#1404): 188 talks and 11 posters per the 2.29.0 dry-run inventory; publications untouched. Also promotes the #1390 diverged-file repair from dry-run to live, recovering 3 talks whose PDFs sat orphaned on disk under dotted names." +ML_WEBSITE_VERSION = "2.30.0" # Keep this updated with each release and also change the short description below +ML_WEBSITE_VERSION_DESCRIPTION = "The public API now serves cropped, sized photos (#1432): a person or project thumbnail honors the crop box you set in the admin instead of handing out the raw upload, which cost one consumer 33.5 MB for 13 headshots. The full-resolution file is still available." DATE_MAKEABILITYLAB_FORMED = datetime.date(2012, 1, 1) # Date Makeability Lab was formed MAX_BANNERS = 7 # Maximum number of banners on a page diff --git a/website/api/serializers.py b/website/api/serializers.py index 4efc0786..49907872 100644 --- a/website/api/serializers.py +++ b/website/api/serializers.py @@ -8,6 +8,10 @@ avoid turning the API into an email-harvesting surface, even where they appear on a member page. +Image fields follow one rule: ``thumbnail`` is a *cropped, sized derivative* +(honoring the editor's crop box, same as the site renders) and ``image_original`` +is the raw upload. See :func:`cropped_thumbnail_url` and #1432. + Existing model helpers are reused rather than re-deriving formatting: ``Person.get_full_name`` / ``get_current_title``, ``Publication`` citation helpers, ``Project.get_display_short_name``, ``Grant.start_date`` / ``grant_url``. @@ -17,6 +21,19 @@ from rest_framework import serializers from website.models import Grant, Person, Project, ProjectRole, Publication +from website.utils.thumbnail_utils import get_cropped_thumbnail + +# Sizes for the cropped derivatives the API serves as ``thumbnail`` (#1432). +# +# Each keeps the aspect ratio of the corresponding model's ImageRatioField -- +# Person's crop box is square (245x245), Project's is 15:9 (500x300). A mismatch +# would make easy-thumbnails center-crop a second time on top of the editor's +# box, which is what clipped heads off the news cards in #1424. +# +# Both are ~2x what the site itself renders, so a consumer can draw a 128px +# avatar (or a 500px-wide project card) sharply on a HiDPI display. +API_PERSON_THUMBNAIL_SIZE = (256, 256) +API_PROJECT_THUMBNAIL_SIZE = (1000, 600) def abs_media_url(request, filefield): @@ -35,6 +52,18 @@ def abs_media_url(request, filefield): return request.build_absolute_uri(url) if request is not None else url +def cropped_thumbnail_url(request, image_field, size, box=None): + """Absolute URL of the cropped derivative of ``image_field`` at ``size``. + + Falls back to the original image when generation fails (a bad/missing source + file), so ``thumbnail`` is never null for a row that *has* an image; returns + ``None`` when there's no image at all. Reuses the same easy-thumbnails + options the site's templates pass, so the API shares their cached files. + """ + thumbnail = get_cropped_thumbnail(image_field, size, box) + return abs_media_url(request, thumbnail or image_field) + + def abs_page_url(request, url_name, *args): """Absolute URL for a named route, tolerant of reverse failures. @@ -50,15 +79,22 @@ def abs_page_url(request, url_name, *args): class PersonSummarySerializer(serializers.ModelSerializer): - """Compact person representation, used when nested in publications/roles.""" + """Compact person representation, used when nested in publications/roles. + + ``thumbnail`` is the cropped 256x256 headshot -- the same derivative the + site's own pages render, honoring the crop box an editor set in the admin. + ``image_original`` is the raw upload, which can be tens of megabytes; only + reach for it if you genuinely need full resolution (#1432). + """ name = serializers.SerializerMethodField() url = serializers.SerializerMethodField() thumbnail = serializers.SerializerMethodField() + image_original = serializers.SerializerMethodField() class Meta: model = Person - fields = ["id", "url_name", "name", "url", "thumbnail"] + fields = ["id", "url_name", "name", "url", "thumbnail", "image_original"] def get_name(self, obj): return obj.get_full_name() @@ -69,6 +105,14 @@ def get_url(self, obj): ) def get_thumbnail(self, obj): + return cropped_thumbnail_url( + self.context.get("request"), + obj.image, + API_PERSON_THUMBNAIL_SIZE, + obj.cropping, + ) + + def get_image_original(self, obj): return abs_media_url(self.context.get("request"), obj.image) @@ -142,9 +186,15 @@ def get_url(self, obj): class ProjectSerializer(ProjectSummarySerializer): - """Full project representation for the projects detail/list endpoints.""" + """Full project representation for the projects detail/list endpoints. + + As with people, ``thumbnail`` is the cropped 1000x600 derivative of the + gallery image (honoring ``Project.cropping``) and ``image_original`` is the + raw upload (#1432). + """ thumbnail = serializers.SerializerMethodField() + image_original = serializers.SerializerMethodField() keywords = serializers.SerializerMethodField() project_umbrellas = serializers.SerializerMethodField() @@ -158,11 +208,20 @@ class Meta(ProjectSummarySerializer.Meta): "data_url", "featured_code_repo_url", "thumbnail", + "image_original", "keywords", "project_umbrellas", ] def get_thumbnail(self, obj): + return cropped_thumbnail_url( + self.context.get("request"), + obj.gallery_image, + API_PROJECT_THUMBNAIL_SIZE, + obj.cropping, + ) + + def get_image_original(self, obj): return abs_media_url(self.context.get("request"), obj.gallery_image) def get_keywords(self, obj): diff --git a/website/management/commands/warm_api_thumbnails.py b/website/management/commands/warm_api_thumbnails.py new file mode 100644 index 00000000..935ccfac --- /dev/null +++ b/website/management/commands/warm_api_thumbnails.py @@ -0,0 +1,96 @@ +""" +Pre-generate the cropped derivatives the public API serves (#1432). + +The API's ``thumbnail`` fields are sizes the *site* never renders (256x256 for a +person, 1000x600 for a project), so without this the first API request after a +deploy generates them inline -- decoding up to a page's worth of multi-megabyte +source photos inside one request, which can approach the Gunicorn timeout set in +docker-entrypoint.sh. This command does that work once at container start. + +It is idempotent and cheap on re-run: easy-thumbnails skips any derivative whose +file is already on disk and newer than its source, so subsequent starts are a +couple of stat calls per row. Safe to run by hand at any time:: + + python manage.py warm_api_thumbnails + python manage.py warm_api_thumbnails --dry-run # just report what's covered + +Scope matches what the API exposes: every person who has a photo -- not just lab +members, because PersonSummarySerializer also nests in publication ``authors``, +where external co-authors show up -- and every publicly visible project. +""" + +import logging +import time + +from django.core.management.base import BaseCommand + +from website.api.serializers import ( + API_PERSON_THUMBNAIL_SIZE, + API_PROJECT_THUMBNAIL_SIZE, +) +from website.models import Person, Project +from website.utils.thumbnail_utils import get_cropped_thumbnail + +_logger = logging.getLogger(__name__) + + +class Command(BaseCommand): + help = "Pre-generate the cropped thumbnails served by the public REST API (#1432)." + + def add_arguments(self, parser): + parser.add_argument( + "--dry-run", + action="store_true", + help="Report how many rows would be warmed without generating anything.", + ) + + def handle(self, *args, **options): + dry_run = options["dry_run"] + + # Rows with no image at all are excluded here rather than skipped in the + # loop: gallery_image is null=True, and Django's .exclude(field="") + # keeps NULLs (NOT (x = '' AND x IS NOT NULL)), so both filters are + # needed or image-less projects get reported as failures. + people = Person.objects.exclude(image="").exclude(image__isnull=True) + projects = ( + Project.objects.filter(is_visible=True) + .exclude(gallery_image="") + .exclude(gallery_image__isnull=True) + ) + + targets = [ + ("person", people, "image", "cropping", API_PERSON_THUMBNAIL_SIZE), + ("project", projects, "gallery_image", "cropping", + API_PROJECT_THUMBNAIL_SIZE), + ] + + for label, queryset, image_attr, box_attr, size in targets: + count = queryset.count() + if dry_run: + msg = f"[dry run] would warm {count} {label} thumbnail(s) at {size}" + _logger.info(msg) + self.stdout.write(msg) + continue + + start = time.monotonic() + generated = cached = failed = 0 + for obj in queryset.iterator(): + image, box = getattr(obj, image_attr), getattr(obj, box_attr) + if get_cropped_thumbnail(image, size, box, generate=False): + cached += 1 + continue + if get_cropped_thumbnail(image, size, box) is None: + # get_cropped_thumbnail already logged the reason (usually a + # source file missing from media/); keep going. + failed += 1 + else: + generated += 1 + + elapsed = time.monotonic() - start + msg = ( + f"{label} thumbnails at {size}: {generated} generated, " + f"{cached} already cached, {failed} failed (of {count}) " + f"in {elapsed:.1f}s" + ) + _logger.info(msg) + self.stdout.write(msg) diff --git a/website/tests/test_api.py b/website/tests/test_api.py index eca46bab..ca7e981f 100644 --- a/website/tests/test_api.py +++ b/website/tests/test_api.py @@ -8,19 +8,56 @@ (Position, Sponsor/Grant, ProjectRole leadership). """ +import io +import os +import shutil +import tempfile from datetime import date +from urllib.parse import unquote, urlparse +from django.conf import settings +from django.core.files.storage import default_storage +from django.core.files.uploadedfile import SimpleUploadedFile from django.db import connection +from django.test import SimpleTestCase, override_settings from django.test.utils import CaptureQueriesContext - -from website.models import Grant, Position, ProjectRole, Sponsor +from PIL import Image + +from website.api.serializers import ( + API_PERSON_THUMBNAIL_SIZE, + API_PROJECT_THUMBNAIL_SIZE, +) +from website.models import Grant, Person, Position, ProjectRole, Sponsor +from website.models.person import PERSON_THUMBNAIL_SIZE from website.models.position import Title +from website.models.project import PROJECT_THUMBNAIL_SIZE from website.models.project_role import LeadProjectRoleTypes from website.models.publication import PubType from website.tests.base import DatabaseTestCase +from website.utils.thumbnail_utils import get_cropped_thumbnail + +# The API now generates cropped derivatives (#1432), so these tests write image +# files. Keep them out of the repo's media/ dir, which already holds tens of +# thousands of files left by earlier suites. +_TEST_MEDIA_ROOT = tempfile.mkdtemp(prefix="ml_api_tests_") + + +def png_upload(name, size=(800, 600), color=(200, 30, 30)): + """A real (not 1x1) PNG upload, so cropping/resizing has something to do.""" + buffer = io.BytesIO() + Image.new("RGB", size, color).save(buffer, format="PNG") + return SimpleUploadedFile(name, buffer.getvalue(), content_type="image/png") +@override_settings(MEDIA_ROOT=_TEST_MEDIA_ROOT) class ApiTestCase(DatabaseTestCase): + @classmethod + def tearDownClass(cls): + # Subclasses inherit this; rmtree is idempotent and FileSystemStorage + # recreates directories on demand, so repeated cleanup is harmless. + shutil.rmtree(_TEST_MEDIA_ROOT, ignore_errors=True) + super().tearDownClass() + def setUp(self): # A visible project (Project Sidewalk) and a hidden one. self.project = self.make_project( @@ -196,6 +233,12 @@ def test_project_people_subresource(self): # against the model in test_project_role.py, where a failure isolates. def _count_queries(self, url): + # Warm the endpoint first: the *first* render generates each person's + # cropped thumbnail (#1432), and easy-thumbnails writes a Source/ + # Thumbnail cache row per generated file. That cost is one-time -- the + # cached path is filesystem stats only, no queries -- so measuring the + # second call keeps this a test of the Position prefetch. + self.assertEqual(self.client.get(url).status_code, 200) with CaptureQueriesContext(connection) as ctx: self.assertEqual(self.client.get(url).status_code, 200) return len(ctx) @@ -341,3 +384,153 @@ def test_cors_options_preflight(self): resp = self.client.options("/api/v1/publications/") self.assertEqual(resp.status_code, 200) self.assertEqual(resp["Access-Control-Allow-Origin"], "*") + + +# ---- image fields (#1432) --------------------------------------------------- + + +class ApiThumbnailTests(ApiTestCase): + """``thumbnail`` is the cropped, sized derivative; ``image_original`` is the + raw upload. + + Before #1432 the API handed out the raw file as ``thumbnail`` -- 33.5 MB for + the 13 headshots Project Sidewalk's About page renders, framed however the + original photo happened to be centered rather than by the editor's crop box. + """ + + CROP_BOX = "100,100,400,400" # square, matching Person's crop ratio + + def _member_with_photo(self, cropping=CROP_BOX): + """A lab member (Position -> visible in /people/) with a real photo.""" + person = self.make_person( + first_name="Photo", + last_name="Person", + image=png_upload("photo_person.png"), + cropping=cropping, + ) + Position.objects.create( + person=person, start_date=date(2020, 1, 1), title=Title.PHD_STUDENT + ) + return person + + def _person_payload(self, person): + resp = self.client.get(f"/api/v1/people/{person.url_name}/") + self.assertEqual(resp.status_code, 200) + return resp.json() + + def _media_path(self, url): + """Filesystem path under MEDIA_ROOT for an absolute media URL.""" + path = unquote(urlparse(url).path) + if path.startswith(settings.MEDIA_URL): + path = path[len(settings.MEDIA_URL):] + return os.path.join(settings.MEDIA_ROOT, path.lstrip("/")) + + def test_person_thumbnail_is_cropped_derivative(self): + person = self._member_with_photo() + body = self._person_payload(person) + + self.assertIn("256x256", body["thumbnail"]) + # The crop box reaches the derivative (commas are percent-encoded). + self.assertIn("box-100%2C100%2C400%2C400", body["thumbnail"]) + self.assertTrue(body["thumbnail"].startswith("http://testserver")) + self.assertNotEqual(body["thumbnail"], body["image_original"]) + + def test_person_thumbnail_file_is_generated_at_the_requested_size(self): + """Not just a URL: the file exists at 256x256, so a consumer that fetches + it gets a real image rather than the 404s that made this unworkable.""" + body = self._person_payload(self._member_with_photo()) + with Image.open(self._media_path(body["thumbnail"])) as img: + self.assertEqual(img.size, (256, 256)) + + def test_person_image_original_is_the_raw_upload(self): + person = self._member_with_photo() + body = self._person_payload(person) + self.assertIn(person.image.name, body["image_original"]) + self.assertTrue(body["image_original"].startswith("http://testserver")) + + def test_person_thumbnail_without_crop_box(self): + """An editor who never touched the cropper still gets a sized thumbnail + (ImageRatioField seeds a centered box on save; an empty one is a no-op).""" + person = self._member_with_photo(cropping="") + body = self._person_payload(person) + self.assertIn("256x256", body["thumbnail"]) + + def test_person_without_image_has_null_image_fields(self): + person = self._member_with_photo() + # .update() bypasses Person.save(), whose Star Wars fallback would + # otherwise put an image back. + Person.objects.filter(pk=person.pk).update(image="", cropping="") + body = self._person_payload(person) + self.assertIsNone(body["thumbnail"]) + self.assertIsNone(body["image_original"]) + + def test_project_people_nested_person_carries_cropped_thumbnail(self): + """The roster reads the nested person, so the cropped URL has to be there + too -- otherwise it's a second request per member just for a photo.""" + person = self._member_with_photo() + ProjectRole.objects.create( + person=person, project=self.project, start_date=date(2020, 1, 1) + ) + results = self.client.get( + "/api/v1/projects/projectsidewalk/people/?page_size=100" + ).json()["results"] + record = next(r for r in results if r["person"]["id"] == person.id) + + self.assertIn("256x256", record["person"]["thumbnail"]) + self.assertIn(person.image.name, record["person"]["image_original"]) + + def test_project_thumbnail_is_cropped_derivative(self): + project = self.make_project( + name="Cropped Project", + short_name="croppedproj", + is_visible=True, + gallery_image=png_upload("cropped_proj.png", size=(1600, 1200)), + cropping="0,100,1500,1000", # 15:9, matching Project's crop ratio + ) + body = self.client.get("/api/v1/projects/croppedproj/").json() + + self.assertIn("1000x600", body["thumbnail"]) + self.assertIn("box-0%2C100%2C1500%2C1000", body["thumbnail"]) + self.assertIn(project.gallery_image.name, body["image_original"]) + with Image.open(self._media_path(body["thumbnail"])) as img: + self.assertEqual(img.size, (1000, 600)) + + def test_thumbnail_falls_back_to_original_when_source_is_missing(self): + """A row whose file vanished must not 500 the whole list response.""" + person = self._member_with_photo() + default_storage.delete(person.image.name) + + with self.assertLogs("website.utils.thumbnail_utils", level="WARNING"): + body = self._person_payload(person) + + self.assertEqual(body["thumbnail"], body["image_original"]) + + def test_helper_returns_none_when_source_is_missing(self): + person = self._member_with_photo() + default_storage.delete(person.image.name) + + with self.assertLogs("website.utils.thumbnail_utils", level="WARNING"): + thumbnail = get_cropped_thumbnail( + person.image, API_PERSON_THUMBNAIL_SIZE, person.cropping + ) + self.assertIsNone(thumbnail) + + +class ApiThumbnailAspectTests(SimpleTestCase): + """Guard against re-introducing the double-crop that clipped heads off the + news cards (#1424): crop_corners applies the editor's box, then scale_and_crop + center-crops again if the requested size's aspect ratio disagrees.""" + + def test_api_sizes_match_the_models_crop_ratios(self): + for label, api_size, model_size in ( + ("person", API_PERSON_THUMBNAIL_SIZE, PERSON_THUMBNAIL_SIZE), + ("project", API_PROJECT_THUMBNAIL_SIZE, PROJECT_THUMBNAIL_SIZE), + ): + with self.subTest(label): + self.assertAlmostEqual( + api_size[0] / api_size[1], + model_size[0] / model_size[1], + places=3, + msg=f"API {label} size {api_size} must keep the aspect ratio " + f"of the model's crop box {model_size}", + ) diff --git a/website/tests/test_warm_api_thumbnails.py b/website/tests/test_warm_api_thumbnails.py new file mode 100644 index 00000000..bafa1b26 --- /dev/null +++ b/website/tests/test_warm_api_thumbnails.py @@ -0,0 +1,175 @@ +""" +Tests for the ``warm_api_thumbnails`` management command (#1432). + +The command runs on every container start (docker-entrypoint.sh, step 4.10e) so +the first API request after a deploy doesn't have to generate every cropped +derivative inline. What matters is that it (a) actually produces the files the +API serves, (b) is safe to re-run, and (c) survives a row whose source image is +missing from media/ -- the startup sequence must never die on one bad row. +""" + +import io +import os +import shutil +import tempfile +from datetime import date + +from django.conf import settings +from django.core.files.storage import default_storage +from django.core.files.uploadedfile import SimpleUploadedFile +from django.core.management import call_command +from django.test import override_settings +from PIL import Image + +from website.api.serializers import ( + API_PERSON_THUMBNAIL_SIZE, + API_PROJECT_THUMBNAIL_SIZE, +) +from website.models import Position +from website.models.position import Title +from website.tests.base import DatabaseTestCase +from website.utils.thumbnail_utils import get_cropped_thumbnail + +_TEST_MEDIA_ROOT = tempfile.mkdtemp(prefix="ml_warm_thumbs_") + + +def _png_upload(name, size=(800, 600)): + buffer = io.BytesIO() + Image.new("RGB", size, (10, 120, 200)).save(buffer, format="PNG") + return SimpleUploadedFile(name, buffer.getvalue(), content_type="image/png") + + +@override_settings(MEDIA_ROOT=_TEST_MEDIA_ROOT) +class WarmApiThumbnailsTests(DatabaseTestCase): + @classmethod + def tearDownClass(cls): + shutil.rmtree(_TEST_MEDIA_ROOT, ignore_errors=True) + super().tearDownClass() + + def _member(self, first_name="Warm"): + person = self.make_person( + first_name=first_name, + last_name="Member", + image=_png_upload(f"{first_name.lower()}_member.png"), + cropping="100,100,400,400", + ) + Position.objects.create( + person=person, start_date=date(2020, 1, 1), title=Title.PHD_STUDENT + ) + return person + + def _cached_thumbnail_path(self, image_field, size, box): + """Path of an ALREADY-cached derivative, or None. + + ``generate=False`` matters: asking the generating way would create the + file, and every assertion here would pass whether or not the command + actually did anything. + """ + thumbnail = get_cropped_thumbnail(image_field, size, box, generate=False) + if thumbnail is None: + return None + return os.path.join(settings.MEDIA_ROOT, thumbnail.name) + + def test_warms_member_and_visible_project_thumbnails(self): + person = self._member() + project = self.make_project( + name="Warm Project", + short_name="warmproject", + is_visible=True, + gallery_image=_png_upload("warm_project.png", size=(1600, 1200)), + cropping="0,100,1500,1000", + ) + self.assertIsNone( + self._cached_thumbnail_path( + person.image, API_PERSON_THUMBNAIL_SIZE, person.cropping + ), + "nothing should be cached before the command runs", + ) + + call_command("warm_api_thumbnails") + + person_thumb = self._cached_thumbnail_path( + person.image, API_PERSON_THUMBNAIL_SIZE, person.cropping + ) + project_thumb = self._cached_thumbnail_path( + project.gallery_image, API_PROJECT_THUMBNAIL_SIZE, project.cropping + ) + self.assertIsNotNone(person_thumb) + self.assertIsNotNone(project_thumb) + with Image.open(person_thumb) as img: + self.assertEqual(img.size, API_PERSON_THUMBNAIL_SIZE) + with Image.open(project_thumb) as img: + self.assertEqual(img.size, API_PROJECT_THUMBNAIL_SIZE) + + def test_warms_people_who_are_not_lab_members(self): + """External co-authors have no Position but are still served by the API, + nested as publication ``authors``.""" + coauthor = self.make_person( + first_name="External", + last_name="Coauthor", + image=_png_upload("external_coauthor.png"), + cropping="0,0,400,400", + ) + + call_command("warm_api_thumbnails") + + self.assertIsNotNone( + self._cached_thumbnail_path( + coauthor.image, API_PERSON_THUMBNAIL_SIZE, coauthor.cropping + ) + ) + + def test_project_without_gallery_image_is_not_a_failure(self): + """gallery_image is null=True, and .exclude(field="") keeps NULLs -- an + image-less project must be filtered out, not reported as a failure.""" + self.make_project(name="No Image", short_name="noimage", is_visible=True) + + out = io.StringIO() + call_command("warm_api_thumbnails", stdout=out) + + project_line = next( + line for line in out.getvalue().splitlines() if line.startswith("project") + ) + self.assertIn("0 failed (of 0)", project_line) + + def test_rerun_is_idempotent(self): + """Second run must reuse the cached file, not rewrite it -- this runs on + every container start.""" + person = self._member() + call_command("warm_api_thumbnails") + path = self._cached_thumbnail_path( + person.image, API_PERSON_THUMBNAIL_SIZE, person.cropping + ) + first_mtime = os.path.getmtime(path) + + out = io.StringIO() + call_command("warm_api_thumbnails", stdout=out) + + self.assertEqual(os.path.getmtime(path), first_mtime) + self.assertIn("0 generated", out.getvalue()) + + def test_missing_source_file_does_not_abort_the_run(self): + broken = self._member(first_name="Broken") + default_storage.delete(broken.image.name) + healthy = self._member(first_name="Healthy") + + with self.assertLogs("website.utils.thumbnail_utils", level="WARNING"): + call_command("warm_api_thumbnails") + + self.assertIsNotNone( + self._cached_thumbnail_path( + healthy.image, API_PERSON_THUMBNAIL_SIZE, healthy.cropping + ) + ) + + def test_dry_run_generates_nothing(self): + person = self._member() + call_command("warm_api_thumbnails", "--dry-run") + + # Scoped to this person's own derivatives: MEDIA_ROOT is shared by every + # test in this class, so a directory-wide check would depend on test order. + self.assertIsNone( + self._cached_thumbnail_path( + person.image, API_PERSON_THUMBNAIL_SIZE, person.cropping + ) + ) diff --git a/website/utils/thumbnail_utils.py b/website/utils/thumbnail_utils.py new file mode 100644 index 00000000..7670873c --- /dev/null +++ b/website/utils/thumbnail_utils.py @@ -0,0 +1,78 @@ +""" +Server-side thumbnail generation that honors the editor's crop box. + +Templates get cropped derivatives via ``{% thumbnail img size box=obj.cropping +crop detail upscale %}``, which silently renders an empty ``src`` if anything +goes wrong. Python callers (views, API serializers, management commands) have no +such safety net -- a missing source file raises +``easy_thumbnails.exceptions.InvalidImageFormatError``, and an unset ImageField +raises ``ValueError`` on ``.url``. :func:`get_cropped_thumbnail` is the one place +that handles both, so every non-template caller produces the *same* derivative +(same options, therefore the same filename on disk) as the site's templates. + +Usage:: + + from website.utils.thumbnail_utils import get_cropped_thumbnail + + thumb = get_cropped_thumbnail(person.image, (256, 256), person.cropping) + url = thumb.url if thumb else person.image.url # caller decides the fallback + +**Match the requested size's aspect ratio to the model's ImageRatioField.** +``crop_corners`` applies the editor's box first, then easy-thumbnails' +``scale_and_crop`` resizes the result to ``size`` -- and center-crops a second +time if the aspect ratios disagree. That is what clipped heads off the news +cards in #1424. +""" + +import logging + +from easy_thumbnails.files import get_thumbnailer + +_logger = logging.getLogger(__name__) + + +def get_cropped_thumbnail(image_field, size, box=None, generate=True): + """ + Generate (or fetch the cached) thumbnail of ``image_field`` at ``size``, + applying the ``box`` crop set by an editor via django-image-cropping. + + Args: + image_field: an ImageField/FieldFile (e.g. ``person.image``). May be + empty/unset. + size: ``(width, height)`` for the derivative. Keep the aspect ratio equal + to the model's ImageRatioField -- see the module docstring. + box: the crop box string stored by ``ImageRatioField`` + (``"x1,y1,x2,y2"``), or falsy for no crop. + generate: when False, return the derivative only if it already exists on + disk (``None`` otherwise) instead of rendering it. Use this to ask + "is this one cached yet?" without paying to make it. + + Returns: + An ``easy_thumbnails`` ``ThumbnailFile`` (has ``.url``, ``.width``, ...), + or ``None`` if there is no image or generation failed. Callers decide + what to fall back to; failures are logged, never raised. + """ + if not image_field: + return None + + # Same option set the person/project templates pass, so the derivative + # filename (and thus the cached file) is shared with the rendered site. + options = { + "size": size, + "crop": True, + "upscale": True, + "detail": True, + } + if box: + options["box"] = box + + try: + return get_thumbnailer(image_field).get_thumbnail(options, generate=generate) + except Exception: + _logger.warning( + "Thumbnail generation failed for %s at %s.", + getattr(image_field, "name", image_field), + size, + exc_info=True, + ) + return None diff --git a/website/views/view_project_people.py b/website/views/view_project_people.py index 92e761fc..458f7f22 100644 --- a/website/views/view_project_people.py +++ b/website/views/view_project_people.py @@ -20,21 +20,19 @@ """ import json -import logging +from datetime import date from django.core.serializers.json import DjangoJSONEncoder from django.db.models import Count, Q from django.shortcuts import render -# easy-thumbnails generates the per-person headshot thumbnails server-side. -from easy_thumbnails.files import get_thumbnailer - from website.models import Person, Project, Publication from website.models.position import Position, Title from website.models.publication import PubType -from datetime import date -_logger = logging.getLogger(__name__) +# Generates the per-person headshot thumbnails server-side (easy-thumbnails + +# the editor's crop box). +from website.utils.thumbnail_utils import get_cropped_thumbnail # Matches PERSON_THUMBNAIL_SIZE in website/models/person.py. PERSON_THUMBNAIL_SIZE = (245, 245) @@ -109,25 +107,10 @@ def _build_thumbnail_url(person): if not person.image: return "" - options = { - "size": PERSON_THUMBNAIL_SIZE, - "crop": True, - "upscale": True, - "detail": True, - } - if person.cropping: - options["box"] = person.cropping - - try: - thumbnail = get_thumbnailer(person.image).get_thumbnail(options) - return thumbnail.url if thumbnail else "" - except Exception: - _logger.warning( - "Thumbnail generation failed for %s; falling back to original image.", - person, - exc_info=True, - ) - return person.image.url + thumbnail = get_cropped_thumbnail( + person.image, PERSON_THUMBNAIL_SIZE, person.cropping + ) + return thumbnail.url if thumbnail else person.image.url def _build_project_roles(person, today):