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

Remove unused parameter from, and add safeguard in, get_room_data #8174

Merged
merged 5 commits into from
Aug 26, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
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
1 change: 1 addition & 0 deletions changelog.d/8174.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove unused `is_guest` parameter from `MessageHandler.get_room_data`.
10 changes: 3 additions & 7 deletions synapse/handlers/message.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,12 +95,7 @@ def __init__(self, hs):
)

async def get_room_data(
self,
user_id: str,
room_id: str,
event_type: str,
state_key: str,
is_guest: bool,
self, user_id: str, room_id: str, event_type: str, state_key: str,
) -> dict:
""" Get data from a room.

Expand All @@ -109,7 +104,6 @@ async def get_room_data(
room_id
event_type
state_key
is_guest
Returns:
The path data content.
Raises:
Expand All @@ -130,6 +124,8 @@ async def get_room_data(
[membership_event_id], StateFilter.from_types([key])
)
data = room_state[membership_event_id].get(key)
else:
raise SynapseError(403, "User not in room", errcode=Codes.FORBIDDEN)
Copy link
Member

Choose a reason for hiding this comment

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

This looked like a behavior change to me, but I think it is just ensuring that we raise a SynapseError instead of a NameError (from data not being defined).

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I've just realised that check_user_in_room will either return a Membership.JOIN or Membership.LEAVE, or raise an AuthError otherwise. check_user_in_room_or_world_readable, which is the calling function, will catch that AuthError, and then either raise another AuthError, or return Membership.JOIN.

Therefore, there isn't a case where data will be something other than Membership.JOIN or Membership.LEAVE. Either it's one of those are an AuthError is raised with essentially the same error message I've already put in.

However, it's still probably a good idea to cover our bases here in case the code below changes. Or, we could just add a comment explaining why there isn't an else statement, but my IDE would still complain :)

Copy link
Member

Choose a reason for hiding this comment

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

Having the else statement seems fine to me, although I suppose it could hide an error in those methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

True. I can stick a log line in?

Copy link
Member Author

Choose a reason for hiding this comment

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

Have added, let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

@anoadragon453 I think those changes seem OK.


return data

Expand Down
1 change: 0 additions & 1 deletion synapse/rest/client/v1/room.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,6 @@ async def on_GET(self, request, room_id, event_type, state_key):
room_id=room_id,
event_type=event_type,
state_key=state_key,
is_guest=requester.is_guest,
)

if not data:
Expand Down
2 changes: 1 addition & 1 deletion tests/rest/client/test_retention.py
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ def _test_retention_event_purged(self, room_id: str, increment: float):
message_handler = self.hs.get_message_handler()
create_event = self.get_success(
message_handler.get_room_data(
self.user_id, room_id, EventTypes.Create, state_key="", is_guest=False
self.user_id, room_id, EventTypes.Create, state_key=""
)
)

Expand Down