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.

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

prev_content is returned at the top-level by most of the CS API #877

Closed
ara4n opened this issue Apr 2, 2017 · 7 comments · Fixed by #3442 or #3524
Closed

prev_content is returned at the top-level by most of the CS API #877

ara4n opened this issue Apr 2, 2017 · 7 comments · Fixed by #3442 or #3524
Labels
client-server Client-Server API wart A point where the protocol is inconsistent or inelegant

Comments

@ara4n
Copy link
Member

ara4n commented Apr 2, 2017

The spec is unclear about where prev_content should be returned. Most parts imply it should be at the top level of any events, but there are also parts that show it being returned under unsigned.

In practice, Synapse returns it in both places, except for the following APIs, where it is only returned under unsigned.

  • /_matrix/client/r0/sync
  • /_matrix/client/r0/notifications
  • /_matrix/client/r0/rooms/{room_id}/state/{type}/{key} when called with the unspecified(!) format=event parameter.
@ara4n
Copy link
Member Author

ara4n commented Apr 2, 2017

all the examples seem to show prev_content for state events being returned as a top level field on the event. but in practice synapse seems to return it most but not all of the time in unsigned. this is incredibly confusing

@ara4n ara4n changed the title "clarify when prev_content is and isn't returned by CS API in unsigned" "all the examples seem to show prev_content for state events being returned as a top level field on the event. but in practice synapse seems to return it most but not all of the time in unsigned. this is incredibly confusingL clarify when prev_content is and isn't returned by CS API in unsigned Apr 2, 2017
@richvdh
Copy link
Member

richvdh commented Apr 5, 2017

basically a subset of #684

@richvdh richvdh added spec-bug Something which is in the spec, but is wrong clarification An area where the spec could do with being more explicit and removed spec-bug Something which is in the spec, but is wrong labels Apr 5, 2017
@turt2live
Copy link
Member

@joepie91
Copy link

Just to clarify: is prev_content being in unsigned rather than top-level a spec bug or a Synapse implementation error?

@richvdh
Copy link
Member

richvdh commented Jul 13, 2020

there's a lot of discussion about this on #2648. In short: the spec says it should be in the top-level, so clients and servers have to respect that. It's a shame that it says that, making this a protocol wart. (As opposed to a spec bug, which is where the spec lies about the protocol.)

@richvdh richvdh changed the title clarify when prev_content is and isn't returned by CS API in unsigned prev_content is returned at the top-level by most of the CS API Sep 2, 2021
@richvdh richvdh added wart A point where the protocol is inconsistent or inelegant and removed clarification An area where the spec could do with being more explicit labels Sep 2, 2021
@richvdh richvdh changed the title prev_content is returned at the top-level by most of the CS API prev_content (and other keys) are returned at the top-level by most of the CS API Sep 2, 2021
@richvdh richvdh changed the title prev_content (and other keys) are returned at the top-level by most of the CS API prev_content is returned at the top-level by most of the CS API Sep 24, 2021
@richvdh
Copy link
Member

richvdh commented Sep 24, 2021

Oh man, this is a whole lot worse than I thought it was.

It's correct that Synapse has a v1 event serialisation format and a v2 serialisation format, and that the latter is only used by /sync, /notifications, and the undocumented format=event for /state.

However, my understanding of what it does for v1 was incorrect. Specifically, a bunch of keys (age, redacted_because, replaces_state, prev_content, invite_room_state, knock_room_state), are sent in both unsigned and at the top level of the event.

For most of the above, I can find no mention in the spec of those keys outside an unsigned section. The exception is prev_content, which is specced in various places (https://matrix.org/docs/spec/client_server/r0.6.1#state-event-fields among others) to be present at the top level as well as being mentioned under unsigned in some places.

Sooo... We should consider the two cases separately.

age, redacted_because, replaces_state, invite_room_state, knock_room_state

The spec says nothing about these being present at the top-level. Synapse returns copies there, but I think we can consider that a spec violation. Accordingly, let's track them on matrix-org/synapse#7925.

prev_content

This is absolutely a mess. There's enough in the spec about it being at the top level that we can't just take the same approach as the other key.

The fact that again, synapse returns it in both unsigned and at the top level, but only in unsigned for /sync makes me suspect that any real clients should be using the copy in unsigned, and updating the spec to say that's where it should be is attractive. But that needs an MSC.

@richvdh
Copy link
Member

richvdh commented Oct 25, 2021

I'm actually going to reopen this, to track it being clarified in the spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API wart A point where the protocol is inconsistent or inelegant
Projects
None yet
4 participants