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

Correct type hints for parse_string(s)_from_args. #10137

Merged
merged 9 commits into from
Jun 8, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
58 changes: 57 additions & 1 deletion synapse/http/servlet.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@
""" This module contains base REST classes for constructing REST servlets. """

import logging
from typing import Iterable, List, Optional, Union, overload
from typing import Dict, Iterable, List, Optional, Union, overload

from typing_extensions import Literal

from twisted.web.server import Request

from synapse.api.errors import Codes, SynapseError
from synapse.util import json_decoder

Expand Down Expand Up @@ -108,6 +110,60 @@ def parse_boolean_from_args(args, name, default=None, required=False):
return default


@overload
def parse_bytes(
request: Request,
name: str,
default: Literal[None] = None,
required: Literal[True] = True,
) -> bytes:
...


@overload
def parse_bytes(
request: Request,
name: str,
default: Optional[bytes] = None,
required: bool = False,
) -> Optional[bytes]:
...


def parse_bytes(
Copy link
Member

Choose a reason for hiding this comment

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

parse_bytes seems like a rubbish name to me (as does parse_string).

Suggestion would be that you replace it with parse_bytes_from_args and make it take an args dict (and do request.args in the callers).

Copy link
Member Author

Choose a reason for hiding this comment

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

That seems fine with me! I was mostly matching parse_string (which should maybe be parse_string_from_request, but it really is just parse_string_from_args...)

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed a commit, it does need some typing foo since request.args pops up as Optional[Any], I think this is probably OK though?

Another option would be to do parse_bytes_from_request (or parse_bytes_from_request_args) and have it still take the Request object.

Copy link
Member

Choose a reason for hiding this comment

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

yet another option might be to allow parse_bytes_from_request to take None and do something plausible?

request: Request,
name: str,
default: Optional[bytes] = None,
required: bool = False,
) -> Optional[bytes]:
"""
Parse a string parameter as bytes from the request query string.

Args:
request: the twisted HTTP request.
name: the name of the query parameter.
default: value to use if the parameter is absent,
defaults to None. Must be bytes if encoding is None.
required: whether to raise a 400 SynapseError if the
parameter is absent, defaults to False.
Returns:
Bytes or the default value.

Raises:
SynapseError if the parameter is absent and required.
"""
args = request.args # type: Dict[bytes, List[bytes]] # type: ignore
name_bytes = name.encode("ascii")

if name_bytes in args:
return args[name_bytes][0]
elif required:
message = "Missing string query parameter %s" % (name,)
raise SynapseError(400, message, errcode=Codes.MISSING_PARAM)

return default


def parse_string(
request,
name: Union[bytes, str],
Expand Down
5 changes: 2 additions & 3 deletions synapse/rest/client/v1/login.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
from synapse.http.server import HttpServer, finish_request
from synapse.http.servlet import (
RestServlet,
parse_bytes,
parse_json_object_from_request,
parse_string,
)
Expand Down Expand Up @@ -437,9 +438,7 @@ async def on_GET(
finish_request(request)
return

client_redirect_url = parse_string(
request, "redirectUrl", required=True, encoding=None
)
client_redirect_url = parse_bytes(request, "redirectUrl", required=True)
sso_url = await self._sso_handler.handle_redirect_request(
request,
client_redirect_url,
Expand Down
6 changes: 3 additions & 3 deletions synapse/rest/consent/consent_resource.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
from synapse.api.errors import NotFoundError, StoreError, SynapseError
from synapse.config import ConfigError
from synapse.http.server import DirectServeHtmlResource, respond_with_html
from synapse.http.servlet import parse_string
from synapse.http.servlet import parse_bytes, parse_string
from synapse.types import UserID

# language to use for the templates. TODO: figure this out from Accept-Language
Expand Down Expand Up @@ -116,7 +116,7 @@ async def _async_render_GET(self, request):
has_consented = False
public_version = username == ""
if not public_version:
userhmac_bytes = parse_string(request, "h", required=True, encoding=None)
userhmac_bytes = parse_bytes(request, "h", required=True)

self._check_hash(username, userhmac_bytes)

Expand Down Expand Up @@ -152,7 +152,7 @@ async def _async_render_POST(self, request):
"""
version = parse_string(request, "v", required=True)
username = parse_string(request, "u", required=True)
userhmac = parse_string(request, "h", required=True, encoding=None)
userhmac = parse_bytes(request, "h", required=True)

self._check_hash(username, userhmac)

Expand Down
10 changes: 5 additions & 5 deletions synapse/rest/media/v1/upload_resource.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@
# limitations under the License.

import logging
from typing import IO, TYPE_CHECKING
from typing import IO, TYPE_CHECKING, Optional

from twisted.web.server import Request

from synapse.api.errors import Codes, SynapseError
from synapse.http.server import DirectServeJsonResource, respond_with_json
from synapse.http.servlet import parse_string
from synapse.http.servlet import parse_bytes
from synapse.http.site import SynapseRequest
from synapse.rest.media.v1.media_storage import SpamMediaException

Expand Down Expand Up @@ -61,10 +61,10 @@ async def _async_render_POST(self, request: SynapseRequest) -> None:
errcode=Codes.TOO_LARGE,
)

upload_name = parse_string(request, b"filename", encoding=None)
if upload_name:
upload_name_bytes = parse_bytes(request, "filename")
if upload_name_bytes:
try:
upload_name = upload_name.decode("utf8")
upload_name = upload_name_bytes.decode("utf8") # type: Optional[str]
except UnicodeDecodeError:
raise SynapseError(
msg="Invalid UTF-8 filename parameter: %r" % (upload_name), code=400
Expand Down