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

MSC2836: Twitter-style Threading #2836

Open
wants to merge 22 commits into
base: old_master
Choose a base branch
from
Open
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
240 changes: 240 additions & 0 deletions proposals/2836-threading.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,240 @@
### MSC2836: Threading
Copy link
Member

@ara4n ara4n Oct 29, 2020

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.

Copy link
Contributor

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.

Copy link
Member Author

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:

A
|
B
| \
C  D
|  |
E  F
|  | \
G  H  I
|  |  |
J  K  L

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.

Choose a reason for hiding this comment

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

explicit references back to the previous event in the swimlane

Wouldn't that regularly break when multiple clients try to reply in a swimlane at the same time?

Copy link
Member

Choose a reason for hiding this comment

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

But at a protocol level it is the most useful to have explicit references back to the previous event in the swimlane.

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).

Copy link

@Pestdoktor Pestdoktor Nov 7, 2020

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:
grafik
(the arrows being m.references)

Copy link
Member

Choose a reason for hiding this comment

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

What's the advantage an m.label would provide over a subthread?

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.

Copy link
Contributor

@erdnaxeli erdnaxeli Nov 22, 2020

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.referencess to define swimlanes. In this graph:

  • A is in the room (main swimlane), and a new thread was created from it (and A should also be shown at the top of the thread swimlane)
  • B C E G J are in the same swimlane created from A by B.
  • D F H K are in the same swimlane. This is another thread, created from B.
  • and so I L are in the same swinlane, this is another thread created from F.

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.

Copy link
Member

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?

Copy link
Contributor

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:

  • all messages sharing the same m.reference are in the same swimlane
  • the message referenced should be shown at the top of the swimlane, for context

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

Choose a reason for hiding this comment

The 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):
Copy link
Member

Choose a reason for hiding this comment

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

Is this now MSC2674 and MSC3267 since MSC1849 has now been split up?

Copy link
Member

Choose a reason for hiding this comment

The 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": {
Copy link
Contributor

Choose a reason for hiding this comment

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

so far everything uses m.relates_to (edits, reactions, replies) and if you want to reference accross rooms you should probably also include the room id

Copy link
Contributor

Choose a reason for hiding this comment

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

I have the same concern about m.relationship and m.relates_to. Is a standardization planned?

"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).
Copy link
Contributor

Choose a reason for hiding this comment

The 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`.

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. Note that it is
expected that events over federation will reference event IDs that the receiving server is unaware of: this is allowed. This check is only
performed when *clients* attempt to make new references.
- It is an error to reference an event ID in another room.
- It is an error to reference yourself. Cyclical loops are still possible by using multiple events and servers should guard against this by
only visiting events once.

#### 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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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 related_events field in unsigned, so that the client does not have to query the client server API for that.

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 not currently allowed but is possible 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 back and forth 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
kegsay marked this conversation as resolved.
Show resolved Hide resolved
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 as `events` (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.

##### Querying relationships over federation

Relationships can be queried over federation using a new endpoint which is the same as the CS API format. See the CS API section for more info. The path
used for this new federation endpoint is `/_matrix/federation/v1/event_relationships`. There is one additional response field: `auth_chain` which contains
all the necessary auth events for the events in `events`, e.g:
```
{
"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.
"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.
"auth_chain": [ // The auth events required to authenticate events in `events`, in any order without duplicates.
{ ... }, { ... }, { ... },
]
}
```

Justification:
- In an ideal world, every server would have the complete room DAG and would therefore be able to explore the full scope of a thread in a room. However,
over federation, servers have an incomplete view of the room and will be missing many events. In absence of a specific API to explore threads over
federation, joining a room with threads will result in an incomplete view.
- The requirements here have a lot in common with the [Event Context API](https://matrix.org/docs/spec/client_server/r0.6.0#id131). However, the context
API has no federated equivalent. This means any event context requests for events the server is unaware of will incorrectly return `404 Not Found`.
- The same API shape is proposed to allow code reuse and because the same concerns and requirements are present for both federation and client-server.

Server behaviour:
- When receiving a request to `/event_relationships`: ensure the server is in the room then walk the thread in the same manner as the CS API form. Do not
make outbound `/event_relationships` requests on behalf of this request to avoid routing loops where 2 servers infinitely call `/event_relationships` to
each other.
- For each event returned: include all `auth_events` for that event recursively to create an auth chain and add them to `auth_chain`.
- Servers should make outbound `/event_relationships` requests *for client requests* when they encounter an event ID they do not have, or they suspect
that the event has children the server does not have (see the next section). The event may have happened much earlier in the room which another server
in the room has.

#### Exploring dense threads

The proposed API so far has no mechanism to:
- List the absolute number of children for a given event.
- Check that the server has all the children for a given parent event.

To aid this, all events returned from `/event_relationships` SHOULD have 2 additional fields in the `unsigned` section of the event:
- `children`: A map of `rel_type` to the number of children with that relation.
- `children_hash`: The base64 string of the SHA256 of all the event IDs of the known children, deduplicated and sorted lexicographically.

For example, an event `$AAA` with three children: `$BBB`, `$CCC`, `$DDD` where the first two are of `rel_type` "m.reference" and the last is "custom", should
produce an `unsigned` section of:
```
unsigned: {
children: {
"m.reference": 2,
"custom": 1
},
children_hash: "GE6QH8oImiq8IoMwQmIDxF9keqtY2Q7KKtJ4caXdYb0=" // base64(SHA256("$BBB$CCC$DDD"))
}
```

Justification:
- Without this information, it's impossible to know if an event has children _at all_. Servers need to know if children exist so they can fetch them
over federation.
- This allows clients to display more detailed "see more" links e.g. "... and 56 more replies".
- This allows servers to identify when a federated `/event_relationships` request has been window culled by `max_depth` or `max_breadth`, or has just not
been explored yet.
- Separating out the counts by `rel_type` allows clients to accurately determine whether children are threaded replies or some other relation.

Server behaviour:
- When processing an `/event_relationships` response from another server:
* For each event, check the children count and hash. If there is no `unsigned` section assume the count to be 0.
* If this event is new, persist it. If this event is not new, compare the children counts. The event with a higher children count should be persisted:
specifically the `unsigned` section should be persisted as the event itself is immutable.
* When returning events to another server or to a client, always return the `unsigned` section which produces the *most* children, and NOT the number
of children the server currently has fetched.
- When processing an `/event_relationships` request from a client:
* If the event ID is unknown, perform a federated `/event_relationships` request. Alternatively, if the event is known **and there are unexplored children**,
perform a federated `/event_relationships` request.
* An event has unexplored children if the `unsigned` child count on the parent does not match how many children the server believes the parent to have.
In addition, if the counts match but the hashes do not match, then the event is unexplored.
* Regardless of whether the federated `/event_relationships` request returns the missing children, mark the event as explored afterwards. This prevents
constantly hitting federation when walking over this event. The easiest way to mark the event as explored is to remember what the highest children count was
when the most recent federated request was made. If that number differs from the current `unsigned` count then it is unexplored.
- Explored events will always remain up-to-date assuming federation between the two servers remains intact. If there is a long outage, any new child will be
marked as "unexplored" (because the parent event will be missing) and trigger an `/event_relationships` request, akin to how the `/send` federation API will
trigger `/get_missing_events` in the event of an unknown `prev_event`. This will then pull in events heading up to the root event, along with `unsigned` children
counts of any potential branches in the thread.

Choose a reason for hiding this comment

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

Suggested change
counts of any potential branches in the thread.
counts of any potential branches in the thread.
#### Alternatives
* Nested arbitrary-text reactions, which is already allowed but not well supported in clients.