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

Synapse returns unspecced property original_event from /relations #12930

Open
richvdh opened this issue May 31, 2022 · 15 comments
Open

Synapse returns unspecced property original_event from /relations #12930

richvdh opened this issue May 31, 2022 · 15 comments
Labels
A-Relations-Endpoint /relations A-Spec-Compliance places where synapse does not conform to the spec S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@richvdh
Copy link
Member

richvdh commented May 31, 2022

Synapse's implementation of /relations returns an unspecced property original_event:

https://github.com/matrix-org/synapse/blob/v1.60.0/synapse/handlers/relations.py#L166

This is not defined in the MSC so should be removed.

@richvdh richvdh added A-Spec-Compliance places where synapse does not conform to the spec S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. labels May 31, 2022
@clokep
Copy link
Member

clokep commented Jun 1, 2022

This is not defined in the MSC so should be removed.

Or should it be defined in MSC2676? See #5626 for why this was originally included. Maybe this is no longer necessary with #12476 though?

@revidee
Copy link

revidee commented Jun 1, 2022

Another use-case to consider:
With it removed, there is no "easy" way to retrieve the most up-to-date version of an event in all cases. the /event/ endpoint currently does not aggregate edits and returns the original event (by design in MSC2676). The server should bundle all relations via m.relations in the unsigned content, however, for e2ee rooms, this is not the case for m.replace events, since get_bundled_aggregations calls get_applicable_edits which only returns applicable edits of type m.room.message, but not m.room.encrypted (SQL 1, SQL 2) (This may be a bug in of itself, I'm not sure). Therefore, the client then needs to also make an additional request to /relations/ (with an filter on m.replace) in order to fetch the most recent applicable edit (decided client-sided). Maybe more, if the client needs to paginate through the relations.

When does this happen? After an initial sync without the whole history loaded and in the case client needs to render some out-of-timeline event due to a reference to an (potentially very) old event (m.reference, m.in_reply_to), the client needs to load this single event from the server, and preferably with all edits applied to it.

So in short, in order to then query the most recent state of an event, one would have to call the /event/ endpoint to get the original message, then, call /relations/ to get all edits and apply the most recent applicable.

With the original_event still included in the /relations/ result, the client would save 1 RTT and only needs to call /relations/.

@richvdh
Copy link
Member Author

richvdh commented Jun 1, 2022

This is not defined in the MSC so should be removed.

Or should it be defined in MSC2676? See #5626 for why this was originally included. Maybe this is no longer necessary with #12476 though?

Well, MSC2676 is concluded and in the process of being specced, so it would need to be a new MSC if there is a viable usecase for it.

@richvdh
Copy link
Member Author

richvdh commented Jun 1, 2022

With it removed, there is no "easy" way to retrieve the most up-to-date version of an event in all cases.

I haven't fully grokked your comment, but this part is incorrect: you can use /context for this.

@revidee
Copy link

revidee commented Jun 1, 2022

With it removed, there is no "easy" way to retrieve the most up-to-date version of an event in all cases.

I haven't fully grokked your comment, but this part is incorrect: you can use /context for this.

Unfortunately, not. In the case of e2ee rooms, the last applicable edit will not return anything, since the query (SQL 1, 2 in my comment) only checks for m.room.message. Hence, the server won't return the most-up-to-date-version.

@richvdh
Copy link
Member Author

richvdh commented Jun 1, 2022

since the query (SQL 1, 2 in my comment) only checks for m.room.message

well, that sounds like a separate bug: #12793 Edit: #12503 actually I think

@clokep
Copy link
Member

clokep commented Jun 1, 2022

This is not defined in the MSC so should be removed.

Or should it be defined in MSC2676? See #5626 for why this was originally included. Maybe this is no longer necessary with #12476 though?

Well, MSC2676 is concluded and in the process of being specced, so it would need to be a new MSC if there is a viable usecase for it.

Ah, I had missed this already had FCP proposed on it.

I think you're correct though that with a combination of using /event or /context and fixing #12503, this becomes unnecessary.

@richvdh
Copy link
Member Author

richvdh commented Jun 6, 2022

Well, MSC2676 is concluded and in the process of being specced, so it would need to be a new MSC if there is a viable usecase for it.

Sorry, this is incorrect; I must have been confusing it with some other MSC. (MSC2675, possibly, which specs /relations).

@clokep clokep mentioned this issue Sep 29, 2022
11 tasks
@clokep
Copy link
Member

clokep commented Sep 30, 2022

It looks like the matrix-js-sdk, matrix-android-sdk2 and matrix-ios-sdk all use this field in the response. matrix-js-sdk actually uses the field, I haven't really checked the other SDKs.

@richvdh
Copy link
Member Author

richvdh commented Oct 3, 2022

Thanks for checking into that, @clokep. Would you be able to raise issues against the relevant projects?

@richvdh
Copy link
Member Author

richvdh commented Oct 3, 2022

(I'd like us to set a timeline for removing this field. Is 6 months realistic?)

@clokep
Copy link
Member

clokep commented Oct 3, 2022

#14025 removes this field when using the /v1 version of the endpoint. It is still returned when querying via the /unstable endpoint. There are not currently any callers of the /v1 endpoint on matrix.org in the logs.

@turt2live
Copy link
Member

ftr, bot-sdk's usage is in an unstable part of the codebase, so if people are using it then that sucks for them :p

@turt2live
Copy link
Member

Per matrix-org/matrix-widget-api#75, the field was also (safely) used by the widget-api. At a glance, it looks like it won't explode if the field goes missing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Relations-Endpoint /relations A-Spec-Compliance places where synapse does not conform to the spec S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Development

No branches or pull requests

5 participants