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

Deduplicate is_server_notices_room. #13780

Merged
merged 6 commits into from
Sep 14, 2022
Merged

Deduplicate is_server_notices_room. #13780

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2fbc514
Deduplicate is_server_notice_room
reivilibre Sep 9, 2022
8356925
Newsfile
reivilibre Sep 12, 2022
f51e9ef
Merge branch 'develop' into rei/dedupe_is_server_notice_room
reivilibre Sep 12, 2022
f811977
Merge branch 'develop' into rei/dedupe_is_server_notice_room
reivilibre Sep 13, 2022
75f0346
Move to the datastore instead
reivilibre Sep 14, 2022
cc82b78
Add docstring
reivilibre Sep 14, 2022
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/13780.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Deduplicate `is_server_notices_room`.
12 changes: 3 additions & 9 deletions synapse/handlers/message.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,20 +752,14 @@ async def _is_exempt_from_privacy_policy(
if builder.type == EventTypes.Member:
membership = builder.content.get("membership", None)
if membership == Membership.JOIN:
return await self._is_server_notices_room(builder.room_id)
return await self.hs.get_room_member_handler().is_server_notice_room(
Copy link
Contributor

Choose a reason for hiding this comment

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

This introduces a dependency MessageHandler -> RoomMemberHandler.

Does it make sense for handlers to interdepend?

(I expect this is no worse than we do elsewhere, so this is probably fine)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one of those things that I don't know the answer to myself, actually. It seems objectively worse to have the redundancy though, but maybe I'll ask around...

Copy link
Member

Choose a reason for hiding this comment

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

We try and avoid it where possible, as we need to make sure that RoomMemberHandler is initialised before we start the homeserver (as handlers may raise on config errors etc), and doing inline hs.get_* obviously means we can't necessarily guarantee that if all other usages also get inlined.

In this particular case: it feels like is_server_notices_room could either be moved into ServerNoticesManager or into the data store?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I never know what the distinction is meant to be between managers and handlers, but a note:

ServerNoticesManager calls into RoomMemberHandler (it takes a reference to it in __init__ too).

If I move the function into there, we'll still have a circular dependency.

I didn't really realise that get_* was doing lazy initialisation.

I'll try putting it into the data store.

builder.room_id
)
elif membership == Membership.LEAVE:
# the user is always allowed to leave (but not kick people)
return builder.state_key == requester.user.to_string()
return False

async def _is_server_notices_room(self, room_id: str) -> bool:
if self.config.servernotices.server_notices_mxid is None:
return False
is_server_notices_room = await self.store.check_local_user_in_room(
user_id=self.config.servernotices.server_notices_mxid, room_id=room_id
)
return is_server_notices_room

async def assert_accepted_privacy_policy(self, requester: Requester) -> None:
"""Check if a user has accepted the privacy policy

Expand Down
4 changes: 2 additions & 2 deletions synapse/handlers/room_member.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -837,7 +837,7 @@ async def update_membership_locked(
old_membership == Membership.INVITE
and effective_membership_state == Membership.LEAVE
):
is_blocked = await self._is_server_notice_room(room_id)
is_blocked = await self.is_server_notice_room(room_id)
if is_blocked:
raise SynapseError(
HTTPStatus.FORBIDDEN,
Expand Down Expand Up @@ -1617,7 +1617,7 @@ async def _is_host_in_room(self, current_state_ids: StateMap[str]) -> bool:

return False

async def _is_server_notice_room(self, room_id: str) -> bool:
async def is_server_notice_room(self, room_id: str) -> bool:
if self._server_notices_mxid is None:
return False
is_server_notices_room = await self.store.check_local_user_in_room(
Expand Down