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

Fix bug in remote thumbnail search #8438

Merged
merged 4 commits into from
Oct 2, 2020
Merged

Fix bug in remote thumbnail search #8438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b263db5
fix remote thumbnails?
richvdh Oct 1, 2020
bb69bb1
changelog
richvdh Oct 1, 2020
399a750
improve logging
richvdh Oct 1, 2020
3db974f
dedup some code
richvdh Oct 2, 2020
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/8438.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a regression in v1.21.0rc1 which broke thumbnails of remote media.
43 changes: 23 additions & 20 deletions synapse/rest/media/v1/media_storage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,31 +141,34 @@ async def fetch_media(self, file_info: FileInfo) -> Optional[Responder]:
Returns:
Returns a Responder if the file was found, otherwise None.
"""
paths = [self._file_info_to_path(file_info)]

path = self._file_info_to_path(file_info)
local_path = os.path.join(self.local_media_directory, path)
if os.path.exists(local_path):
return FileResponder(open(local_path, "rb"))
Comment on lines -146 to -148
Copy link
Member Author

Choose a reason for hiding this comment

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

this is just moved down by a few lines.

Copy link
Member

Choose a reason for hiding this comment

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

In general I wonder if a bunch of this code could be de-duplicated and simplified by generating a set of paths to check (the "correct" path and maybe the legacy one) something like:

paths = [local_path]
if legacy_path:
    paths.append(legacy_path)

for path in paths:
    # Check if exists in local media dir

for path in paths:
    for provider in self.storage_providers:
        # Check if can fetch from provider

🤷 There's potentially a couple ways to organize those loops and such too.

Overall this seems like a fine change though I do wonder if it makes sense to check each provider for the "correct" file name first, then the legacy one or to iterate through each provider (as the PR has now). I think we only expect one match ever, so it shouldn't really matter?

Copy link
Member Author

Choose a reason for hiding this comment

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

I do wonder if it makes sense to check each provider for the "correct" file name first, then the legacy one or to iterate through each provider (as the PR has now).

The current implementation means that we can check all local stores first, before making any S3 requests. Flipping the order around would mean adding the delay of an S3 lookup for a file that is stored in the local system with a legacy name.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've deduplicated the code a bit.

Copy link
Member

Choose a reason for hiding this comment

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

I find the logic much easier to follow now! Thanks. 👍


# Fallback for paths without method names
# Should be removed in the future
# fallback for remote thumbnails with no method in the filename
if file_info.thumbnail and file_info.server_name:
legacy_path = self.filepaths.remote_media_thumbnail_rel_legacy(
server_name=file_info.server_name,
file_id=file_info.file_id,
width=file_info.thumbnail_width,
height=file_info.thumbnail_height,
content_type=file_info.thumbnail_type,
paths.append(
self.filepaths.remote_media_thumbnail_rel_legacy(
server_name=file_info.server_name,
file_id=file_info.file_id,
width=file_info.thumbnail_width,
height=file_info.thumbnail_height,
content_type=file_info.thumbnail_type,
)
)
legacy_local_path = os.path.join(self.local_media_directory, legacy_path)
if os.path.exists(legacy_local_path):
return FileResponder(open(legacy_local_path, "rb"))

for path in paths:
local_path = os.path.join(self.local_media_directory, path)
if os.path.exists(local_path):
logger.debug("responding with local file %s", local_path)
return FileResponder(open(local_path, "rb"))
logger.debug("local file %s did not exist", local_path)

for provider in self.storage_providers:
res = await provider.fetch(path, file_info) # type: Any
if res:
logger.debug("Streaming %s from %s", path, provider)
return res
for path in paths:
res = await provider.fetch(path, file_info) # type: Any
if res:
logger.debug("Streaming %s from %s", path, provider)
return res
logger.debug("%s not found on %s", path, provider)

return None

Expand Down