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

MSC3051: A scalable relation format #3051

Open
wants to merge 3 commits into
base: old_master
Choose a base branch
from

Conversation

deepbluev7
Copy link
Contributor

@deepbluev7 deepbluev7 commented Mar 5, 2021

Signed-off-by: Nicolas Werner nicolas.werner@hotmail.de

Rendered

Nheko's implementation for edits uses this released with version 0.8.2 and newer. This falls back transparently to the current format and adds both to events. mtxclient PR (some bugfixes happened at a later date).

@deepbluev7 deepbluev7 changed the title MSC0000: scalable relation format MSC3051: scalable relation format Mar 5, 2021
@turt2live turt2live added kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal proposal-in-review client-server Client-Server API labels Mar 5, 2021
@deepbluev7 deepbluev7 changed the title MSC3051: scalable relation format MSC3051: A scalable relation format Mar 5, 2021
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
proposals/3051-scalable-relations.md Outdated Show resolved Hide resolved
…fications

Co-authored-by: Kevin Cox <kevincox@kevincox.ca>
@turt2live turt2live added needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. and removed needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Jun 8, 2021
@chayleaf
Copy link

Since this has "needs-implementation", I believe I can go ahead and add experimental support to Synapse/Element?

@deepbluev7
Copy link
Contributor Author

deepbluev7 commented Aug 28, 2021

It has an implementation, where do you see "needs-implementation"? :D

(Not that I mind more implementations)

@chayleaf
Copy link

chayleaf commented Aug 28, 2021

oh, i only quickly glanced at this paper and thought it reused m.relates_to. In that case, no special changes in particular are needed indeed.

edit: i think a fallback option needs to be specified, so old clients can render a "best effort" version of the message

@chayleaf
Copy link

to add to the above - i just want to know how the implementations should act when e.g. both the array and the relates_to specifies the fact the message is an edit, and whether editing a message should set relates_to for fallback

@deepbluev7
Copy link
Contributor Author

Since relates_to is the older format, I would use the array, when both are present, since clients will be sending the old format as fallback.

@chayleaf
Copy link

Another thing worth thinking about is encryption (though it could alternatively be discussed in MSC2674). Relation aggregations seem useful, but other than anything needed for that, the server doesn't seem to need to know anything (as discussed in https://github.com/matrix-org/matrix-doc/issues/2678)

@deepbluev7
Copy link
Contributor Author

Best case you open a thread for every concern. Makes it easier to keep track of what is still open. For that just leave a comment on one of the lines in the proposal.

relations are a strict superset, which may be useful to make handling inside of
a client easier.

## Potential issues

Choose a reason for hiding this comment

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

I think fallback needs to be touched upon as well - mostly just what should clients send as fallback info

There are many cases where 2 or more relations on an event would be useful. This
MSC proposes a simple way to do that and replace the currently proposed format.

## Proposal
Copy link

@chayleaf chayleaf Aug 30, 2021

Choose a reason for hiding this comment

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

You should potentially touch upon encryption as well? See https://github.com/matrix-org/matrix-doc/issues/2678 for ongoing discussion.

In short, aggregations are useful - so the server needs to be able to return all events relating to a specific message - but the server doesn't have to know any more than that. Potentially, even filtering by event type isn't needed (and if it becomes necessary, it can always be added later, adding unencrypted metadata is easier than removing it). In the unencrypted version of the message content, you could hash the event_id field using a message-specific salt, and rel_type could either be omitted or hashed as well; other data has not to be included.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What @Sorunome mentioned on #2678 is an idea we worked on together. Until that is properly worked out, we would just not encrypt the relations. In theory you don't need to know the actual values to aggregate relations with APIs. You can just tell the API what values it should aggregate for you. It is just less efficient and you run into trouble, if you automatically want to include the aggregations in the unsigned section. I think encryption for relations can be solved in an independent MSC, since it is quite a difficult topic. In theory privacy sensitive clients could also just not put unencrypted relations into the event at all, although currently that would be disallowed.

Choose a reason for hiding this comment

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

i see, that makes sense

Copy link
Contributor

Choose a reason for hiding this comment

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

Creating an unencrypted protocol and trying to layer encryption on top later is not a good way to make a secure protocol. We should avoid adding leaky features until the encryption had been sorted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevincox, you can complain about that on the original relations MSC. I made this MSC to fix some issues with the original MSC, but I didn't want any big changes that would make them hard to compare or lead to additional bikeshedding. I think encrypted relations can just be a separate relation type without the need of having to define the exact format upfront, the same way that Matrix defined messages first and later added an encrypted type.

