From a6f4bca44eb0b030c1f8783a23ce117c3b7be063 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 10 Nov 2021 14:44:37 -0800 Subject: [PATCH 1/4] type hints for http.py and associated type stubs --- stubs/twisted/__init__.pyi | 0 stubs/twisted/internet/__init__.pyi | 0 stubs/twisted/web/__init__.pyi | 0 stubs/twisted/web/http.pyi | 66 +++++++++++++++++++++++++++++ sygnal/http.py | 56 +++++++++++++++--------- 5 files changed, 102 insertions(+), 20 deletions(-) create mode 100644 stubs/twisted/__init__.pyi create mode 100644 stubs/twisted/internet/__init__.pyi create mode 100644 stubs/twisted/web/__init__.pyi create mode 100644 stubs/twisted/web/http.pyi diff --git a/stubs/twisted/__init__.pyi b/stubs/twisted/__init__.pyi new file mode 100644 index 00000000..e69de29b diff --git a/stubs/twisted/internet/__init__.pyi b/stubs/twisted/internet/__init__.pyi new file mode 100644 index 00000000..e69de29b diff --git a/stubs/twisted/web/__init__.pyi b/stubs/twisted/web/__init__.pyi new file mode 100644 index 00000000..e69de29b diff --git a/stubs/twisted/web/http.pyi b/stubs/twisted/web/http.pyi new file mode 100644 index 00000000..528c1d50 --- /dev/null +++ b/stubs/twisted/web/http.pyi @@ -0,0 +1,66 @@ +import typing +from typing import AnyStr, Dict, List, Optional + +from twisted.internet.defer import Deferred +from twisted.internet.interfaces import IAddress, ITCPTransport +from twisted.logger import Logger +from twisted.web.http_headers import Headers +from twisted.web.iweb import IRequest, IAccessLogFormatter +from zope.interface import implementer, provider + +class HTTPChannel: ... + +# Type ignore: I don't want to respecify the methods on the interface that we +# don't use. +@implementer(IRequest) # type: ignore[misc] +class Request: + code = 200 + # Instance attributes mentioned in the docstring + method: bytes + uri: bytes + path: bytes + args: Dict[bytes, List[bytes]] + content: typing.BinaryIO + cookies: List[bytes] + requestHeaders: Headers + responseHeaders: Headers + notifications: List[Deferred[None]] + _disconnected: bool + _log: Logger + + # Other instance attributes set in __init__ + channel: HTTPChannel + client: IAddress + # This was hard to derive. + # - `transport` is `self.channel.transport` + # - `self.channel` is set in the constructor, and looks like it's always + # an `HTTPChannel`. + # - `HTTPChannel` is a `LineReceiver` is a `Protocol` is a `BaseProtocol`. + # - `BaseProtocol` sets `self.transport` to initially `None`. + # + # Note that `transport` is set to an ITransport in makeConnection, + # so is almost certainly not None by the time it reaches our code. + # + # I've narrowed this to ITCPTransport because + # - we use `self.transport.abortConnection`, which belongs to that interface + # - twisted does too! in its implementation of HTTPChannel.forceAbortClient + transport: Optional[ITCPTransport] + def __init__(self, channel: HTTPChannel): ... + def getHeader(self, key: AnyStr) -> Optional[AnyStr]: ... + def handleContentChunk(self, data: bytes) -> None: ... + def setResponseCode(self, code: int, message: Optional[bytes] = ...) -> None: ... + def setHeader(self, k: AnyStr, v: AnyStr) -> None: ... + def write(self, data: bytes) -> None: ... + def finish(self) -> None: ... + def getClientAddress(self) -> IAddress: ... + +@provider(IAccessLogFormatter) +def proxiedLogFormatter(timestamp: str, request: Request) -> str: + ... + +@provider(IAccessLogFormatter) +def combinedLogFormatter(timestamp: str, request: Request) -> str: + ... + +def datetimeToLogString(msSinceEpoch: Optional[int]=None) -> str: + ... \ No newline at end of file diff --git a/sygnal/http.py b/sygnal/http.py index 743a1fc7..b80f2563 100644 --- a/sygnal/http.py +++ b/sygnal/http.py @@ -20,13 +20,15 @@ import sys import time import traceback +from typing import TYPE_CHECKING, Callable, List, Union from uuid import uuid4 -from opentracing import Format, logs, tags +from opentracing import Format, Span, logs, tags from prometheus_client import Counter, Gauge, Histogram from twisted.internet.defer import ensureDeferred from twisted.web import server from twisted.web.http import ( + Request, combinedLogFormatter, datetimeToLogString, proxiedLogFormatter, @@ -34,11 +36,15 @@ from twisted.web.resource import Resource from twisted.web.server import NOT_DONE_YET -from sygnal.notifications import NotificationContext +from sygnal.exceptions import ( + InvalidNotificationException, + NotificationDispatchException, +) +from sygnal.notifications import Notification, NotificationContext, Pushkin from sygnal.utils import NotificationLoggerAdapter, json_decoder -from .exceptions import InvalidNotificationException, NotificationDispatchException -from .notifications import Notification +if TYPE_CHECKING: + from sygnal.sygnal import Sygnal logger = logging.getLogger(__name__) @@ -76,13 +82,13 @@ class V1NotifyHandler(Resource): - def __init__(self, sygnal): + def __init__(self, sygnal: "Sygnal"): super().__init__() self.sygnal = sygnal isLeaf = True - def _make_request_id(self): + def _make_request_id(self) -> str: """ Generates a request ID, intended to be unique, for a request so it can be followed through logging. @@ -90,17 +96,17 @@ def _make_request_id(self): """ return str(uuid4()) - def render_POST(self, request): + def render_POST(self, request: Request) -> Union[int, bytes]: response = self._handle_request(request) if response != NOT_DONE_YET: PUSHGATEWAY_HTTP_RESPONSES_COUNTER.labels(code=request.code).inc() return response - def _handle_request(self, request): + def _handle_request(self, request: Request) -> Union[int, bytes]: """ Actually handle the request. Args: - request (Request): The request, corresponding to a POST request. + request: The request, corresponding to a POST request. Returns: Either a str instance or NOT_DONE_YET. @@ -202,12 +208,12 @@ async def cb(): if not root_span_accounted_for: root_span.finish() - def find_pushkins(self, appid): + def find_pushkins(self, appid: str) -> List[Pushkin]: """Finds matching pushkins in self.sygnal.pushkins according to the appid. Args: - appid (str): app identifier to search in self.sygnal.pushkins. + appid: app identifier to search in self.sygnal.pushkins. Returns: list of `Pushkin`: If it finds a specific pushkin with @@ -226,7 +232,14 @@ def find_pushkins(self, appid): result.append(value) return result - async def _handle_dispatch(self, root_span, request, log, notif, context): + async def _handle_dispatch( + self, + root_span: Span, + request: Request, + log: NotificationLoggerAdapter, + notif: Notification, + context: NotificationContext, + ) -> None: """ Actually handle the dispatch of notifications to devices, sequentially for simplicity. @@ -234,8 +247,8 @@ async def _handle_dispatch(self, root_span, request, log, notif, context): root_span: the OpenTracing span request: the Twisted Web Request log: the logger to use - notif (Notification): the notification to dispatch - context (NotificationContext): the context of the notification + notif: the notification to dispatch + context: the context of the notification """ try: rejected = [] @@ -251,7 +264,7 @@ async def _handle_dispatch(self, root_span, request, log, notif, context): continue if len(found_pushkins) > 1: - log.warning("Got notification for an ambigious app ID %s", appid) + log.warning("Got notification for an ambiguous app ID %s", appid) rejected.append(d.pushkey) continue @@ -298,7 +311,7 @@ async def _handle_dispatch(self, root_span, request, log, notif, context): class HealthHandler(Resource): - def render_GET(self, request): + def render_GET(self, request: Request) -> bytes: """ `/health` is used for automatic checking of whether the service is up. It should just return a blank 200 OK response. @@ -310,7 +323,7 @@ class SizeLimitingRequest(server.Request): # Arbitrarily limited to 512 KiB. MAX_REQUEST_SIZE = 512 * 1024 - def handleContentChunk(self, data): + def handleContentChunk(self, data: bytes) -> None: # we should have a content by now assert self.content, "handleContentChunk() called before gotLength()" if self.content.tell() + len(data) > self.MAX_REQUEST_SIZE: @@ -318,6 +331,7 @@ def handleContentChunk(self, data): "Aborting connection from %s because the request exceeds maximum size", self.client.host, ) + assert self.transport is not None self.transport.abortConnection() return @@ -330,12 +344,14 @@ class SygnalLoggedSite(server.Site): Sygnal. """ - def __init__(self, *args, log_formatter, **kwargs): + def __init__( + self, *args, log_formatter: Callable[[str, server.Request], str], **kwargs + ): super().__init__(*args, **kwargs) self.log_formatter = log_formatter self.logger = logging.getLogger("sygnal.access") - def log(self, request): + def log(self, request: server.Request) -> None: """Log this request. Called by request.finish.""" # this also works around a bug in twisted.web.http.HTTPFactory which uses a # monotonic time as an epoch time. @@ -345,7 +361,7 @@ def log(self, request): class PushGatewayApiServer(object): - def __init__(self, sygnal): + def __init__(self, sygnal: "Sygnal"): """ Initialises the /_matrix/push/* (Push Gateway API) server. Args: From 6b26836592825470dbd1b038ffb941ea4b77463d Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Thu, 11 Nov 2021 11:22:46 -0800 Subject: [PATCH 2/4] make http.py pass mypy --- sygnal/http.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sygnal/http.py b/sygnal/http.py index b80f2563..41eaa426 100644 --- a/sygnal/http.py +++ b/sygnal/http.py @@ -80,7 +80,6 @@ labelnames=["resource"], ) - class V1NotifyHandler(Resource): def __init__(self, sygnal: "Sygnal"): super().__init__() @@ -329,7 +328,7 @@ def handleContentChunk(self, data: bytes) -> None: if self.content.tell() + len(data) > self.MAX_REQUEST_SIZE: logger.info( "Aborting connection from %s because the request exceeds maximum size", - self.client.host, + self.client, ) assert self.transport is not None self.transport.abortConnection() From d0831c631a729d0b7ba35218cb28ef22aeed7d64 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Thu, 11 Nov 2021 11:27:24 -0800 Subject: [PATCH 3/4] remove unsued type stub directory --- stubs/twisted/internet/__init__.pyi | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 stubs/twisted/internet/__init__.pyi diff --git a/stubs/twisted/internet/__init__.pyi b/stubs/twisted/internet/__init__.pyi deleted file mode 100644 index e69de29b..00000000 From 6a9497cd5820f84d24f032306b6d20110e7390c5 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 16 Nov 2021 08:23:14 -0800 Subject: [PATCH 4/4] add changelog and fix lints --- changelog.d/273.misc | 1 + sygnal/http.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 changelog.d/273.misc diff --git a/changelog.d/273.misc b/changelog.d/273.misc new file mode 100644 index 00000000..daf788e8 --- /dev/null +++ b/changelog.d/273.misc @@ -0,0 +1 @@ +Add type hints to sygnal/http.py. \ No newline at end of file diff --git a/sygnal/http.py b/sygnal/http.py index 98e0002f..d816574a 100644 --- a/sygnal/http.py +++ b/sygnal/http.py @@ -40,7 +40,6 @@ InvalidNotificationException, NotificationDispatchException, ) - from sygnal.notifications import Notification, NotificationContext, Pushkin from sygnal.utils import NotificationLoggerAdapter, json_decoder @@ -81,6 +80,7 @@ labelnames=["resource"], ) + class V1NotifyHandler(Resource): def __init__(self, sygnal: "Sygnal"): super().__init__()