-
Notifications
You must be signed in to change notification settings - Fork 380
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
MSC2836: Twitter-style Threading #2836
base: old_master
Are you sure you want to change the base?
Changes from 5 commits
eae8251
4dcc8c6
2b58fff
4fcd3fc
1af2e3c
d5ddccb
9835503
5c7c8f3
77db3f6
48f386a
0ed3c96
1e0a01d
0a27d4c
cab8077
a0a9b4f
75c5ba0
d6ed3d5
496c9e1
101e98d
ee44392
7628a4d
d9bf918
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,208 @@ | ||
### MSC2836: Threading | ||
kegsay marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
*This MSC probably supersedes https://github.com/matrix-org/matrix-doc/issues/1198* | ||
|
||
Matrix does not have arbitrarily nested threading for events. This is a desirable feature for implementing clones of social | ||
media websites like Twitter and Reddit. The aim of this MSC is to define the simplest possible API shape to implement threading | ||
in a useful way. This MSC does NOT attempt to consider use cases like editing or reactions, which have different requirements | ||
to simple threading (replacing event content and aggregating reactions respectively). | ||
Comment on lines
+3
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I disagree with this. While I do think, this MSC is useful, I don't think it should be called threading. Twitter and Reddit don't have threads, they have replies. It looks like threads, because of how the replies get rendered. I think for chat clients, you want the swimlane stuff Matthew talked about. Instead this should be called a replacement for replies, imo. To be fair, I don't think the name is that important, but I think it would be useful to deprecate the existing replies in the spec after this lands and encourage clients to use this feature for replies instead. Most of the APIs are also useful for replies and it does sound useful, if a client could focus on all the replies to a specific message with a thread UI. I see this more as a proper replacement for replies, since for IM threads you want to prevent messages from leaking outside a thread. For example if you have a discussion in TWIM, you want to be able to do that in a thread to stop annoying others with your messages, when they want to just read the weekly updates. I don't think this MSC does encourage clients to implement threading that way. Instead this MSC looks more like your replies will get rendered properly, if you remember to reply. Does that distinction make sense? |
||
|
||
The API can be broken down into 2 sections: | ||
- Making relationships: specifying a relationship between two events. | ||
- Querying relationships: asking the server for relationships between events. | ||
|
||
The rest of this proposal will outline the proposed API shape along with the considerations and justifications for it. | ||
|
||
#### Making relationships | ||
|
||
Relationships are made when sending or updating events. The proposed API shape is identical to | ||
[MSC1849](https://github.com/matrix-org/matrix-doc/blob/matthew/msc1849/proposals/1849-aggregations.md): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From a call about this, the answer seems to be yes -- this just hasn't been updated to take into account changes to the various relation MSCs. |
||
``` | ||
{ | ||
"type": "m.room.message", | ||
"content": { | ||
"body": "i <3 shelties", | ||
"m.relationship": { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so far everything uses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have the same concern about |
||
"rel_type": "m.reference", | ||
"event_id": "$another_event_id" | ||
} | ||
} | ||
} | ||
``` | ||
|
||
Justifications for this were as follows: | ||
- Quicker iterations by having it in event content rather than at the top-level (at the `event_id` level). | ||
- Ability for relationships to be modified post-event creation (e.g by editing the event). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a note, since I don't want to have this discussion again: This relation format actually makes editing relations a bit awkward, as it does not allow you to have multiple relations, so you need to add the relation in new_content, but you can't remove it. But that discussion is already far too long in the other MSCs, so I don't want to repeat it here. But I guess this MSC will then follow the event format in #2674, if that changes to address this? Also, this MSC forms some argument in that discussion, since it requires relations to not be redactable, so one could argue, one should not be able to edit them away, but on the other hand you probably still want to be able to move a comment to the top level or to a different comment, if you messed up your relationship. On the other hand that would break the existing structure, so you may not want that, huh? Anyway, this is not a concern to address here, I just thought it would be interesting enough to add as a note. |
||
- Doesn't require any additional server-side work (as opposed to adding the event ID as a query param e.g `?in-reply-to=$foo:bar`). | ||
|
||
Drawbacks include: | ||
- Additional work required for threading to work with E2EE. See MSC1849 for proposals, but they all boil down to having the `m.relationship` | ||
field unencrypted in the event `content`. | ||
|
||
Additional concerns: | ||
- The name of the `rel_type` is prone to bike-shedding: "m.reference", "m.reply", "m.in_reply_to", etc. This proposal opts for the decisions made | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's more that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Perhaps this is reason to look back at event aggregation MSCs and adapt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. m.reference also clashes with MSC2241. They definitely should not look the same. Regarding the relations format, I commented about that here, mentioning the limitations regarding threads for example: #2674 (comment) Although if you just want nested threads, replies don't make that much sense anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't care what the
So after reading this, it comes down to how you see an "event". Is it:
I see it as the 2nd option, whereas the map/array relations format sees it as the 1st option. Therefore, if you are replying to an event which already has an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking of events as diffs is really annoying for clients, since now you need to pull an entire graph out of the database instead of just a single events. For simpler clients, that is simply impossible (which is why weechat doesn't implement proper replies yet) and for others it is just annoying (in Nheko events are rendered using the current event json. Replies are just embedded different events. Having to parse and merge multiple events for edits is not something I want to implement, since it makes a lot of stuff more complicated to handle). Multiple relations does cause some conflicts in some cases, yes, but I don't think editing a reaction makes any sense anyway. On the other hand there is no order relation, when you edit a reply. It does not matter, if the current event is an edited reply or a replied edit. Forcing a specific order and the client to jump through hoops to have multiple relations, just causes more issues. For example you can't edit a relation away, if edits are just diffs, as there is no "remove that key from the original event" functionality. If events are not diffs, that entire issue goes away.
Is that meant as you send one edit to event X and an edit to event X? In that case you have that issue today already in clients not implementing edits. You really should specify that in edits, that you should reply, thread from, react to or edit only the original event id. Nothing to do with diffs vs idempotency. Which one of those sounds simpler?
A diff on the event dag sounds horrible from a complexity perspective, even if a single event is simpler. I much prefer having the complexity localized. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm aware that handling diffs is tricky for clients. Clients generally need to be able to handle diffs anyway though if they are listening for incremental updates down There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I thought all relationships are about referencing other events? Replies reference other events, as do edits and reactions. The type is about how the event references the other event. Really, imo m.reference should only be used, if an event references other events loosely, without attaching any meaning. This does not apply to in dm verifications, nor does it apply to threads. While I'm not sure how to call the former, why not call threads m.thread or something similar, that actually conveys the meaning? m.reference and m.replaces are pretty bad names imo, since they are too generic.
Not really, the only thing that really requires it is rendering membership events and a few others nicely, and most clients only do that for memberships. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think this is a confusion of terminology. All #1849 style relationships are about relating events together. The relationship can have one of 3 flavours: So given that Does that make any more sense? I think both @kegsay and @deepbluev7 are failing to grok the intention of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not really. If "m.reference" is the generic one, why use it at all? Obviously every relations references another event. But why not default to handling events like "m.reference" in you proposal? That way all events get a somewhat reasonable default behaviour, and you can then special case it in the spec. There is not really a problem with I think using "m.reference" is fine in some cases, but threads are not one of them imo, because clients want to handle them specifically and servers may want to at some point too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For better or worse, I've tried to make MSC 2674 somewhat opinionated in this regard in that |
||
in MSC1849 which is "m.reference". | ||
|
||
Edge cases: | ||
- Any event `type` can have an `m.relationship` field in its `content`. | ||
- Redacting an event with an `m.relationship` field DOES NOT remove the relationship. Instead, it is preserved similar to how `membership` | ||
is preserved for `m.room.member` events, with the following rules: | ||
* Remove all fields except `rel_type` and `event_id`. | ||
* If `rel_type` is not any of the three types `m.reference`, `m.annotation` or `m.replace` then remove it. | ||
* If `event_id` is not a valid event ID (`$` sigil, correct max length), then remove it. | ||
The decision to preserve this field is made so that users can delete offensive material without breaking the structure of a thread. This is | ||
different to MSC1849 which proposes to delete the relationship entirely. | ||
- It is an error to reference an event ID that the server is unaware of. Servers MUST check that they have the event in question: it need not | ||
be part of the connected DAG; it can be an outlier. This prevents erroneous relationships being made by abusing the CS API. | ||
- It is NOT an error to reference an event ID in another room. Cross-room threading is allowed and this proposal goes into more detail on how | ||
servers should handle this as a possible extension. | ||
- It is an error to reference yourself. | ||
kegsay marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
#### Querying relationships | ||
|
||
Relationships are queryed via a new CS API endpoint: | ||
``` | ||
POST /_matrix/client/r0/event_relationships | ||
{ | ||
"event_id": "$abc123", // the anchor point for the search, must be in a room you are allowed to see (normal history visibility checks apply) | ||
"max_depth": 4, // if negative unbounded, default: 3. | ||
"max_breadth": 10, // if negative unbounded, default: 10. | ||
"limit": 100, // the maximum number of events to return, server can override this, default: 100. | ||
"depth_first": true|false, // how to walk the DAG, if false, breadth first, default: false. | ||
"recent_first": true|false, // how to select nodes at the same level, if false oldest_first - servers compare against origin_server_ts, default: true. | ||
"include_parent": true|false, // if event_id has a parent relation, include it in the response, default: false. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since I want this to replace replies, it would be useful, if that could be added as a /sync filter too, that clients can opt into. The parent event is then added to a Justification: Currently clients rely on the fallback in replies to render them, if it is too hard for them to make a separate request to render the reply (because it is single threaded or otherwise to simple to defer rendering an event or rerender the event). If they could be served the related event over the /sync API instead, I think the need for the fallback would go away, since worst case they just create the fallback locally. There may also be some value in allowing that for some children, similarly to how reactions are supposed to get aggregated, but it would bloat /sync considerably, so that may be a bad idea, especially if you can't really paginate them. :D |
||
"include_children": true|false // if there are events which reply to $event_id, include them all (depth:1) in the response: default: false. | ||
"direction": up|down // if up, parent events (the events $event_id is replying to) are returned. If down, children events (events which reference $event_id) are returned, default: "down". | ||
"batch": "opaque_string" // A token to use if this is a subsequent HTTP hit, default: "". | ||
} | ||
``` | ||
which returns: | ||
``` | ||
{ | ||
"events": [ // the returned events, ordered by the 'closest' (by number of hops) to the anchor point. | ||
{ ... }, { ... }, { ... }, | ||
], | ||
"next_batch": "opaque_string", // A token which can be used to retrieve the next batch of events, if the response is limited. | ||
// Optional: can be omitted if the server doesn't implement threaded pagination. | ||
kegsay marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"limited": true|false // True if there are more events to return because the `limit` was reached. Servers are not obligated | ||
// to return more events, see if the next_batch token is provided or not. | ||
} | ||
``` | ||
|
||
Justifications for the request API shape are as follows: | ||
- The HTTP path: cross-room threading is allowed hence the path not being underneath `/rooms`. An alternative could be | ||
`/events/$event_id/relationships` but there's already an `/events/$event_id` deprecated endpoint and nesting this new MSC | ||
underneath a deprecated endpoint conveys the wrong meaning. | ||
- The HTTP method: there's a lot of data to provide to the server, and GET requests shouldn't have an HTTP body, hence opting | ||
for POST. The same request can produce different results over time so PUT isn't acceptable as an alternative. | ||
- The anchor point: pinning queries on an event is desirable as often websites have permalinks to events with replies underneath. | ||
- The max depth: Very few UIs show depths deeper than a few levels, so allowing this to be constrained in the API is desirable. | ||
- The max breadth: Very few UIs show breadths wider than a few levels, so allowing this to be constrained in the API is desirable. | ||
- The limit: For very large threads, a max depth/breadth can quickly result in huge numbers of events, so bounding the overall | ||
number of events is desirable. Furthermore, querying relationships is computationally expensive on the server, hence allowing | ||
it to arbitrarily override the client's limit (to avoid malicious clients setting a very high limit). | ||
- The depth first flag: Some UIs show a 'conversation thread' first which is depth-first (e.g Twitter), whereas others show | ||
immediate replies first with a little bit of depth (e.g Reddit). | ||
- The recent first flag: Some UIs show recent events first whereas others show the most up-voted or by some other metric. | ||
This MSC does not specify how to sort by up-votes, but it leaves it possible in a compatible way (e.g by adding a | ||
`sort_by_reaction: 👍` which takes precedence which then uses `recent_first` to tie-break). | ||
- The include parent flag: Some UIs allow permalinks in the middle of a conversation, with a "Replying to [link to parent]" | ||
message. Allowing this parent to be retrieved in one API hit is desirable. | ||
- The include_children flag: Some UIs allow permalinks in the middle of a conversation, with immediate children responses | ||
visible. Allowing the children to be retrieved in one API hit is desirable. | ||
- The direction enum: The decision for literal `up` and `down` makes for easier reading than `is_direction_up: false` or | ||
equivalent. The direction is typically `down` - find all children from this event - but there is no reason why this cannot | ||
be inverted to walk up the DAG instead. | ||
- The batch token: This allows clients to retrieve additional results. It's contained inside the HTTP body rather than as | ||
a query param for simplicity - all the required data that the server needs is in the HTTP body. This token is optional as | ||
paginating is reasonably complex and should be opt-in to allow for ease of server implementation. | ||
|
||
Justifications for the response API shape are as follows: | ||
- The events array: There are many possible ways to structure the thread, and the best way is known only to the client | ||
implementation. This API shape is unopinionated and simple. | ||
- The next batch token: Its presence indicates if there are more events and it is opaque to allow server implementations the | ||
flexibility for their own token format. There is no 'prev batch' token as it is intended for clients to request and persist | ||
the data on their side rather than page through results like traditional pagination. | ||
- The limited flag: Required in order to distinguish between "no more events" and "more events but I don't allow pagination". | ||
This additional state cannot be accurately represented by an empty `next_batch` token. | ||
|
||
Server implementation: | ||
- Sanity check request and set defaults. | ||
- Can the user see (according to history visibility) `event_id`? If no, reject the request, else continue. | ||
- Retrieve the event. Add it to response array. | ||
- If `include_parent: true` and there is a valid `m.relationship` field in the event, retrieve the referenced event. | ||
Apply history visibility check to that event and if it passes, add it to the response array. | ||
- If `include_children: true`, lookup all events which have `event_id` as an `m.relationship` - this will almost certainly require | ||
servers to store this lookup in a dedicated table when events are created. Apply history visibility checks to all these | ||
events and add the ones which pass into the response array, honouring the `recent_first` flag and the `limit`. | ||
- Begin to walk the thread DAG in the `direction` specified, either depth or breadth first according to the `depth_first` flag, | ||
honouring the `limit`, `max_depth` and `max_breadth` values according to the following rules: | ||
* If the response array is `>= limit`, stop. | ||
* If already processed event, skip. | ||
* Check how deep the event is compared to `event_id`, does it *exceed* (greater than) `max_depth`? If yes, skip. | ||
* Check what number child this event is (ordered by `recent_first`) compared to its parent, does it *exceed* (greater than) `max_breadth`? If yes, skip. | ||
* Process the event. | ||
If the event has been added to the response array already, do not include it a second time. If an event fails history visibiilty | ||
checks, do not add it to the response array and do not follow any references it may have. This algorithm bounds an infinite DAG | ||
into a "window" (governed by `max_depth` and `max_breadth`) and serves up to `limit` events at a time, until the entire window | ||
has been served. Critically, the `limit` _has not been reached_ when the algorithm hits a `max_depth` or `max_breadth`, it is only | ||
reached when the response array is `>= limit`. | ||
- When the thread DAG has been fully visited or the limit is reached, return the response array (and a `next_batch` if the request | ||
was limited). If a request comes in with the `next_batch` set to a valid value, continue walking the thread DAG from where it | ||
was previously left, ensuring that no duplicate events are sent, and that any `max_depth` or `max_breadth` are honoured | ||
_based on the original request_ - the max values always relate to the original `event_id`, NOT the event ID previously stopped at. | ||
|
||
#### Cross-room threading extension | ||
|
||
This MSC expands on the basic form to allow cross-room threading by allowing 2 extra fields to be specified | ||
in the `m.relationship` object: `servers` and `room_id`: | ||
``` | ||
{ | ||
"type": "m.room.message", | ||
"content": { | ||
"body": "i <3 shelties", | ||
"m.relationship": { | ||
"rel_type": "m.reference", | ||
"event_id": "$another_event_id", | ||
"servers": [ "localhost", "anotherhost" ], | ||
"room_id": "!someroomid:anotherhost", | ||
} | ||
} | ||
} | ||
``` | ||
|
||
Only servers can set these fields. If clients attempt to set them they will be replaced. The server should set these fields | ||
when an event is sent according to the following rules: | ||
- Check the client can view the event ID in question. If they cannot, reject the request. | ||
- Check that the room's `m.room.create` event allows cross-room threading by the presence of `"m.cross_room_threading": true` | ||
in the `content` field. If absent, it is `false`. | ||
- Fetch the servers *currently in the room* for the event. Add them all to `servers`. | ||
- Fetch the room ID that the event belongs to. Add it to `room_id`. | ||
|
||
This proposal does not require any changes to `/createRoom` as `"m.cross_room_threading": true` can be specified via the | ||
`creation_content` field in that API. | ||
|
||
The `POST /relationships` endpoint includes a new field: | ||
- `auto_join: true|false`: if `true`, will automatically join the user calling `/relationships` to rooms they are not joined to | ||
in order to explore the thread. Default: `false`. | ||
|
||
Server implementation: | ||
- When walking the thread DAG, if there is an event that is not known, check the relationship for `room_id` and `servers`. If | ||
they exist, peek into the room ([MSC2444](https://github.com/matrix-org/matrix-doc/pull/2444)), or if that is not supported, | ||
join the room as the user querying `/relationships` if and only if `"auto_join": true` in the `/relationships` request. This | ||
is required in order to allow the event to be retrieved. Server implementations can treat the auto join as the same as if the | ||
client made a request to [/join/{roomIdOrAlias}](https://matrix.org/docs/spec/client_server/r0.6.0#post-matrix-client-r0-join-roomidoralias) | ||
with the `server_name` query parameters set to those in `servers`. | ||
|
||
Security considerations: | ||
- Allowing cross-room threading leaks the event IDs in a given room, as well as which servers are in the room at the point | ||
the reply is sent. It is for this reason that there is an opt-in flag at room creation time. | ||
|
||
Justifications: | ||
- Because the client needs to be able to view the event being replied to, it is impossible for the replying client to | ||
respond to an event which the server is unaware of. However, it is entirely possible for queries to contain events | ||
which the server is unaware of if the sender was on another homeserver. For this reason, we need to contain routing | ||
information *somewhere* so servers can retrieve the event and continue navigating the thread. As the client and server | ||
already have the event which contains a relationship to another event inside an unknown room, the simplest option is to | ||
also contain the routing information with that relationship. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really promising, imo. Would be good to mention the swimlane stuff (i.e. "can I have messages in a thread which aren't explicit references? I don't want to have to set an explicit reply whenever I type within a thread - I just want to send a message!"). Also, think the pagination needs a bit more thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't the swimlane stuff a client side feature? Although it probably makes sense to mention the expectation, that clients implement it like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swimlanes are entirely a client-side problem imo. It would be up to the clients to determine the 3 swimlanes in a graph like:
But at a protocol level it is the most useful to have explicit references back to the previous event in the swimlane. This PR is motivated for more Twitter style use cases which don't really have swimlanes though, hence their omission.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that regularly break when multiple clients try to reply in a swimlane at the same time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would break swimlanes, which is the point i'm trying to make. We should acknowledge that we should support a mix of messages with explicit
m.reference
replies (an explicit fork in convo) versus ones without them (normal messages in a room, or messages within a thread). (Even though the twitter/reddit/HN-style use cases don't need this).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To illustrate, given the example conversation from Kegsays comment, this is what a client could do:
(the arrows being
m.reference
s)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the example of your graph (is that yEd?) above, it would tell you whether B is in lane 1 or lane 2. And would let you mix together filter-by-swimlane with filter-by-labels for clients with filter buttons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I may, I like the idea of using
m.references
s to define swimlanes. In this graph:From a client perspective, this means you can create threads in threads. You can create a new thread from any message, even a message already in a thread. Contrary to slack for example where you can only create a thread from a message in the channel (i.e. in the main swinlane).
So if you want to post to a swimlane, you have to
m.references
the same event that the previous event in the swimlane does. Clients that want to have a tree style (like twitter/reddit/...) could just create a new swimlane, i.e they reference the previous message.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree the idea is interesting, although i'm still a bit worried about how the layout algorithm would know that B should be a new swimlane or not. It feels like the only way of telling is whether B has children of its own?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the behavior should be something like:
m.reference
are in the same swimlane