/createRoom returns spurious alias property

[jcgruenhage]

Description

According to the spec, _matrix/client/r0/createRoom returns only the room_id. When setting an alias while creating the room, synapse also returns that, which is not spec compliant. Relevant spec bit: https://matrix.org/docs/spec/client_server/latest#post-matrix-client-r0-createroom

Steps to reproduce

• create room with alias
• look at response
• find additional property that the spec does not list

Version information

• Homeserver: jcg.re

If not matrix.org:

• Version: Synapse version 1.7.3, but that bit hasn't changed since then (am updating to 1.9.1 atm)

• Install method: docker container

• Platform: n/a

[richvdh]

I don't think it's correct to say that returning additional properties makes for a non-compliant implementation. As a client, you are free to ignore it (indeed you should, since other server implementations will not return it).

[jcgruenhage]

What good is it to return unspecced additional properties (sounds a bit polemic, but I'm genuinely wondering whether I might not be seeing something here)? Best case they are ignored, worst case clients start to depend on them and make the ecosystem divert from spec and stuff starts to break when people use other servers.

Aside of that, depending on the way clients verify/parse json, adding additional properties breaks stuff. Room creation in matrix-nio was broken because of this. Does the spec say that additional properties may be returned?

[Half-Shot]

@jcgruenhage It may be that you can specify a room_alias_name which is just the localpart of an alias. While you can obviously construct this yourself, I can imagine this is why the property is returned?

[jcgruenhage]

@Half-Shot Then it might make sense to add to the spec that this should be returned when an alias is passed for room creation. Clients shouldn't use information returned by a server that is unspecced (for reasons brought forward earlier), so returning it has no use for them unless this became specced (it could be argued that this is just a spec omission after all)

[richvdh]

Does the spec say that additional properties may be returned?

if it doesn't, it should. This is one of the key reasons for using json rather than a rigid binary format: it gives the ability to introduce new fields without breaking existing clients. Any client which breaks when additional properties are returned should be considered buggy.

I'm not saying that synapse should return this property. I am saying that returning it doesn't make it non-compliant.

IIRC on this particular property we went round the loop a while ago and decided it was redundant and should not be added to the spec.

[jcgruenhage]

it gives the ability to introduce new fields without breaking existing clients

makes sense, yeah

Any client which breaks when additional properties are returned should be considered buggy.

@poljar I think that means all "additionalProperties": False lines should be removed from the schema file in nio then

I am saying that returning it doesn't make it non-compliant.

It's at least an unspecced deviation, but I sort of agree now.

I still think that returning unspecced properties is harmful. Having additional properties be a non-breaking change is something else than returning additional stuff that is neither specced nor to-be-specced. Can we get this removed from synapse?

[poljar]

@poljar I think that means all "additionalProperties": False lines should be removed from the schema file in nio then

Sure.

[DMRobertson]

The name of the property seems to be room_alias:

synapse/synapse/handlers/room.py

Lines 1007 to 1008 in 357561c

	if room_alias:
	result["room_alias"] = room_alias.to_string()

The v1.3 spec still only mandates a room_id field in its 200 response, see https://spec.matrix.org/v1.3/client-server-api/#post_matrixclientv3createroom . The openAPI definition doesn't have "additionalProperties": false, which means that additional properties are allowed, c.f. here.

IIRC on this particular property we went round the loop a while ago and decided it was redundant and should not be added to the spec.

The best reference for this I could find was #10321 which was apparently moved from the spec repo.

I suggest we close this issue in favour of that one.

[DMRobertson]

Duplicate of #10321.