Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Provide more info why we don't have any thumbnails to serve #13038

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/13038.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Provide more info why we don't have any thumbnails to serve.
25 changes: 20 additions & 5 deletions synapse/config/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,14 @@
# method: %(method)s
"""

THUMBNAIL_SUPPORTED_MEDIA_FORMAT_MAP = {
"image/jpeg": "jpeg",
"image/jpg": "jpeg",
"image/webp": "jpeg",
"image/gif": "png",
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
"image/png": "png",
}
richvdh marked this conversation as resolved.
Show resolved Hide resolved

HTTP_PROXY_SET_WARNING = """\
The Synapse config url_preview_ip_range_blacklist will be ignored as an HTTP(s) proxy is configured."""

Expand Down Expand Up @@ -81,11 +89,18 @@ def parse_thumbnail_requirements(
method = size["method"]
jpeg_thumbnail = ThumbnailRequirement(width, height, method, "image/jpeg")
png_thumbnail = ThumbnailRequirement(width, height, method, "image/png")
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
requirements.setdefault("image/jpeg", []).append(jpeg_thumbnail)
requirements.setdefault("image/jpg", []).append(jpeg_thumbnail)
requirements.setdefault("image/webp", []).append(jpeg_thumbnail)
requirements.setdefault("image/gif", []).append(png_thumbnail)
requirements.setdefault("image/png", []).append(png_thumbnail)

for format, thumbnail_format in THUMBNAIL_SUPPORTED_MEDIA_FORMAT_MAP.items():
requirement = requirements.setdefault(format, [])
if thumbnail_format == "jpeg":
requirement.append(jpeg_thumbnail)
elif thumbnail_format == "png":
requirement.append(png_thumbnail)
else:
raise Exception(
"Unknown thumbnail mapping from %s to %s. This is a Synapse problem, please report!"
% (format, thumbnail_format)
)
return {
media_type: tuple(thumbnails) for media_type, thumbnails in requirements.items()
}
Expand Down
51 changes: 48 additions & 3 deletions synapse/rest/media/v1/thumbnail_resource.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,16 @@
import logging
from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple

from synapse.api.errors import SynapseError
from synapse.http.server import DirectServeJsonResource, set_cors_headers
from synapse.api.errors import Codes, SynapseError, cs_error
from synapse.http.server import (
DirectServeJsonResource,
set_cors_headers,
respond_with_json,
)
from synapse.http.servlet import parse_integer, parse_string
from synapse.http.site import SynapseRequest
from synapse.rest.media.v1.media_storage import MediaStorage
from synapse.config.repository import THUMBNAIL_SUPPORTED_MEDIA_FORMAT_MAP

from ._base import (
FileInfo,
Expand Down Expand Up @@ -304,6 +309,14 @@ async def _select_and_respond_with_thumbnail(
url_cache: True if this is from a URL cache.
server_name: The server name, if this is a remote thumbnail.
"""
logger.debug(
"_select_and_respond_with_thumbnail: media_id=%s desired=%sx%s (%s) thumbnail_infos=%s",
media_id,
desired_width,
desired_height,
desired_method,
thumbnail_infos,
)
if thumbnail_infos:
file_info = self._select_thumbnail(
desired_width,
Expand Down Expand Up @@ -379,8 +392,40 @@ async def _select_and_respond_with_thumbnail(
file_info.thumbnail.length,
)
else:
# This might be because:
# 1. We can't create thumbnails for the given media (corrupted or
# unsupported file type)
# 2. The thumbnailing process never ran or errored out initially
# when the media was first uploaded (these bugs should be
# reported and fixed).
# 3. `dynamic_thumbnails` is disabled (see `homeserver.yaml`) and we
# can't try to generate one. If this was enabled, Synapse would
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
# go down a different code path.
logger.info("Failed to find any generated thumbnails")
respond_404(request)

# We could get away with putting this error text directly in the
# error below because it never hits this code path otherwise when
# `dynamic_thumbnails` is disabled. But it would be good not to
# always assume this will be the case in the future so we provide
# accurate information to the user.
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
dynamic_thumbnails_disabled_warning_text = ""
if not self.dynamic_thumbnails:
dynamic_thumbnails_disabled_warning_text = " We also cannot try to generate a new thumbnail because `dynamic_thumbnails` is disabled (see `homeserver.yaml`)."
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved

respond_with_json(
request,
400,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still think this should be a 404 and M_NOT_FOUND.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec says no right now, matrix-org/matrix-spec#1122

It does make sense to me in some cases:

400 makes a lot of sense for all of the scenarios where the media repository can't provide a thumbnail for whatever reason when the media exists.

But 404 probably makes more sense when mediaId isn't valid and doesn't exist.

In the cases I ran into, 400 makes more sense since the media did exist, Synapse just didn't thumbnail it.

cs_error(
"Cannot find any thumbnails for the requested media (%r). This might mean the media is not a supported_media_format=(%s) or your media is corrupted and cannot be thumbnailed.%s"
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
% (
request.postpath,
", ".join(THUMBNAIL_SUPPORTED_MEDIA_FORMAT_MAP.keys()),
dynamic_thumbnails_disabled_warning_text,
),
code=Codes.UNKNOWN,
),
send_cors=True,
)

def _select_thumbnail(
self,
Expand Down
54 changes: 47 additions & 7 deletions tests/rest/media/v1/test_media_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ class _TestImage:
expected_cropped: Optional[bytes] = None
expected_scaled: Optional[bytes] = None
expected_found: bool = True
unable_to_thumbnail: bool = False
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved


@parameterized_class(
Expand Down Expand Up @@ -190,6 +191,7 @@ class _TestImage:
b"image/gif",
b".gif",
expected_found=False,
unable_to_thumbnail=True,
),
),
],
Expand Down Expand Up @@ -364,18 +366,29 @@ def test_disposition_none(self) -> None:
def test_thumbnail_crop(self) -> None:
"""Test that a cropped remote thumbnail is available."""
self._test_thumbnail(
"crop", self.test_image.expected_cropped, self.test_image.expected_found
"crop",
self.test_image.expected_cropped,
expected_found=self.test_image.expected_found,
unable_to_thumbnail=self.test_image.unable_to_thumbnail,
)

