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

Synapse does not bundle m.replace aggregations for events other than m.room.message #12503

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

Synapse does not bundle m.replace aggregations for events other than m.room.message #12503

richvdh opened this issue Apr 19, 2022 · 10 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 Apr 19, 2022

Per MSC2675, when the server returns an event, it is supposed to bundle the aggregation of any related events into the response.

This includes m.replace relations: for an m.room.message event, /context/$original_event_id might return:

{
  "event": {
    ...
    "event_id": "$original_event_id",
    "type": "m.room.message",
    "unsigned": {
      "m.relations": {
        "m.replace": {
          "event_id": "$replacement_event_id",
          "origin_server_ts": 1650390324643,
          "sender": "@richvdh:matrix.org"
        }
      }
    },
    "user_id": "@richvdh:matrix.org"
  }
}

However, for other event types, no m.replace aggregation is returned.

It's not as if m.room.message is the only event type you can edit (consider, for example, encrypted events), so this seems odd.

@richvdh
Copy link
Member Author

richvdh commented Apr 19, 2022

Presumably this means that if you forward-paginate through a room with /messages and get as far as the updated event, you'll see the old content rather than the edited content.

@clokep
Copy link
Contributor

clokep commented Apr 19, 2022

Presumably this means that if you forward-paginate through a room with /messages and get as far as the updated event, you'll see the old content rather than the edited content.

Another situation this can impact clients is when viewing a thread (which uses the /relations API). @t3chguy mentioned to me that clients essentially assume all encrypted events have an edit and attempt to re-fetch them from the server to see if this is accurate. (matrix-js-sdk is using /relations to see if there are edits, see https://github.com/matrix-org/matrix-js-sdk/blob/v17.1.0-rc.1/src/models/thread.ts#L240.)

See also element-hq/element-web#21447 and element-hq/element-web#21445

@richvdh
Copy link
Member Author

richvdh commented Apr 19, 2022

@t3chguy mentioned to me that clients essentially assume all encrypted events have an edit and attempt to re-fetch them from the server to see if this is accurate.

so to be clear: that is a workaround for this bug, and if we fixed this bug, clients could remove that workaround?

@clokep
Copy link
Contributor

clokep commented Apr 19, 2022

@t3chguy mentioned to me that clients essentially assume all encrypted events have an edit and attempt to re-fetch them from the server to see if this is accurate.

so to be clear: that is a workaround for this bug, and if we fixed this bug, clients could remove that workaround?

This is my understanding, yes. 👍

@t3chguy
Copy link
Member

t3chguy commented Apr 19, 2022

Yes, once enough server uptake EW will gladly remove the check which asks the server for /relations on each and every thread response

@clokep clokep added S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. labels Apr 19, 2022
@richvdh richvdh added 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 labels May 19, 2022
@clokep clokep self-assigned this Oct 3, 2022
@clokep clokep reopened this Oct 24, 2022
@clokep
Copy link
Contributor

clokep commented Oct 24, 2022

See #14252.

@clokep
Copy link
Contributor

clokep commented Jan 23, 2023

@richvdh I guess we might be able to re-land this once MSC3925 becomes generally available? Otherwise, the support for that might be lacking as-is.

@richvdh
Copy link
Member Author

richvdh commented Jan 24, 2023

@richvdh I guess we might be able to re-land this once MSC3925 becomes generally available?

That is my hope, yes!

@clokep
Copy link
Contributor

clokep commented Mar 6, 2023

@richvdh I guess we might be able to re-land this once MSC3925 becomes generally available?

That is my hope, yes!

We've now merged #15193, so we can probably try this again soon.

@clokep
Copy link
Contributor

clokep commented Mar 17, 2023

@richvdh I guess we might be able to re-land this once MSC3925 becomes generally available?

That is my hope, yes!

We've now merged #15193, so we can probably try this again soon.

The changes in #15193 just shipped this week (in v1.79.0). I haven't seen any reports of regression due to it, but to be safe I think we should likely wait until v1.81 before making this change.

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