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

Rename various ApplicationServices interested methods #11915

Merged
merged 10 commits into from
Mar 3, 2022
123 changes: 83 additions & 40 deletions synapse/appservice/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -175,27 +175,14 @@ def _is_exclusive(self, namespace_key: str, test_string: str) -> bool:
return namespace.exclusive
return False

async def _matches_user(self, event: EventBase, store: "DataStore") -> bool:
if self.is_interested_in_user(event.sender):
return True

# also check m.room.member state key
if event.type == EventTypes.Member and self.is_interested_in_user(
event.state_key
):
return True

does_match = await self.matches_user_in_member_list(event.room_id, store)
return does_match

@cached(num_args=1, cache_context=True)
async def matches_user_in_member_list(
self,
room_id: str,
store: "DataStore",
cache_context: _CacheContext,
) -> bool:
"""Check if this service is interested a room based upon it's membership
"""Check if this service is interested a room based upon its membership

Args:
room_id: The room to check.
Expand All @@ -214,20 +201,72 @@ async def matches_user_in_member_list(
return True
return False

def _matches_room_id(self, event: EventBase) -> bool:
if hasattr(event, "room_id"):
return self.is_interested_in_room(event.room_id)
return False
def is_interested_in_user(
self,
user_id: str,
) -> bool:
"""
Returns whether the application is interested in a given user ID.

async def _matches_aliases(self, event: EventBase, store: "DataStore") -> bool:
alias_list = await store.get_aliases_for_room(event.room_id)
The appservice is considered to be interested in a user if either: the
user ID is in the appservice's user namespace, or if the user is the
appservice's configured sender_localpart.

Args:
user_id: The ID of the user to check.

Returns:
True if the application service is interested in the user, False if not.
"""
return (
# User is the appservice's sender_localpart user
user_id == self.sender
# User is in the appservice's user namespace
or self.is_user_in_namespace(user_id)
)

@cached(num_args=1, cache_context=True)
async def is_interested_in_room(
self,
room_id: str,
store: "DataStore",
cache_context: _CacheContext,
) -> bool:
"""
Returns whether the application service is interested in a given room ID.

The appservice is considered to be interested in the room if either: the ID or one
of the aliases of the room is in the appservice's room ID or alias namespace
respectively, or if one of the members of the room fall into the appservice's user
namespace.

Args:
room_id: The ID of the room to check.
store: The homeserver's datastore class.

Returns:
True if the application service is interested in the room, False if not.
"""
# Check if we have interest in this room ID
if self.is_room_id_in_namespace(room_id):
return True

# likewise with the room's aliases (if it has any)
alias_list = await store.get_aliases_for_room(room_id)
for alias in alias_list:
if self.is_interested_in_alias(alias):
if self.is_room_alias_in_namespace(alias):
return True

return False
# And finally, perform an expensive check on whether any of the
# users in the room match the appservice's user namespace
return await self.matches_user_in_member_list(
room_id, store, on_invalidate=cache_context.invalidate
)

async def is_interested(self, event: EventBase, store: "DataStore") -> bool:
@cached(num_args=1, cache_context=True)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this will work exactly as we're hoping -- EventBase equality is checked by it being the exact same object (i.e. if you built two EventBase instances with identical properties they aren't equal).

This might be OK, but I'm not 100% sure.

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, good catch. Ideally we'd cache on the event's ID instead of EventBase. I don't suppose there's an easy way to do that with @cached, though I can just create an LruCache much as we do for _get_event_cache:

self._get_event_cache: LruCache[Tuple[str], EventCacheEntry] = LruCache(
cache_name="*getEvent*",
max_size=hs.config.caches.event_cache_size,
)

Copy link
Member

Choose a reason for hiding this comment

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

That might be better, in the past I've wonder if we should define __eq__ on EventBase which compares IDs, but I could see that not...being great in some situations.

Maybe using it is fine though? Might want to check if we do it anywhere else.

Copy link
Member Author

Choose a reason for hiding this comment

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

From the conversation in #synapse-dev, I believe we concluded on adding an event_id argument to is_interested_in_event. Doing so was simple, though updating the tests was slightly tedious.

Done in 453fdac!

async def is_interested_in_event(
self, event: EventBase, store: "DataStore", cache_context: _CacheContext
) -> bool:
"""Check if this service is interested in this event.

Args:
Expand All @@ -237,24 +276,29 @@ async def is_interested(self, event: EventBase, store: "DataStore") -> bool:
Returns:
True if this service would like to know about this event.
"""
# Do cheap checks first
if self._matches_room_id(event):
# Check if we're interested in this event's sender by namespace (or if they're the
# sender_localpart user)
if self.is_interested_in_user(event.sender):
return True

