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

Ambiguous behaviour when messages field is not provided in calls to PUT /_matrix/client/r0/sendToDevice/{eventType}/{txnId} #2927

Closed
anoadragon453 opened this issue Dec 29, 2020 · 5 comments · Fixed by #2928
Labels
client-server Client-Server API spec-bug Something which is in the spec, but is wrong

Comments

@anoadragon453
Copy link
Member

anoadragon453 commented Dec 29, 2020

Currently the spec defines the messages field in the JSON body of requests to PUT /_matrix/client/r0/sendToDevice/{eventType}/{txnId} as not required. It's not really clear what the homeserver should do when messages is not provided.

Indeed, there isn't anything for the homeserver to do when messages is not provided, and thus not including message is a bit pointless.

I feel that we should mark messages as a required field. It doesn't really make sense to define a default message.

Note that Synapse currently Internal Server Errors when messages is not provided, though this is fixed by matrix-org/synapse#8975.

@anoadragon453 anoadragon453 added client-server Client-Server API wart A point where the protocol is inconsistent or inelegant labels Dec 29, 2020
@turt2live
Copy link
Member

I'm pretty sure it's just a documentation error and the field is supposed to be required.

@turt2live turt2live added spec-bug Something which is in the spec, but is wrong and removed wart A point where the protocol is inconsistent or inelegant labels Dec 29, 2020
@anoadragon453
Copy link
Member Author

Thank you. In that case, have a pr: #2928

@turt2live
Copy link
Member

well, we also need to dredge up the history to figure out if that's actually the case :p

@anoadragon453
Copy link
Member Author

After some dredging I discovered that this bit of spec was originally introduced in #386. That PR is a spec doc implementation of https://docs.google.com/document/d/1v8lxwKi1y074ggYYAjZTHN2EJYBFPtnot2nM3vVWTR0/edit.

From reading both, I believe that it was intended for messages to always be provided. I don't see any discussion or a clear reason as to why it should have been optional.

@turt2live
Copy link
Member

Yea, that seems fair. The approving reviews on the PR also help justify the decision imo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API spec-bug Something which is in the spec, but is wrong
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants