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

[WIP] MSC2356: Bulk /joined_members endpoint #2356

Closed
wants to merge 2 commits into from
Closed

Conversation

Half-Shot
Copy link
Contributor

@Half-Shot Half-Shot commented Nov 15, 2019

@Half-Shot Half-Shot added the proposal A matrix spec change proposal label Nov 15, 2019
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

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": {...}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 turt2live left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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": [
Copy link
Member

Choose a reason for hiding this comment

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

Should this deprecate the old endpoint?

(from @anoadragon453 : #2356 (review) )

Please don't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, this serves a different use case.

@turt2live turt2live changed the title MSC2356: Bulk /joined_members endpoint [WIP] MSC2356: Bulk /joined_members endpoint Dec 7, 2019
@turt2live turt2live added the kind:feature MSC for not-core and not-maintenance stuff label Apr 20, 2020
@turt2live turt2live marked this pull request as draft April 8, 2021 23:36
@Half-Shot
Copy link
Contributor Author

Having thought about this, I don't think this is useful. It's basically only saving HTTP hits to the server at this point, which feels like an over-optimisation for the spec.

@Half-Shot Half-Shot closed this Apr 29, 2021
@turt2live turt2live added the abandoned A proposal where the author/shepherd is not responsive label Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned A proposal where the author/shepherd is not responsive kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants