add Module API callback for user directory search #12318

Samonitari · 2022-03-29T06:40:08Z

Signed-off-by: Krisztian Szegi k.git.hub@meszaros-szegi.cloud

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
- Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
Pull request includes a sign off
Code style is correct
(run the linters)

Signed-off-by: Krisztian Szegi <k.git.hub@meszaros-szegi.cloud>

Samonitari · 2022-03-29T06:48:07Z

synapse/handlers/user_directory.py

@@ -97,6 +108,23 @@ async def search_users(
        """
        results = await self.store.search_user_dir(user_id, search_term, limit)

+        merged_results = results["results"]
+        existing_user_ids = [user["user_id"] for user in merged_results]


merging the external user directory with synapse's works so that: if user_id match -> user match.
I don't know if it is realistic that the display name or avatar is different in the "backend", but this seems like the safe(r) thing to do

I feel like we'd want the external backend to have the last word here, so I think we can just call results["results"].update(external_users).

synapse/handlers/user_directory.py

Samonitari · 2022-03-29T07:19:20Z

I'd like this to be merged, so #12247 could be closed.
I could then add the callback to matrix-synapse-ldap3 auth backend, making the latter a 100% replacement for the dead ma1sd, at least in a LDAP based setup.

dklimpel · 2022-03-29T20:10:39Z

Is there no docs needed for synapse/docs/modules/?

Samonitari · 2022-03-30T07:00:51Z

Fair enough - I'll add the docs, and remove the commented out loop!

babolivier

Thanks for your contribution! I've left a few points I'd like to see addressed before we can merge this. I'd also like to see some tests for this new callback - see this snippet for a few examples of tests that involve module callbacks. I'd also like to see some documentation about this, but from the previous comments on this PR it looks like you're already on it.

babolivier · 2022-03-30T13:45:47Z

synapse/handlers/user_directory.py

@@ -28,6 +28,9 @@

 logger = logging.getLogger(__name__)

+# Types for callbacks to be registered via the module api
+ON_SEARH_USERS_CALLBACK = Callable[[str, str, int], Awaitable[Optional[SearchResult]]]


Suggested change

ON_SEARH_USERS_CALLBACK = Callable[[str, str, int], Awaitable[Optional[SearchResult]]]

ON_SEARCH_USERS_CALLBACK = Callable[[str, str, int], Awaitable[Optional[SearchResult]]]

babolivier · 2022-03-30T13:59:56Z

synapse/handlers/user_directory.py

@@ -28,6 +28,9 @@

 logger = logging.getLogger(__name__)

+# Types for callbacks to be registered via the module api
+ON_SEARH_USERS_CALLBACK = Callable[[str, str, int], Awaitable[Optional[SearchResult]]]


I would also like this callback's name to better reflect what it does. on_action implies that the callback allows modules to react after action has happened (which is also the logic other most on_[...] callbacks follow, with an exception or two). In this case, it implies that this callback is called after a user directory search is complete.

In this case, maybe something like get_external_results would cut it?

Fair enough, I didn't consider the sequential meaning on_[...] would bring
get_external_results feels a bit too general, and external potentially isn't the most correct either, as these users could be already mirrored to database.
SEARCH_USERS_CALLBACK simply?

search_users feels to me like it can be a bit misleading as the callback is only for searching users that are on an separate platform (ie not Matrix users that are either local to the homeserver or share rooms with the server).
search_external_users could be a good middle ground, wdyt?

babolivier · 2022-03-30T14:34:14Z

synapse/handlers/user_directory.py

@@ -97,6 +108,23 @@ async def search_users(
        """
        results = await self.store.search_user_dir(user_id, search_term, limit)

+        merged_results = results["results"]
+        existing_user_ids = [user["user_id"] for user in merged_results]


I feel like we'd want the external backend to have the last word here, so I think we can just call results["results"].update(external_users).

babolivier · 2022-03-30T15:10:46Z

synapse/handlers/user_directory.py

+            external_users = await callback(user_id, search_term, limit)
+            if external_users is not None:
+                merged_results += [
+                    user
+                    for user in external_users["results"]
+                    if user["user_id"] not in existing_user_ids
+                ]


I'd like to see some error handling around this so that a faulty module doesn't completely disable user search. See this snippet for inspiration.

Fair enough, correcting

babolivier · 2022-03-30T15:15:05Z

synapse/handlers/user_directory.py

+                ]
+
+        results["results"] = merged_results
+        results["limited"] = len(merged_results) > limit


I don't think it makes much sense to update limited like this. We do this in search_user_dir because we request limit + 1 elements from the database (but even that is a bit meh), but this doesn't really apply here. I think we should just leave results["limited"] as it is.

I did only really understood the logic behind limited after I committed.
SQL query does not report if the limit was needed or not - so the +1 is a workaround so we can now if there are more users. And that is a bit "meh":
Maybe the workaround in synapse/storage/databases/main/user_directory could also strip the the last found result if query returned limit + 1, as: technically we maybe are falsely reporting limited when in fact, there is exactly limit+1 users.

OTOH, the correct merging would be maybe:

if limited is false before merge:

pass limit - database_result_size to callback

callback should also return limit - database_result_size + 1 if more users are found than requested

check the callback's returned result size, set the limited accordingly, and merge all or all - 1

if limited is true before merge, don't merge at all

