Change the server aggregation for edits #1440
Conversation
| 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. |
There was a problem hiding this comment.
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 }}
There was a problem hiding this comment.
{{< 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.
| @@ -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", | |||
There was a problem hiding this comment.
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.
| 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)). |
There was a problem hiding this comment.
| 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:
| 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. |
There was a problem hiding this comment.
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.
| Note that server implementations must not *actually* overwrite | ||
| the original event's `content`: instead the server presents it as being overwritten |
There was a problem hiding this comment.
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 %}}
There was a problem hiding this comment.
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).
|
@turt2live PTAL. I also realised I'd left in some untruths in the "client behaviour section" - cf 05d1779 |
per matrix-org/matrix-spec-proposals#3925
Fixes: #1299
Preview: https://pr1440--matrix-spec-previews.netlify.app