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

Move standard error schema to appendices and share between all APIs #2026

Closed
wants to merge 3 commits into from

Conversation

olivia-fl
Copy link

@olivia-fl olivia-fl commented Dec 9, 2024

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 from M_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

Sorry, something went wrong.

@olivia-fl olivia-fl requested a review from a team as a code owner December 9, 2024 08:29
@richvdh
Copy link
Member

richvdh commented Dec 10, 2024

@olivia-fl apologies, this has conflicted with #2016. Could you merge latest main into it?

content/appendices.md Outdated Show resolved Hide resolved

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
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]>
@olivia-fl
Copy link
Author

Rebased on main, but found a duplicated error code introduced in #1944. I've removed that in af6e8a2, before the change to move the error codes to the appendix. I also decided to move the rate limiting section to the appendix, because it is referenced by the common M_LIMIT_EXCEEDED error, and because we have "Rate-Limited: [Yes/No]" on non-C2S endpoints.

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.

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

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"

Suggested change
#### Common error codes
### Common error codes

`M_UNKNOWN`
An unknown error has occurred.

#### Other error codes
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#### Other error codes
### Other error codes

not support. Inspect the `room_version` property of the error response
for the room's version.

#### Rate limiting
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#### Rate limiting
### Rate limiting

Comment on lines +71 to +112
#### 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.
Copy link
Member

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.

Copy link
Author

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).

Comment on lines +42 to +45
### Standard error response

All push gateway API endpoints MUST return error responses conforming to the
[standard error response](/appendices#standard-error-response) schema.
Copy link
Member

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.

Comment on lines +19 to +21
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
Copy link
Member

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.

@tulir
Copy link
Member

tulir commented Jan 16, 2025

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 M_UNRECOGNIZED without actually saying how to return it 🙈

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.

@olivia-fl
Copy link
Author

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.

@olivia-fl olivia-fl closed this Jan 20, 2025
@ara4n
Copy link
Member

ara4n commented Jan 20, 2025

(I'd have thought Synapse Pro would drive alternative FOSS homeserver implementions, tbh!)

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.

None yet

4 participants