From bb0f520a02e5f7838b4bbd9d0e49a73189406f83 Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Tue, 3 Aug 2021 17:49:52 +0100 Subject: [PATCH 01/13] Moved presence_router to use new callback registering pattern load_legacy_presence_router loads a presence router configured the old way (as opposed to in the modules config section) --- synapse/app/_base.py | 2 + synapse/events/presence_router.py | 168 ++++++++++++++++++++++++------ synapse/module_api/__init__.py | 6 ++ 3 files changed, 143 insertions(+), 33 deletions(-) diff --git a/synapse/app/_base.py b/synapse/app/_base.py index 50a02f51f54a..39e28aff9fb6 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -37,6 +37,7 @@ from synapse.app.phone_stats_home import start_phone_stats_home from synapse.config.homeserver import HomeServerConfig from synapse.crypto import context_factory +from synapse.events.presence_router import load_legacy_presence_router from synapse.events.spamcheck import load_legacy_spam_checkers from synapse.events.third_party_rules import load_legacy_third_party_event_rules from synapse.logging.context import PreserveLoggingContext @@ -370,6 +371,7 @@ def run_sighup(*args, **kwargs): load_legacy_spam_checkers(hs) load_legacy_third_party_event_rules(hs) + load_legacy_presence_router(hs) # If we've configured an expiry time for caches, start the background job now. setup_expire_lru_cache_entries(hs) diff --git a/synapse/events/presence_router.py b/synapse/events/presence_router.py index 6c37c8a7a430..1bed578698c3 100644 --- a/synapse/events/presence_router.py +++ b/synapse/events/presence_router.py @@ -11,45 +11,110 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. - -from typing import TYPE_CHECKING, Dict, Iterable, Set, Union +from typing import ( + TYPE_CHECKING, + Awaitable, + Callable, + Dict, + Iterable, + List, + Optional, + Set, + Union, +) from synapse.api.presence import UserPresenceState +from synapse.util.async_helpers import maybe_awaitable if TYPE_CHECKING: from synapse.server import HomeServer +GET_USERS_FOR_STATES = Callable[ + [Iterable[UserPresenceState]], Awaitable[Dict[str, Set[UserPresenceState]]] +] +GET_INTERESTED_USERS = Callable[[str], Awaitable[Union[Set[str], str]]] + + +def load_legacy_presence_router(hs: "HomeServer"): + """Wrapper that loads a presence router module configured using the old + configuration, and registers the hooks they implement. + """ + + if hs.config.presence_router_module_class is None: + return + + module = hs.config.presence_router_module_class + config = hs.config.presence_router_config + api = hs.get_module_api() + + presence_router = module(config=config, module_api=api) + + # The known hooks. If a module implements a method which name appears in this set, + # we'll want to register it. + presence_router_methods = { + "get_users_for_states", + "get_interested_users", + } + + # All methods that the module provides should be async, but this wasn't enforced + # in the old module system, so we wrap them if needed + def async_wrapper(f: Optional[Callable]) -> Optional[Callable[..., Awaitable]]: + # f might be None if the callback isn't implemented by the module. In this + # case we don't want to register a callback at all so we return None. + if f is None: + return None + + def run(*args, **kwargs): + # mypy doesn't do well across function boundaries so we need to tell it + # f is definitely not None. + assert f is not None + + return maybe_awaitable(f(*args, **kwargs)) + + return run + + # Register the hooks through the module API. + hooks = { + hook: async_wrapper(getattr(presence_router, hook, None)) + for hook in presence_router_methods + } + + api.register_presence_router_callbacks(**hooks) + class PresenceRouter: """ A module that the homeserver will call upon to help route user presence updates to - additional destinations. If a custom presence router is configured, calls will be - passed to that instead. + additional destinations. """ ALL_USERS = "ALL" def __init__(self, hs: "HomeServer"): - self.custom_presence_router = None + # Initially there are no callbacks + self._get_users_for_states_callbacks: List[GET_USERS_FOR_STATES] = [] + self._get_interested_users_callbacks: List[GET_INTERESTED_USERS] = [] - # Check whether a custom presence router module has been configured - if hs.config.presence_router_module_class: - # Initialise the module - self.custom_presence_router = hs.config.presence_router_module_class( - config=hs.config.presence_router_config, module_api=hs.get_module_api() + def register_presence_router_callbacks( + self, + get_users_for_states: Optional[GET_USERS_FOR_STATES] = None, + get_interested_users: Optional[GET_INTERESTED_USERS] = None, + ): + # PresenceRouter modules are required to implement both of these methods + # or neither of them as they are assumed to act in a complementary manner + paired_methods = [get_users_for_states, get_interested_users] + if paired_methods.count(None) == 1: + raise Exception( + "PresenceRouter modules must register neither or both of the paired callbacks: " + "[get_users_for_states, get_interested_users]" ) - # Ensure the module has implemented the required methods - required_methods = ["get_users_for_states", "get_interested_users"] - for method_name in required_methods: - if not hasattr(self.custom_presence_router, method_name): - raise Exception( - "PresenceRouter module '%s' must implement all required methods: %s" - % ( - hs.config.presence_router_module_class.__name__, - ", ".join(required_methods), - ) - ) + # Append the methods provided to the lists of callbacks + if get_users_for_states is not None: + self._get_users_for_states_callbacks.append(get_users_for_states) + + if get_interested_users is not None: + self._get_interested_users_callbacks.append(get_interested_users) async def get_users_for_states( self, @@ -66,14 +131,33 @@ async def get_users_for_states( A dictionary of user_id -> set of UserPresenceState, indicating which presence updates each user should receive. """ - if self.custom_presence_router is not None: - # Ask the custom module - return await self.custom_presence_router.get_users_for_states( - state_updates=state_updates + + # Bail out early without if we don't have any callbacks to run. + if len(self._get_users_for_states_callbacks) == 0: + # Don't include any extra destinations for presence updates + return {} + + # If there are multiple callbacks for get_users_for_state then we want to + # return ALL of the extra destinations, this method joins two sets of extra + # destinations into one + def combine( + dict1: Dict[str, Set[UserPresenceState]], + dict2: Dict[str, Set[UserPresenceState]], + ) -> Dict[str, Set[UserPresenceState]]: + for key, new_entries in dict2.items(): + old_entries = dict1.get(key, set()) + dict1[key] = old_entries.union(new_entries) + + return dict1 + + users_for_states = {} + # run all the callbacks for get_users_for_states and combine the results + for callback in self._get_users_for_states_callbacks: + users_for_states = combine( + users_for_states, await callback(state_updates=state_updates) ) - # Don't include any extra destinations for presence updates - return {} + return users_for_states async def get_interested_users(self, user_id: str) -> Union[Set[str], ALL_USERS]: """ @@ -92,12 +176,30 @@ async def get_interested_users(self, user_id: str) -> Union[Set[str], ALL_USERS] A set of user IDs to return presence updates for, or ALL_USERS to return all known updates. """ - if self.custom_presence_router is not None: + + # Bail out early if we don't have any callbacks to run. + if len(self._get_interested_users_callbacks) == 0: + # Don't report any additional interested users + return set() + + # If there are multiple callbacks for get_interested_users then we want to + # return ALL of the users, this method joins two sets of users into one + def combine( + set1: Union[Set[str], str], + set2: Union[Set[str], str], + ) -> Union[Set[str], str]: + # if one of the two sets is ALL_USERS then the union is also ALL_USERS + if set1 == PresenceRouter.ALL_USERS or set2 == PresenceRouter.ALL_USERS: + return PresenceRouter.ALL_USERS + else: + return set1.union(set2) + + interested_users = set() + # run all the callbacks for get_interested_users and combine the results + for callback in self._get_interested_users_callbacks: # Ask the custom module for interested users - return await self.custom_presence_router.get_interested_users( - user_id=user_id + interested_users = combine( + interested_users, await callback(user_id=user_id) ) - # A custom presence router is not defined. - # Don't report any additional interested users - return set() + return interested_users diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 473812b8e295..9691743b5b66 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -111,6 +111,7 @@ def __init__(self, hs: "HomeServer", auth_handler): self._spam_checker = hs.get_spam_checker() self._account_validity_handler = hs.get_account_validity_handler() self._third_party_event_rules = hs.get_third_party_event_rules() + self._presence_router = hs.get_presence_router() ################################################################################# # The following methods should only be called during the module's initialisation. @@ -130,6 +131,11 @@ def register_third_party_rules_callbacks(self): """Registers callbacks for third party event rules capabilities.""" return self._third_party_event_rules.register_third_party_rules_callbacks + @property + def register_presence_router_callbacks(self): + """Registers callbacks for presence router capabilities.""" + return self._presence_router.register_presence_router_callbacks + def register_web_resource(self, path: str, resource: IResource): """Registers a web resource to be served at the given path. From ca24345a7bde5619d438233483cdf6074fd07d5b Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Tue, 3 Aug 2021 17:51:20 +0100 Subject: [PATCH 02/13] Updated presence router tests The same test bodies used to test two modules, one configured with the legacy style, and the other with the new style --- tests/events/test_presence_router.py | 109 +++++++++++++++++++++++++-- 1 file changed, 104 insertions(+), 5 deletions(-) diff --git a/tests/events/test_presence_router.py b/tests/events/test_presence_router.py index 3f41e9995039..edfb6420f5ca 100644 --- a/tests/events/test_presence_router.py +++ b/tests/events/test_presence_router.py @@ -17,7 +17,7 @@ import attr from synapse.api.constants import EduTypes -from synapse.events.presence_router import PresenceRouter +from synapse.events.presence_router import PresenceRouter, load_legacy_presence_router from synapse.federation.units import Transaction from synapse.handlers.presence import UserPresenceState from synapse.module_api import ModuleApi @@ -34,7 +34,7 @@ class PresenceRouterTestConfig: users_who_should_receive_all_presence = attr.ib(type=List[str], default=[]) -class PresenceRouterTestModule: +class LegacyPresenceRouterTestModule: def __init__(self, config: PresenceRouterTestConfig, module_api: ModuleApi): self._config = config self._module_api = module_api @@ -77,6 +77,53 @@ def parse_config(config_dict: dict) -> PresenceRouterTestConfig: return config +class PresenceRouterTestModule: + def __init__(self, config: PresenceRouterTestConfig, api: ModuleApi): + self._config = config + self._module_api = api + api.register_presence_router_callbacks( + get_users_for_states=self.get_users_for_states, + get_interested_users=self.get_interested_users, + ) + + async def get_users_for_states( + self, state_updates: Iterable[UserPresenceState] + ) -> Dict[str, Set[UserPresenceState]]: + users_to_state = { + user_id: set(state_updates) + for user_id in self._config.users_who_should_receive_all_presence + } + return users_to_state + + async def get_interested_users( + self, user_id: str + ) -> Union[Set[str], PresenceRouter.ALL_USERS]: + if user_id in self._config.users_who_should_receive_all_presence: + return PresenceRouter.ALL_USERS + + return set() + + @staticmethod + def parse_config(config_dict: dict) -> PresenceRouterTestConfig: + """Parse a configuration dictionary from the homeserver config, do + some validation and return a typed PresenceRouterConfig. + + Args: + config_dict: The configuration dictionary. + + Returns: + A validated config object. + """ + # Initialise a typed config object + config = PresenceRouterTestConfig() + + config.users_who_should_receive_all_presence = config_dict.get( + "users_who_should_receive_all_presence" + ) + + return config + + class PresenceRouterTestCase(FederatingHomeserverTestCase): servlets = [ admin.register_servlets, @@ -86,9 +133,17 @@ class PresenceRouterTestCase(FederatingHomeserverTestCase): ] def make_homeserver(self, reactor, clock): - return self.setup_test_homeserver( + hs = self.setup_test_homeserver( federation_transport_client=Mock(spec=["send_transaction"]), ) + # Load the modules into the homeserver + module_api = hs.get_module_api() + for module, config in hs.config.modules.loaded_modules: + module(config=config, api=module_api) + + load_legacy_presence_router(hs) + + return hs def prepare(self, reactor, clock, homeserver): self.sync_handler = self.hs.get_sync_handler() @@ -98,7 +153,7 @@ def prepare(self, reactor, clock, homeserver): { "presence": { "presence_router": { - "module": __name__ + ".PresenceRouterTestModule", + "module": __name__ + ".LegacyPresenceRouterTestModule", "config": { "users_who_should_receive_all_presence": [ "@presence_gobbler:test", @@ -109,7 +164,28 @@ def prepare(self, reactor, clock, homeserver): "send_federation": True, } ) + def test_receiving_all_presence_legacy(self): + self.receiving_all_presence_test_body() + + @override_config( + { + "modules": [ + { + "module": __name__ + ".PresenceRouterTestModule", + "config": { + "users_who_should_receive_all_presence": [ + "@presence_gobbler:test", + ] + }, + }, + ], + "send_federation": True, + } + ) def test_receiving_all_presence(self): + self.receiving_all_presence_test_body() + + def receiving_all_presence_test_body(self): """Test that a user that does not share a room with another other can receive presence for them, due to presence routing. """ @@ -203,7 +279,7 @@ def test_receiving_all_presence(self): { "presence": { "presence_router": { - "module": __name__ + ".PresenceRouterTestModule", + "module": __name__ + ".LegacyPresenceRouterTestModule", "config": { "users_who_should_receive_all_presence": [ "@presence_gobbler1:test", @@ -216,7 +292,30 @@ def test_receiving_all_presence(self): "send_federation": True, } ) + def test_send_local_online_presence_to_with_module_legacy(self): + self.send_local_online_presence_to_with_module_test_body() + + @override_config( + { + "modules": [ + { + "module": __name__ + ".PresenceRouterTestModule", + "config": { + "users_who_should_receive_all_presence": [ + "@presence_gobbler1:test", + "@presence_gobbler2:test", + "@far_away_person:island", + ] + }, + }, + ], + "send_federation": True, + } + ) def test_send_local_online_presence_to_with_module(self): + self.send_local_online_presence_to_with_module_test_body() + + def send_local_online_presence_to_with_module_test_body(self): """Tests that send_local_presence_to_users sends local online presence to a set of specified local and remote users, with a custom PresenceRouter module enabled. """ From 9aef9fb51505e257727ca006990737746b44928f Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Tue, 3 Aug 2021 17:51:59 +0100 Subject: [PATCH 03/13] old config style no longer appears in sample_config --- docs/sample_config.yaml | 14 -------------- synapse/config/server.py | 15 +-------------- 2 files changed, 1 insertion(+), 28 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 16843dd8c9ed..8fc19fa75487 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -108,20 +108,6 @@ presence: # #enabled: false - # Presence routers are third-party modules that can specify additional logic - # to where presence updates from users are routed. - # - presence_router: - # The custom module's class. Uncomment to use a custom presence router module. - # - #module: "my_custom_router.PresenceRouter" - - # Configuration options of the custom module. Refer to your module's - # documentation for available options. - # - #config: - # example_option: 'something' - # Whether to require authentication to retrieve profile data (avatars, # display names) of other users through the client API. Defaults to # 'false'. Note that profile data is also available via the federation diff --git a/synapse/config/server.py b/synapse/config/server.py index 187b4301a04b..871422ea285f 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -248,6 +248,7 @@ def read_config(self, config, **kwargs): self.use_presence = config.get("use_presence", True) # Custom presence router module + # This is the legacy way of configuring it (the config should now be put in the modules section) self.presence_router_module_class = None self.presence_router_config = None presence_router_config = presence_config.get("presence_router") @@ -858,20 +859,6 @@ def generate_config_section( # #enabled: false - # Presence routers are third-party modules that can specify additional logic - # to where presence updates from users are routed. - # - presence_router: - # The custom module's class. Uncomment to use a custom presence router module. - # - #module: "my_custom_router.PresenceRouter" - - # Configuration options of the custom module. Refer to your module's - # documentation for available options. - # - #config: - # example_option: 'something' - # Whether to require authentication to retrieve profile data (avatars, # display names) of other users through the client API. Defaults to # 'false'. Note that profile data is also available via the federation From 3d71c76d450c7f96096d27ed00170addc9d9299c Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Wed, 4 Aug 2021 10:00:20 +0100 Subject: [PATCH 04/13] Updated docs with information on presence_router callbacks --- docs/modules.md | 43 ++++++++++++++++++++++++++++++++++ docs/presence_router_module.md | 17 +++++++------- 2 files changed, 52 insertions(+), 8 deletions(-) diff --git a/docs/modules.md b/docs/modules.md index 9a430390a4cb..2cc9a3111f6f 100644 --- a/docs/modules.md +++ b/docs/modules.md @@ -282,6 +282,49 @@ the request is a server admin. Modules can modify the `request_content` (by e.g. adding events to its `initial_state`), or deny the room's creation by raising a `module_api.errors.SynapseError`. +#### Presence router callbacks + +Presence router callbacks allow module developers to specify additional users (local or remote) +to receive certain presence updates from local users. Presence router callbacks can be +registered using the module API's `register_presence_router_callbacks` method. + +The available presence router callbacks are: + +```python +async def get_users_for_states( + self, + state_updates: Iterable[UserPresenceState], +) -> Dict[str, Set[UserPresenceState]]: +``` +**Requires** `get_interested_users` to also be registered + +An asynchronous method that is passed an iterable of user presence state. This method can +determine whether a given presence update should be sent to certain users. It does this by +returning a dictionary with keys representing local or remote Matrix User IDs, and values +being a python set of synapse.handlers.presence.UserPresenceState instances. + +Synapse will then attempt to send the specified presence updates to each user when possible. + +```python +async def get_interested_users(self, user_id: str) -> Union[Set[str], str] +``` +**Requires** `get_users_for_states` to also be registered + +An asynchronous method that is passed a single Matrix User ID. This method is expected to +return the users that the passed in user may be interested in the presence of. Returned +users may be local or remote. The presence routed as a result of what this method returns +is sent in addition to the updates already sent between users that share a room together. +Presence updates are deduplicated. + +This method should return a python set of Matrix User IDs, or the object +`synapse.events.presence_router.PresenceRouter.ALL_USERS` to indicate that the passed user +should receive presence information for all known users. + +For clarity, if the user `@alice:example.org` is passed to this method, and the Set +`{"@bob:example.com", "@charlie:somewhere.org"}` is returned, this signifies that Alice +should receive presence updates sent by Bob and Charlie, regardless of whether these users +share a room. + ### Porting an existing module that uses the old interface diff --git a/docs/presence_router_module.md b/docs/presence_router_module.md index 4a3e720240c5..42081c69df5a 100644 --- a/docs/presence_router_module.md +++ b/docs/presence_router_module.md @@ -115,11 +115,15 @@ class ExamplePresenceRouter: Args: config: A configuration object. - module_api: An instance of Synapse's ModuleApi. + api: An instance of Synapse's ModuleApi. """ - def __init__(self, config: PresenceRouterConfig, module_api: ModuleApi): + def __init__(self, config: PresenceRouterConfig, api: ModuleApi): self._config = config - self._module_api = module_api + self._module_api = api + api.register_presence_router_callbacks( + get_users_for_states=self.get_users_for_states, + get_interested_users=self.get_interested_users, + ) @staticmethod def parse_config(config_dict: dict) -> PresenceRouterConfig: @@ -221,11 +225,8 @@ Once you've crafted your module and installed it into the same Python environmen Synapse, amend your homeserver config file with the following. ```yaml -presence: - enabled: true - - presence_router: - module: my_module.ExamplePresenceRouter +modules: + - module: my_module.ExamplePresenceRouter config: # Any configuration options for your module. The below is an example. # of setting options for ExamplePresenceRouter. From 54e7e05dd1398a48a0fbeb58771fd316ad70e800 Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Wed, 4 Aug 2021 11:03:00 +0100 Subject: [PATCH 05/13] Added changelog --- changelog.d/10524.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10524.feature diff --git a/changelog.d/10524.feature b/changelog.d/10524.feature new file mode 100644 index 000000000000..288c9bd74e09 --- /dev/null +++ b/changelog.d/10524.feature @@ -0,0 +1 @@ +Port the PresenceRouter module interface to the new generic interface. \ No newline at end of file From 54ca9375690e9242cb9233c932892ec6fd0f45dc Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Fri, 6 Aug 2021 11:26:52 +0100 Subject: [PATCH 06/13] Undid changes to legacy presence router module docs Have replaced with a deprecation notice at the top like was done with spam checkers --- docs/presence_router_module.md | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/docs/presence_router_module.md b/docs/presence_router_module.md index 42081c69df5a..face54fe2bb3 100644 --- a/docs/presence_router_module.md +++ b/docs/presence_router_module.md @@ -1,3 +1,9 @@ +

