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

Typecheck tests.rest.media.v1.test_media_storage #15008

Merged
merged 5 commits into from
Feb 7, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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/15008.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Improve type hints.
1 change: 0 additions & 1 deletion mypy.ini
Original file line number Diff line number Diff line change
Expand Up @@ -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
)$
Expand Down
7 changes: 3 additions & 4 deletions synapse/rest/media/v1/media_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -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__)


Expand All @@ -68,7 +67,7 @@ def __init__(
hs: "HomeServer",
local_media_directory: str,
filepaths: MediaFilePaths,
storage_providers: Sequence["StorageProviderWrapper"],
storage_providers: Sequence["StorageProvider"],
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
):
self.hs = hs
self.reactor = hs.get_reactor()
Expand Down Expand Up @@ -360,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:
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
"""Reads the file in chunks and calls the callback with each chunk."""

with open(self.path, "rb") as file:
Expand Down
52 changes: 34 additions & 18 deletions tests/rest/media/v1/test_media_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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]]]]:
clokep marked this conversation as resolved.
Show resolved Hide resolved
"""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]
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
self.fetches.append((d, destination, path, args))
return make_deferred_yieldable(d)
return make_deferred_yieldable(d_after_callback)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a huge surprise to me and feels like a footgun waiting to happen. Or am I overreacting?

Copy link
Member

Choose a reason for hiding this comment

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

Callbacks chain, this is a fundamental part of Twisted and is well documented.

https://docs.twistedmatrix.com/en/stable/core/howto/defer.html#visual-explanation is the best resource I know of which explains this.


# Mock out the homeserver's MatrixFederationHttpClient
client = Mock()
client.get_file = get_file

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -581,18 +596,18 @@ 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,
"thumbnail_height": 32,
"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,
)
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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)

Expand Down