From 7d858078b693ebc5f85b6c05e894551eb3f69ad1 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Tue, 24 Aug 2021 14:51:19 +0100 Subject: [PATCH 1/7] Add some type annotations to `ThumbnailResource`'s `_select_thumbnail` Signed-off-by: Sean Quah --- synapse/rest/media/v1/thumbnail_resource.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/rest/media/v1/thumbnail_resource.py b/synapse/rest/media/v1/thumbnail_resource.py index a029d426f0b6..45eff30b3e7a 100644 --- a/synapse/rest/media/v1/thumbnail_resource.py +++ b/synapse/rest/media/v1/thumbnail_resource.py @@ -15,7 +15,7 @@ import logging -from typing import TYPE_CHECKING, Any, Dict, List, Optional +from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple from twisted.web.server import Request @@ -414,9 +414,9 @@ def _select_thumbnail( if desired_method == "crop": # Thumbnails that match equal or larger sizes of desired width/height. - crop_info_list = [] + crop_info_list: List[Tuple[int, int, int, bool, int, Dict[str, Any]]] = [] # Other thumbnails. - crop_info_list2 = [] + crop_info_list2: List[Tuple[int, int, int, bool, int, Dict[str, Any]]] = [] for info in thumbnail_infos: # Skip thumbnails generated with different methods. if info["thumbnail_method"] != "crop": @@ -457,9 +457,9 @@ def _select_thumbnail( thumbnail_info = min(crop_info_list2)[-1] elif desired_method == "scale": # Thumbnails that match equal or larger sizes of desired width/height. - info_list = [] + info_list: List[Tuple[int, bool, int, Dict[str, Any]]] = [] # Other thumbnails. - info_list2 = [] + info_list2: List[Tuple[int, bool, int, Dict[str, Any]]] = [] for info in thumbnail_infos: # Skip thumbnails generated with different methods. From 0a395e66b0758e5010d998f68ed904ce5713fcf9 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Tue, 24 Aug 2021 14:53:13 +0100 Subject: [PATCH 2/7] Fix error when selecting between thumbnails with the same quality Signed-off-by: Sean Quah --- synapse/rest/media/v1/thumbnail_resource.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/synapse/rest/media/v1/thumbnail_resource.py b/synapse/rest/media/v1/thumbnail_resource.py index 45eff30b3e7a..12bd745cb21c 100644 --- a/synapse/rest/media/v1/thumbnail_resource.py +++ b/synapse/rest/media/v1/thumbnail_resource.py @@ -451,10 +451,14 @@ def _select_thumbnail( info, ) ) + # Pick the most appropriate thumbnail. Some values of `desired_width` and + # `desired_height` may result in a tie, in which case we avoid comparing on + # the thumbnail info dictionary and pick the thumbnail that appears earlier + # in the list of candidates. if crop_info_list: - thumbnail_info = min(crop_info_list)[-1] + thumbnail_info = min(crop_info_list, key=lambda t: t[:-1])[-1] elif crop_info_list2: - thumbnail_info = min(crop_info_list2)[-1] + thumbnail_info = min(crop_info_list2, key=lambda t: t[:-1])[-1] elif desired_method == "scale": # Thumbnails that match equal or larger sizes of desired width/height. info_list: List[Tuple[int, bool, int, Dict[str, Any]]] = [] @@ -477,10 +481,14 @@ def _select_thumbnail( info_list2.append( (size_quality, type_quality, length_quality, info) ) + # Pick the most appropriate thumbnail. Some values of `desired_width` and + # `desired_height` may result in a tie, in which case we avoid comparing on + # the thumbnail info dictionary and pick the thumbnail that appears earlier + # in the list of candidates. if info_list: - thumbnail_info = min(info_list)[-1] + thumbnail_info = min(info_list, key=lambda t: t[:-1])[-1] elif info_list2: - thumbnail_info = min(info_list2)[-1] + thumbnail_info = min(info_list2, key=lambda t: t[:-1])[-1] if thumbnail_info: return FileInfo( From 23cd48ad750f7d14a9046b6f6fbcebd36e6a6228 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Tue, 24 Aug 2021 19:05:59 +0100 Subject: [PATCH 3/7] Add test for selecting between thumbnails with the same quality Signed-off-by: Sean Quah --- tests/rest/media/v1/test_media_storage.py | 39 ++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py index 6085444b9da8..dc959c0cf7d5 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -21,7 +21,7 @@ from urllib import parse import attr -from parameterized import parameterized_class +from parameterized import parameterized, parameterized_class from PIL import Image as Image from twisted.internet import defer @@ -473,6 +473,43 @@ def _test_thumbnail(self, method, expected_body, expected_found): }, ) + @parameterized.expand([("crop", 16), ("crop", 64), ("scale", 16), ("scale", 64)]) + def test_same_quality(self, method, desired_size): + """Test that choosing between thumbnails with the same quality rating succeeds. + + We are not particular about which thumbnail is chosen.""" + self.assertTrue( + self.thumbnail_resource._select_thumbnail( + desired_width=desired_size, + desired_height=desired_size, + desired_method=method, + desired_type=self.test_image.content_type, + # Provide two identical thumbnails which are guaranteed to have the same + # quality rating. + thumbnail_infos=[ + { + "thumbnail_width": 32, + "thumbnail_height": 32, + "thumbnail_method": method, + "thumbnail_type": self.test_image.content_type, + "thumbnail_length": 256, + "filesystem_id": f"thumbnail1{self.test_image.extension}", + }, + { + "thumbnail_width": 32, + "thumbnail_height": 32, + "thumbnail_method": method, + "thumbnail_type": self.test_image.content_type, + "thumbnail_length": 256, + "filesystem_id": f"thumbnail2{self.test_image.extension}", + }, + ], + file_id=f"image{self.test_image.extension}", + url_cache=None, + server_name=None, + ) + ) + def test_x_robots_tag_header(self): """ Tests that the `X-Robots-Tag` header is present, which informs web crawlers From 7d670c2898452ff2e1cc0d66befa5d20c403cb70 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Tue, 24 Aug 2021 14:57:40 +0100 Subject: [PATCH 4/7] Newsfile Signed-off-by: Sean Quah --- changelog.d/10684.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10684.bugfix diff --git a/changelog.d/10684.bugfix b/changelog.d/10684.bugfix new file mode 100644 index 000000000000..2d784b1ee0a0 --- /dev/null +++ b/changelog.d/10684.bugfix @@ -0,0 +1 @@ +Fix edge case error when requesting thumbnails. From 44fc4eef9a4cf8f4c497857767a847b0c22ebef9 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Tue, 24 Aug 2021 19:17:11 +0100 Subject: [PATCH 5/7] fixup Newsfile Signed-off-by: Sean Quah --- changelog.d/10684.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/10684.bugfix b/changelog.d/10684.bugfix index 2d784b1ee0a0..9b662c4571fc 100644 --- a/changelog.d/10684.bugfix +++ b/changelog.d/10684.bugfix @@ -1 +1 @@ -Fix edge case error when requesting thumbnails. +Fix error when a thumbnail is requested and there are multiple thumbnails with the same quality rating. From 5e1750a210558bad6ead98d2ff049a0d12cd5c06 Mon Sep 17 00:00:00 2001 From: Sean Date: Wed, 25 Aug 2021 10:27:26 +0100 Subject: [PATCH 6/7] Update changelog.d/10684.bugfix Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- changelog.d/10684.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/10684.bugfix b/changelog.d/10684.bugfix index 9b662c4571fc..311b17601a3e 100644 --- a/changelog.d/10684.bugfix +++ b/changelog.d/10684.bugfix @@ -1 +1 @@ -Fix error when a thumbnail is requested and there are multiple thumbnails with the same quality rating. +Fix long-standing issue which caused an error when a thumbnail is requested and there are multiple thumbnails with the same quality rating. From 8ff46080dc3c2258c30a904264fdc48314e463e5 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Wed, 25 Aug 2021 10:28:42 +0100 Subject: [PATCH 7/7] fixup Add tests Signed-off-by: Sean Quah --- tests/rest/media/v1/test_media_storage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py index dc959c0cf7d5..2f7eebfe6931 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -478,7 +478,7 @@ def test_same_quality(self, method, desired_size): """Test that choosing between thumbnails with the same quality rating succeeds. We are not particular about which thumbnail is chosen.""" - self.assertTrue( + self.assertIsNotNone( self.thumbnail_resource._select_thumbnail( desired_width=desired_size, desired_height=desired_size,