From eec73e61bb25de988b3202a51fb5a3df9618880c Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 7 Feb 2023 01:11:19 +0000 Subject: [PATCH 1/5] Fix MediaStorage type hint --- synapse/rest/media/v1/media_storage.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/synapse/rest/media/v1/media_storage.py b/synapse/rest/media/v1/media_storage.py index a5c3de192f6f..6857dec508f1 100644 --- a/synapse/rest/media/v1/media_storage.py +++ b/synapse/rest/media/v1/media_storage.py @@ -46,10 +46,9 @@ from .filepath import MediaFilePaths if TYPE_CHECKING: + from synapse.rest.media.v1.storage_provider import StorageProvider from synapse.server import HomeServer - from .storage_provider import StorageProviderWrapper - logger = logging.getLogger(__name__) @@ -68,7 +67,7 @@ def __init__( hs: "HomeServer", local_media_directory: str, filepaths: MediaFilePaths, - storage_providers: Sequence["StorageProviderWrapper"], + storage_providers: Sequence["StorageProvider"], ): self.hs = hs self.reactor = hs.get_reactor() From 4528d6fea1a8a271c9dfff0b862c811c3a661663 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 7 Feb 2023 01:52:40 +0000 Subject: [PATCH 2/5] Typecheck tests.rest.media.v1.test_media_storage --- mypy.ini | 1 - synapse/rest/media/v1/media_storage.py | 2 +- tests/rest/media/v1/test_media_storage.py | 52 +++++++++++++++-------- 3 files changed, 35 insertions(+), 20 deletions(-) diff --git a/mypy.ini b/mypy.ini index 11e683b7042c..204907f0d37b 100644 --- a/mypy.ini +++ b/mypy.ini @@ -33,7 +33,6 @@ exclude = (?x) |synapse/storage/schema/ |tests/module_api/test_api.py - |tests/rest/media/v1/test_media_storage.py |tests/server.py |tests/test_terms_auth.py )$ diff --git a/synapse/rest/media/v1/media_storage.py b/synapse/rest/media/v1/media_storage.py index 6857dec508f1..db25848744de 100644 --- a/synapse/rest/media/v1/media_storage.py +++ b/synapse/rest/media/v1/media_storage.py @@ -359,7 +359,7 @@ class ReadableFileWrapper: clock: Clock path: str - async def write_chunks_to(self, callback: Callable[[bytes], None]) -> None: + async def write_chunks_to(self, callback: Callable[[bytes], object]) -> None: """Reads the file in chunks and calls the callback with each chunk.""" with open(self.path, "rb") as file: diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py index d18fc13c21d6..b15a476fc34c 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -16,7 +16,7 @@ import tempfile from binascii import unhexlify from io import BytesIO -from typing import Any, BinaryIO, Dict, List, Optional, Union +from typing import Any, BinaryIO, ClassVar, Dict, List, Optional, Tuple, Union from unittest.mock import Mock from urllib import parse @@ -32,6 +32,7 @@ from synapse.api.errors import Codes from synapse.events import EventBase from synapse.events.spamcheck import load_legacy_spam_checkers +from synapse.http.types import QueryParams from synapse.logging.context import make_deferred_yieldable from synapse.module_api import ModuleApi from synapse.rest import admin @@ -41,7 +42,7 @@ from synapse.rest.media.v1.media_storage import MediaStorage, ReadableFileWrapper from synapse.rest.media.v1.storage_provider import FileStorageProviderBackend from synapse.server import HomeServer -from synapse.types import RoomAlias +from synapse.types import JsonDict, RoomAlias from synapse.util import Clock from tests import unittest @@ -201,36 +202,49 @@ class _TestImage: ], ) class MediaRepoTests(unittest.HomeserverTestCase): - + test_image: ClassVar[_TestImage] hijack_auth = True user_id = "@test:user" def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: - self.fetches = [] + self.fetches: List[ + Tuple[ + "Deferred[Tuple[bytes, Tuple[int, Dict[bytes, List[bytes]]]]]", + str, + str, + Optional[QueryParams], + ] + ] = [] def get_file( destination: str, path: str, output_stream: BinaryIO, - args: Optional[Dict[str, Union[str, List[str]]]] = None, + args: Optional[QueryParams] = None, + retry_on_dns_fail: bool = True, max_size: Optional[int] = None, - ) -> Deferred: - """ - Returns tuple[int,dict,str,int] of file length, response headers, - absolute URI, and response code. - """ + ignore_backoff: bool = False, + ) -> Deferred[Tuple[int, Dict[bytes, List[bytes]]]]: + """A mock for MatrixFederationHttpClient.get_file.""" - def write_to(r): + def write_to( + r: Tuple[bytes, Tuple[int, Dict[bytes, List[bytes]]]] + ) -> Tuple[int, Dict[bytes, List[bytes]]]: data, response = r output_stream.write(data) return response - d = Deferred() - d.addCallback(write_to) + d: Deferred[Tuple[bytes, Tuple[int, Dict[bytes, List[bytes]]]]] = Deferred() + # Callbacks can _change_ the value held within a resolved Deferred(!!). + # The return type of addCallback reflects this... + d_after_callback = d.addCallback(write_to) + # ... even though d.addCallback returns d! + assert d_after_callback is d # type: ignore[comparison-overlap] self.fetches.append((d, destination, path, args)) - return make_deferred_yieldable(d) + return make_deferred_yieldable(d_after_callback) + # Mock out the homeserver's MatrixFederationHttpClient client = Mock() client.get_file = get_file @@ -461,6 +475,7 @@ def test_thumbnail_repeated_thumbnail(self) -> None: # 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)) + assert info is not None file_id = info["filesystem_id"] thumbnail_dir = self.media_repo.filepaths.remote_media_thumbnail_dir( @@ -581,7 +596,7 @@ def test_same_quality(self, method: str, desired_size: int) -> None: "thumbnail_method": method, "thumbnail_type": self.test_image.content_type, "thumbnail_length": 256, - "filesystem_id": f"thumbnail1{self.test_image.extension}", + "filesystem_id": f"thumbnail1{self.test_image.extension.decode()}", }, { "thumbnail_width": 32, @@ -589,10 +604,10 @@ def test_same_quality(self, method: str, desired_size: int) -> None: "thumbnail_method": method, "thumbnail_type": self.test_image.content_type, "thumbnail_length": 256, - "filesystem_id": f"thumbnail2{self.test_image.extension}", + "filesystem_id": f"thumbnail2{self.test_image.extension.decode()}", }, ], - file_id=f"image{self.test_image.extension}", + file_id=f"image{self.test_image.extension.decode()}", url_cache=None, server_name=None, ) @@ -637,6 +652,7 @@ def __init__(self, config: Dict[str, Any], api: ModuleApi) -> None: self.config = config self.api = api + @staticmethod def parse_config(config: Dict[str, Any]) -> Dict[str, Any]: return config @@ -748,7 +764,7 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: async def check_media_file_for_spam( self, file_wrapper: ReadableFileWrapper, file_info: FileInfo - ) -> Union[Codes, Literal["NOT_SPAM"]]: + ) -> Union[Codes, Literal["NOT_SPAM"], Tuple[Codes, JsonDict]]: buf = BytesIO() await file_wrapper.write_chunks_to(buf.write) From a33a758328017b4f69b0595b90f712fa3726f75b Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 7 Feb 2023 01:55:34 +0000 Subject: [PATCH 3/5] Changelog --- changelog.d/15008.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/15008.misc diff --git a/changelog.d/15008.misc b/changelog.d/15008.misc new file mode 100644 index 000000000000..93ceaeafc9b9 --- /dev/null +++ b/changelog.d/15008.misc @@ -0,0 +1 @@ +Improve type hints. From e22b4e2727890c4858bf35353143f86c78aaf6d5 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 7 Feb 2023 13:31:53 +0000 Subject: [PATCH 4/5] Remove assert and make the comment succinct --- tests/rest/media/v1/test_media_storage.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tests/rest/media/v1/test_media_storage.py b/tests/rest/media/v1/test_media_storage.py index b15a476fc34c..abdfc9c3b292 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -236,12 +236,9 @@ def write_to( return response d: Deferred[Tuple[bytes, Tuple[int, Dict[bytes, List[bytes]]]]] = Deferred() - # Callbacks can _change_ the value held within a resolved Deferred(!!). - # The return type of addCallback reflects this... - d_after_callback = d.addCallback(write_to) - # ... even though d.addCallback returns d! - assert d_after_callback is d # type: ignore[comparison-overlap] self.fetches.append((d, destination, path, args)) + # Note that this callback changes the value held by d. + d_after_callback = d.addCallback(write_to) return make_deferred_yieldable(d_after_callback) # Mock out the homeserver's MatrixFederationHttpClient From e7fe5f0b58de2e485ceb9b3753e68e9bb79b4360 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 7 Feb 2023 14:40:48 +0000 Subject: [PATCH 5/5] Fix syntax for olddeps --- 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 abdfc9c3b292..17a3b06a8e25 100644 --- a/tests/rest/media/v1/test_media_storage.py +++ b/tests/rest/media/v1/test_media_storage.py @@ -225,7 +225,7 @@ def get_file( retry_on_dns_fail: bool = True, max_size: Optional[int] = None, ignore_backoff: bool = False, - ) -> Deferred[Tuple[int, Dict[bytes, List[bytes]]]]: + ) -> "Deferred[Tuple[int, Dict[bytes, List[bytes]]]]": """A mock for MatrixFederationHttpClient.get_file.""" def write_to(