Skip to content
This repository was 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

Changes from 1 commit
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
Prev Previous commit
Next Next commit
Use the proper request in replication.
  • Loading branch information
@clokep
clokep committed Mar 1, 2021
commit 16b96eb4684d9dccb6a12a5de83a52d08b4a6593
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