Add a bulk user info endpoint and deprecate the old one #46
Conversation
We can't just use a federation query here as we need to send a JSON body of information. This makes things slightly more complex, but it basically boils down to creating server and client methods. The server method had some slight annoyance in that you're supposed to do all the logic in FederationServer, however the logic already exists in the data_store. You can only access the data_store from the handler, so... cue relatively useless method in FederationServer.
And deprecate the single-user one. We also update the old one to use the new store method.
anoadragon453
force-pushed
the
anoa/bulk_user_info_endpoint
branch
from
68239bc
to
134ada0
Compare
| Returns a dictionary of user_id to info dictionary. Supports remote users | ||
| """ | ||
|
|
||
| PATTERNS = client_patterns("/user/info$") |
There was a problem hiding this comment.
More of a nitpick, but we probably want to explicitly not serve this endpoint on anything else than /unstable (releases=(0,)) given it's not spec'd. This also applies to the non-bulk one (except if Tchap already relies on it being served on /r0 in which case ugh fine I guess).
There was a problem hiding this comment.
Good point, I definitely agree. I've switched the new endpoint to /unstable.
@giomfo Do you know whether tchap uses /_matrix/client/r0/user/info or _matrix/client/unstable/user/info?
There was a problem hiding this comment.
The current implementation on ios and android was using /r0, but that's ok to switch on /unstable the corresponding code is not used in the current published mobile applications.
I just have to check this point on tchap-web side, but we will handle it. You can go for /unstable
There was a problem hiding this comment.
As the single-user version is being deprecated I'll just leave it as it is for now.
But the bulk version will be /unstable over (POST /_matrix/client/unstable/users/info).
There was a problem hiding this comment.
lgtm otherwise, though would like to hear from @giomfo on #46 (comment) first (plus my unresolved point wrt type hints)
The current
/user/<user_id>/infoAPI was useful in that it could be used by any user to lookup whether another user was deactivate or expired. However, it was impractical as it only allowed for a single lookup at once. Clients trying to use this API were met with speed issues as they tried to query this information for all users in a room.This PR adds an equivalent CS and Federation API that takes a list of user IDs, and returning a mapping from user ID to info dictionary.
Note that the federation in this PR was a bit trickier than in the original #12 as we can no longer use a federation query, as those don't allow for JSON bodies - which we require to pass a list of user IDs. Instead we do the whole thing of adding a method to transport/client and transport/server.
This PR also adds unittests. The earlier PR used Sytest, presumably for testing across federation, but as this is Synapse-specific that felt a little gross. Unit tests for the deprecated endpoint have not been added.
Best to be reviewed commit-by-commit.