event properties in API responses and AS API duplicated as top-level field

[joepie91]

When calling the /messages endpoint (also /search, /context, etc) each event in the chunk will have a number of top-level properties that duplicate those within unsigned:

• age (see also: age logic for events is broken #8429)
• redacted_because
• replaces_state
• invite_room_state
• knock_room_state.
• prev_content (though this one was specced until MSC3442)
• user_id (duplicates sender)

These top-level properties are not a part of the spec.

(Edited by @richvdh to make the list of affected properties clear)

[richvdh]

as long as clients aren't depending on them.

well, therein lies the rub.

Thinking about this a bit harder: any client that wants to use these properties, and be compatible with /sync, must be using the values from unsigned rather than the top-level properties... so I'd be reasonably confident removing them from the top level.

[richvdh]

For anyone thinking of looking at this: the code that does the copying is at https://github.com/matrix-org/synapse/blob/v1.43.0/synapse/events/utils.py#L255-L265. Note that that code also includes prev_content which is a slightly more complex case (see matrix-org/matrix-spec-proposals#877). I'd also be inclined to get rid of the unspecced user_id property.

[richvdh]

any client that wants to use these properties, and be compatible with /sync, must be using the values from unsigned rather than the top-level properties... so I'd be reasonably confident removing them from the top level.

ARGH. Of course that neglects application services, which don't need to /sync (and the AS API copies the keys to the top level)

[richvdh]

I still think we should work to get rid of these spurious top-level keys, but it seems like there is potential for breakage. Accordingly it might be best for us to clarify the situation for prev_content first (matrix-org/matrix-spec-proposals#877) so that then we can break everything at once.

[richvdh]

MSC3442 clarified that prev_content should be returned under unsigned.

It's still possible that some things - most likely ASes which don't need to worry about /sync - will be broken by removal of these properties, but we really should try and clean them up anyway.