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

MSC3981: /relations recursion #3981

Merged
merged 25 commits into from
Jan 30, 2024
Merged
Changes from 21 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
098b343
Initial Draft for MSC 3981: /relations recursion
justjanne Mar 19, 2023
abddab8
fix accidentally copy-pasted title
justjanne Mar 20, 2023
4f61e43
Remove unnecessarily specific sentence
justjanne Mar 22, 2023
381a355
Add warning to avoid unlimited recursion
justjanne Mar 22, 2023
bbcc81a
Clean-up links in MSC
justjanne Mar 28, 2023
f7e5ca3
Apply reviewer suggestions
justjanne Mar 28, 2023
ffb316d
More clarifications on examples
justjanne Mar 28, 2023
077f6b1
Address feedback on formatting
justjanne Apr 19, 2023
c638630
Address feedback on linking related specs
justjanne Apr 19, 2023
dad341e
Rephrase technical definition of the parameter
justjanne Apr 19, 2023
6d88fe8
Correct mistake in examples
justjanne Apr 19, 2023
123eef1
Rephrase paragraph on O(n+1) issue
justjanne Apr 19, 2023
65cab28
fix: correct mixup between event type and rel type
justjanne May 9, 2023
6baafd8
feat: add clarification for why this was needed
justjanne May 9, 2023
b6119c5
feat: add clarification for how it affects intermediate events
justjanne May 9, 2023
cfd4223
feat: add clarification for how it affects intermediate events
justjanne May 9, 2023
94561ad
Add /version unstable feature flag
weeman1337 May 11, 2023
f404254
feat: improve phrasing of unstable prefix section
justjanne May 11, 2023
03e8d68
feat: add clarification for how clients can make use of this
justjanne May 11, 2023
0e50bdb
Clarify unstable features map
dbkr Dec 13, 2023
a4d8e73
Attempt at resolving the discussion threads for 3981.
dbkr Dec 13, 2023
a027354
Add note that security is discussed elsewhere.
dbkr Jan 3, 2024
6ace9f4
Add recursion_depth response parameter.
dbkr Jan 3, 2024
c160d36
Note that recursion_depth is sent un-prefixed.
dbkr Jan 3, 2024
bdae577
Add summary of security discussions
dbkr Jan 16, 2024
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
143 changes: 143 additions & 0 deletions proposals/3981-relations-recursion.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
# 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 -
Copy link
Member

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?

Copy link
Member

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.

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

Choose a reason for hiding this comment

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

I wonder if existing client implementations are good enough to face the recurse parameter set to true by default? That would bring in the awesomeness of recursion to the existing fleet automagically (still with a caveat of limiting recursion to a sane value).

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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 (false, that is) being inherently broken in some way. I mean, it's just not right when reactions don't get pulled along with other messages in the same thread, right? I'm totally fine to punt the removal of this into some future MSC, just in case.


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

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

Choose a reason for hiding this comment

The 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:

If it helps, you can think of relations as a "subject verb object" triple, where the subject is the relation event itself; the verb is the rel_type field of the m.relationship and the object is the event_id field.

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.

Copy link
Member

Choose a reason for hiding this comment

The 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

None.
dbkr marked this conversation as resolved.
Show resolved Hide resolved

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

### 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
Loading