Just to clarify, this would mean that if the database query yielded a limited result set we would only show results in the external backend if their user ID appears in the result set from the database. That's assuming we change the merge logic as suggested in #12318 (comment). Otherwise it means we don't show results (and presumably don't call the callback at all) unless the database doesn't manage to provide a limit amount of results.

This I guess would make sense even though it's not a super obvious behaviour at first. Though it sounds to me like it would fill a rather niche case; in my experience it would be more logical to expect the other way around, with the custom external backend being the source of truth and falling back to Synapse's generic search to fill in the blanks. I think doing it this way would be more helpful.

babolivier

Very sorry about the delay in my review, this had fallen off my radar somehow. I've added a few thoughts, though as mentioned before I'd also like to see some test for this. The callback will also need to be documented, see https://github.com/matrix-org/synapse/tree/master/docs/modules for reference.

babolivier · 2022-08-08T08:47:33Z

synapse/handlers/user_directory.py

@@ -28,6 +28,9 @@

 logger = logging.getLogger(__name__)

+# Types for callbacks to be registered via the module api
+ON_SEARH_USERS_CALLBACK = Callable[[str, str, int], Awaitable[Optional[SearchResult]]]


search_users feels to me like it can be a bit misleading as the callback is only for searching users that are on an separate platform (ie not Matrix users that are either local to the homeserver or share rooms with the server).
search_external_users could be a good middle ground, wdyt?

babolivier · 2022-08-08T13:26:30Z

synapse/handlers/user_directory.py

+                ]
+
+        results["results"] = merged_results
+        results["limited"] = len(merged_results) > limit


Just to clarify, this would mean that if the database query yielded a limited result set we would only show results in the external backend if their user ID appears in the result set from the database. That's assuming we change the merge logic as suggested in #12318 (comment). Otherwise it means we don't show results (and presumably don't call the callback at all) unless the database doesn't manage to provide a limit amount of results.

This I guess would make sense even though it's not a super obvious behaviour at first. Though it sounds to me like it would fill a rather niche case; in my experience it would be more logical to expect the other way around, with the custom external backend being the source of truth and falling back to Synapse's generic search to fill in the blanks. I think doing it this way would be more helpful.

janpawellek · 2022-08-23T16:02:35Z

Cool, thank you for the PR 🥳

We also need this feature, thus your PR looks very promising. Keep up your good work!
Looking forward to get it into Synapse 😃

clokep · 2022-10-19T19:43:30Z

@Samonitari or @janpawellek -- do you wish to continue this PR? It looks like it needs some updates and the feedback addressed.

Samonitari · 2022-11-04T08:49:54Z

Yes, but to be a bit more direct - and to not make any bloody estimation lies - sometime this year I'll do this.
I'll keep you updated, but the next two weeks I am consolidating two hosting servers into one - I am halfway there!

add Module API callback for user directory search

02303aa

Signed-off-by: Krisztian Szegi <k.git.hub@meszaros-szegi.cloud>

Samonitari requested a review from a team as a code owner March 29, 2022 06:40

Samonitari commented Mar 29, 2022

View reviewed changes

synapse/handlers/user_directory.py Show resolved Hide resolved

Remove commented out loop version of user merging

b2dbb30

babolivier suggested changes Mar 30, 2022

View reviewed changes

Catch exceptions of search user callbacck handling

1bb0679

babolivier self-requested a review April 14, 2022 13:09

babolivier suggested changes Aug 8, 2022

View reviewed changes

DMRobertson added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Aug 23, 2022

matrixbot mentioned this pull request Dec 21, 2023

add Module API callback for user directory search element-hq/synapse#12318

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Module API callback for user directory search #12318

add Module API callback for user directory search #12318

Samonitari commented Mar 29, 2022

Samonitari Mar 29, 2022

babolivier Mar 30, 2022

Samonitari commented Mar 29, 2022

dklimpel commented Mar 29, 2022

Samonitari commented Mar 30, 2022

babolivier left a comment

babolivier Mar 30, 2022

babolivier Mar 30, 2022

Samonitari Mar 31, 2022

babolivier Aug 8, 2022

babolivier Mar 30, 2022

babolivier Mar 30, 2022

Samonitari Mar 31, 2022

babolivier Mar 30, 2022

Samonitari Mar 31, 2022 •

edited

Loading

babolivier Aug 8, 2022

babolivier left a comment

babolivier Aug 8, 2022

babolivier Aug 8, 2022

janpawellek commented Aug 23, 2022

clokep commented Oct 19, 2022

Samonitari commented Nov 4, 2022

	ON_SEARH_USERS_CALLBACK = Callable[[str, str, int], Awaitable[Optional[SearchResult]]]
	ON_SEARCH_USERS_CALLBACK = Callable[[str, str, int], Awaitable[Optional[SearchResult]]]

add Module API callback for user directory search #12318

Are you sure you want to change the base?

add Module API callback for user directory search #12318

Conversation

Samonitari commented Mar 29, 2022

Pull Request Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Samonitari commented Mar 29, 2022

dklimpel commented Mar 29, 2022

Samonitari commented Mar 30, 2022

babolivier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Samonitari Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babolivier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janpawellek commented Aug 23, 2022

clokep commented Oct 19, 2022

Samonitari commented Nov 4, 2022

Samonitari Mar 31, 2022 •

edited

Loading