Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSC2356: Bulk /joined_members endpoint #2356

Open
wants to merge 2 commits into
base: master
from

Conversation

@Half-Shot
Copy link
Contributor

Half-Shot commented Nov 15, 2019

@Half-Shot Half-Shot added the proposal label Nov 15, 2019
Copy link
Member

anoadragon453 left a comment

Seems fairly straightforward, just a couple changes.

Should this deprecate the old endpoint?

proposals/2356-bulk-joined-members.md Outdated Show resolved Hide resolved
proposals/2356-bulk-joined-members.md Outdated Show resolved Hide resolved

`joined["room_id"]` follows the same format as `joined` in `/rooms/{roomId}/joined_members`.

Any rooms where membership could not be fetched MUST be returned as null.

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Nov 15, 2019

Member

We could also just not send anything back for these rooms? So instead of:

{
  "!foo:bar": {...},
  "!not:real": null,
  "!not2:real": null,
  "!not3:real": null,
  "!not4:real": null
}

just have:

{
  "!foo:bar": {...}
}

This comment has been minimized.

Copy link
@Half-Shot

Half-Shot Nov 15, 2019

Author Contributor

Yeah, I guess you can make the assumption that the room isn't readable if it's not in the set.

This comment has been minimized.

Copy link
@turt2live

turt2live Nov 15, 2019

Member

yea, let's just not include the room. We do this elsewhere (though I can't remember where).

proposals/2356-bulk-joined-members.md Outdated Show resolved Hide resolved
spelling fixes

Co-Authored-By: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Copy link
Member

turt2live left a comment

seems straightforward as an API which could benefit from other technologies, but those technologies don't exist in the spec yet :(


This proposal covers the creation of a bulk room membership endpoint.

The new endpoint will be called `POST /_matrix/client/r0/joined_members` and will not

This comment has been minimized.

Copy link
@turt2live

turt2live Nov 15, 2019

Member

This makes it sound like you can change the members of the room. /get_joined_members instead to make it sound like an action?


```
{
"rooms": [

This comment has been minimized.

Copy link
@turt2live

turt2live Nov 15, 2019

Member

We're sending IDs, not rooms.

Suggested change
"rooms": [
"room_ids": [

`joined["room_id"]` follows the same format as `joined` in `/rooms/{roomId}/joined_members`.

Any rooms where membership could not be fetched MUST be returned as null.

This comment has been minimized.

Copy link
@turt2live

turt2live Nov 15, 2019

Member

yea, let's just not include the room. We do this elsewhere (though I can't remember where).

## Potential issues

If an implementation is done poorly, it's possible to use this request to wedge
a homeserver by requesting membership from a huge number of rooms. Homeservers COULD

This comment has been minimized.

Copy link
@turt2live

turt2live Nov 15, 2019

Member

we don't have a keyword for COULD. Lowercase it to avoid my complaining:

Suggested change
a homeserver by requesting membership from a huge number of rooms. Homeservers COULD
a homeserver by requesting membership from a huge number of rooms. Homeservers could

or use SHOULD:

Suggested change
a homeserver by requesting membership from a huge number of rooms. Homeservers COULD
a homeserver by requesting membership from a huge number of rooms. Homeservers SHOULD
allow for large payloads.

The time taken to serve this request may exceed the timeout of the request, so similiar semantics
COULD be applied as they are in /sync where the request continues to run on the homeserver

This comment has been minimized.

Copy link
@turt2live

turt2live Nov 15, 2019

Member
Suggested change
COULD be applied as they are in /sync where the request continues to run on the homeserver
SHOULD be applied as they are in /sync where the request continues to run on the homeserver

## Security considerations

None. The endpoint should follow the same authorisation checks that the `/joined_members` endpoint makes.

This comment has been minimized.

Copy link
@turt2live

turt2live Nov 15, 2019

Member

would be good to reinforce this up above by saying the requesting user must be in the room in order to get results, and whatever other appservice-specific semantics (if any beyond impersonation) apply.

COULD be applied as they are in /sync where the request continues to run on the homeserver
and is resumed when the next call to the endpoint is made.

## Alternatives

This comment has been minimized.

Copy link
@turt2live

turt2live Nov 15, 2019

Member

Can we just fix filtering in Synapse and make /sync faster? (which also introduces a use case for appservices to sync...)

Another alternative would be GraphQL.

@@ -0,0 +1,105 @@
# MSC2356 - Bulk /joined_members endpoint

This comment has been minimized.

Copy link
@turt2live

turt2live Nov 15, 2019

Member

Should this deprecate the old endpoint?

(from @anoadragon453 : #2356 (review) )

Please don't.

This comment has been minimized.

Copy link
@Half-Shot

Half-Shot Nov 18, 2019

Author Contributor

Yep, this serves a different use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.