-
-
Notifications
You must be signed in to change notification settings - Fork 102
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
Change the server aggregation for edits #1440
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Changes to the server-side aggregation of `m.replace` (edit) events, as per [MSC3925](https://github.com/matrix-org/matrix-spec-proposals/pull/3925). |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -133,13 +133,6 @@ being overwritten entirely by `m.new_content`, with the exception of `m.relates_ | |
which is left *unchanged*. Any `m.relates_to` property within `m.new_content` | ||
is ignored. | ||
|
||
{{% boxes/note %}} | ||
Note that server implementations must not *actually* overwrite | ||
the original event's `content`: instead the server presents it as being overwritten | ||
when it is served over the client-server API. See [Server-side replacement of content](#server-side-replacement-of-content) | ||
below. | ||
{{% /boxes/note %}} | ||
|
||
For example, given a pair of events: | ||
|
||
```json | ||
|
@@ -195,14 +188,17 @@ replacement event. | |
|
||
##### Server-side aggregation of `m.replace` relationships | ||
|
||
{{< changed-in v="1.7" >}} | ||
|
||
Note that there can be multiple events with an `m.replace` relationship to a | ||
given event (for example, if an event is edited multiple times). These should | ||
be [aggregated](#aggregations) by the homeserver. | ||
|
||
The aggregation format of `m.replace` relationships gives the `event_id`, | ||
`origin_server_ts`, and `sender` of the **most recent** replacement event. The | ||
most recent event is determined by comparing `origin_server_ts`; if two or more | ||
replacement events have identical `origin_server_ts`, the event with the | ||
The aggregation format of `m.replace` relationships gives the **most recent** | ||
replacement event, formatted [as normal](#room-event-format). | ||
|
||
The most recent event is determined by comparing `origin_server_ts`; if two or | ||
more replacement events have identical `origin_server_ts`, the event with the | ||
lexicographically largest `event_id` is treated as more recent. | ||
|
||
This aggregation is bundled under the `unsigned` property as `m.relations` for any | ||
|
@@ -211,49 +207,61 @@ event that is the target of an `m.replace` relationship. For example: | |
```json | ||
{ | ||
"event_id": "$original_event_id", | ||
// irrelevant fields not shown | ||
"type": "m.room.message", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fwiw, I'm still very sad that #688 isn't a thing, which would ideally make this sort of example easy to update in the future. Non-blocking for this review. |
||
"content": { | ||
"body": "I really like cake", | ||
"msgtype": "m.text", | ||
"formatted_body": "I really like cake" | ||
}, | ||
"unsigned": { | ||
"m.relations": { | ||
"m.replace": { | ||
"event_id": "$latest_edit_event_id", | ||
"origin_server_ts": 1649772304313, | ||
"sender": "@editing_user:localhost" | ||
"type": "m.room.message", | ||
"content": { | ||
"body": "* I really like *chocolate* cake", | ||
"msgtype": "m.text", | ||
"m.new_content": { | ||
"body": "I really like *chocolate* cake", | ||
"msgtype": "m.text" | ||
}, | ||
"m.relates_to": { | ||
"rel_type": "m.replace", | ||
"event_id": "$original_event_id" | ||
} | ||
} | ||
} | ||
} | ||
} | ||
// irrelevant fields not shown | ||
} | ||
``` | ||
|
||
If the original event is | ||
[redacted](#redactions), any | ||
If the original event is [redacted](#redactions), any | ||
`m.replace` relationship should **not** be bundled with it (whether or not any | ||
subsequent replacements are themselves redacted). Note that this behaviour is | ||
specific to the `m.replace` relationship. See also [redactions of edited | ||
events](#redactions-of-edited-events) below. | ||
|
||
##### Server-side replacement of content | ||
**Note:** the `content` of the original event is left intact. In particular servers | ||
should **not** replace the content with that of the replacement event. | ||
|
||
Whenever an `m.replace` is to be bundled with an event as above, the server | ||
should also modify the content of the original event according to the | ||
`m.new_content` of the most recent replacement event (determined as above). | ||
|
||
An exception applies to [`GET /_matrix/client/v3/rooms/{roomId}/event/{eventId}`](#get_matrixclientv3roomsroomideventeventid), | ||
which should return the unmodified event (though the relationship should still | ||
be bundled, as described above). | ||
{{ boxes/rationale }} | ||
In previous versions of the specification, servers were expected to replace the | ||
content of an edited event whenever it was served to clients (with the | ||
exception of the | ||
[`GET /_matrix/client/v3/rooms/{roomId}/event/{eventId}`](#get_matrixclientv3roomsroomideventeventid) | ||
endpoint). However, that behaviour made reliable client-side implementation | ||
difficult, and servers should no longer make this replacement. | ||
{{ /boxes/rationale }} | ||
|
||
#### Client behaviour | ||
|
||
Clients can often ignore `m.replace` events, because any events returned | ||
by the server to the client will be updated by the server to account for | ||
subsequent edits. | ||
|
||
However, clients should apply the replacement themselves when the server is | ||
unable to do so. This happens in the following situations: | ||
|
||
* The client has already received and stored the original event before the | ||
message edit event arrives. | ||
|
||
* The original event (and hence its replacement) are encrypted. | ||
Since the server will not replace the content of any edited events, clients | ||
should take note of any replacement events they receive, and apply the | ||
replacement whenever possible and appropriate. | ||
|
||
Client authors are reminded to take note of the requirements for [Validity of | ||
replacement events](#validity-of-replacement-events), and to ignore any | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We might want to stick the first part of this note somewhere still, just in case servers do super strange things with their events?
Possibly at the end of the server implementation section with a bit more detail?
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 feels like it is more confusing than helpful. As a server implementer, I am led to wonder wtf this "actually" means.
I have added some more words to the "server-side aggregation" section which might help here (97b7544#diff-7791fd17e89af8337a50aaa7e5fdf6cb1c20013d38cd491cda852e696884be76R248).