+This page of the Synapse documentation is now deprecated. For up to date +documentation on setting up or writing a presence router module, please see +this page. +

+ # Presence Router Module Synapse supports configuring a module that can specify additional users @@ -115,15 +121,11 @@ class ExamplePresenceRouter: Args: config: A configuration object. - api: An instance of Synapse's ModuleApi. + module_api: An instance of Synapse's ModuleApi. """ - def __init__(self, config: PresenceRouterConfig, api: ModuleApi): + def __init__(self, config: PresenceRouterConfig, module_api: ModuleApi): self._config = config - self._module_api = api - api.register_presence_router_callbacks( - get_users_for_states=self.get_users_for_states, - get_interested_users=self.get_interested_users, - ) + self._module_api = module_api @staticmethod def parse_config(config_dict: dict) -> PresenceRouterConfig: @@ -225,8 +227,11 @@ Once you've crafted your module and installed it into the same Python environmen Synapse, amend your homeserver config file with the following. ```yaml -modules: - - module: my_module.ExamplePresenceRouter +presence: + enabled: true + + presence_router: + module: my_module.ExamplePresenceRouter config: # Any configuration options for your module. The below is an example. # of setting options for ExamplePresenceRouter. From 2c68a9115ad818decd8b47f500e88267871ea675 Mon Sep 17 00:00:00 2001 From: Azrenbeth <77782548+Azrenbeth@users.noreply.github.com> Date: Fri, 6 Aug 2021 16:29:17 +0100 Subject: [PATCH 07/13] Apply suggestions from code review Co-authored-by: Brendan Abolivier --- synapse/events/presence_router.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/events/presence_router.py b/synapse/events/presence_router.py index 1bed578698c3..c118c43f9b60 100644 --- a/synapse/events/presence_router.py +++ b/synapse/events/presence_router.py @@ -37,7 +37,7 @@ def load_legacy_presence_router(hs: "HomeServer"): """Wrapper that loads a presence router module configured using the old - configuration, and registers the hooks they implement. + configuration, and registers the hooks it implements. """ if hs.config.presence_router_module_class is None: @@ -138,7 +138,7 @@ async def get_users_for_states( return {} # If there are multiple callbacks for get_users_for_state then we want to - # return ALL of the extra destinations, this method joins two sets of extra + # return all of the extra destinations, this method joins two sets of extra # destinations into one def combine( dict1: Dict[str, Set[UserPresenceState]], @@ -183,7 +183,7 @@ async def get_interested_users(self, user_id: str) -> Union[Set[str], ALL_USERS] return set() # If there are multiple callbacks for get_interested_users then we want to - # return ALL of the users, this method joins two sets of users into one + # return all of the users, this method joins two sets of users into one def combine( set1: Union[Set[str], str], set2: Union[Set[str], str], From b9a69b71a5d740cbacaaae15cbe10b6a6e8038ac Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Wed, 11 Aug 2021 10:23:16 +0100 Subject: [PATCH 08/13] Applied suggestions from code review --- docs/modules.md | 30 +++++------ synapse/events/presence_router.py | 88 +++++++++++++++++++++---------- 2 files changed, 73 insertions(+), 45 deletions(-) diff --git a/docs/modules.md b/docs/modules.md index 2cc9a3111f6f..dd6d7941f5a9 100644 --- a/docs/modules.md +++ b/docs/modules.md @@ -293,39 +293,37 @@ The available presence router callbacks are: ```python async def get_users_for_states( self, - state_updates: Iterable[UserPresenceState], -) -> Dict[str, Set[UserPresenceState]]: + state_updates: Iterable[synapse.api.UserPresenceState], +) -> Dict[str, Set[synapse.api.UserPresenceState]]: ``` **Requires** `get_interested_users` to also be registered -An asynchronous method that is passed an iterable of user presence state. This method can -determine whether a given presence update should be sent to certain users. It does this by -returning a dictionary with keys representing local or remote Matrix User IDs, and values -being a python set of synapse.handlers.presence.UserPresenceState instances. +Called when processing updates to the presence state of one or more users. This callback can +be used to instruct the server to forward that presence state to specific users. The module +must return a dictionary that maps from Matrix user IDs (which can be local or remote) to the +UserPresenceState changes that they should be forwarded. Synapse will then attempt to send the specified presence updates to each user when possible. - ```python async def get_interested_users(self, user_id: str) -> Union[Set[str], str] ``` **Requires** `get_users_for_states` to also be registered -An asynchronous method that is passed a single Matrix User ID. This method is expected to -return the users that the passed in user may be interested in the presence of. Returned -users may be local or remote. The presence routed as a result of what this method returns -is sent in addition to the updates already sent between users that share a room together. -Presence updates are deduplicated. +Called when determining which users someone should be able to see the presence state of. This +callback should return complementary results to `get_users_for_state` or the presence info +may not be properly forwarded. + +The callback is given a single Matrix user ID and should return a set of additional Matrix user +ID's who's presence state should be queried. The returned users can be local or remote. -This method should return a python set of Matrix User IDs, or the object -`synapse.events.presence_router.PresenceRouter.ALL_USERS` to indicate that the passed user -should receive presence information for all known users. +Alternatively the callback can return `synapse.events.presence_router.PresenceRouter.ALL_USERS` +to indicate that the user should receive updates from all known users. For clarity, if the user `@alice:example.org` is passed to this method, and the Set `{"@bob:example.com", "@charlie:somewhere.org"}` is returned, this signifies that Alice should receive presence updates sent by Bob and Charlie, regardless of whether these users share a room. - ### Porting an existing module that uses the old interface In order to port a module that uses Synapse's old module interface, its author needs to: diff --git a/synapse/events/presence_router.py b/synapse/events/presence_router.py index c118c43f9b60..41b803fd343a 100644 --- a/synapse/events/presence_router.py +++ b/synapse/events/presence_router.py @@ -27,17 +27,17 @@ from synapse.util.async_helpers import maybe_awaitable if TYPE_CHECKING: - from synapse.server import HomeServer + from synapse.server import HomeServer, logger -GET_USERS_FOR_STATES = Callable[ +GET_USERS_FOR_STATES_CALLBACK = Callable[ [Iterable[UserPresenceState]], Awaitable[Dict[str, Set[UserPresenceState]]] ] -GET_INTERESTED_USERS = Callable[[str], Awaitable[Union[Set[str], str]]] +GET_INTERESTED_USERS_CALLBACK = Callable[[str], Awaitable[Union[Set[str], str]]] def load_legacy_presence_router(hs: "HomeServer"): """Wrapper that loads a presence router module configured using the old - configuration, and registers the hooks it implements. + configuration, and registers the hooks they implement. """ if hs.config.presence_router_module_class is None: @@ -92,19 +92,19 @@ class PresenceRouter: def __init__(self, hs: "HomeServer"): # Initially there are no callbacks - self._get_users_for_states_callbacks: List[GET_USERS_FOR_STATES] = [] - self._get_interested_users_callbacks: List[GET_INTERESTED_USERS] = [] + self._get_users_for_states_callbacks: List[GET_USERS_FOR_STATES_CALLBACK] = [] + self._get_interested_users_callbacks: List[GET_INTERESTED_USERS_CALLBACK] = [] def register_presence_router_callbacks( self, - get_users_for_states: Optional[GET_USERS_FOR_STATES] = None, - get_interested_users: Optional[GET_INTERESTED_USERS] = None, + get_users_for_states: Optional[GET_USERS_FOR_STATES_CALLBACK] = None, + get_interested_users: Optional[GET_INTERESTED_USERS_CALLBACK] = None, ): # PresenceRouter modules are required to implement both of these methods # or neither of them as they are assumed to act in a complementary manner paired_methods = [get_users_for_states, get_interested_users] if paired_methods.count(None) == 1: - raise Exception( + raise RuntimeError( "PresenceRouter modules must register neither or both of the paired callbacks: " "[get_users_for_states, get_interested_users]" ) @@ -138,7 +138,7 @@ async def get_users_for_states( return {} # If there are multiple callbacks for get_users_for_state then we want to - # return all of the extra destinations, this method joins two sets of extra + # return ALL of the extra destinations, this method joins two sets of extra # destinations into one def combine( dict1: Dict[str, Set[UserPresenceState]], @@ -153,9 +153,32 @@ def combine( users_for_states = {} # run all the callbacks for get_users_for_states and combine the results for callback in self._get_users_for_states_callbacks: - users_for_states = combine( - users_for_states, await callback(state_updates=state_updates) - ) + try: + result = await callback(state_updates) + except Exception as e: + logger.warning("Failed to run module API callback %s: %s", callback, e) + continue + + if not isinstance(result, Dict): + logger.warning( + "Wrong type returned by module API callback %s: %s, expected Dict", + callback, + result, + ) + continue + + for key, new_entries in result.items(): + if not isinstance(new_entries, Set): + logger.warning( + "Wrong type returned by module API callback %s: %s, expected Set", + callback, + new_entries, + ) + break + if key not in users_for_states: + users_for_states[key] = new_entries + else: + users_for_states[key].update(new_entries) return users_for_states @@ -182,24 +205,31 @@ async def get_interested_users(self, user_id: str) -> Union[Set[str], ALL_USERS] # Don't report any additional interested users return set() - # If there are multiple callbacks for get_interested_users then we want to - # return all of the users, this method joins two sets of users into one - def combine( - set1: Union[Set[str], str], - set2: Union[Set[str], str], - ) -> Union[Set[str], str]: - # if one of the two sets is ALL_USERS then the union is also ALL_USERS - if set1 == PresenceRouter.ALL_USERS or set2 == PresenceRouter.ALL_USERS: - return PresenceRouter.ALL_USERS - else: - return set1.union(set2) - interested_users = set() # run all the callbacks for get_interested_users and combine the results for callback in self._get_interested_users_callbacks: - # Ask the custom module for interested users - interested_users = combine( - interested_users, await callback(user_id=user_id) - ) + try: + result = await callback(user_id) + except Exception as e: + logger.warning("Failed to run module API callback %s: %s", callback, e) + continue + + if not isinstance(result, set): + logger.warning( + "Wrong type returned by module API callback %s: %s, expected set", + callback, + result, + ) + continue + + # If one of the callbacks returns ALL_USERS then we can stop calling all + # of the other callbacks, since the set of interested_users is already as + # large as it can possibly be + if result == PresenceRouter.ALL_USERS: + interested_users = PresenceRouter.ALL_USERS + break + + # Add the new interested users to the set + interested_users.update(result) return interested_users From 8fb5363436d515ebaf3d62d3fdabea0d935a475c Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Wed, 11 Aug 2021 11:08:21 +0100 Subject: [PATCH 09/13] Added error handling when calling callbacks Also renamed the callback types to the form of XYZ_CALLBACK --- synapse/events/presence_router.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/synapse/events/presence_router.py b/synapse/events/presence_router.py index 41b803fd343a..b3014a7972d7 100644 --- a/synapse/events/presence_router.py +++ b/synapse/events/presence_router.py @@ -11,6 +11,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import logging from typing import ( TYPE_CHECKING, Awaitable, @@ -27,13 +28,15 @@ from synapse.util.async_helpers import maybe_awaitable if TYPE_CHECKING: - from synapse.server import HomeServer, logger + from synapse.server import HomeServer GET_USERS_FOR_STATES_CALLBACK = Callable[ [Iterable[UserPresenceState]], Awaitable[Dict[str, Set[UserPresenceState]]] ] GET_INTERESTED_USERS_CALLBACK = Callable[[str], Awaitable[Union[Set[str], str]]] +logger = logging.getLogger(__name__) + def load_legacy_presence_router(hs: "HomeServer"): """Wrapper that loads a presence router module configured using the old From 9689e5843548b63937be04d246fcd38f8866d257 Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Wed, 11 Aug 2021 14:09:32 +0100 Subject: [PATCH 10/13] Moved type check to after ALL_USERS check removed now unneeded combine function --- synapse/events/presence_router.py | 27 +++++++-------------------- 1 file changed, 7 insertions(+), 20 deletions(-) diff --git a/synapse/events/presence_router.py b/synapse/events/presence_router.py index b3014a7972d7..cb40c1196d9a 100644 --- a/synapse/events/presence_router.py +++ b/synapse/events/presence_router.py @@ -140,19 +140,6 @@ async def get_users_for_states( # Don't include any extra destinations for presence updates return {} - # If there are multiple callbacks for get_users_for_state then we want to - # return ALL of the extra destinations, this method joins two sets of extra - # destinations into one - def combine( - dict1: Dict[str, Set[UserPresenceState]], - dict2: Dict[str, Set[UserPresenceState]], - ) -> Dict[str, Set[UserPresenceState]]: - for key, new_entries in dict2.items(): - old_entries = dict1.get(key, set()) - dict1[key] = old_entries.union(new_entries) - - return dict1 - users_for_states = {} # run all the callbacks for get_users_for_states and combine the results for callback in self._get_users_for_states_callbacks: @@ -217,6 +204,13 @@ async def get_interested_users(self, user_id: str) -> Union[Set[str], ALL_USERS] logger.warning("Failed to run module API callback %s: %s", callback, e) continue + # If one of the callbacks returns ALL_USERS then we can stop calling all + # of the other callbacks, since the set of interested_users is already as + # large as it can possibly be + if result == PresenceRouter.ALL_USERS: + interested_users = PresenceRouter.ALL_USERS + break + if not isinstance(result, set): logger.warning( "Wrong type returned by module API callback %s: %s, expected set", @@ -225,13 +219,6 @@ async def get_interested_users(self, user_id: str) -> Union[Set[str], ALL_USERS] ) continue - # If one of the callbacks returns ALL_USERS then we can stop calling all - # of the other callbacks, since the set of interested_users is already as - # large as it can possibly be - if result == PresenceRouter.ALL_USERS: - interested_users = PresenceRouter.ALL_USERS - break - # Add the new interested users to the set interested_users.update(result) From 816e4228637081fce1e7339df95e504b1e78fc23 Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Mon, 16 Aug 2021 12:58:38 +0100 Subject: [PATCH 11/13] Added suggestions from code reivew --- docs/modules.md | 22 +++++++++++++--------- synapse/events/presence_router.py | 9 +++++---- synapse/module_api/__init__.py | 6 ++++++ 3 files changed, 24 insertions(+), 13 deletions(-) diff --git a/docs/modules.md b/docs/modules.md index dd6d7941f5a9..a3959dc79d72 100644 --- a/docs/modules.md +++ b/docs/modules.md @@ -293,33 +293,37 @@ The available presence router callbacks are: ```python async def get_users_for_states( self, - state_updates: Iterable[synapse.api.UserPresenceState], -) -> Dict[str, Set[synapse.api.UserPresenceState]]: + state_updates: Iterable["synapse.api.UserPresenceState"], +) -> Dict[str, Set["synapse.api.UserPresenceState"]]: ``` **Requires** `get_interested_users` to also be registered Called when processing updates to the presence state of one or more users. This callback can be used to instruct the server to forward that presence state to specific users. The module must return a dictionary that maps from Matrix user IDs (which can be local or remote) to the -UserPresenceState changes that they should be forwarded. +`UserPresenceState` changes that they should be forwarded. Synapse will then attempt to send the specified presence updates to each user when possible. ```python -async def get_interested_users(self, user_id: str) -> Union[Set[str], str] +async def get_interested_users( + self, + user_id: str +) -> Union[Set[str], "synapse.module_api.ModuleApi.PRESENCE_ALL_USERS"] ``` **Requires** `get_users_for_states` to also be registered Called when determining which users someone should be able to see the presence state of. This -callback should return complementary results to `get_users_for_state` or the presence info +callback should return complementary results to `get_users_for_state` or the presence information may not be properly forwarded. -The callback is given a single Matrix user ID and should return a set of additional Matrix user -ID's who's presence state should be queried. The returned users can be local or remote. +The callback is given the Matrix user ID for the user that's requesting presence data and +should return the Matrix user IDs of the users whose presence state they are allowed to +query. The returned users can be local or remote. -Alternatively the callback can return `synapse.events.presence_router.PresenceRouter.ALL_USERS` +Alternatively the callback can return `synapse.module_api.ModuleApi.PRESENCE_ALL_USERS` to indicate that the user should receive updates from all known users. -For clarity, if the user `@alice:example.org` is passed to this method, and the Set +For example, if the user `@alice:example.org` is passed to this method, and the Set `{"@bob:example.com", "@charlie:somewhere.org"}` is returned, this signifies that Alice should receive presence updates sent by Bob and Charlie, regardless of whether these users share a room. diff --git a/synapse/events/presence_router.py b/synapse/events/presence_router.py index cb40c1196d9a..f9abd6cc5787 100644 --- a/synapse/events/presence_router.py +++ b/synapse/events/presence_router.py @@ -33,7 +33,9 @@ GET_USERS_FOR_STATES_CALLBACK = Callable[ [Iterable[UserPresenceState]], Awaitable[Dict[str, Set[UserPresenceState]]] ] -GET_INTERESTED_USERS_CALLBACK = Callable[[str], Awaitable[Union[Set[str], str]]] +GET_INTERESTED_USERS_CALLBACK = Callable[ + [str], Awaitable[Union[Set[str], "PresenceRouter.ALL_USERS"]] +] logger = logging.getLogger(__name__) @@ -208,10 +210,9 @@ async def get_interested_users(self, user_id: str) -> Union[Set[str], ALL_USERS] # of the other callbacks, since the set of interested_users is already as # large as it can possibly be if result == PresenceRouter.ALL_USERS: - interested_users = PresenceRouter.ALL_USERS - break + return PresenceRouter.ALL_USERS - if not isinstance(result, set): + if not isinstance(result, Set): logger.warning( "Wrong type returned by module API callback %s: %s, expected set", callback, diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 9691743b5b66..cf465e580f74 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -32,6 +32,7 @@ from twisted.web.resource import IResource from synapse.events import EventBase +from synapse.events.presence_router import PresenceRouter from synapse.http.client import SimpleHttpClient from synapse.http.server import ( DirectServeHtmlResource, @@ -113,6 +114,11 @@ def __init__(self, hs: "HomeServer", auth_handler): self._third_party_event_rules = hs.get_third_party_event_rules() self._presence_router = hs.get_presence_router() + ################################################################################# + # Constants for modules to ues + + PRESENCE_ALL_USERS = PresenceRouter.ALL_USERS + ################################################################################# # The following methods should only be called during the module's initialisation. From 6af5c18568d503dd121a7a088038d4af718467d5 Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Tue, 17 Aug 2021 11:20:41 +0100 Subject: [PATCH 12/13] Added newline and moved constant to __all__ --- docs/modules.md | 5 +++-- synapse/module_api/__init__.py | 8 +++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/docs/modules.md b/docs/modules.md index a3959dc79d72..08efddbc9795 100644 --- a/docs/modules.md +++ b/docs/modules.md @@ -304,11 +304,12 @@ must return a dictionary that maps from Matrix user IDs (which can be local or r `UserPresenceState` changes that they should be forwarded. Synapse will then attempt to send the specified presence updates to each user when possible. + ```python async def get_interested_users( self, user_id: str -) -> Union[Set[str], "synapse.module_api.ModuleApi.PRESENCE_ALL_USERS"] +) -> Union[Set[str], "synapse.module_api.PRESENCE_ALL_USERS"] ``` **Requires** `get_users_for_states` to also be registered @@ -320,7 +321,7 @@ The callback is given the Matrix user ID for the user that's requesting presence should return the Matrix user IDs of the users whose presence state they are allowed to query. The returned users can be local or remote. -Alternatively the callback can return `synapse.module_api.ModuleApi.PRESENCE_ALL_USERS` +Alternatively the callback can return `synapse.module_api.PRESENCE_ALL_USERS` to indicate that the user should receive updates from all known users. For example, if the user `@alice:example.org` is passed to this method, and the Set diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 8d2a0a98ff42..1b6e589f6b30 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -58,6 +58,8 @@ are loaded into Synapse. """ +PRESENCE_ALL_USERS = PresenceRouter.ALL_USERS + __all__ = [ "errors", "make_deferred_yieldable", @@ -71,6 +73,7 @@ "DirectServeHtmlResource", "DirectServeJsonResource", "ModuleApi", + "PRESENCE_ALL_USERS", ] logger = logging.getLogger(__name__) @@ -114,11 +117,6 @@ def __init__(self, hs: "HomeServer", auth_handler): self._third_party_event_rules = hs.get_third_party_event_rules() self._presence_router = hs.get_presence_router() - ################################################################################# - # Constants for modules to ues - - PRESENCE_ALL_USERS = PresenceRouter.ALL_USERS - ################################################################################# # The following methods should only be called during the module's initialisation. From 6e27e01417485c0a779a9316d24058c421f737f4 Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Tue, 17 Aug 2021 14:00:48 +0100 Subject: [PATCH 13/13] Applied suggestions from code review --- docs/modules.md | 2 +- synapse/events/presence_router.py | 7 ++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/docs/modules.md b/docs/modules.md index 08efddbc9795..ae8d6f5b731a 100644 --- a/docs/modules.md +++ b/docs/modules.md @@ -317,7 +317,7 @@ Called when determining which users someone should be able to see the presence s callback should return complementary results to `get_users_for_state` or the presence information may not be properly forwarded. -The callback is given the Matrix user ID for the user that's requesting presence data and +The callback is given the Matrix user ID for a local user that is requesting presence data and should return the Matrix user IDs of the users whose presence state they are allowed to query. The returned users can be local or remote. diff --git a/synapse/events/presence_router.py b/synapse/events/presence_router.py index f9abd6cc5787..eb4556cdc10a 100644 --- a/synapse/events/presence_router.py +++ b/synapse/events/presence_router.py @@ -137,7 +137,7 @@ async def get_users_for_states( presence updates each user should receive. """ - # Bail out early without if we don't have any callbacks to run. + # Bail out early if we don't have any callbacks to run. if len(self._get_users_for_states_callbacks) == 0: # Don't include any extra destinations for presence updates return {} @@ -167,10 +167,7 @@ async def get_users_for_states( new_entries, ) break - if key not in users_for_states: - users_for_states[key] = new_entries - else: - users_for_states[key].update(new_entries) + users_for_states.setdefault(key, set()).update(new_entries) return users_for_states