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.

Sign up for GitHub

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

MSC4023: Thread ID for 2nd order-relation #4023

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

MSC4023: Thread ID for 2nd order-relation #4023

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f34e69e
Create proposal for m.thread_id unsigned field
May 31, 2023
d771c62
Update proposal wording
germain-gg Jun 2, 2023
8699c34
Rename 'm.thread_id' to 'thread_id'
Jun 2, 2023
036666c
Require all events to inform the unsigned thread_id property
Jun 2, 2023
fc9a8c4
Document edge case with missing first-order relation
Jun 2, 2023
541f210
Rewrite some parts of this MSC for clarity and address feedback.
dbkr Dec 6, 2023
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
64 changes: 64 additions & 0 deletions proposals/4023-unsigned-thread-id.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
# MSC4023: Thread ID for second-order relation

[MSC3981](https://github.com/matrix-org/matrix-spec-proposals/pull/3981) defines
a way to recursively load relations in a thread context. However this does not
let clients determine with certainty in what timeline an event coming from a `/sync`
should end up in.

If the related event is unknown to the client, the only way to partition things
correctly is to fetch the related event and confirm whether this event belongs to
a thread or not.

This proposal wants to reduce the amount of work required for clients to partition
events with certainty in a time efficient manner.

## Proposal

All events in a thread and the second-order relation events should add a `thread_id`
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 that this sentence needs a bit more clarity with regards to who is adding the thread_id property. When I first read this, I thought that it was added by the sending client, but clients don't set unsigned, so it can't be that. I'm guessing then, that it's set by the recipient's server, but I think it would help to be explicit.

Also, what endpoints is it returned in? I'm guessing all the endpoints that return events?

Copy link
Member

Choose a reason for hiding this comment

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

Hopefully I've clarified this.

property in their `unsigned` field definition, referencing the thread root – as
defined in MSC3440.

```jsonc
{
"thread_id": "$event_id"
MatMaul marked this conversation as resolved.
Show resolved Hide resolved
}
```

All events that are not part of a thread should fill the `thread_id` property with
the special value `main` – as defined in MSC3771.

If a server does not have the first-order event, the unsigned `thread_id` property
should be filled with a `null` value. When the server gets a hold of the first-order
event, it should retroactively update the `thread_id` property and communicate the
change to clients.
germain-gg marked this conversation as resolved.
Show resolved Hide resolved

## Potential issues

### Database query performances

This could have performance implications on the server side, requiring more work
to be performed when fetching events in a room.

### Missing first-order relation

It is possible that a server will have the second-order event, but not have the
first-order event (eg, it has received a reaction over federation, but has not
yet received the event being reacted to).

## Alternatives

If "[MSC3051: A scalable relation format](https://github.com/matrix-org/matrix-spec-proposals/pull/3051)"
was to be adopted, this MSC would be nulled.
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These relations would also carry an m.thread relation, meaning they would already carry the piece of information we're after.
I guess it would still have some relevance for historical events.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, maybe I don't think MSC3051 says they must include that information though? At least that wasn't how I read it.

Copy link
Member

@tulir tulir May 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Then the alternative would be MSC3051 + a MSC that says relations to thread events must include the thread relation too (or adjusting 3051 to require that)

I'd certainly prefer fixing relations instead of building more hacks into the current non-scalable format (although fixing doesn't necessarily have to be MSC3051, it could also be switching to something more like what was originally planned, where each relation type is its own key like m.in_reply_to)

Copy link
Contributor

Choose a reason for hiding this comment

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

That was one of the motivating usecases behind MSC3051:

You don't need to look up reply relations in multiple events for edits. The edited event is canonical and can be used standalone, without having to look up the original event to figure out, what was replied to. You can also remove a relation with an edit now. (Useful if you replied to the wrong message or didn't mean to reply to anyone.)

If you don't include the thread id, the event simply isn't in a thread with the relation format. This means you can edit events out of threads or move them between threads and you get rid of the second level lookup. (Also a lot of stuff just makes more sense)

It doesn't spell out reactions specifically, because currently those are assumed to not be possible to edit and threads weren't in the spec (neither were relations), but since the MSC is about including all current relations for an event inside of the event, that is the only thing that makes sense if you combine threads and reactions.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, I meant 3051 instead of 4023. Have edited my comment.

Copy link
Member

Choose a reason for hiding this comment

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

@andybalaam, I think you meant MSC3051 rather than MSC3501?

Thank you, fixed in my comment. Seems like you've had just enough coffee!

Copy link
Member

@anoadragon453 anoadragon453 Jun 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@andybalaam It's even different from "What message am I a reaction to?" because, crucially, if we receive a receipt for a reaction, we need to act on it even if we don't have the event it is a reaction to (where by act on it I mean consider this thread to be read).

I'm confused why MSC3051 would not solve this use case. Would the reaction not have the following in its m.relations array property:

      {
        "event_id": "$root-of-some-thread",
        "rel_type": "m.thread"
      },

...which would allow you to know whether to mark the thread as read or not?

@uhoreg one key difference between the two is that MSC3051 relies on the sending client to populate the relations, so using that for this purpose would mean that the sending client would add the thread ID, which it could fail to do for whatever reason. On the other hand, this proposal relies on the server to add the thread ID, so would be more reliable. So I'm not entirely sure that MSC3051 is a replacement for this.

Consequently, if the server does not have the first-order event, it won't know to add the unsigned thread_id property to a second-order event.

Copy link
Member

Choose a reason for hiding this comment

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

This somewhat randomly came up in discussions around stuck notifications. I'm more preferential to considering this MSC (4023) in the short term to fix the problem scope, as asking sending clients to do the right thing will take longer, potentially require the use of smart proxies (for constrained environments), and generally be harder to adopt in the short term.

This MSC will need to outline why MSC3051 is a good solution for a longer term, and that having the server calculate thread-ness in the meantime is best.

Copy link
Member

Choose a reason for hiding this comment

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

Hopefully my edits clarify this. I haven't put in a great deal about why 3051 is more long term other than, well, it's a change that will literally take a long time to get rolled out. I don't think there's really much more to it?


## Security considerations

No security considerations

## Unstable prefix

While this MSC is not considered stable by the specification, implementations _must_ use
`org.matrix.msc4023.thread_id` in place of `thread_id`.

## Dependencies

This MSC does not have dependencies