def test_thumbnail_scale(self) -> None:
"""Test that a scaled remote thumbnail is available."""
self._test_thumbnail(
"scale", self.test_image.expected_scaled, self.test_image.expected_found
"scale",
self.test_image.expected_scaled,
expected_found=self.test_image.expected_found,
unable_to_thumbnail=self.test_image.unable_to_thumbnail,
)

def test_invalid_type(self) -> None:
"""An invalid thumbnail type is never available."""
self._test_thumbnail("invalid", None, False)
self._test_thumbnail(
"invalid",
None,
expected_found=False,
unable_to_thumbnail=self.test_image.unable_to_thumbnail,
)

@unittest.override_config(
{"thumbnail_sizes": [{"width": 32, "height": 32, "method": "scale"}]}
Expand All @@ -384,7 +397,12 @@ def test_no_thumbnail_crop(self) -> None:
"""
Override the config to generate only scaled thumbnails, but request a cropped one.
"""
self._test_thumbnail("crop", None, False)
self._test_thumbnail(
"crop",
None,
expected_found=False,
unable_to_thumbnail=self.test_image.unable_to_thumbnail,
)

@unittest.override_config(
{"thumbnail_sizes": [{"width": 32, "height": 32, "method": "crop"}]}
Expand All @@ -393,14 +411,22 @@ def test_no_thumbnail_scale(self) -> None:
"""
Override the config to generate only cropped thumbnails, but request a scaled one.
"""
self._test_thumbnail("scale", None, False)
self._test_thumbnail(
"scale",
None,
expected_found=False,
unable_to_thumbnail=self.test_image.unable_to_thumbnail,
)

def test_thumbnail_repeated_thumbnail(self) -> None:
"""Test that fetching the same thumbnail works, and deleting the on disk
thumbnail regenerates it.
"""
self._test_thumbnail(
"scale", self.test_image.expected_scaled, self.test_image.expected_found
"scale",
self.test_image.expected_scaled,
expected_found=self.test_image.expected_found,
unable_to_thumbnail=self.test_image.unable_to_thumbnail,
)

if not self.test_image.expected_found:
Expand Down Expand Up @@ -457,7 +483,11 @@ def test_thumbnail_repeated_thumbnail(self) -> None:
)

def _test_thumbnail(
self, method: str, expected_body: Optional[bytes], expected_found: bool
self,
method: str,
expected_body: Optional[bytes],
expected_found: bool,
unable_to_thumbnail: bool = False,
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
) -> None:
params = "?width=32&height=32&method=" + method
channel = make_request(
Expand Down Expand Up @@ -488,6 +518,16 @@ def _test_thumbnail(
else:
# ensure that the result is at least some valid image
Image.open(BytesIO(channel.result["body"]))
elif unable_to_thumbnail:
# A 400 with a JSON body.
self.assertEqual(channel.code, 400)
self.assertEqual(
channel.json_body,
{
"errcode": "M_UNKNOWN",
"error": "Cannot find any thumbnails for the requested media ([b'example.com', b'12345']). This might mean the media is not a supported_media_format=(image/jpeg, image/jpg, image/webp, image/gif, image/png) or your media is corrupted and cannot be thumbnailed. We also cannot try to generate a new thumbnail because `dynamic_thumbnails` is disabled (see `homeserver.yaml`).",
},
)
else:
# A 404 with a JSON body.
self.assertEqual(channel.code, 404)
Expand Down