MurzNN added a commit to MurzNN/matrix-doc that referenced this pull request Sep 7, 2021
Option two is moved to matrix-org#3382
Also added link to matrix-org#3051 with array implementation of relations.
Comment on lines +59 to +67
If clients want to stay backwards compatible (for a while at least), in many
instances it is possible to generate an `m.relates_to` object from the relations
list. This can be done by picking a primary relation, i.e. the edit relation,
and then packaging up the remaining relations in `m.new_content` or simply
throwing them away. Since this proposal uses `m.relations`, this does not
conflict with the current relations from the other MSCs. One can also generate
the relations object from this MSC from the old relations, since the new
relations are a strict superset, which may be useful to make handling inside of
a client easier.
Copy link
Member

Choose a reason for hiding this comment

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

This is unclear how this would be implemented, is there a prioritized list of what relations to use? What happens if m.relates_to conflicts with m.relations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there is m.relations, you use that. it is a superset of m.relates_to and if a client sends both, the m.relates_to is probably a fallback.

A concrete example of how one can implement the fallback parsing logic is here: https://github.com/Nheko-Reborn/mtxclient/pull/48/files#diff-6c2fae13f9cbfbde2c2f9e0f681b252e3d6f33df71d3f495637ce6e17b1286a9R211-R263

Basically for parsing you can always convert relations to the new format by just parsing any relation you can and stuffing the in the list. One issue is that replies might get lost, for that we use a flag to indicate, that this was generated and in that case use the normal lookup rules for what an edit is a reply to.

Emitting the fallback is a bit more tricky. What my implementation does, is that it orders relations by priority. If something is an edit, we send an edit in m.relates_to. Otherwise we just send the first relation we find, because the other relations usually don't get combined so far.

Copy link
Member

Choose a reason for hiding this comment

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

You probably want to do some assertion that what's in m.relates_to is the same as what's in m.relations, otherwise this could be used to show different clients different content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That does sound reasonable, but I don't think it is that important and we don't have the same for the other fallbacks either. I.e. the edit fallback, the reply fallback. If you implement sanity checks for those, you probably want them here too. Tbh, I would prefer to keep the period where one needs to emit a fallback to a minimum, because I don't like that clients can see different things, but #2781 doesn't seem to be a priority for anyone either, so the consensus seems to be, that this is an acceptable risk.

Comment on lines +13 to +14
There are many cases where 2 or more relations on an event would be useful. This
MSC proposes a simple way to do that and replace the currently proposed format.
Copy link
Member

Choose a reason for hiding this comment

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

I would find it useful to mention some of these use-cases. The only one I see below is "a description for multiple files", which I don't think even has a relation proposed.

Are there other use-cases you can think of that would be useful? The only one I know of is threads.

Choose a reason for hiding this comment

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

the other use case mentioned is replacing the original message's replied to message with an edit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replacing replies in an edit (or removing it), editing inside of threads, replying inside of threads.

Considering what relations we currently have:

  • replies
  • edits
  • threads
  • annotations
  • (references)

I can see it being useful for edits and threads, while for annotations and replies it might only be useful in combination with other relations. No idea about references, since those are currently not very well defined. I don't think it is that unlikely to say in the future there will be more relation types, that can benefit from it. (I.e. I could imagine wanting to reply to multiple messages, to show someone when something was mentioned before and other cool stuff)

Comment on lines +50 to +54
- 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.)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really sure I follow what this is suggested. Does this propose changes to MSC2676? I don't see how this really helps, maybe this section could use an example of an event which gets edited twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a few examples of how this would affect the other relations in 2 Appendices. Those are just ideas but not actual changes to those MSCs, since that is probably better done on those MSCs.

Comment on lines +85 to +91
Some relation types should probably not be combined. For example you may
disallow editing a reaction, because clients probably won't be handling that
correctly. This MSC however does not disallow that. Specifications that define relations should specify,
how clients should handle that and clients sending such combinations should be
aware, that those probably won't get handled. I don't think just allowing 1
relation is the solution to handling such conflicts and I don't think they will
happen much in practice.
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 it is up to this MSC to define how this would work for the existing relations (which are MSCs, but are widely implemented so are in a weird place, standards-wise).

I'm a bit nervous this will put us down a path where we need to have "relation rules" to define what a valid set of relations on an event is. This might be worth it, but would need to be thought through and could add a lot of complexity to servers (as it is another set of "auth rules").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added examples for this, why I think we don't need very strict rules for this and how implicit rules could look like. Maybe you can give me an opinion on that, if that is enough to resolve your concern or if the MSC actually needs to spell out explicit rules for conflict resolution.

Comment on lines +77 to +81
I don't believe that is an issue in practice. If you edit a message with a
reply, there is a natural meaning to the combination of both relations. You can
even apply them in any order, imo. But there may be other relations, where this
causes more issues. An MSC introducing such a relation should specify how to
handle conflicts then.
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 if you have a message that contains an edit relation and a reply relation would mean:

