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

Fix thumbnailing remote files #2789

Merged
merged 9 commits into from
Jan 17, 2018
Merged

Fix thumbnailing remote files #2789

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a4c5e4a
Fix thumbnailing remote files
erikjohnston Jan 16, 2018
c5b589f
Log when we respond with 404
erikjohnston Jan 16, 2018
9795b9e
Correctly use server_name/file_id when generating/fetching remote thu…
erikjohnston Jan 16, 2018
307f88d
Fix up log lines
erikjohnston Jan 16, 2018
5dfc837
Fix typo
erikjohnston Jan 16, 2018
0a90d9e
Move setting of file_id up to caller
erikjohnston Jan 16, 2018
6368e5c
Change _generate_thumbnails to take media_type
erikjohnston Jan 16, 2018
d863f68
Use local vars
erikjohnston Jan 16, 2018
d728c47
Add docstring
erikjohnston Jan 17, 2018
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
7 changes: 5 additions & 2 deletions synapse/rest/media/v1/media_repository.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,10 @@ def _generate_thumbnails(self, server_name, media_id, media_info, url_cache=Fals
Deferred[dict]: Dict with "width" and "height" keys of original image
"""
media_type = media_info["media_type"]
file_id = media_info.get("filesystem_id")
if server_name:
file_id = media_info["filesystem_id"]
else:
file_id = media_id
Copy link
Member

Choose a reason for hiding this comment

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

the fact that media_info["filesystem_id"] and media_id give different values feels like a red flag to me. why doesn't media_info["filesystem_id"] work for the local case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mainly because we handle local and remote media differently in the DB, and so it returns different things. I think what has happened is that the local/remote code paths were almost entirely separate and have slowly converged over time, making this a bit strange.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. I've now had a bit of a look at how this works. Taking a dict which can take one of two completely unspecified shapes is a horrible API. media_info is only used to pass the media_type and the file_id. Why don't we make both of those explicit parameters?

Copy link
Member

Choose a reason for hiding this comment

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

[Note to self: this is effectively reinstating the condition at https://github.com//pull/2767/files#diff-043e4890d840f3862222c34adf3a9459L476, which constructed the local filename from either file_id or media_id. We now always use file_id so need to get it right]

requirements = self._get_thumbnail_requirements(media_type)
if not requirements:
return
Expand Down Expand Up @@ -597,7 +600,7 @@ def _generate_thumbnails(self, server_name, media_id, media_info, url_cache=Fals
try:
file_info = FileInfo(
server_name=server_name,
file_id=media_id,
file_id=file_id,
thumbnail=True,
thumbnail_width=t_width,
thumbnail_height=t_height,
Expand Down
6 changes: 3 additions & 3 deletions synapse/rest/media/v1/thumbnail_resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ def _select_or_generate_remote_thumbnail(self, request, server_name, media_id,

if t_w and t_h and t_method and t_type:
file_info = FileInfo(
server_name=None, file_id=media_id,
server_name=server_name, file_id=media_info["filesystem_id"],
thumbnail=True,
thumbnail_width=info["thumbnail_width"],
thumbnail_height=info["thumbnail_height"],
Expand Down Expand Up @@ -221,7 +221,7 @@ def _respond_remote_thumbnail(self, request, server_name, media_id, width,
# TODO: Don't download the whole remote file
# We should proxy the thumbnail from the remote server instead of
# downloading the remote file and generating our own thumbnails.
yield self.media_repo.get_remote_media_info(server_name, media_id)
media_info = yield self.media_repo.get_remote_media_info(server_name, media_id)

thumbnail_infos = yield self.store.get_remote_media_thumbnails(
server_name, media_id,
Expand All @@ -232,7 +232,7 @@ def _respond_remote_thumbnail(self, request, server_name, media_id, width,
width, height, method, m_type, thumbnail_infos
)
file_info = FileInfo(
server_name=None, file_id=media_id,
server_name=server_name, file_id=media_info["filesystem_id"],
thumbnail=True,
thumbnail_width=thumbnail_info["thumbnail_width"],
thumbnail_height=thumbnail_info["thumbnail_height"],
Expand Down