-
Notifications
You must be signed in to change notification settings - Fork 380
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
base: old_master
Are you sure you want to change the base?
Conversation
4160b4b
to
d5fad32
Compare
Signed-off-by: Nicolas Werner <[email protected]>
d5fad32
to
cf18320
Compare
…fications Co-authored-by: Kevin Cox <[email protected]>
Since this has "needs-implementation", I believe I can go ahead and add experimental support to Synapse/Element? |
It has an implementation, where do you see "needs-implementation"? :D (Not that I mind more implementations) |
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 |
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 |
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. |
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) |
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 |
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 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 |
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.
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.
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.
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.
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 see, that makes sense
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.
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.
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.
@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.
Option two is moved to matrix-org#3382 Also added link to matrix-org#3051 with array implementation of relations.
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. |
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.
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
?
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.
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.
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.
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.
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.
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.
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. |
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 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.
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.
the other use case mentioned is replacing the original message's replied to message with an edit
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.
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)
- 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.) |
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'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?
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'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.
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. |
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 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").
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'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.
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. |
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 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?
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.
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.
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). |
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'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.
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 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 <[email protected]>
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 |
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.
MSC2674 is now canon. Also, some minor grammar/wording improvements:
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 |
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.
Note that the spec does not use the term "relation" anywhere in the text.
# 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 |
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 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.
Signed-off-by: Nicolas Werner [email protected]
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).