search

matrix-org / matrix-spec

The Matrix protocol specification
Apache License 2.0
172 stars 91 forks source link

Spec incorrectly states that `lang` field in response to `/pushers` is required #1643

Open nico-famedly opened 9 months ago

nico-famedly commented 9 months ago

Link to problem area: https://github.com/matrix-org/matrix-spec/blob/99e2ff4927b56db927d52e82ff802b38f6fa8407/data/api/client-server/pusher.yaml#L103

Issue

Synapse seems to not set that field, which seems to cause problems in SDKs generated from the openapi spec:

https://github.com/matrix-org/synapse/blob/3de82bb2af28f56696a79bf41ccffc81385b6e2c/synapse/rest/admin/users.py#L470

https://github.com/matrix-org/synapse/blob/3de82bb2af28f56696a79bf41ccffc81385b6e2c/synapse/handlers/register.py#L1022

https://github.com/famedly/dart_matrix_api_lite/pull/136

richvdh commented 3 months ago

Please do link to the relevant part of the spec when reporting issues - it makes it way easier to understand the context.

This is about https://spec.matrix.org/v1.9/client-server-api/#get_matrixclientv3pushers, I believe.

If synapse has been omitting this since 2015 we can probably just update the spec to match, though it would be good to get evidence of that.

Johennes commented 1 week ago

If synapse has been omitting this since 2015 we can probably just update the spec to match, though it would be good to get evidence of that.

It seems like the lang field on pushers in Synapse was introduced in 2015 and has been nullable from the start. The email pusher that is set up for 3PID registrations with lang=None was added in 2016.

Johennes commented 1 week ago

Tagging on to that, when creating a pusher via the API, Synapse does enforce the presence of lang.

So any pusher created directly by a client will always have the lang field but some of the pushers that Synapse creates autonomously don't.

Johennes commented 1 week ago

Maybe rather than making lang optional in GET /pushers we should try to make lang available in the places where Synapse currently sets None? I haven't actually checked these places but I suppose that would require adding lang to further APIs.