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

Regenerate exact thumbnails if missing #9438

Merged
merged 4 commits into from
Feb 19, 2021
Merged
Show file tree
Hide file tree
Changes from all 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/9438.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add support for regenerating thumbnails if they have been deleted but the original image is still stored.
2 changes: 1 addition & 1 deletion synapse/rest/media/v1/media_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ async def generate_local_exact_thumbnail(
t_height: int,
t_method: str,
t_type: str,
url_cache: str,
url_cache: Optional[str],
) -> Optional[str]:
input_path = await self.media_storage.ensure_media_is_in_local_cache(
FileInfo(None, media_id, url_cache=url_cache)
Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/media/v1/media_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ async def ensure_media_is_in_local_cache(self, file_info: FileInfo) -> str:
await consumer.wait()
return local_path

raise Exception("file could not be found")
raise NotFoundError()

def _file_info_to_path(self, file_info: FileInfo) -> str:
"""Converts file_info into a relative path.
Expand Down
56 changes: 55 additions & 1 deletion synapse/rest/media/v1/thumbnail_resource.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ async def _respond_local_thumbnail(
m_type,
thumbnail_infos,
media_id,
media_id,
url_cache=media_info["url_cache"],
server_name=None,
)
Expand Down Expand Up @@ -269,6 +270,7 @@ async def _respond_remote_thumbnail(
method,
m_type,
thumbnail_infos,
media_id,
media_info["filesystem_id"],
url_cache=None,
server_name=server_name,
Expand All @@ -282,6 +284,7 @@ async def _select_and_respond_with_thumbnail(
desired_method: str,
desired_type: str,
thumbnail_infos: List[Dict[str, Any]],
media_id: str,
file_id: str,
url_cache: Optional[str] = None,
server_name: Optional[str] = None,
Expand Down Expand Up @@ -316,9 +319,60 @@ async def _select_and_respond_with_thumbnail(
respond_404(request)
return

responder = await self.media_storage.fetch_media(file_info)
if responder:
await respond_with_responder(
request,
responder,
file_info.thumbnail_type,
file_info.thumbnail_length,
)
return

# If we can't find the thumbnail we regenerate it. This can happen
# if e.g. we've deleted the thumbnails but still have the original
# image somewhere.
clokep marked this conversation as resolved.
Show resolved Hide resolved
#
# Since we have an entry for the thumbnail in the DB we a) know we
# have have successfully generated the thumbnail in the past (so we
# don't need to worry about repeatedly failing to generate
# thumbnails), and b) have already calculated that appropriate
# width/height/method so we can just call the "generate exact"
# methods.

# First let's check that we do actually have the original image
# still. This will throw a 404 if we don't.
# TODO: We should refetch the thumbnails for remote media.
await self.media_storage.ensure_media_is_in_local_cache(
FileInfo(server_name, file_id, url_cache=url_cache)
)
Comment on lines +343 to +348
Copy link
Member

Choose a reason for hiding this comment

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

Ah, this is actually the first line of generate_remote_exact_thumbnail and generate_local_exact_thumbnail 🤦 so maybe we don't need to check this again?

Copy link
Member Author

Choose a reason for hiding this comment

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

The second time it gets called is fast (just a check if the path exists), so I think its better to be explicit by keeping this here (and I think its good to check again in the functions to avoid foot guns)


if server_name:
await self.media_repo.generate_remote_exact_thumbnail(
server_name,
file_id=file_id,
media_id=media_id,
t_width=file_info.thumbnail_width,
t_height=file_info.thumbnail_height,
t_method=file_info.thumbnail_method,
t_type=file_info.thumbnail_type,
)
else:
await self.media_repo.generate_local_exact_thumbnail(
media_id=media_id,
t_width=file_info.thumbnail_width,
t_height=file_info.thumbnail_height,
t_method=file_info.thumbnail_method,
t_type=file_info.thumbnail_type,
url_cache=url_cache,
)

responder = await self.media_storage.fetch_media(file_info)
await respond_with_responder(
request, responder, file_info.thumbnail_type, file_info.thumbnail_length
request,
responder,
file_info.thumbnail_type,
file_info.thumbnail_length,
)
else:
logger.info("Failed to find any generated thumbnails")
Expand Down
18 changes: 9 additions & 9 deletions synapse/storage/databases/main/media_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -344,16 +344,16 @@ async def store_local_thumbnail(
thumbnail_method,
thumbnail_length,
):
await self.db_pool.simple_insert(
"local_media_repository_thumbnails",
{
await self.db_pool.simple_upsert(
table="local_media_repository_thumbnails",
keyvalues={
"media_id": media_id,
"thumbnail_width": thumbnail_width,
"thumbnail_height": thumbnail_height,
"thumbnail_method": thumbnail_method,
"thumbnail_type": thumbnail_type,
"thumbnail_length": thumbnail_length,
},
values={"thumbnail_length": thumbnail_length},
desc="store_local_thumbnail",
)

Expand Down Expand Up @@ -498,18 +498,18 @@ async def store_remote_media_thumbnail(
thumbnail_method,
thumbnail_length,
):
await self.db_pool.simple_insert(
"remote_media_cache_thumbnails",
{
await self.db_pool.simple_upsert(
table="remote_media_cache_thumbnails",
keyvalues={
"media_origin": origin,
"media_id": media_id,
"thumbnail_width": thumbnail_width,
"thumbnail_height": thumbnail_height,
"thumbnail_method": thumbnail_method,
"thumbnail_type": thumbnail_type,
"thumbnail_length": thumbnail_length,
"filesystem_id": filesystem_id,
},
values={"thumbnail_length": thumbnail_length},
insertion_values={"filesystem_id": filesystem_id},
desc="store_remote_media_thumbnail",
)

Expand Down
69 changes: 66 additions & 3 deletions tests/rest/media/v1/test_media_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -231,9 +231,11 @@ def write_to(r):

def prepare(self, reactor, clock, hs):

self.media_repo = hs.get_media_repository_resource()
self.download_resource = self.media_repo.children[b"download"]
self.thumbnail_resource = self.media_repo.children[b"thumbnail"]
media_resource = hs.get_media_repository_resource()
self.download_resource = media_resource.children[b"download"]
self.thumbnail_resource = media_resource.children[b"thumbnail"]
self.store = hs.get_datastore()
self.media_repo = hs.get_media_repository()

self.media_id = "example.com/12345"

Expand Down Expand Up @@ -357,6 +359,67 @@ def test_no_thumbnail_scale(self):
"""
self._test_thumbnail("scale", None, False)

def test_thumbnail_repeated_thumbnail(self):
"""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
)

if not self.test_image.expected_found:
return

# Fetching again should work, without re-requesting the image from the
# remote.
params = "?width=32&height=32&method=scale"
channel = make_request(
self.reactor,
FakeSite(self.thumbnail_resource),
"GET",
self.media_id + params,
shorthand=False,
await_result=False,
)
self.pump()

self.assertEqual(channel.code, 200)
if self.test_image.expected_scaled:
self.assertEqual(
channel.result["body"],
self.test_image.expected_scaled,
channel.result["body"],
)

# Deleting the thumbnail on disk then re-requesting it should work as
# Synapse should regenerate missing thumbnails.
origin, media_id = self.media_id.split("/")
info = self.get_success(self.store.get_cached_remote_media(origin, media_id))
file_id = info["filesystem_id"]

thumbnail_dir = self.media_repo.filepaths.remote_media_thumbnail_dir(
origin, file_id
)
shutil.rmtree(thumbnail_dir, ignore_errors=True)

channel = make_request(
self.reactor,
FakeSite(self.thumbnail_resource),
"GET",
self.media_id + params,
shorthand=False,
await_result=False,
)
self.pump()

self.assertEqual(channel.code, 200)
if self.test_image.expected_scaled:
self.assertEqual(
channel.result["body"],
self.test_image.expected_scaled,
channel.result["body"],
)

def _test_thumbnail(self, method, expected_body, expected_found):
params = "?width=32&height=32&method=" + method
channel = make_request(
Expand Down