Document all event keys shown in examples (SPEC-416)

[matrixbot]

The client-server spec shows events as JSON in examples for various API responses. Many of these contain keys that are not described anywhere in the spec. Reading the unstable federation spec, it seems that they are related to federation, but anything that appears in the client-server spec should be explained there. In particular, I've noticed at least the following event keys that are never explained:

• age
• origin_server_ts
• replaces_state

Looking at the implementation in Synapse, there many more keys that appear in the stored JSON for each event that are not shown at all in the client-server spec, including at least:

• auth_events
• depth
• origin
• hashes
• prev_events
• prev_state
• signatures
• unsigned

These are all mostly explained in the federation spec, but if they would ever appear in the JSON of an event sent to or returned by the client-server API, they should be explained at least briefly there.

There is also an inconsistency for the "age" key, which appears at the top level of the JSON in the client-server spec's examples, but exists within the "unsigned" object under the "age_ts" key in Synapse's database.

Since many of these undocumented keys seem to be stored as part of the event itself, it's important to understand how they should be persisted for future compatibility with federation (for example by ruma, before ruma-federation is implemented). This also means that a lot of the details in the "signing events" section of the federation spec may in fact be relevant to the client-server spec.

(Imported from https://matrix.org/jira/browse/SPEC-416)

(Reported by Jimmy Cuadra)

[matrixbot]

I don't see a way to edit the issue, but I wanted to add:

I think the "event structure" section of the client-server spec is where this information should be. It should explain, in addition to the purpose/meanings of these keys, which of them apply to which "kinds" of events (basic events, room events, and state events).

-- Jimmy Cuadra

[kyrias]

Regarding replace_state and prev_state:

commit 4317c8e5835f0c15bf882f737d3e3c2a5b85f73f
Author: Erik Johnston <erik@matrix.org>
Date:   Thu Nov 6 15:10:55 2014 +0000

    Implement new replace_state and changed prev_state
    
    `prev_state` is now a list of previous state ids, similiar to
    prev_events. `replace_state` now points to what we think was replaced.

[NegativeMjark]

It looks like "prev_state" is now always the empty list. It used to be a list of state events that were replaced by this event, but that information isn't needed anymore.

https://github.com/matrix-org/synapse/blob/e457034synapse/handlers/message.py#L452
https://github.com/matrix-org/synapse/blob/e457034/synapse/state.py#L202
https://github.com/matrix-org/synapse/blob/e457034/synapse/state.py#L223
https://github.com/matrix-org/synapse/blob/e457034/synapse/state.py#L266

We now use "replaces_state" which is taken from the resolved state generated by the state conflict resolution for the state before the event.

https://github.com/matrix-org/synapse/blob/e457034/synapse/state.py#L217
https://github.com/matrix-org/synapse/blob/e457034/synapse/state.py#L246

The "prev_content" and "prev_sender" are taken from the "replaces_state".

https://github.com/matrix-org/synapse/blob/e457034/synapse/storage/events.py#L963

[Ericson2314]

Not sure how/whether the status of the others has changed, but Signatures is still a block-box as of the client-server API, r0.4.0.

[turt2live]

A lot of the fields are well-described in the server server spec, as they are more for server-to-server communication.

[Ericson2314]

Ok thanks, that does help in the short term. But long term, that's not a very satisfactory answer is it? I'd hope for every type to have a hyperlink to its (single) definition. (There's some duplicated ones that thankfully agree, so far.) If that means a new mechanism to share code between the different interfaces, so be it.

[turt2live]

it's definitely not a long-term answer. This issue serves as a reminder that we absolutely need to improve the documentation here.

[jplatte]

Could the priority of this be raised? I think this is the biggest spec issue we have to deal with in Ruma.

[turt2live]

It's already at its highest priority, sorry. We'd happily accept PRs which make events more understandable.

[jplatte]

I would be happy to help clarify things! For when there are disconnects between the spec and Synapse though, what do I do? (asking mostly because of prev_content, which according to #877 (comment) is handled differently in Synapse than shown in the spec)

[turt2live]

that particular issue is probably best left resolved on its own tbh. The first step would be figuring out what synapse's approach is (finding the determinism) then asking in #matrix-spec why it's so awful.

[jplatte]

Haha, okay ^^

Thanks for the guidance!

[joepie91]

More fundamentally: are all of these keys supposed to appear in events in the C2S API to begin with? Or is a homeserver supposed to strip them out from responses to clients, even if they are stored internally for S2S purposes?

[kegsay]

Can the SCT please make some progress on this. @turt2live you say this is at the highest priority already but I don't see any label or milestone or anything on this issue.

I'm particularly frustrated by this due to #877 - we have a conclusion that it should sit in unsigned but the PR to actually land this in the spec was closed because it was deemed to be a backwards-incompatible change to the spec. What's worse is that synapse just sends it in multiple places, consuming needless bandwidth in the process for every single state change.

Dendrite only sends it in unsigned, and that seems to be the "right" place for it based on #2648 (comment)

I've asked previously what the reasoning was for moving prev_content to unsigned in the first place and was told this is because prev_content isn't something that's sent over the federation. Is the plan then to change the spec for the other endpoints as well eventually?

We also get Matrix developers frustrated that Dendrite doesn't contain certain keys mentioned in this issue, see matrix-org/dendrite#1754

Yes, I get that it's hard to remove fields because clients may be using them, but we have to clean this up before we get more server implementations that are just as inconsistent, especially when it comes to commonly used fields like prev_content. We should at least have the spec reflect what we want things to look like, rather than sit and do nothing.

[richvdh]

I'm particularly frustrated by this due to #877 - we have a conclusion that it should sit in unsigned

"should" as in "in an ideal world". We can't just unilaterally decide to change it due to compatibility problems.

What's worse is that synapse just sends it in multiple places, consuming needless bandwidth in the process for every single state change.

Well yes, that does sound annoying. I'm aware of matrix-org/synapse#7925 - does this also apply to fields other than age?

Dendrite only sends it in unsigned, and that seems to be the "right" place for it based on #2648 (comment)

Yes it's the "right" place in asmuchas servers shouldn't really be modifying the "signed" part of the event, but changing to that would be a compatibility-breaking change, so without introducing new API endpoints, we can't change it. I've tried to update #877 to covers that particular aspect.

I'll try and spend some time updating the spec to reflect the current, irritating, reality.

[richvdh]

I think, that as of #3658, all the fields that are meant to appear in the C-S API are correctly documented. Synapse tends to return a bunch of spurious ones, but those are synapse bugs (see matrix-org/synapse#7925, for example).