-
Notifications
You must be signed in to change notification settings - Fork 385
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
MSC3981: /relations
recursion
#3981
Changes from all commits
098b343
abddab8
4f61e43
381a355
bbcc81a
f7e5ca3
ffb316d
077f6b1
c638630
dad341e
6d88fe8
123eef1
65cab28
6baafd8
b6119c5
cfd4223
94561ad
f404254
03e8d68
0e50bdb
a4d8e73
a027354
6ace9f4
c160d36
bdae577
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,165 @@ | ||
# MSC3981: `/relations` recursion | ||
|
||
The [`/relations`] API allows clients to retrieve events which directly relate | ||
to a given event. | ||
|
||
This API has been used as basis of many other features and MSCs since then, | ||
including threads. | ||
|
||
Threads was one of the first usages of this API that allowed nested relations - | ||
an event may have an [`m.reaction`] or [`m.replace`] relation to another event, | ||
which in turn may have an `m.thread` relation to the thread root. | ||
|
||
This forms a tree of relations. A client wanting to display a thread will want | ||
to display reactions and edits to messages in the thread, and will therefore need | ||
the second-order related events in addition to just the events with a direct thread | ||
relation to the root. | ||
|
||
Clients can recursively perform the /relations queries on each event but this is | ||
very slow and does not give the client any information on how the events are ordered | ||
for the purpose of sending read receipts. | ||
|
||
justjanne marked this conversation as resolved.
Show resolved
Hide resolved
|
||
## Proposal | ||
|
||
It is proposed to add the `recurse` parameter to the `/relations` API, defined | ||
as follows: | ||
|
||
> Whether to additionally include events which only relate indirectly to the | ||
> given event, | ||
> ie. events related to the root events via one or more direct relationships. | ||
> | ||
> If set to false, only events which have direct a relation with the given | ||
> event will be included. | ||
> | ||
> If set to true, all events which relate to the given event, or relate to | ||
> events that relate to the given event, will be included. | ||
> | ||
> It is recommended that at least 3 levels of relationships are traversed. | ||
uhoreg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
> Implementations may perform more but should be careful to not infinitely recurse. | ||
> | ||
> One of: `[true false]`. | ||
|
||
In order to be backwards compatible the `recurse` parameter must be | ||
optional (defaulting to `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. I wonder if existing client implementations are good enough to face the 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. Maybe, but personally I'd rather just flip the flag in the client once I know it worked that go scouting for what clients implement it and try to make sure they all behave sensibly when the server breaks (old) spec. 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. My main concern here is that we may extend the API surface for no actual use, with one option ( |
||
|
||
Regardless of the value of the `recurse` parameter, events will always be | ||
returned in topological ordering, ie. the same order in which the `/messages` API | ||
would return them (given the same `dir` parameter). | ||
|
||
It is also proposed to add a response parameter, `recursion_depth` to the response | ||
which gives the actual depth limit the server used in its recursion. This key is mandatory if | ||
the `recurse` parameter was passed and is absent otherwise. eg: | ||
|
||
```json | ||
{ | ||
"chunk": [...], | ||
"recursion_depth": 3 | ||
} | ||
``` | ||
|
||
Note that there no way in this MSC for a client to affect how much the server recurses. | ||
If the client decides that the server's recursion level is insufficient, it could, for example, | ||
perform the recursion manually, or disable whatever feature requires more recursion. | ||
|
||
Filters specified via `event_type` or `rel_type` will be applied to all events | ||
returned, whether direct or indirect relations. Events that would match the filter, | ||
but whose only relation to the original given event is through a non-matching | ||
intermediate event, will not be included. This means that supplying a `rel_type` | ||
parameter of `m.thread` is not appropriate for fetching all events in a thread since | ||
relations to the threaded events would be filtered out. For this purpose, clients should | ||
omit the `rel_type` parameter and perform any necessary filtering on the client side. | ||
|
||
## Potential issues | ||
|
||
Naive implementations might be tempted to provide support for this parameter | ||
through a thin shim which is functionally identical to the client doing | ||
separate recursive `/relations` requests itself. However this would allow a | ||
client to craft a set of events that would cause unreasonable load. | ||
|
||
## Alternatives | ||
justjanne marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
1. Clients could fetch all relations recursively client-side, which would | ||
increase network traffic and server load significantly. | ||
2. A new, specialised endpoint could be created for threads, specifically | ||
designed to present separate timelines that, in all other ways, would | ||
behave identically to `/messages`. | ||
3. Twitter-style threads (see [MSC2836]). | ||
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. When I proposed aggregations (in the mess that is MSC1849) the intention was basically to define a graph of relations linking events together:
I assumed that when users wanted to do complicated graph queries (e.g. "give me all the events in this thread, including their aggregations") or "give me the siblings of this twitter thread") we'd end up with a generic API for navigating the relations graph - and that's what kegan & I came up with in [MSC2836]. Personally, my hunch is still that a single generic API for navigating the relation graph would have been a better solution than having lots of incremental MSCs which gradually nudge the existing APIs in the right direction. However, we've ended up down the incrementalist path anyway now, and this MSC seems a good enough way to solve the immediate problem of "give me all the events which relate to this thread". However, I have a horrible feeling that we're promptly going to hit another limitation for another relations use case (e.g. "I want to see the last 20 beacon locations for the 5 most recent location-shares in this thread. Including if the location-share event subsequently got edited" - or a similar one for VoIP signalling using aggregations, or even MSC4016-style file transfer updates via aggregations)... and find ourselves wanting a more generic API, again. I'm also dubious about "the server may just ignore your recursion request, impose an arbitrary depth, and the client will have to figure it out". I'd have thought it would be better for the client to state its requirements (i.e. "i need depth=3 for my use case") and for the server to fulfil those... but to cap resource usage by the server then batching up the responses in whatever size it's most comfortable with (or by capping the pagination requests that the client makes). So even if the client asks for some horribly inefficient query, the server can decline to calculate more than 10 results at a time or whatever. Otherwise it's passing a lot of complexity onto the client to work around the server's limitations, and the design of Matrix has always been for the server to do the heavy-lifting rather than the client, in order to make clients as easy to write as possible. So, TL;DR: I don't love this, but it's an acceptable solution for where we're at right now and I'm not going to block it, lest perfect be the enemy of good. In future I really hope we converge on a robust generic way in a new MSC (or MSC2836 gets dusted off) in order to walk the relations DAG rather than sprouting incremental tweaks. 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 feel fairly similarly about this really in that much more advanced uses of relations will likely require a more sophisticated API that can properly control recursion depth and what rel_types get recursively returned, etc. I also think the way we've chosen to implement features, ie. with threads as a live feature in Matrix now that, apart from not working very well itself is also causing other parts of the protocol to be flakey: we need to be fairly tactical here. Obviously we can introduce a more full-featured API separately but equally it's a shame that servers will have both APIs to maintain. |
||
4. Alternatively a `depth` parameter could have been specified, as in [MSC2836]. | ||
We believe that a customizable depth would add unnecessary constraints to | ||
server implementers, as different server implementations may have different | ||
performance considerations and may choose different limits. Additionally, | ||
the maximum currently achievable depth is still low enough to avoid this | ||
becoming an issue. | ||
|
||
## Security considerations | ||
|
||
Security considerations are discussed inline throughout this proposal. To summarise: | ||
* Allowing a client to control recursion depth could allow a client to cause outsize | ||
load on the server if the server doesn't check the recursion depth. | ||
* Naive server implementations could allow a client to craft a set of events that would | ||
cause high load. | ||
|
||
## Examples | ||
|
||
Given the following graph: | ||
|
||
```mermaid | ||
flowchart RL | ||
subgraph Thread | ||
G | ||
E-->D | ||
B-->A | ||
end | ||
B-.->|m.thread|A | ||
G-.->|m.thread|A | ||
E-.->|m.annotation|B | ||
D-.->|m.edit|A | ||
G-->F-->E | ||
D-->C-->B | ||
``` | ||
|
||
`/messages` with `dir=f` would | ||
return `[A, B, C, D, E, F, G]`. | ||
|
||
`/relations` on event `A` with `rel_type=m.thread` and `dir=f` would | ||
return `[B, G]`. | ||
|
||
`/relations` on event `A` with `recurse=true` and `dir=f` would | ||
return `[B, D, E, G]`. | ||
|
||
`/relations` on event `A` with `recurse=true`, `dir=b` and `limit=2` would | ||
return `[G, E]`. | ||
|
||
`/relations` on event `A` with `rel_type=m.annotation`, | ||
`event_type=m.reaction` and `recurse=true` would return `[G, E]`. | ||
|
||
## Unstable prefix | ||
|
||
### While the MSC is not yet part of a spec version | ||
|
||
During this period, to detect server support, clients should check for the | ||
presence of the `org.matrix.msc3981` flag in the `unstable_features` map | ||
on [`/versions`](https://spec.matrix.org/v1.7/client-server-api/#get_matrixclientversions). | ||
|
||
Clients are also required to use `org.matrix.msc3981.recurse` in place | ||
of `recurse` at this time. | ||
|
||
`recursion_depth` is always used un-namespaced (it would only ever be sent | ||
if the client had already sent the recurse parameter). | ||
|
||
### Once the MSC is in a spec version | ||
|
||
Once this MSC becomes a part of a spec version, clients should rely on the | ||
presence of the spec version that supports this MSC in the `/version` response | ||
to determine support. | ||
|
||
Servers are encouraged to keep the `org.matrix.msc3827` flag around for a | ||
reasonable amount of time to help smooth over the transition for clients. | ||
"Reasonable" is intentionally left as an implementation detail, however the MSC | ||
process currently recommends at most 2 months from the date of spec release. | ||
|
||
[MSC2836]: https://github.com/matrix-org/matrix-spec-proposals/pull/2836 | ||
[MSC3771]: https://github.com/matrix-org/matrix-spec-proposals/pull/3771 | ||
[`/relations`]: https://spec.matrix.org/v1.6/client-server-api/#get_matrixclientv1roomsroomidrelationseventid | ||
[`m.reaction`]: https://github.com/matrix-org/matrix-spec-proposals/pull/2677 | ||
[`m.replace`]: https://spec.matrix.org/v1.6/client-server-api/#event-replacements |
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.
FTR I think this is only an issue with thread & reference relations?
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 this is saying only thread & reference relations can have second-order relations? In which case I think probably (although I'd be surprised if references ever did) but I'm not sure it makes a huge difference here.