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

Describe return codes for account data endpoints + minor clarifications #1155

Merged
merged 15 commits into from
Jul 14, 2022
Merged

Describe return codes for account data endpoints + minor clarifications #1155

merged 15 commits into from
Jul 14, 2022

Conversation

DMRobertson
Copy link
Contributor

@DMRobertson DMRobertson commented Jun 30, 2022

@DMRobertson DMRobertson marked this pull request as ready for review June 30, 2022 22:29
@DMRobertson DMRobertson requested a review from a team as a code owner June 30, 2022 22:29
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally this lgtm, though the errors need to reference the error schema and ideally have examples set for clarity/rendering in the spec.

content/client-server-api/modules/account_data.md Outdated Show resolved Hide resolved
@DMRobertson
Copy link
Contributor Author

generally this lgtm, though the errors need to reference the error schema and ideally have examples set for clarity/rendering in the spec.

This should be done now. Error messages are cribbed from Synapse.

f58ea57 scope-creeps this a little by suggesting that bad room ids should also be rejected with a 400 status code. (Synapse doesn't do this yet, but I'm about to PR a change making that so.) This caused me to notice #1157.

@DMRobertson DMRobertson requested a review from richvdh July 12, 2022 17:28
@@ -71,6 +71,38 @@ paths:
application/json: {}
schema:
type: object
400:
description: |-
The request body is not a JSON object. Errcode: `M_BAD_JSON`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hum. https://spec.matrix.org/v1.3/client-server-api/#common-error-codes says we should use M_NOT_JSON in this case. If synapse is using M_BAD_JSON, I'd say that's a synapse bug. (And yes, those are terrible names for the error codes. Don't blame me.)

(similarly for PUT /user/{userId}/rooms/{roomId}/account_data/{type})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll fixup both.

When the spec speaks of "valid JSON", I guess it really means "a valid JSON object" as opposed to e.g. a list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, now I'm not sure if this change is correct. Synapse hits that code when it's parsed a JSON value that isn't an object (so list, bool, null, number or string). They're arguably all "valid JSON", just not the form of JSON that we were expecting?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh right, yes, I'd expect M_BAD_JSON for a list where we expect an object. Which means that this endpoint can return either M_BAD_JSON or M_NOT_JSON.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See b66b199

@DMRobertson DMRobertson requested a review from richvdh July 13, 2022 18:14
@richvdh richvdh removed their request for review July 14, 2022 10:32
@DMRobertson DMRobertson requested a review from richvdh July 14, 2022 11:42
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@richvdh richvdh merged commit f9028ac into matrix-org:main Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lack of comprehensive status codes/error conditions in account data endpoints
3 participants