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

Use the proper Request in type hints. #9515

Merged
merged 9 commits into from
Mar 1, 2021
Merged

Use the proper Request in type hints. #9515

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
82df4c4
Use the proper Request in handlers.
clokep Mar 1, 2021
ad57f41
Use the proper request in media.
clokep Mar 1, 2021
bea9314
Use the proper request in rest/synapse.
clokep Mar 1, 2021
d54cf70
Use the proper request in rest/client.
clokep Mar 1, 2021
16b96eb
Use the proper request in replication.
clokep Mar 1, 2021
c040e42
Pin Twisted when checking mypy for now.
clokep Mar 1, 2021
b0e0c72
Newsfragment
clokep Mar 1, 2021
d91327d
Clarify comment.
clokep Mar 1, 2021
8cbc815
Add back the replication requester.
clokep Mar 1, 2021
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/9515.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix incorrect type hints.
4 changes: 2 additions & 2 deletions synapse/handlers/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
import bcrypt
import pymacaroons

from twisted.web.http import Request
from twisted.web.server import Request

from synapse.api.constants import LoginType
from synapse.api.errors import (
Expand Down Expand Up @@ -481,7 +481,7 @@ async def check_ui_auth(
sid = authdict["session"]

# Convert the URI and method to strings.
uri = request.uri.decode("utf-8")
uri = request.uri.decode("utf-8") # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

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

The type of request.uri is incorrectly specified in Twisted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well it isn't "incorrect", but the placeholder is a string and the value is bytes, which is annoying.

Copy link
Contributor

@ShadowJonathan ShadowJonathan Mar 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

(Maybe make notes on the places where this happens, and submit that issue on twisted/twisted?)

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'm already planning to.

method = request.method.decode("utf-8")

# If there's no session ID, create a new session.
Expand Down
2 changes: 1 addition & 1 deletion synapse/handlers/sso.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@
import attr
from typing_extensions import NoReturn, Protocol

from twisted.web.http import Request
from twisted.web.iweb import IRequest
from twisted.web.server import Request

from synapse.api.constants import LoginType
from synapse.api.errors import Codes, NotFoundError, RedirectException, SynapseError
Expand Down
8 changes: 1 addition & 7 deletions synapse/replication/http/membership.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
import logging
from typing import TYPE_CHECKING, List, Optional, Tuple

from twisted.web.http import Request
from twisted.web.server import Request

from synapse.http.servlet import parse_json_object_from_request
from synapse.replication.http._base import ReplicationEndpoint
Expand Down Expand Up @@ -85,10 +85,6 @@ async def _handle_request( # type: ignore
remote_room_hosts = content["remote_room_hosts"]
event_content = content["content"]

requester = Requester.deserialize(self.store, content["requester"])

request.requester = requester
Copy link
Member Author

Choose a reason for hiding this comment

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

Here (and below) the request is never used after this line, so I don't think this is doing anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

How confident are we? If we're getting request as an input parameter and than mutating it as a side effect, might that have issues elsewhere?

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'm not sure what you're asking. Are you asking if we're confident in this change or if the code before this change might have been masking other bugs or something?

Note that this is the replication API, which I don't think has any concept of requester? The Requester.deserialize doesn't do much except generate an object from a string, so it shouldn't have any real side-effects. Maybe this is here to change logging?

@erikjohnston do you know what the proper thing to do here is?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think is so that the requester is logged, rather than the default {None}. We should probably make it happen automatically, somehow, rather than having it only work for a subset of the replication APIs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, OK. The weird thing is that that implies this is actually a SynapseRequest then, not a Request. I'll see if I can rework this bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

8cbc815 reverts removing this.


logger.info("remote_join: %s into room: %s", user_id, room_id)

event_id, stream_id = await self.federation_handler.do_invite_join(
Expand Down Expand Up @@ -156,8 +152,6 @@ async def _handle_request( # type: ignore

requester = Requester.deserialize(self.store, content["requester"])

request.requester = requester

# hopefully we're now on the master, so this won't recurse!
event_id, stream_id = await self.member_handler.remote_reject_invite(
invite_event_id,
Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/admin/media.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import logging
from typing import TYPE_CHECKING, Tuple

from twisted.web.http import Request
from twisted.web.server import Request

from synapse.api.errors import AuthError, Codes, NotFoundError, SynapseError
from synapse.http.servlet import RestServlet, parse_boolean, parse_integer
Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/client/v2_alpha/groups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
from functools import wraps
from typing import TYPE_CHECKING, Optional, Tuple

from twisted.web.http import Request
from twisted.web.server import Request

from synapse.api.constants import (
MAX_GROUP_CATEGORYID_LENGTH,
Expand Down
18 changes: 10 additions & 8 deletions synapse/rest/media/v1/_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

from twisted.internet.interfaces import IConsumer
from twisted.protocols.basic import FileSender
from twisted.web.http import Request
from twisted.web.server import Request

from synapse.api.errors import Codes, SynapseError, cs_error
from synapse.http.server import finish_request, respond_with_json
Expand Down Expand Up @@ -49,18 +49,20 @@

def parse_media_id(request: Request) -> Tuple[str, str, Optional[str]]:
try:
# The type on postpath seems incorrect in Twisted.
clokep marked this conversation as resolved.
Show resolved Hide resolved
postpath = request.postpath # type: List[bytes] # type: ignore
assert postpath

# This allows users to append e.g. /test.png to the URL. Useful for
# clients that parse the URL to see content type.
server_name, media_id = request.postpath[:2]

if isinstance(server_name, bytes):
Copy link
Member Author

Choose a reason for hiding this comment

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

This is always bytes so I don't think we need this if-statement.

server_name = server_name.decode("utf-8")
media_id = media_id.decode("utf8")
server_name_bytes, media_id_bytes = postpath[:2]
server_name = server_name_bytes.decode("utf-8")
media_id = media_id_bytes.decode("utf8")

file_name = None
if len(request.postpath) > 2:
if len(postpath) > 2:
try:
file_name = urllib.parse.unquote(request.postpath[-1].decode("utf-8"))
file_name = urllib.parse.unquote(postpath[-1].decode("utf-8"))
except UnicodeDecodeError:
pass
return server_name, media_id, file_name
Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/media/v1/config_resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

from typing import TYPE_CHECKING

from twisted.web.http import Request
from twisted.web.server import Request

from synapse.http.server import DirectServeJsonResource, respond_with_json

Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/media/v1/download_resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
import logging
from typing import TYPE_CHECKING

from twisted.web.http import Request
from twisted.web.server import Request

from synapse.http.server import DirectServeJsonResource, set_cors_headers
from synapse.http.servlet import parse_boolean
Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/media/v1/media_repository.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@

import twisted.internet.error
import twisted.web.http
from twisted.web.http import Request
from twisted.web.resource import Resource
from twisted.web.server import Request

from synapse.api.errors import (
FederationDeniedError,
Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/media/v1/preview_url_resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
import attr

from twisted.internet.error import DNSLookupError
from twisted.web.http import Request
from twisted.web.server import Request

from synapse.api.errors import Codes, SynapseError
from synapse.http.client import SimpleHttpClient
Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/media/v1/thumbnail_resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import logging
from typing import TYPE_CHECKING, Any, Dict, List, Optional

from twisted.web.http import Request
from twisted.web.server import Request

from synapse.api.errors import SynapseError
from synapse.http.server import DirectServeJsonResource, set_cors_headers
Expand Down
11 changes: 7 additions & 4 deletions synapse/rest/media/v1/upload_resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@
# limitations under the License.

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

from twisted.web.http import Request
from twisted.web.server import Request

from synapse.api.errors import Codes, SynapseError
from synapse.http.server import DirectServeJsonResource, respond_with_json
Expand Down Expand Up @@ -79,7 +79,9 @@ async def _async_render_POST(self, request: Request) -> None:
headers = request.requestHeaders

if headers.hasHeader(b"Content-Type"):
media_type = headers.getRawHeaders(b"Content-Type")[0].decode("ascii")
content_type_headers = headers.getRawHeaders(b"Content-Type")
assert content_type_headers # for mypy
Copy link
Member Author

Choose a reason for hiding this comment

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

We know that the header will exist due to the above check, but mypy doesn't...

media_type = content_type_headers[0].decode("ascii")
else:
raise SynapseError(msg="Upload request missing 'Content-Type'", code=400)

Expand All @@ -88,8 +90,9 @@ async def _async_render_POST(self, request: Request) -> None:
# TODO(markjh): parse content-dispostion

try:
content = request.content # type: IO # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

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

The type of this is undefined right now in Twisted (I think)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why the type: ignore after the IO type definition?

Copy link
Member Author

Choose a reason for hiding this comment

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

It overrides the definition previously calculated.

content_uri = await self.media_repo.create_content(
media_type, upload_name, request.content, content_length, requester.user
media_type, upload_name, content, content_length, requester.user
)
except SpamMediaException:
# For uploading of media we want to respond with a 400, instead of
Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/synapse/client/new_user_consent.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
import logging
from typing import TYPE_CHECKING

from twisted.web.http import Request
from twisted.web.server import Request

from synapse.api.errors import SynapseError
from synapse.handlers.sso import get_username_mapping_session_cookie_from_request
Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/synapse/client/password_reset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
import logging
from typing import TYPE_CHECKING, Tuple

from twisted.web.http import Request
from twisted.web.server import Request

from synapse.api.errors import ThreepidValidationError
from synapse.config.emailconfig import ThreepidBehaviour
Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/synapse/client/pick_username.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
import logging
from typing import TYPE_CHECKING, List

from twisted.web.http import Request
from twisted.web.resource import Resource
from twisted.web.server import Request

from synapse.api.errors import SynapseError
from synapse.handlers.sso import get_username_mapping_session_cookie_from_request
Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/synapse/client/sso_register.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
import logging
from typing import TYPE_CHECKING

from twisted.web.http import Request
from twisted.web.server import Request

from synapse.api.errors import SynapseError
from synapse.handlers.sso import get_username_mapping_session_cookie_from_request
Expand Down
2 changes: 2 additions & 0 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,5 +189,7 @@ commands=
[testenv:mypy]
deps =
{[base]deps}
# Type hints are broken with Twisted > 20.3.0, see https://github.com/matrix-org/synapse/issues/9513
twisted==20.3.0
extras = all,mypy
commands = mypy