From b263db5651d68256f544101083fc8c4ed160040d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 1 Oct 2020 15:53:02 +0100 Subject: [PATCH 1/4] fix remote thumbnails? --- synapse/rest/media/v1/media_storage.py | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/synapse/rest/media/v1/media_storage.py b/synapse/rest/media/v1/media_storage.py index 5681677fc93d..4827cb2a3f5e 100644 --- a/synapse/rest/media/v1/media_storage.py +++ b/synapse/rest/media/v1/media_storage.py @@ -143,12 +143,9 @@ async def fetch_media(self, file_info: FileInfo) -> Optional[Responder]: """ 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")) - # Fallback for paths without method names - # Should be removed in the future + # fallback for remote thumbnails with no method in the filename + legacy_path = None if file_info.thumbnail and file_info.server_name: legacy_path = self.filepaths.remote_media_thumbnail_rel_legacy( server_name=file_info.server_name, @@ -157,8 +154,19 @@ async def fetch_media(self, file_info: FileInfo) -> Optional[Responder]: height=file_info.thumbnail_height, content_type=file_info.thumbnail_type, ) + + 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")) + + if legacy_path: + logger.debug( + "local file %s did not exist; checking legacy name", local_path + ) legacy_local_path = os.path.join(self.local_media_directory, legacy_path) if os.path.exists(legacy_local_path): + logger.debug("responding with local file %s", legacy_local_path) return FileResponder(open(legacy_local_path, "rb")) for provider in self.storage_providers: @@ -166,6 +174,14 @@ async def fetch_media(self, file_info: FileInfo) -> Optional[Responder]: if res: logger.debug("Streaming %s from %s", path, provider) return res + if legacy_path: + logger.debug( + "Provider %s did not find %s; checking legacy name", provider, path + ) + res = await provider.fetch(legacy_path, file_info) + if res: + logger.debug("Streaming %s from %s", legacy_path, provider) + return res return None From bb69bb1a31b06ba5cd94742d88c4f77bd5d8c7f3 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 1 Oct 2020 16:04:17 +0100 Subject: [PATCH 2/4] changelog --- changelog.d/8438.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/8438.bugfix diff --git a/changelog.d/8438.bugfix b/changelog.d/8438.bugfix new file mode 100644 index 000000000000..3edc394149d3 --- /dev/null +++ b/changelog.d/8438.bugfix @@ -0,0 +1 @@ +Fix a regression in v1.21.0rc1 which broke thumbnails of remote media. From 399a7500ffca2b5a7440b52aa0f6fa6542ce2e18 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 1 Oct 2020 16:13:00 +0100 Subject: [PATCH 3/4] improve logging --- synapse/rest/media/v1/media_storage.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/synapse/rest/media/v1/media_storage.py b/synapse/rest/media/v1/media_storage.py index 4827cb2a3f5e..dd53c6f0edbd 100644 --- a/synapse/rest/media/v1/media_storage.py +++ b/synapse/rest/media/v1/media_storage.py @@ -159,29 +159,27 @@ async def fetch_media(self, file_info: FileInfo) -> Optional[Responder]: 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) if legacy_path: - logger.debug( - "local file %s did not exist; checking legacy name", local_path - ) legacy_local_path = os.path.join(self.local_media_directory, legacy_path) if os.path.exists(legacy_local_path): logger.debug("responding with local file %s", legacy_local_path) return FileResponder(open(legacy_local_path, "rb")) + logger.debug("legacy local file %s did not exist", legacy_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 + logger.debug("%s not found on %s", path, provider) if legacy_path: - logger.debug( - "Provider %s did not find %s; checking legacy name", provider, path - ) res = await provider.fetch(legacy_path, file_info) if res: logger.debug("Streaming %s from %s", legacy_path, provider) return res + logger.debug("%s not found on %s", legacy_path, provider) return None From 3db974f5164726441248f6cbd72b0759b40f4176 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 2 Oct 2020 12:00:32 +0100 Subject: [PATCH 4/4] dedup some code --- synapse/rest/media/v1/media_storage.py | 49 ++++++++++---------------- 1 file changed, 19 insertions(+), 30 deletions(-) diff --git a/synapse/rest/media/v1/media_storage.py b/synapse/rest/media/v1/media_storage.py index dd53c6f0edbd..a9586fb0b73d 100644 --- a/synapse/rest/media/v1/media_storage.py +++ b/synapse/rest/media/v1/media_storage.py @@ -141,45 +141,34 @@ async def fetch_media(self, file_info: FileInfo) -> Optional[Responder]: Returns: Returns a Responder if the file was found, otherwise None. """ - - path = self._file_info_to_path(file_info) + paths = [self._file_info_to_path(file_info)] # fallback for remote thumbnails with no method in the filename - legacy_path = None 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, + ) ) - 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) - - if legacy_path: - legacy_local_path = os.path.join(self.local_media_directory, legacy_path) - if os.path.exists(legacy_local_path): - logger.debug("responding with local file %s", legacy_local_path) - return FileResponder(open(legacy_local_path, "rb")) - logger.debug("legacy local file %s did not exist", legacy_local_path) + 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 - logger.debug("%s not found on %s", path, provider) - if legacy_path: - res = await provider.fetch(legacy_path, file_info) + for path in paths: + res = await provider.fetch(path, file_info) # type: Any if res: - logger.debug("Streaming %s from %s", legacy_path, provider) + logger.debug("Streaming %s from %s", path, provider) return res - logger.debug("%s not found on %s", legacy_path, provider) + logger.debug("%s not found on %s", path, provider) return None