From c26c25220d40d650a56d586eef7ca37ee8396acd Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 20 Apr 2023 15:38:28 -0400 Subject: [PATCH 1/8] Require types in matrixfederationclient. --- mypy.ini | 3 --- synapse/http/matrixfederationclient.py | 19 +++++++++---------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/mypy.ini b/mypy.ini index 945f7925cb2c..f3704dbc78a8 100644 --- a/mypy.ini +++ b/mypy.ini @@ -36,9 +36,6 @@ exclude = (?x) [mypy-synapse.federation.transport.client] disallow_untyped_defs = False -[mypy-synapse.http.matrixfederationclient] -disallow_untyped_defs = False - [mypy-synapse.metrics._reactor_metrics] disallow_untyped_defs = False # This module imports select.epoll. That exists on Linux, but doesn't on macOS. diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 3302d4e48a0f..1e420296b940 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -17,7 +17,6 @@ import logging import random import sys -import typing import urllib.parse from http import HTTPStatus from io import BytesIO, StringIO @@ -30,9 +29,11 @@ Generic, List, Optional, + TextIO, Tuple, TypeVar, Union, + cast, overload, ) @@ -313,9 +314,7 @@ async def _handle_response( class BinaryIOWrapper: """A wrapper for a TextIO which converts from bytes on the fly.""" - def __init__( - self, file: typing.TextIO, encoding: str = "utf-8", errors: str = "strict" - ): + def __init__(self, file: TextIO, encoding: str = "utf-8", errors: str = "strict"): self.decoder = codecs.getincrementaldecoder(encoding)(errors) self.file = file @@ -825,8 +824,8 @@ async def put_json( ignore_backoff: bool = False, backoff_on_404: bool = False, try_trailing_slash_on_400: bool = False, - parser: Optional[ByteParser] = None, - ): + parser: Optional[ByteParser[T]] = None, + ) -> Union[JsonDict, list, T]: """Sends the specified json data using PUT Args: @@ -902,7 +901,7 @@ async def put_json( _sec_timeout = self.default_timeout if parser is None: - parser = JsonParser() + parser = cast(ByteParser[T], JsonParser()) body = await _handle_response( self.reactor, @@ -1024,8 +1023,8 @@ async def get_json( timeout: Optional[int] = None, ignore_backoff: bool = False, try_trailing_slash_on_400: bool = False, - parser: Optional[ByteParser] = None, - ): + parser: Optional[ByteParser[T]] = None, + ) -> Union[JsonDict, list, T]: """GETs some json from the given host homeserver and path Args: @@ -1091,7 +1090,7 @@ async def get_json( _sec_timeout = self.default_timeout if parser is None: - parser = JsonParser() + parser = cast(ByteParser[T], JsonParser()) body = await _handle_response( self.reactor, From 3b378d9c3ead77a2cea306ba5ecd54848e02f255 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 20 Apr 2023 16:16:11 -0400 Subject: [PATCH 2/8] Fix-up types in federation client. --- mypy.ini | 3 -- synapse/federation/federation_client.py | 8 +--- synapse/federation/transport/client.py | 63 ++++++++++++++++++++----- synapse/http/matrixfederationclient.py | 51 +++++++++++++++++--- tests/federation/test_complexity.py | 10 ++-- 5 files changed, 103 insertions(+), 32 deletions(-) diff --git a/mypy.ini b/mypy.ini index f3704dbc78a8..8fb87b9b7452 100644 --- a/mypy.ini +++ b/mypy.ini @@ -33,9 +33,6 @@ exclude = (?x) |synapse/storage/schema/ )$ -[mypy-synapse.federation.transport.client] -disallow_untyped_defs = False - [mypy-synapse.metrics._reactor_metrics] disallow_untyped_defs = False # This module imports select.epoll. That exists on Linux, but doesn't on macOS. diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 4cf4957a42ca..ba34573d466d 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -280,15 +280,11 @@ async def backfill( logger.debug("backfill transaction_data=%r", transaction_data) if not isinstance(transaction_data, dict): - # TODO we probably want an exception type specific to federation - # client validation. - raise TypeError("Backfill transaction_data is not a dict.") + raise InvalidResponseError("Backfill transaction_data is not a dict.") transaction_data_pdus = transaction_data.get("pdus") if not isinstance(transaction_data_pdus, list): - # TODO we probably want an exception type specific to federation - # client validation. - raise TypeError("transaction_data.pdus is not a list.") + raise InvalidResponseError("transaction_data.pdus is not a list.") room_version = await self.store.get_room_version(room_id) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index c05d598b70cf..2d938d7b101c 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -16,6 +16,7 @@ import logging import urllib from typing import ( + TYPE_CHECKING, Any, Callable, Collection, @@ -42,18 +43,25 @@ ) from synapse.events import EventBase, make_event_from_dict from synapse.federation.units import Transaction -from synapse.http.matrixfederationclient import ByteParser +from synapse.http.matrixfederationclient import ( + ByteParser, + JsonDictParser, + LegacyJsonDictParser, +) from synapse.http.types import QueryParams from synapse.types import JsonDict from synapse.util import ExceptionBundle +if TYPE_CHECKING: + from synapse.app.homeserver import HomeServer + logger = logging.getLogger(__name__) class TransportLayerClient: """Sends federation HTTP requests to other servers""" - def __init__(self, hs): + def __init__(self, hs: "HomeServer"): self.server_name = hs.hostname self.client = hs.get_federation_http_client() self._faster_joins_enabled = hs.config.experimental.faster_joins_enabled @@ -80,6 +88,7 @@ async def get_room_state_ids( path=path, args={"event_id": event_id}, try_trailing_slash_on_400=True, + parser=JsonDictParser(), ) async def get_room_state( @@ -128,12 +137,16 @@ async def get_event( path = _create_v1_path("/event/%s", event_id) return await self.client.get_json( - destination, path=path, timeout=timeout, try_trailing_slash_on_400=True + destination, + path=path, + timeout=timeout, + try_trailing_slash_on_400=True, + parser=JsonDictParser(), ) async def backfill( self, destination: str, room_id: str, event_tuples: Collection[str], limit: int - ) -> Optional[JsonDict]: + ) -> Optional[Union[JsonDict, list]]: """Requests `limit` previous PDUs in a given context before list of PDUs. @@ -248,6 +261,7 @@ async def send_transaction( long_retries=True, backoff_on_404=True, # If we get a 404 the other side has gone try_trailing_slash_on_400=True, + parser=JsonDictParser(), ) async def make_query( @@ -268,6 +282,7 @@ async def make_query( retry_on_dns_fail=retry_on_dns_fail, timeout=10000, ignore_backoff=ignore_backoff, + parser=JsonDictParser(), ) async def make_membership_event( @@ -329,6 +344,7 @@ async def make_membership_event( retry_on_dns_fail=retry_on_dns_fail, timeout=20000, ignore_backoff=ignore_backoff, + parser=JsonDictParser(), ) async def send_join_v1( @@ -388,6 +404,7 @@ async def send_leave_v1( # server was just having a momentary blip, the room will be out of # sync. ignore_backoff=True, + parser=LegacyJsonDictParser(), ) async def send_leave_v2( @@ -404,6 +421,7 @@ async def send_leave_v2( # server was just having a momentary blip, the room will be out of # sync. ignore_backoff=True, + parser=JsonDictParser(), ) async def send_knock_v1( @@ -436,7 +454,10 @@ async def send_knock_v1( path = _create_v1_path("/send_knock/%s/%s", room_id, event_id) return await self.client.put_json( - destination=destination, path=path, data=content + destination=destination, + path=path, + data=content, + parser=JsonDictParser(), ) async def send_invite_v1( @@ -445,7 +466,11 @@ async def send_invite_v1( path = _create_v1_path("/invite/%s/%s", room_id, event_id) return await self.client.put_json( - destination=destination, path=path, data=content, ignore_backoff=True + destination=destination, + path=path, + data=content, + ignore_backoff=True, + parser=LegacyJsonDictParser(), ) async def send_invite_v2( @@ -454,7 +479,11 @@ async def send_invite_v2( path = _create_v2_path("/invite/%s/%s", room_id, event_id) return await self.client.put_json( - destination=destination, path=path, data=content, ignore_backoff=True + destination=destination, + path=path, + data=content, + ignore_backoff=True, + parser=JsonDictParser(), ) async def get_public_rooms( @@ -515,7 +544,11 @@ async def get_public_rooms( try: response = await self.client.get_json( - destination=remote_server, path=path, args=args, ignore_backoff=True + destination=remote_server, + path=path, + args=args, + ignore_backoff=True, + parser=JsonDictParser(), ) except HttpResponseException as e: if e.code == 403: @@ -535,7 +568,7 @@ async def exchange_third_party_invite( path = _create_v1_path("/exchange_third_party_invite/%s", room_id) return await self.client.put_json( - destination=destination, path=path, data=event_dict + destination=destination, path=path, data=event_dict, parser=JsonDictParser() ) async def get_event_auth( @@ -543,7 +576,9 @@ async def get_event_auth( ) -> JsonDict: path = _create_v1_path("/event_auth/%s/%s", room_id, event_id) - return await self.client.get_json(destination=destination, path=path) + return await self.client.get_json( + destination=destination, path=path, parser=JsonDictParser() + ) async def query_client_keys( self, destination: str, query_content: JsonDict, timeout: int @@ -622,7 +657,7 @@ async def query_user_devices( path = _create_v1_path("/user/devices/%s", user_id) return await self.client.get_json( - destination=destination, path=path, timeout=timeout + destination=destination, path=path, timeout=timeout, parser=JsonDictParser() ) async def claim_client_keys( @@ -695,7 +730,9 @@ async def get_room_complexity(self, destination: str, room_id: str) -> JsonDict: """ path = _create_path(FEDERATION_UNSTABLE_PREFIX, "/rooms/%s/complexity", room_id) - return await self.client.get_json(destination=destination, path=path) + return await self.client.get_json( + destination=destination, path=path, parser=JsonDictParser() + ) async def get_room_hierarchy( self, destination: str, room_id: str, suggested_only: bool @@ -712,6 +749,7 @@ async def get_room_hierarchy( destination=destination, path=path, args={"suggested_only": "true" if suggested_only else "false"}, + parser=JsonDictParser(), ) async def get_room_hierarchy_unstable( @@ -731,6 +769,7 @@ async def get_room_hierarchy_unstable( destination=destination, path=path, args={"suggested_only": "true" if suggested_only else "false"}, + parser=JsonDictParser(), ) async def get_account_status( diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 1e420296b940..f4f3a777f616 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -184,20 +184,54 @@ def get_json(self) -> Optional[JsonDict]: return self.json -class JsonParser(ByteParser[Union[JsonDict, list]]): +class _BaseJsonParser(ByteParser[T]): """A parser that buffers the response and tries to parse it as JSON.""" CONTENT_TYPE = "application/json" - def __init__(self) -> None: + def __init__(self, validator: Optional[Callable[[Any], bool]] = None) -> None: self._buffer = StringIO() self._binary_wrapper = BinaryIOWrapper(self._buffer) + self._validator = validator def write(self, data: bytes) -> int: return self._binary_wrapper.write(data) - def finish(self) -> Union[JsonDict, list]: - return json_decoder.decode(self._buffer.getvalue()) + def finish(self) -> T: + result = json_decoder.decode(self._buffer.getvalue()) + if self._validator is not None and not self._validator(result): + raise ValueError("Unexpected JSON object") + return result + + +class JsonParser(_BaseJsonParser[Union[JsonDict, list]]): + """A parser that buffers the response and tries to parse it as JSON.""" + + +class JsonDictParser(_BaseJsonParser[JsonDict]): + """Ensure the response is a JSON object.""" + def __init__(self) -> None: + super().__init__(self._validate) + + @staticmethod + def _validate(v: Any) -> bool: + return isinstance(v, dict) + + +class LegacyJsonDictParser(_BaseJsonParser[Tuple[int, JsonDict]]): + """Ensure the legacy responses of /send_join & /send_leave are correct.""" + def __init__(self) -> None: + super().__init__(self._validate) + + @staticmethod + def _validate(v: Any) -> bool: + # Match [integer, JSON dict] + return ( + isinstance(v, list) + and len(v) == 2 + and type(v[0]) == int + and isinstance(v[1], dict) + ) async def _handle_response( @@ -923,7 +957,7 @@ async def post_json( timeout: Optional[int] = None, ignore_backoff: bool = False, args: Optional[QueryParams] = None, - ) -> Union[JsonDict, list]: + ) -> JsonDict: """Sends the specified json data using POST Args: @@ -982,7 +1016,12 @@ async def post_json( _sec_timeout = self.default_timeout body = await _handle_response( - self.reactor, _sec_timeout, request, response, start_ms, parser=JsonParser() + self.reactor, + _sec_timeout, + request, + response, + start_ms, + parser=JsonDictParser(), ) return body diff --git a/tests/federation/test_complexity.py b/tests/federation/test_complexity.py index 33af8770fde8..129d7cfd93f5 100644 --- a/tests/federation/test_complexity.py +++ b/tests/federation/test_complexity.py @@ -75,7 +75,7 @@ def test_join_too_large(self) -> None: fed_transport = self.hs.get_federation_transport_client() # Mock out some things, because we don't want to test the whole join - fed_transport.client.get_json = Mock(return_value=make_awaitable({"v1": 9999})) + fed_transport.client.get_json = Mock(return_value=make_awaitable({"v1": 9999})) # type: ignore[assignment] handler.federation_handler.do_invite_join = Mock( # type: ignore[assignment] return_value=make_awaitable(("", 1)) ) @@ -106,7 +106,7 @@ def test_join_too_large_admin(self) -> None: fed_transport = self.hs.get_federation_transport_client() # Mock out some things, because we don't want to test the whole join - fed_transport.client.get_json = Mock(return_value=make_awaitable({"v1": 9999})) + fed_transport.client.get_json = Mock(return_value=make_awaitable({"v1": 9999})) # type: ignore[assignment] handler.federation_handler.do_invite_join = Mock( # type: ignore[assignment] return_value=make_awaitable(("", 1)) ) @@ -143,7 +143,7 @@ def test_join_too_large_once_joined(self) -> None: fed_transport = self.hs.get_federation_transport_client() # Mock out some things, because we don't want to test the whole join - fed_transport.client.get_json = Mock(return_value=make_awaitable(None)) + fed_transport.client.get_json = Mock(return_value=make_awaitable(None)) # type: ignore[assignment] handler.federation_handler.do_invite_join = Mock( # type: ignore[assignment] return_value=make_awaitable(("", 1)) ) @@ -200,7 +200,7 @@ def test_join_too_large_no_admin(self) -> None: fed_transport = self.hs.get_federation_transport_client() # Mock out some things, because we don't want to test the whole join - fed_transport.client.get_json = Mock(return_value=make_awaitable({"v1": 9999})) + fed_transport.client.get_json = Mock(return_value=make_awaitable({"v1": 9999})) # type: ignore[assignment] handler.federation_handler.do_invite_join = Mock( # type: ignore[assignment] return_value=make_awaitable(("", 1)) ) @@ -230,7 +230,7 @@ def test_join_too_large_admin(self) -> None: fed_transport = self.hs.get_federation_transport_client() # Mock out some things, because we don't want to test the whole join - fed_transport.client.get_json = Mock(return_value=make_awaitable({"v1": 9999})) + fed_transport.client.get_json = Mock(return_value=make_awaitable({"v1": 9999})) # type: ignore[assignment] handler.federation_handler.do_invite_join = Mock( # type: ignore[assignment] return_value=make_awaitable(("", 1)) ) From 6a5e24a20ebb0a5ecb58517ec07d39f2ade999f7 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 20 Apr 2023 16:23:10 -0400 Subject: [PATCH 3/8] Newsfragment --- changelog.d/15465.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/15465.misc diff --git a/changelog.d/15465.misc b/changelog.d/15465.misc new file mode 100644 index 000000000000..93ceaeafc9b9 --- /dev/null +++ b/changelog.d/15465.misc @@ -0,0 +1 @@ +Improve type hints. From 68e558059ad27cb42187dbcda11be587867492d6 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 20 Apr 2023 16:26:15 -0400 Subject: [PATCH 4/8] Lint --- synapse/http/matrixfederationclient.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index f4f3a777f616..877cb3120a28 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -210,6 +210,7 @@ class JsonParser(_BaseJsonParser[Union[JsonDict, list]]): class JsonDictParser(_BaseJsonParser[JsonDict]): """Ensure the response is a JSON object.""" + def __init__(self) -> None: super().__init__(self._validate) @@ -220,6 +221,7 @@ def _validate(v: Any) -> bool: class LegacyJsonDictParser(_BaseJsonParser[Tuple[int, JsonDict]]): """Ensure the legacy responses of /send_join & /send_leave are correct.""" + def __init__(self) -> None: super().__init__(self._validate) From d8f7fabbe0142cd33c357ae3e12b93b86fa9c38f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 21 Apr 2023 09:37:27 -0400 Subject: [PATCH 5/8] Update error message. --- synapse/http/matrixfederationclient.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 877cb3120a28..8b36f30b11ed 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -200,7 +200,7 @@ def write(self, data: bytes) -> int: def finish(self) -> T: result = json_decoder.decode(self._buffer.getvalue()) if self._validator is not None and not self._validator(result): - raise ValueError("Unexpected JSON object") + raise ValueError(f"Received incorrect JSON value: {result.__class__.__name__}") return result From 567ea431912e63895255a8728b689465a321f368 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 21 Apr 2023 12:04:02 -0400 Subject: [PATCH 6/8] Use object, not Any. --- synapse/http/matrixfederationclient.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 8b36f30b11ed..5709ceccd6fa 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -189,7 +189,14 @@ class _BaseJsonParser(ByteParser[T]): CONTENT_TYPE = "application/json" - def __init__(self, validator: Optional[Callable[[Any], bool]] = None) -> None: + def __init__( + self, validator: Optional[Callable[[Optional[object]], bool]] = None + ) -> None: + """ + Args: + validator: A callable which takes the parsed JSON value and returns + true if the value is valid. + """ self._buffer = StringIO() self._binary_wrapper = BinaryIOWrapper(self._buffer) self._validator = validator @@ -200,7 +207,9 @@ def write(self, data: bytes) -> int: def finish(self) -> T: result = json_decoder.decode(self._buffer.getvalue()) if self._validator is not None and not self._validator(result): - raise ValueError(f"Received incorrect JSON value: {result.__class__.__name__}") + raise ValueError( + f"Received incorrect JSON value: {result.__class__.__name__}" + ) return result From abc004e53e898ec80f685d1c3326806d92a926fd Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 24 Apr 2023 08:19:27 -0400 Subject: [PATCH 7/8] Parse dicts by default. --- synapse/federation/transport/client.py | 11 ----------- synapse/http/matrixfederationclient.py | 12 ++++++------ 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index 2d938d7b101c..eca9c9b44b6a 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -88,7 +88,6 @@ async def get_room_state_ids( path=path, args={"event_id": event_id}, try_trailing_slash_on_400=True, - parser=JsonDictParser(), ) async def get_room_state( @@ -141,7 +140,6 @@ async def get_event( path=path, timeout=timeout, try_trailing_slash_on_400=True, - parser=JsonDictParser(), ) async def backfill( @@ -261,7 +259,6 @@ async def send_transaction( long_retries=True, backoff_on_404=True, # If we get a 404 the other side has gone try_trailing_slash_on_400=True, - parser=JsonDictParser(), ) async def make_query( @@ -282,7 +279,6 @@ async def make_query( retry_on_dns_fail=retry_on_dns_fail, timeout=10000, ignore_backoff=ignore_backoff, - parser=JsonDictParser(), ) async def make_membership_event( @@ -344,7 +340,6 @@ async def make_membership_event( retry_on_dns_fail=retry_on_dns_fail, timeout=20000, ignore_backoff=ignore_backoff, - parser=JsonDictParser(), ) async def send_join_v1( @@ -421,7 +416,6 @@ async def send_leave_v2( # server was just having a momentary blip, the room will be out of # sync. ignore_backoff=True, - parser=JsonDictParser(), ) async def send_knock_v1( @@ -457,7 +451,6 @@ async def send_knock_v1( destination=destination, path=path, data=content, - parser=JsonDictParser(), ) async def send_invite_v1( @@ -483,7 +476,6 @@ async def send_invite_v2( path=path, data=content, ignore_backoff=True, - parser=JsonDictParser(), ) async def get_public_rooms( @@ -548,7 +540,6 @@ async def get_public_rooms( path=path, args=args, ignore_backoff=True, - parser=JsonDictParser(), ) except HttpResponseException as e: if e.code == 403: @@ -749,7 +740,6 @@ async def get_room_hierarchy( destination=destination, path=path, args={"suggested_only": "true" if suggested_only else "false"}, - parser=JsonDictParser(), ) async def get_room_hierarchy_unstable( @@ -769,7 +759,6 @@ async def get_room_hierarchy_unstable( destination=destination, path=path, args={"suggested_only": "true" if suggested_only else "false"}, - parser=JsonDictParser(), ) async def get_account_status( diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 5709ceccd6fa..30795dd54be5 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -837,7 +837,7 @@ async def put_json( backoff_on_404: bool = False, try_trailing_slash_on_400: bool = False, parser: Literal[None] = None, - ) -> Union[JsonDict, list]: + ) -> JsonDict: ... @overload @@ -870,7 +870,7 @@ async def put_json( backoff_on_404: bool = False, try_trailing_slash_on_400: bool = False, parser: Optional[ByteParser[T]] = None, - ) -> Union[JsonDict, list, T]: + ) -> Union[JsonDict, T]: """Sends the specified json data using PUT Args: @@ -946,7 +946,7 @@ async def put_json( _sec_timeout = self.default_timeout if parser is None: - parser = cast(ByteParser[T], JsonParser()) + parser = cast(ByteParser[T], JsonDictParser()) body = await _handle_response( self.reactor, @@ -1047,7 +1047,7 @@ async def get_json( ignore_backoff: bool = False, try_trailing_slash_on_400: bool = False, parser: Literal[None] = None, - ) -> Union[JsonDict, list]: + ) -> JsonDict: ... @overload @@ -1074,7 +1074,7 @@ async def get_json( ignore_backoff: bool = False, try_trailing_slash_on_400: bool = False, parser: Optional[ByteParser[T]] = None, - ) -> Union[JsonDict, list, T]: + ) -> Union[JsonDict, T]: """GETs some json from the given host homeserver and path Args: @@ -1140,7 +1140,7 @@ async def get_json( _sec_timeout = self.default_timeout if parser is None: - parser = cast(ByteParser[T], JsonParser()) + parser = cast(ByteParser[T], JsonDictParser()) body = await _handle_response( self.reactor, From 62f07ffb57abcc59896d4abad749360c55cc2454 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 24 Apr 2023 08:28:03 -0400 Subject: [PATCH 8/8] Parse Json objects by default. --- synapse/federation/transport/client.py | 41 ++++++----------------- synapse/http/matrixfederationclient.py | 23 ++++--------- tests/http/test_matrixfederationclient.py | 6 ++-- 3 files changed, 21 insertions(+), 49 deletions(-) diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py index eca9c9b44b6a..bedbd23dedee 100644 --- a/synapse/federation/transport/client.py +++ b/synapse/federation/transport/client.py @@ -43,11 +43,7 @@ ) from synapse.events import EventBase, make_event_from_dict from synapse.federation.units import Transaction -from synapse.http.matrixfederationclient import ( - ByteParser, - JsonDictParser, - LegacyJsonDictParser, -) +from synapse.http.matrixfederationclient import ByteParser, LegacyJsonSendParser from synapse.http.types import QueryParams from synapse.types import JsonDict from synapse.util import ExceptionBundle @@ -136,10 +132,7 @@ async def get_event( path = _create_v1_path("/event/%s", event_id) return await self.client.get_json( - destination, - path=path, - timeout=timeout, - try_trailing_slash_on_400=True, + destination, path=path, timeout=timeout, try_trailing_slash_on_400=True ) async def backfill( @@ -399,7 +392,7 @@ async def send_leave_v1( # server was just having a momentary blip, the room will be out of # sync. ignore_backoff=True, - parser=LegacyJsonDictParser(), + parser=LegacyJsonSendParser(), ) async def send_leave_v2( @@ -448,9 +441,7 @@ async def send_knock_v1( path = _create_v1_path("/send_knock/%s/%s", room_id, event_id) return await self.client.put_json( - destination=destination, - path=path, - data=content, + destination=destination, path=path, data=content ) async def send_invite_v1( @@ -463,7 +454,7 @@ async def send_invite_v1( path=path, data=content, ignore_backoff=True, - parser=LegacyJsonDictParser(), + parser=LegacyJsonSendParser(), ) async def send_invite_v2( @@ -472,10 +463,7 @@ async def send_invite_v2( path = _create_v2_path("/invite/%s/%s", room_id, event_id) return await self.client.put_json( - destination=destination, - path=path, - data=content, - ignore_backoff=True, + destination=destination, path=path, data=content, ignore_backoff=True ) async def get_public_rooms( @@ -536,10 +524,7 @@ async def get_public_rooms( try: response = await self.client.get_json( - destination=remote_server, - path=path, - args=args, - ignore_backoff=True, + destination=remote_server, path=path, args=args, ignore_backoff=True ) except HttpResponseException as e: if e.code == 403: @@ -559,7 +544,7 @@ async def exchange_third_party_invite( path = _create_v1_path("/exchange_third_party_invite/%s", room_id) return await self.client.put_json( - destination=destination, path=path, data=event_dict, parser=JsonDictParser() + destination=destination, path=path, data=event_dict ) async def get_event_auth( @@ -567,9 +552,7 @@ async def get_event_auth( ) -> JsonDict: path = _create_v1_path("/event_auth/%s/%s", room_id, event_id) - return await self.client.get_json( - destination=destination, path=path, parser=JsonDictParser() - ) + return await self.client.get_json(destination=destination, path=path) async def query_client_keys( self, destination: str, query_content: JsonDict, timeout: int @@ -648,7 +631,7 @@ async def query_user_devices( path = _create_v1_path("/user/devices/%s", user_id) return await self.client.get_json( - destination=destination, path=path, timeout=timeout, parser=JsonDictParser() + destination=destination, path=path, timeout=timeout ) async def claim_client_keys( @@ -721,9 +704,7 @@ async def get_room_complexity(self, destination: str, room_id: str) -> JsonDict: """ path = _create_path(FEDERATION_UNSTABLE_PREFIX, "/rooms/%s/complexity", room_id) - return await self.client.get_json( - destination=destination, path=path, parser=JsonDictParser() - ) + return await self.client.get_json(destination=destination, path=path) async def get_room_hierarchy( self, destination: str, room_id: str, suggested_only: bool diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 30795dd54be5..634882487c06 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -213,12 +213,8 @@ def finish(self) -> T: return result -class JsonParser(_BaseJsonParser[Union[JsonDict, list]]): - """A parser that buffers the response and tries to parse it as JSON.""" - - -class JsonDictParser(_BaseJsonParser[JsonDict]): - """Ensure the response is a JSON object.""" +class JsonParser(_BaseJsonParser[JsonDict]): + """A parser that buffers the response and tries to parse it as a JSON object.""" def __init__(self) -> None: super().__init__(self._validate) @@ -228,7 +224,7 @@ def _validate(v: Any) -> bool: return isinstance(v, dict) -class LegacyJsonDictParser(_BaseJsonParser[Tuple[int, JsonDict]]): +class LegacyJsonSendParser(_BaseJsonParser[Tuple[int, JsonDict]]): """Ensure the legacy responses of /send_join & /send_leave are correct.""" def __init__(self) -> None: @@ -946,7 +942,7 @@ async def put_json( _sec_timeout = self.default_timeout if parser is None: - parser = cast(ByteParser[T], JsonDictParser()) + parser = cast(ByteParser[T], JsonParser()) body = await _handle_response( self.reactor, @@ -1027,12 +1023,7 @@ async def post_json( _sec_timeout = self.default_timeout body = await _handle_response( - self.reactor, - _sec_timeout, - request, - response, - start_ms, - parser=JsonDictParser(), + self.reactor, _sec_timeout, request, response, start_ms, parser=JsonParser() ) return body @@ -1140,7 +1131,7 @@ async def get_json( _sec_timeout = self.default_timeout if parser is None: - parser = cast(ByteParser[T], JsonDictParser()) + parser = cast(ByteParser[T], JsonParser()) body = await _handle_response( self.reactor, @@ -1161,7 +1152,7 @@ async def delete_json( timeout: Optional[int] = None, ignore_backoff: bool = False, args: Optional[QueryParams] = None, - ) -> Union[JsonDict, list]: + ) -> JsonDict: """Send a DELETE request to the remote expecting some json response Args: diff --git a/tests/http/test_matrixfederationclient.py b/tests/http/test_matrixfederationclient.py index fdd22a8e9437..d89a91c59d93 100644 --- a/tests/http/test_matrixfederationclient.py +++ b/tests/http/test_matrixfederationclient.py @@ -26,7 +26,7 @@ from synapse.api.errors import RequestSendFailed from synapse.http.matrixfederationclient import ( - JsonParser, + ByteParser, MatrixFederationHttpClient, MatrixFederationRequest, ) @@ -618,9 +618,9 @@ def test_too_big(self) -> None: while not test_d.called: protocol.dataReceived(b"a" * chunk_size) sent += chunk_size - self.assertLessEqual(sent, JsonParser.MAX_RESPONSE_SIZE) + self.assertLessEqual(sent, ByteParser.MAX_RESPONSE_SIZE) - self.assertEqual(sent, JsonParser.MAX_RESPONSE_SIZE) + self.assertEqual(sent, ByteParser.MAX_RESPONSE_SIZE) f = self.failureResultOf(test_d) self.assertIsInstance(f.value, RequestSendFailed)