Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Synapse does not apply edits to events other than m.room.message #12793

Closed
richvdh opened this issue May 19, 2022 · 7 comments · Fixed by #14034 or #15295
Closed

Synapse does not apply edits to events other than m.room.message #12793

richvdh opened this issue May 19, 2022 · 7 comments · Fixed by #14034 or #15295
Assignees
Labels
A-Spec-Compliance places where synapse does not conform to the spec P3 (OBSOLETE: use S- labels.) Approved backlog: not yet scheduled, will accept patches S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@richvdh
Copy link
Member

richvdh commented May 19, 2022

MSC2676 says:

Whenever a homeserver would return an event via the Client-Server API, it should check for any valid, applicable m.replace event, and if one is found, it should first modify the content of the original event according to the m.new_content of the most recent edit.

It also gives a comprehensive list of what comprises a "valid" edit event.

Synapse does this, but only where the events are of type m.room.message. This is incorrect: it should also do so for other event types. Examples include polls (org.matrix.msc3381.poll.start), which can be edited via the Element UI, and stickers (m.sticker), which cannot be edited via the UI but can via the API.

This is closely related to #12503, but that concerns aggregations, whereas this issue is about updating the content of the original event.

@richvdh richvdh added A-Spec-Compliance places where synapse does not conform to the spec T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. P3 (OBSOLETE: use S- labels.) Approved backlog: not yet scheduled, will accept patches S-Minor Blocks non-critical functionality, workarounds exist. labels May 19, 2022
@clokep clokep self-assigned this Oct 3, 2022
@alariej
Copy link

alariej commented Oct 6, 2022

I came across this issue while doing tests on matrix.org. As far as I can tell, Synapse doesn't even seem to "modify the content of the original event according to the m.new_content of the most recent edit.", even for m.room.message events. The original message content does not get modified after one or even several edits. Below are the message objects from a test I just did on matrix.org. The message was created and edited in a fresh non-encrypted private room using Element Web. After editing the message, I logged out, refreshed the browser, and logged back in to make sure the events were fetched again from the server.

Original message, before and after editing:

content:
  body: "hi"
  msgtype: "m.text"
  org.matrix.msc1767.text: "hi"
event_id: "$PbrLqfPm7MBDHLZd5gVREn3VA8UyFc8qb9220xiKYlw"
origin_server_ts: 1665067355108
sender: "@xyz:matrix.org"
type: "m.room.message"

Message created by editing:

content: 
  body: " * hi hi"
  msgtype: "m.text"
  org.matrix.msc1767.text: " * hi hi"
  m.new_content: 
    body: "hi hi"
    msgtype: "m.text"
    org.matrix.msc1767.text: "hi hi"
  m.relates_to: 
    event_id: "$PbrLqfPm7MBDHLZd5gVREn3VA8UyFc8qb9220xiKYlw"
    rel_type: "m.replace"
event_id: "$rOEXMZKBNp8iYoBcSvdOFIE7bAqVVpJGLhc68ENPLw8"
origin_server_ts: 1665067377760
sender: "@xyz:matrix.org"
type: "m.room.message"

I guess this would not get picked up by users as Element will anyway display the edited content of messages, but I'm wondering if something has changed in the protocol or implementation since this issue was opened. Or am I missing something completely obvious here?

@clokep
Copy link
Member

clokep commented Oct 6, 2022

As of today, the correct behavior should be applied, but only for m.room.message events. How are you requesting the event?

@alariej
Copy link

alariej commented Oct 6, 2022

Via the sync API.

@clokep
Copy link
Member

clokep commented Oct 6, 2022

Via the sync API.

Bundled aggregations are only returned if the result is limited, see here:

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

You can then see here when aggregations are bundled:

[GET /sync](https://spec.matrix.org/v1.4/client-server-api/#get_matrixclientv3sync) when the relevant section has a limited value of true.

@alariej
Copy link

alariej commented Oct 6, 2022

Oh right, I had missed the point with the "limited". Not quite sure about the logic of it, but in any case, thanks for the info!

@clokep
Copy link
Member

clokep commented Oct 24, 2022

See #14252.

@clokep
Copy link
Member

clokep commented Mar 21, 2023

Hm actually I think this was fixed by #15193 to no longer apply edits.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Spec-Compliance places where synapse does not conform to the spec P3 (OBSOLETE: use S- labels.) Approved backlog: not yet scheduled, will accept patches S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
3 participants