Remove invite_room_state, knock_room_state from the CS API. Fix examples in SS API

[anoadragon453]

Fixes #1212
Fixes #848

More context in the linked issues. This PR makes a couple changes:

• It removes any reference to invite_room_state or knock_room_state on m.room.member in the CS API. Clients should instead be receiving this information down /sync, under rooms.invite.!room.invite_state.
• It removes the unsigned.invite_room_state from the PUT /_matrix/federation/v2/invite/{roomId}/{eventId} response example.
  • This wasn't specced, but Synapse is returning it, which may be why it is here. I believe that's a bug though: Stop returning unsigned.invite_room_state in PUT /_matrix/federation/v2/invite/{roomId}/{eventId} responses synapse#14064

[turt2live]

What if the client isn't syncing - how is it supposed to get this information? I'm not so sure it's a spec bug that these fields exist, at least.

[anoadragon453]

If the client isn't syncing, how do they know they've been invited in the first place?

In practice I'm not sure if there's any way for clients to currently access this field, at least before joining the room (#848), which is when they need this data. unsigned.invite_room_state is used to pass around stripped state events over the old v1 federation invite API, but I consider the v2 federation invite APIs method of putting them in a separate field more sensible.

I wouldn't be opposed to making it available without syncing, perhaps by allowing the client to call GET /_matrix/client/v3/rooms/{roomId}/state or GET /_matrix/client/v3/rooms/{roomId}/state/{eventType}/{stateKey} to retrieve their membership event before joining the room. But I'd like to see how the client would know to even do this beforehand.

[turt2live]

Appservices who don't sync :)

[anoadragon453]

Ahh. OK, yes, appservices do receive this information from Synapse via unsigned.invite_room_state today.

It still feels odd to include this field in the CS API spec when clients have no way to ever see it. Perhaps it should just be an extension in the AS API spec? Are application services the only consumers of this field?

[turt2live]

It might be the mandela effect, but I swear clients had/have access to this via unsigned too. It sort of feels like a thing which might need an MSC? (at least to identify how clients get access to this if they aren't /syncing, such as when previewing a room they were linked to or whatever)

[anoadragon453]

It might be the mandela effect, but I swear clients had/have access to this via unsigned too.

It shouldn't have been the case in Synapse, at least. We try to strip the field before sending it to clients in all cases: https://github.com/matrix-org/synapse/blob/06df5d4250f54d5a95b0c90bfc9352ec6f02c520/synapse/events/utils.py#L371-L377

The only case where this conditional doesn't pop the entries is for sending to application services, and down /sync (but later on in the /sync code we pop the field anyways).

It sort of feels like a thing which might need an MSC? (at least to identify how clients get access to this if they aren't /syncing, such as when previewing a room they were linked to or whatever)

It could perhaps all just be replaced by GET /_matrix/client/v1/rooms/{roomId}/hierarchy or MSC3266 (room summary API), which both return stripped state events?

[richvdh]

It might be the mandela effect, but I swear clients had/have access to this via unsigned too.

It shouldn't have been the case in Synapse, at least. We try to strip the field before sending it to clients in all cases: matrix-org/synapse@06df5d4/synapse/events/utils.py#L371-L377

The obvious question here is: how would clients get hold of the invite event anyway? It doesn't really matter if they would see the room state via unsigned.invite_room_state if they can't see the event itself, other than via /sync (which, as noted, exposes the info in a better way.

One thought: is the whole thing a holdover from old-style /events?

The only case where this conditional doesn't pop the entries is for sending to application services, and down /sync (but later on in the /sync code we pop the field anyways).

I think it's worth noting that, up until https://github.com/matrix-org/synapse/pull/6739/files#diff-cd497404c31b1cbaa0bcc442a4c10f1151d151c01e1208e40aede55635e9ab8eL325, there was a comment:

If this is an invite for somebody else, then we don't care about the invite_room_state

(emphasis mine), which adds some background on the intention here.

Still, I'm not sure we can just rip it out - particularly for appservices - without an MSC (as annoying as that is - it looks like a nice cleanup)

[richvdh]

^

[anoadragon453]

Perhaps an MSC to remove invite_room_state and knock_room_state from the CS API then, including a note to move the definitions of those fields to the AS API spec only?

[richvdh]

yes, an MSC sounds good...

[richvdh]

@anoadragon453 is this still on your radar?

[anoadragon453]

@richvdh Not really I'm afraid, it's definitely fallen off.

[richvdh]

Closing for now then, I guess.