-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
Move standard error schema to appendices and share between all APIs #2026
Conversation
46dc871
to
b2d8f2a
Compare
@olivia-fl apologies, this has conflicted with #2016. Could you merge latest main into it? |
This was added in 9198182, but it was already present earlier in the list, with a slightly different description. Signed-off-by: Olivia Lee <[email protected]>
I asked about this on matrix, and tulir confirmed[1] that the error schema was intended to be shared between all APIs. I would like the shared schema to be explicit in the spec text so that ruma can move to using the same error type for client-server and server-server endpoints. This would simplify error handling in server implementations. Error codes that are used in more than one API were moved to the appendices, while error codes specific to only one API were left there. Since the docs on which error codes are actually used by which endpoints aren't very complemete, I determined this by looking at the synapse source code. [1]: https://matrix.to/#/#matrix-spec-discussion:neko.dev/$Y2yTCeR_AeW6g_4jViFbx4gTE_AwF0RN7yrHJ25F5Q8 Signed-off-by: Olivia Lee <[email protected]>
b2d8f2a
to
9bbb9c5
Compare
Rebased on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this. Unfortunately it's one of those things that has years of cruft, and cleaning it up isn't as easy as one might hope. A few comments here.
`M_UNKNOWN` with a 400 Bad Request, the client should assume that the | ||
request being made was invalid. | ||
|
||
#### Common error codes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not jump from h2 to h4, otherwise we get weird section numbers like "1.0.1"
#### Common error codes | |
### Common error codes |
`M_UNKNOWN` | ||
An unknown error has occurred. | ||
|
||
#### Other error codes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#### Other error codes | |
### Other error codes |
not support. Inspect the `room_version` property of the error response | ||
for the room's version. | ||
|
||
#### Rate limiting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#### Rate limiting | |
### Rate limiting |
#### Other error codes | ||
|
||
The following error codes are specific to certain endpoints. | ||
|
||
<!-- TODO: move them to the endpoints that return them --> | ||
|
||
`M_UNAUTHORIZED` | ||
The request was not correctly authorized. Usually due to login failures. | ||
|
||
`M_MISSING_PARAM` | ||
A required parameter was missing from the request. | ||
|
||
`M_INVALID_PARAM` | ||
A parameter that was specified has the wrong value. For example, the | ||
server expected an integer and instead received a string. | ||
|
||
`M_TOO_LARGE` | ||
The request or entity was too large. | ||
|
||
`M_EXCLUSIVE` | ||
The resource being requested is reserved by an application service, or | ||
the application service making the request has not created the resource. | ||
|
||
`M_RESOURCE_LIMIT_EXCEEDED` | ||
The request cannot be completed because the homeserver has reached a | ||
resource limit imposed on it. For example, a homeserver held in a shared | ||
hosting environment may reach a resource limit if it starts using too | ||
much memory or disk space. The error MUST have an `admin_contact` field | ||
to provide the user receiving the error a place to reach out to. | ||
Typically, this error will appear on routes which attempt to modify | ||
state (e.g.: sending messages, account data, etc) and not routes which | ||
only read state (e.g.: [`/client/sync`](/client-server-api/#get_matrixclientv3sync), | ||
[`/client/user/{userId}/account_data/{type}`](/client-server-api/#get_matrixclientv3useruseridaccount_datatype), etc). | ||
|
||
`M_UNSUPPORTED_ROOM_VERSION` | ||
The client's request to create a room used a room version that the | ||
server does not support. | ||
|
||
`M_INCOMPATIBLE_ROOM_VERSION` | ||
The client attempted to join a room that has a version the server does | ||
not support. Inspect the `room_version` property of the error response | ||
for the room's version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this section has any place here. If it is true that they are specific to certain endpoints, then, as the TODO says, they should be documented at those endpoints, rather than as part of the "Standard Error response" list. I'd suggest moving most of them back to the C-S API.
(I'm aware that a couple of them are used in the S-S API, such as M_UNSUPPORTED_ROOM_VERSION
, but let's solve that another time.)
There's a lot of overlap between these codes and the common codes. (eg, what's the difference between M_UNAUTHORIZED
and M_FORBIDDEN
? Or M_INVALID_PARAM
and M_BAD_JSON
?) Again, let's solve it another time, but moving it to the appendices (and thus somehow declaring them valid for all 5 APIs) seems a retrograde step to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can move this back to C-S. I don't think documenting these only on the affected endpoints would necessarily be an improvement, because the definition would then be duplicated in many places (and likely go out of sync over time).
### Standard error response | ||
|
||
All push gateway API endpoints MUST return error responses conforming to the | ||
[standard error response](/appendices#standard-error-response) schema. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unconvinced this is true; Synapse does not require the push gateway to return standard error responses, and I don't think Sygnal does so.
If we want to change this, I think it requires an MSC.
All homeserver -> application service API endpoints MUST return error responses | ||
conforming to the [standard error response](/appendices#standard-error-response) | ||
schema. Similarly, all application service client-server API extension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with the push gateway API, I think we need to think harder about whether it applies to the AS API. Are application services really required to return standard error responses? I'm far from convinced that is something we can just mandate today without an MSC.
Sorry about the miscommunication here. The error schema is definitely meant to be shared and moving that to appendices doesn't require an MSC. It's already shared in practice: there are copies of the schema in the S-S, AS and IS specs. Even the push gateway spec references However, mandating all endpoints return standard errors does require an MSC, because the current AS and push gateway specs don't have such requirements. The other error codes section shouldn't really exist at all, so probably best to leave it alone in the C-S spec until someone gets around to cleaning it up properly. |
The whole synapse pro thing has really killed my motivation to work on most matrix stuff at this point. I'm gonna close this and maybe I'll take another shot at it later if things are looking better. |
(I'd have thought Synapse Pro would drive alternative FOSS homeserver implementions, tbh!) |
I asked about this on matrix, and tulir confirmed that the error schema was intended to be shared between all APIs. I would like the shared schema to be explicit in the spec text so that ruma can move to using the same error type for client-server and server-server endpoints. This would simplify error handling in server implementations.
I'm unsure what the correct thing to do with the error codes that were named in the identity service API but not the client-server API.
M_MISSING_PARAMS
(distinct fromM_MISSING_PARAM
from C2S) is particularly weird. I checked that these are actually used in sydent, and not used in synapse, so I just left them in the identity service API spec.Pull Request Checklist
Preview: https://pr2026--matrix-spec-previews.netlify.app