# This will check the namespaces first before
# checking the store, so should be run before _matches_aliases
if await self._matches_user(event, store):
# additionally, if this is a membership event, perform the same checks on
# the user it references
if event.type == EventTypes.Member and self.is_interested_in_user(
event.state_key
):
return True

# This will check the store, so should be run last
if await self._matches_aliases(event, store):
# This will check the datastore, so should be run last
if await self.is_interested_in_room(
event.room_id, store, on_invalidate=cache_context.invalidate
):
return True

return False

@cached(num_args=1)
@cached(num_args=1, cache_context=True)
async def is_interested_in_presence(
self, user_id: UserID, store: "DataStore"
self, user_id: UserID, store: "DataStore", cache_context: _CacheContext
) -> bool:
"""Check if this service is interested a user's presence

Expand All @@ -272,20 +316,19 @@ async def is_interested_in_presence(

# Then find out if the appservice is interested in any of those rooms
for room_id in room_ids:
if await self.matches_user_in_member_list(room_id, store):
if await self.matches_user_in_member_list(
room_id, store, on_invalidate=cache_context.invalidate
):
return True
return False

def is_interested_in_user(self, user_id: str) -> bool:
return (
bool(self._matches_regex(ApplicationService.NS_USERS, user_id))
or user_id == self.sender
)
def is_user_in_namespace(self, user_id: str) -> bool:
return bool(self._matches_regex(ApplicationService.NS_USERS, user_id))

def is_interested_in_alias(self, alias: str) -> bool:
def is_room_alias_in_namespace(self, alias: str) -> bool:
return bool(self._matches_regex(ApplicationService.NS_ALIASES, alias))

def is_interested_in_room(self, room_id: str) -> bool:
def is_room_id_in_namespace(self, room_id: str) -> bool:
return bool(self._matches_regex(ApplicationService.NS_ROOMS, room_id))

def is_exclusive_user(self, user_id: str) -> bool:
Expand Down
4 changes: 2 additions & 2 deletions synapse/handlers/appservice.py
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,7 @@ async def query_room_alias_exists(
room_alias_str = room_alias.to_string()
services = self.store.get_app_services()
alias_query_services = [
s for s in services if (s.is_interested_in_alias(room_alias_str))
s for s in services if (s.is_room_alias_in_namespace(room_alias_str))
]
for alias_service in alias_query_services:
is_known_alias = await self.appservice_api.query_alias(
Expand Down Expand Up @@ -660,7 +660,7 @@ async def _get_services_for_event(
# inside of a list comprehension anymore.
interested_list = []
for s in services:
if await s.is_interested(event, self.store):
if await s.is_interested_in_event(event, self.store):
interested_list.append(s)

return interested_list
Expand Down
6 changes: 3 additions & 3 deletions synapse/handlers/directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ async def create_association(

service = requester.app_service
if service:
if not service.is_interested_in_alias(room_alias_str):
if not service.is_room_alias_in_namespace(room_alias_str):
raise SynapseError(
400,
"This application service has not reserved this kind of alias.",
Expand Down Expand Up @@ -221,7 +221,7 @@ async def delete_association(
async def delete_appservice_association(
self, service: ApplicationService, room_alias: RoomAlias
) -> None:
if not service.is_interested_in_alias(room_alias.to_string()):
if not service.is_room_alias_in_namespace(room_alias.to_string()):
raise SynapseError(
400,
"This application service has not reserved this kind of alias",
Expand Down Expand Up @@ -376,7 +376,7 @@ def can_modify_alias(self, alias: RoomAlias, user_id: Optional[str] = None) -> b
# non-exclusive locks on the alias (or there are no interested services)
services = self.store.get_app_services()
interested_services = [
s for s in services if s.is_interested_in_alias(alias.to_string())
s for s in services if s.is_room_alias_in_namespace(alias.to_string())
]

for service in interested_services:
Expand Down
23 changes: 13 additions & 10 deletions tests/appservice/test_appservice.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def test_regex_user_id_prefix_match(self):
self.assertTrue(
(
yield defer.ensureDeferred(
self.service.is_interested(self.event, self.store)
self.service.is_interested_in_event(self.event, self.store)
)
)
)
Expand All @@ -62,7 +62,7 @@ def test_regex_user_id_prefix_no_match(self):
self.assertFalse(
(
yield defer.ensureDeferred(
self.service.is_interested(self.event, self.store)
self.service.is_interested_in_event(self.event, self.store)
)
)
)
Expand All @@ -76,7 +76,7 @@ def test_regex_room_member_is_checked(self):
self.assertTrue(
(
yield defer.ensureDeferred(
self.service.is_interested(self.event, self.store)
self.service.is_interested_in_event(self.event, self.store)
)
)
)
Expand All @@ -90,7 +90,8 @@ def test_regex_room_id_match(self):
self.assertTrue(
(
yield defer.ensureDeferred(
self.service.is_interested(self.event, self.store)
# We need to provide the store here in order to carry out room checks
self.service.is_interested_in_event(self.event, self.store)
)
)
)
Expand All @@ -104,7 +105,7 @@ def test_regex_room_id_no_match(self):
self.assertFalse(
(
yield defer.ensureDeferred(
self.service.is_interested(self.event, self.store)
self.service.is_interested_in_event(self.event, self.store)
)
)
)
Expand All @@ -121,7 +122,7 @@ def test_regex_alias_match(self):
self.assertTrue(
(
yield defer.ensureDeferred(
self.service.is_interested(self.event, self.store)
self.service.is_interested_in_event(self.event, self.store)
)
)
)
Expand Down Expand Up @@ -174,7 +175,7 @@ def test_regex_alias_no_match(self):
self.assertFalse(
(
yield defer.ensureDeferred(
self.service.is_interested(self.event, self.store)
self.service.is_interested_in_event(self.event, self.store)
)
)
)
Expand All @@ -191,7 +192,7 @@ def test_regex_multiple_matches(self):
self.assertTrue(
(
yield defer.ensureDeferred(
self.service.is_interested(self.event, self.store)
self.service.is_interested_in_event(self.event, self.store)
)
)
)
Expand All @@ -207,7 +208,7 @@ def test_interested_in_self(self):
self.assertTrue(
(
yield defer.ensureDeferred(
self.service.is_interested(self.event, self.store)
self.service.is_interested_in_event(self.event, self.store)
)
)
)
Expand All @@ -225,7 +226,9 @@ def test_member_list_match(self):
self.assertTrue(
(
yield defer.ensureDeferred(
self.service.is_interested(event=self.event, store=self.store)
self.service.is_interested_in_event(
event=self.event, store=self.store
)
)
)
)
17 changes: 9 additions & 8 deletions tests/handlers/test_appservice.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ def test_notify_interested_services(self):
def test_query_user_exists_unknown_user(self):
user_id = "@someone:anywhere"
services = [self._mkservice(is_interested=True)]
services[0].is_interested_in_user.return_value = True
self.mock_store.get_app_services.return_value = services
self.mock_store.get_user_by_id.return_value = make_awaitable(None)

Expand Down Expand Up @@ -127,11 +126,11 @@ def test_query_room_alias_exists(self):

room_id = "!alpha:bet"
servers = ["aperture"]
interested_service = self._mkservice_alias(is_interested_in_alias=True)
interested_service = self._mkservice_alias(is_room_alias_in_namespace=True)
services = [
self._mkservice_alias(is_interested_in_alias=False),
self._mkservice_alias(is_room_alias_in_namespace=False),
interested_service,
self._mkservice_alias(is_interested_in_alias=False),
self._mkservice_alias(is_room_alias_in_namespace=False),
]

self.mock_as_api.query_alias.return_value = make_awaitable(True)
Expand Down Expand Up @@ -325,17 +324,19 @@ def test_notify_interested_services_ephemeral_out_of_order(self):
interested_service, ephemeral=[]
)

def _mkservice(self, is_interested, protocols=None):
def _mkservice(self, is_interested: bool, protocols=None):
clokep marked this conversation as resolved.
Show resolved Hide resolved
service = Mock()
service.is_interested.return_value = make_awaitable(is_interested)
service.is_interested_in_event.return_value = make_awaitable(is_interested)
service.is_interested_in_user.return_value = make_awaitable(is_interested)
service.is_interested_in_room.return_value = make_awaitable(is_interested)
service.token = "mock_service_token"
service.url = "mock_service_url"
service.protocols = protocols
return service

def _mkservice_alias(self, is_interested_in_alias):
def _mkservice_alias(self, is_room_alias_in_namespace):
service = Mock()
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
service.is_interested_in_alias.return_value = is_interested_in_alias
service.is_room_alias_in_namespace.return_value = is_room_alias_in_namespace
service.token = "mock_service_token"
service.url = "mock_service_url"
return service
Expand Down