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

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

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

Conversation

germain-gg
Copy link
Contributor

@germain-gg germain-gg commented May 31, 2023

Rendered

Pull Request Checklist

  • Pull request includes a sign off
  • Update the title and file name of your proposal to match this PR's number (after opening).
  • Pull request includes a 'Rendered' link above.
  • Ask in
    #matrix-spec:matrix.org to
    get feedback on this PR.

@germain-gg germain-gg force-pushed the gsouquet/unsigned-thread-id branch from c83070e to f34e69e Compare May 31, 2023 15:58
@germain-gg germain-gg marked this pull request as ready for review May 31, 2023 15:58
Comment on lines 46 to 47
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
Contributor

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
Contributor

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

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
Contributor

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

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?

@clokep clokep changed the title Thread ID for 2nd order-relation MSC4023: Thread ID for 2nd order-relation May 31, 2023
@turt2live turt2live added proposal A matrix spec change proposal client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels May 31, 2023
proposals/4023-unsigned-thread-id.md Outdated Show resolved Hide resolved
proposals/4023-unsigned-thread-id.md Outdated Show resolved Hide resolved
@germain-gg germain-gg requested review from clokep and richvdh June 2, 2023 07:59
@richvdh richvdh removed their request for review June 2, 2023 10:11

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

@dbkr
Copy link
Member

dbkr commented Dec 6, 2023

I've taken the liberty of taking over this MSC as I've been looking at the problem and I'm assuming @germain-gg has no desire to keep pushing it forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
Status: Scheduled - v1.8
Development

Successfully merging this pull request may close these issues.

None yet