diff --git a/zerver/tests/test_thumbnail.py b/zerver/tests/test_thumbnail.py index affb060d36..9a3cb70975 100644 --- a/zerver/tests/test_thumbnail.py +++ b/zerver/tests/test_thumbnail.py @@ -1,4 +1,6 @@ import re +from collections.abc import Iterator +from contextlib import contextmanager from dataclasses import asdict from io import StringIO from unittest.mock import patch @@ -469,3 +471,136 @@ class TestStoreThumbnail(ZulipTestCase): "zerver.lib.thumbnail.THUMBNAIL_OUTPUT_FORMATS", [still_webp, anim_webp, still_jpeg] ): self.assertEqual(missing_thumbnails(image_attachment), [anim_webp, still_jpeg]) + + +class TestThumbnailRetrieval(ZulipTestCase): + @contextmanager + def mock_formats(self, thumbnail_formats: list[ThumbnailFormat]) -> Iterator[None]: + with ( + patch("zerver.lib.thumbnail.THUMBNAIL_OUTPUT_FORMATS", thumbnail_formats), + patch("zerver.views.upload.THUMBNAIL_OUTPUT_FORMATS", thumbnail_formats), + ): + yield + + def test_get_thumbnail(self) -> None: + assert settings.LOCAL_FILES_DIR + hamlet = self.example_user("hamlet") + self.login_user(hamlet) + + webp_anim = ThumbnailFormat("webp", 100, 75, animated=True) + webp_still = ThumbnailFormat("webp", 100, 75, animated=False) + with self.mock_formats([webp_anim, webp_still]): + with ( + self.captureOnCommitCallbacks(execute=True), + get_test_image_file("animated_unequal_img.gif") as image_file, + ): + json_response = self.assert_json_success( + self.client_post("/json/user_uploads", {"file": image_file}) + ) + path_id = re.sub(r"/user_uploads/", "", json_response["url"]) + + # Image itself is available immediately + response = self.client_get(f"/user_uploads/{path_id}") + self.assertEqual(response.status_code, 200) + self.assertEqual(response.headers["Content-Type"], "image/gif") + + # Exit the block, triggering the thumbnailing worker + + thumbnail_response = self.client_get(f"/user_uploads/thumbnail/{path_id}/{webp_still!s}") + self.assertEqual(thumbnail_response.status_code, 200) + self.assertEqual(thumbnail_response.headers["Content-Type"], "image/webp") + self.assertLess( + int(thumbnail_response.headers["Content-Length"]), + int(response.headers["Content-Length"]), + ) + + animated_response = self.client_get(f"/user_uploads/thumbnail/{path_id}/{webp_anim!s}") + self.assertEqual(animated_response.status_code, 200) + self.assertEqual(animated_response.headers["Content-Type"], "image/webp") + self.assertLess( + int(thumbnail_response.headers["Content-Length"]), + int(animated_response.headers["Content-Length"]), + ) + + # Invalid thumbnail format + response = self.client_get(f"/user_uploads/thumbnail/{path_id}/bogus") + self.assertEqual(response.status_code, 404) + self.assertEqual(response.headers["Content-Type"], "image/png") + + # Format we don't have + response = self.client_get(f"/user_uploads/thumbnail/{path_id}/1x1.png") + self.assertEqual(response.status_code, 404) + self.assertEqual(response.headers["Content-Type"], "image/png") + + # path_id for a non-image + with ( + self.captureOnCommitCallbacks(execute=True), + get_test_image_file("text.txt") as text_file, + ): + json_response = self.assert_json_success( + self.client_post("/json/user_uploads", {"file": text_file}) + ) + text_path_id = re.sub(r"/user_uploads/", "", json_response["url"]) + response = self.client_get(f"/user_uploads/thumbnail/{text_path_id}/{webp_still!s}") + self.assertEqual(response.status_code, 404) + self.assertEqual(response.headers["Content-Type"], "image/png") + + # Shrink the list of formats, and check that we can still get + # the thumbnails that were generated at the time + with self.mock_formats([webp_still]): + response = self.client_get(f"/user_uploads/thumbnail/{path_id}/{webp_still!s}") + self.assertEqual(response.status_code, 200) + + response = self.client_get(f"/user_uploads/thumbnail/{path_id}/{webp_anim!s}") + self.assertEqual(response.status_code, 200) + + # Grow the format list, and check that fetching that new + # format generates all of the missing formats + jpeg_still = ThumbnailFormat("jpg", 100, 75, animated=False) + big_jpeg_still = ThumbnailFormat("jpg", 200, 150, animated=False) + with ( + self.mock_formats([webp_still, jpeg_still, big_jpeg_still]), + patch.object( + pyvips.Image, "thumbnail_buffer", wraps=pyvips.Image.thumbnail_buffer + ) as thumb_mock, + ): + small_response = self.client_get(f"/user_uploads/thumbnail/{path_id}/{jpeg_still!s}") + self.assertEqual(small_response.status_code, 200) + self.assertEqual(small_response.headers["Content-Type"], "image/jpeg") + # This made two thumbnails + self.assertEqual(thumb_mock.call_count, 2) + + thumb_mock.reset_mock() + big_response = self.client_get(f"/user_uploads/thumbnail/{path_id}/{big_jpeg_still!s}") + self.assertEqual(big_response.status_code, 200) + self.assertEqual(big_response.headers["Content-Type"], "image/jpeg") + thumb_mock.assert_not_called() + + self.assertLess( + int(small_response.headers["Content-Length"]), + int(big_response.headers["Content-Length"]), + ) + + # Upload a static image, and verify that we only generate the still versions + with self.mock_formats([webp_anim, webp_still, jpeg_still]): + with ( + self.captureOnCommitCallbacks(execute=True), + get_test_image_file("img.tif") as image_file, + ): + json_response = self.assert_json_success( + self.client_post("/json/user_uploads", {"file": image_file}) + ) + path_id = re.sub(r"/user_uploads/", "", json_response["url"]) + # Exit the block, triggering the thumbnailing worker + + response = self.client_get(f"/user_uploads/thumbnail/{path_id}/{webp_still!s}") + self.assertEqual(response.status_code, 200) + self.assertEqual(response.headers["Content-Type"], "image/webp") + + response = self.client_get(f"/user_uploads/thumbnail/{path_id}/{webp_anim!s}") + self.assertEqual(response.status_code, 404) + self.assertEqual(response.headers["Content-Type"], "image/png") + + response = self.client_get(f"/user_uploads/thumbnail/{path_id}/{jpeg_still!s}") + self.assertEqual(response.status_code, 200) + self.assertEqual(response.headers["Content-Type"], "image/jpeg") diff --git a/zerver/views/upload.py b/zerver/views/upload.py index faf5f85df7..b8fc646e6f 100644 --- a/zerver/views/upload.py +++ b/zerver/views/upload.py @@ -8,6 +8,7 @@ from django.conf import settings from django.contrib.auth.models import AnonymousUser from django.core.files.uploadedfile import UploadedFile from django.core.signing import BadSignature, TimestampSigner +from django.db import transaction from django.http import ( FileResponse, HttpRequest, @@ -29,15 +30,22 @@ from zerver.lib.exceptions import JsonableError from zerver.lib.mime_types import guess_type from zerver.lib.response import json_success from zerver.lib.storage import static_path +from zerver.lib.thumbnail import ( + THUMBNAIL_OUTPUT_FORMATS, + BaseThumbnailFormat, + StoredThumbnailFormat, +) from zerver.lib.upload import ( check_upload_within_quota, + get_image_thumbnail_path, get_public_upload_root_url, upload_message_attachment_from_request, ) from zerver.lib.upload.base import INLINE_MIME_TYPES from zerver.lib.upload.local import assert_is_local_storage_path from zerver.lib.upload.s3 import get_signed_upload_url -from zerver.models import UserProfile +from zerver.models import ImageAttachment, UserProfile +from zerver.worker.thumbnail import ensure_thumbnails def patch_disposition_header(response: HttpResponse, url: str, is_attachment: bool) -> None: @@ -155,8 +163,16 @@ def serve_file_backend( maybe_user_profile: UserProfile | AnonymousUser, realm_id_str: str, filename: str, + thumbnail_format: str | None = None, ) -> HttpResponseBase: - return serve_file(request, maybe_user_profile, realm_id_str, filename, url_only=False) + return serve_file( + request, + maybe_user_profile, + realm_id_str, + filename, + url_only=False, + thumbnail_format=thumbnail_format, + ) def serve_file_url_backend( @@ -192,6 +208,7 @@ def serve_file( maybe_user_profile: UserProfile | AnonymousUser, realm_id_str: str, filename: str, + thumbnail_format: str | None = None, url_only: bool = False, force_download: bool = False, ) -> HttpResponseBase: @@ -227,6 +244,50 @@ def serve_file( url = generate_unauthed_file_access_url(path_id) return json_success(request, data=dict(url=url)) + if thumbnail_format is not None: + # Check if this is something that we thumbnail at all + try: + image_attachment = ImageAttachment.objects.get(path_id=path_id) + except ImageAttachment.DoesNotExist: + return serve_image_error(404, "images/errors/image-not-exist.png") + + # Validate that this is a potential thumbnail format + requested_format = BaseThumbnailFormat.from_string(thumbnail_format) + if requested_format is None: + return serve_image_error(404, "images/errors/image-not-exist.png") + + rendered_formats = [StoredThumbnailFormat(**f) for f in image_attachment.thumbnail_metadata] + + # We never generate animated versions if the input was still, + # so filter those out + if image_attachment.frames == 1: + potential_output_formats = [ + thumbnail_format + for thumbnail_format in THUMBNAIL_OUTPUT_FORMATS + if not thumbnail_format.animated + ] + else: + potential_output_formats = THUMBNAIL_OUTPUT_FORMATS + if requested_format not in potential_output_formats: + if requested_format in rendered_formats: + # Not a _current_ format, but we did render it at the time, so fine to serve + pass + else: + return serve_image_error(404, "images/errors/image-not-exist.png") + elif requested_format not in rendered_formats: + # They requested a valid format, but one we've not + # rendered yet. Take a lock on the row, then render every + # missing format, synchronously. The lock prevents us + # from doing double-work if the background worker is + # currently processing the row. + with transaction.atomic(savepoint=False): + ensure_thumbnails( + ImageAttachment.objects.select_for_update().get(id=image_attachment.id) + ) + + # Update the path that we are fetching to be the thumbnail + path_id = get_image_thumbnail_path(image_attachment, requested_format) + if settings.LOCAL_UPLOADS_DIR is not None: return serve_local(request, path_id, force_download=force_download) else: diff --git a/zerver/worker/thumbnail.py b/zerver/worker/thumbnail.py index 8e8cc05ba6..b887c4672b 100644 --- a/zerver/worker/thumbnail.py +++ b/zerver/worker/thumbnail.py @@ -24,6 +24,11 @@ class ThumbnailWorker(QueueProcessingWorker): start = time.time() with transaction.atomic(savepoint=False): try: + # This lock prevents us from racing with the on-demand + # rendering that can be triggered if a request is made + # directly to a thumbnail URL we have not made yet. + # This may mean that we may generate 0 thumbnail + # images once we get the lock. row = ImageAttachment.objects.select_for_update().get(id=event["id"]) except ImageAttachment.DoesNotExist: # nocoverage logger.info("ImageAttachment row %d missing", event["id"]) diff --git a/zproject/urls.py b/zproject/urls.py index a7bed3f157..e24ec43036 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -657,6 +657,10 @@ urls += [ "user_uploads/download//", GET=(serve_file_download_backend, {"override_api_url_scheme", "allow_anonymous_user_web"}), ), + rest_path( + "user_uploads/thumbnail///", + GET=(serve_file_backend, {"override_api_url_scheme", "allow_anonymous_user_web"}), + ), rest_path( "user_uploads//", GET=(serve_file_backend, {"override_api_url_scheme", "allow_anonymous_user_web"}),