Find the event that the edit refers to and replace it with this event, which is now also a reply.

That seems fairly hierarchical to me and I don't see how you can apply those in the opposite order to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends on how you implement your client. You can render the event as a reply first, and then place it at the location of the event, that was edited. Or you replace the event data in the database for that location first, then tell the UI to rerender that event, and it will naturally pick up that this event now is a reply to X.

At least in my clients, rendering events is usually a sequence if "is this a reply?", "is this an edit?", "is this in a thread?", but those things can be applied pretty much independently. There isn't really a need to order it protocol wise, because my clients just pick from the list, what they need. If you have [edit, reply] or [reply, edit], that should be easy to handle.

Alternatively, you could make it hierarchical, specify what each relation can contain as other relations. But I can't see much benefit there, it is just making a more complicated list/graph. I.e. if you have:

{
  "rel_type": "m.thread",
  "event_id": "$something",
  "m.in_reply_to": { "event_id": "$abc" }
}

What is the benefit over:

{
  {
    "rel_type": "m.thread",
    "event_id": "$something"
  },
  {
    "rel_type": "m.in_reply_to",
    "event_id": "$abc"
  }
}

In my case I found the first one to be harder to work with, because I needed to add a lot of special cases to the parser, while the second one didn't make the UI any harder to implement, while the SDK is much simpler. The first one also doesn't tell me how to extend it to support edits, that would be another special case, while in the second one it is natural. And the first one actually needs you to define an order, while a client might have an easier time, if the order was different.

I guess what I am trying to say, I don't see an explicit order that helpful. It is very much like a() && b() && c(), while that statement does have an order, if a, b and c don't have sideeffects, the result is the same, even if you reorder it.

Comment on lines +103 to +107
Multiple releations may increase load on the server and the client and provide
more opportunities to introduce bad data. Servers and clients should take
additional care and validate accordingly. It should not be considerably worse
than single relations though and servers may limit relations to a reasonable
amount (like they do for devices already).
Copy link
Member

Choose a reason for hiding this comment

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

I'm quite nervous at the potential for abuse here, it seems like it would be quite easy to put odd groups of relations together, maybe this is already possible with the current system and not made much worse though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is the biggest problem with this MSC. But I think in practice the amount of shenanigans you can do is somewhat limited. One issue I found, is that one can basically make a reply point to "itself" by having the edit relation and the reply relation point to the same event. So some clientside validation is definitely needed (same for the server side pagination APIs), but most of that is fixed by just doing basic sanity checks (maximum recursion depths, not rendering a reply relation on reactions, etc), I think most of those validations are fairly natural and you will have a harder time with the other fields in events having bad data (i.e. all the crypto events trying to cause overflows when parsing or similar). I think even if you cause an issue by making weird combinations, the result should in most cases be pretty harmless.

Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Comment on lines +4 to +6
shown, that relations between events are very powerful and useful. Currently the
format from [MSC2674](https://github.com/matrix-org/matrix-doc/pull/2674) is
used. That format however limits each event to exactly one relation. As a result
Copy link
Member

Choose a reason for hiding this comment

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

MSC2674 is now canon. Also, some minor grammar/wording improvements:

Suggested change
shown, that relations between events are very powerful and useful. Currently the
format from [MSC2674](https://github.com/matrix-org/matrix-doc/pull/2674) is
used. That format however limits each event to exactly one relation. As a result
shown that relationships between events are very powerful and useful.
However, the [current format](https://spec.matrix.org/v1.7/client-server-api/#definition-mrelates_to)
limits each event to at most one relationship. As a result

@@ -0,0 +1,371 @@
# MSC3051: Scalable relations
Copy link
Member

Choose a reason for hiding this comment

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

Note that the spec does not use the term "relation" anywhere in the text.

Suggested change
# MSC3051: Scalable relations
# MSC3051: Scalable event relationships

clients don't seem to support that and the actual deletion of a relation is
unexplored as well.

There are many cases where 2 or more relations on an event would be useful. This
Copy link
Member

@ara4n ara4n Jun 6, 2023

Choose a reason for hiding this comment

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

I agree that we seem to be coming up against more and more scenarios where having multiple relations on a single event could be useful. I've tried to summarise my original rationale at #4023 (comment) for sticking to a simple {Subject, Verb, Object} triple on relations, and concluding that the limits might outweigh the benefits - especially given the existence of extensible events, where we can decorate a given event with additional structured metadata; so why not also be able to decorate a given event with additional relations too. but tl;dr: i'd be supportive of changing to lists of relations rather than hacking around them with stuff like the is_falling_back field in MSC3440.

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:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants