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

Change the server aggregation for edits #1440

Merged
merged 4 commits into from
Mar 21, 2023
Merged

Change the server aggregation for edits #1440

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3ec3737
Change the server aggregation for edits
richvdh Feb 21, 2023
97b7544
Address review comments
richvdh Feb 28, 2023
05d1779
Clients cannot ignore edits
richvdh Feb 28, 2023
f6063f9
more review tweaks
richvdh Feb 28, 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
1 change: 1 addition & 0 deletions changelogs/client_server/newsfragments/1440.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Changes to the server-side aggregation of `m.replace` (edit) events, per [MSC3925](https://github.com/matrix-org/matrix-spec-proposals/pull/3925).
richvdh marked this conversation as resolved.
Show resolved Hide resolved
52 changes: 32 additions & 20 deletions content/client-server-api/modules/event_replacements.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines -137 to -138
Copy link
Member

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?

{{% boxes/note %}}
Note that server implementations must not *actually* overwrite
the original event's `content`. Doing so would cause the original
event to fail signature checks.
{{% /boxes/note %}}

Copy link
Member Author

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

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
Expand Down Expand Up @@ -199,10 +192,11 @@ 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 for the client-server API (see [Room Event Format](#room-event-format)).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
replacement event, formatted as normal for the client-server API (see [Room Event Format](#room-event-format)).
replacement event. {{< changed-in v="1.7" >}} That event is formatted as normal for the client-server API (see [Room Event Format](#room-event-format)).

arguably we can also use less words because we're already in the CS API:

Suggested change
replacement event, formatted as normal for the client-server API (see [Room Event Format](#room-event-format)).
replacement event. {{< changed-in v="1.7" >}} That event is formatted [as normal](#room-event-format) for clients.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that I've moved the changed-in to the top of the section I don't think this extra changed-in is helpful. I've applied the wording changes, with tweaks.


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
Expand All @@ -211,16 +205,35 @@ 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",
Copy link
Member

Choose a reason for hiding this comment

The 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
}
```

Expand All @@ -231,15 +244,14 @@ 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

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).
{{< changed-in v="1.7" >}} In previous versions of the specification, only the
`event_id`, `origin_server_ts`, and `sender` of the replacement event were included
in the aggregation. v1.7 extends the aggregation to the full event.
richvdh marked this conversation as resolved.
Show resolved Hide resolved

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).
{{< changed-in v="1.7" >}} In previous versions of the specification, servers
were expected to replace the content of the original event with that of the
replacement event. However that behaviour made reliable client-side
implementation difficult, and servers should no longer make this replacement.
Copy link
Member

Choose a reason for hiding this comment

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

As far as spec language is concerned, this should be something like:

{{< changed-in v="1.7" >}} Servers MUST NOT return the most recent edit from [/event].

{{ boxes/rationale }}
In previous versions...
{{ /boxes/rationale }}

Copy link
Member Author

Choose a reason for hiding this comment

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

{{< changed-in v="1.7" >}} Servers MUST NOT return the most recent edit from [/event].

this seems to say the opposite to what is intended.

I've moved the changed-in to the top of the section, which seems to be what we've done elsewhere, and moved this explanation into a "rationale" as you suggest.


#### Client behaviour

Expand Down