Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
nico-famedly opened this issue Sep 21, 2023 · 4 comments
Labels
A-Client-Server Issues affecting the CS API A-Push clarification An area where the expected behaviour is understood, but the spec could do with being more explicit

Comments

@nico-famedly
Copy link
Contributor

Link to problem area:

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

famedly/dart_matrix_api_lite#136

@nico-famedly nico-famedly added the clarification An area where the expected behaviour is understood, but the spec could do with being more explicit label Sep 21, 2023
@richvdh richvdh added the A-Push label Apr 9, 2024
@richvdh richvdh changed the title lang field for pushers should probably not be required Spec incorrectly states that lang field for pushers is required Apr 9, 2024
@richvdh richvdh changed the title Spec incorrectly states that lang field for pushers is required Spec incorrectly states that lang field in response to /pushers is required Apr 9, 2024
@richvdh richvdh added the A-Client-Server Issues affecting the CS API label Apr 9, 2024
@richvdh
Copy link
Member

richvdh commented Apr 9, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Client-Server Issues affecting the CS API A-Push clarification An area where the expected behaviour is understood, but the spec could do with being more explicit
Projects
None yet
Development

No branches or pull requests

3 participants