Define common error structure

[pavish]

Description

Currently, there is no common error structure on the backend.

• Most requests with bad input return 500, while they should return 400.
• The current 400 errors do not follow a common structure. In some observed cases:
  • they return a JSON array of strings
  • they don't return anything
  • they return a JSON object with a property named detail

Expected behaviour

• There needs to be a well defined error structure that is common for all requests.
• Errors with bad input should only return status code 400.

[kgodey]

@pavish Any 500 errors you encounter are bugs, please create individual issues for each 500 error you encounter. Thanks!

[seancolsen]

I bumped into today and would like to help move this issue towards completion. As I see it, we can close this issue once we've agreed upon a standard behavior for API error responses. We would then use subsequent tickets to track the work of transforming specific API endpoints into conformance with that structure.

I'd like to propose the following:

API Error Response Behavior

• The response status code should be 4xx when the error is handled, with 400 being the default.
• The response body should be JSON.
• The JSON should use an object at its root.
• The JSON root object should always have a details property with an error message as a string.
• The error message should be formatted in one or more sentences, ending with a period and having no trailing spaces.
• We should strive to make each error message unique to its situation.
• Untrusted user input is allowed inside error messages. Care should be taken to appropriately escape error messages when printing them in various contexts, as all error messages should be presumed to contain potentially malicious content.
• API error messages should be written primarily for an audience of Mathesar developers. The front end will typically print user-targeted error messages first, followed by the API error message if appropriate.

@kgodey @pavish @mathemancer Can you either approve or suggest changes to the above?

Once we're in agreement, I'll document our decision on the wiki. Having a decision here will help me with some of the front end work I'm doing, and it will allow me to begin reporting issues as I notice APIs which don't meet the standards.

[mathemancer]

I think we should additionally have a mathesar-specific (maybe optional) error code (beyond the HTTP status code). This will help mapping DB-layer problems to more user-friendly error messages without string parsing, and enable custom error message generation logic in the front end if desired.

[kgodey]

I have lots of thoughts and this is in my queue but I might not get to it for a couple of days.

[kgodey]

I put a proposed structure and standards for API error handling in the API standards page based on @seancolsen and @mathemancer's ideas and some of my own.

@dmos62 @mathemancer @pavish @seancolsen @silentninja Please review and leave feedback here, unassign when done (or if you have no feedback). Feel free to also edit the wiki page for clarity if needed.

[seancolsen]

@kgodey I made some small edits for clarity.

Suggested changes:

• I like the idea of providing a way to return multiple errors in one response. I don't really like having the response schema change to meet that goal though. I'd rather always return an array of error objects, even if there is only one error. That would make the schema more consistent and thus more predictable for API consumers. If a consumer has only ever seen error responses with single errors, they might assume that they can rely on the details property always being present on the root, which it won't when there are multiple errors.
• I like the error_code too. Within this API Standards doc, I think it would be good to document the location where we keep track of all error codes, globally.

[silentninja]

I like the spec. Have a few suggestions

1. Error Status code field - I would prefer to extend upon HTTP status codes rather than having our own subset.

API error messages should be written primarily for an audience of Mathesar developers. The front end will typically print user-targeted error messages first, followed by the API error message if appropriate.

At this point, I am not really sure if we really need a custom list as it would make a tight coupling with the frontend, we should rather be relying on the detail field to convey the message to the users.

2. One error per error object - The spec should list out that If a field contains more than one error, it should not be grouped into a single object rather each error should get its own error object.

3. Detail field - detail seems to be an apt name compared to details as each error object should be limited to a single error.

I agree with @seancolsen that the error response should be a list always. I also agree with @pavish that any error specific detail such as field should be inside the meta object

[seancolsen]

@silentninja

I would prefer to extend upon HTTP status codes

I'd prefer to maintain our own set of error codes. From my perspective, one use of the error code is to allow the front end to perform custom actions depending on the specific error. Say a request has two failure modes. In mode A I want to perform a special action, and in mode B I don't. I'd like to rely on the error code for that control flow logic so that changes to the message don't break things. So with that reliance, yes, the front end would become somewhat more tightly coupled with the back end vs a case where we had no error codes -- but I think it will be useful in certain scenarios. That coupling implies the need to maintain strict immutability of error codes in the long-term. Once we've used a code, we want to make sure we don't change it or use it for some other purpose. That immutability means that we'll be expending error codes somewhat regularly as our codebase grows -- like an auto incrementing ID as we add and delete records.

I'm not seeing a clean way to extend upon the HTTP status codes while satisfying the above goal of having somewhat infinite pool of "expendable" error codes. All of these errors should be 4xx error, if we're to adhere to the semantics within the HTTP spec. Many of the codes in the 4xx block already have their own designated meaning. That leaves us with a pretty limited number of codes to assign for our own purpose, and I think we're going to want to assign highly specific meaning to each of these codes.

[silentninja]

I am not opposed to it entirely, just trying to avoid it If possible. Do you have any specific API or scenario that we could use as an example?

[dmos62]

I like the idea of accurate HTTP status codes (4xx for bad input; 5xx for serverside bugs) and then a JSON error_code field to further identify what happened. In the backend this implies some hierarchy of error enumerations to prevent error_code conflicts. The convention would motivate creating less generic errors.

The meta object also makes a lot of sense.

a way to return multiple errors in one response

Are there situations where we can catch multiple errors at once on the backend?

[silentninja]

Are there situations where we can catch multiple errors at once on the backend?

Validation errors can return multiple error objects.

[kgodey]

@silentninja For an example of error codes, imagine that you're updating the type of a column. You might run into a variety of errors:
(1) Type options are unavailable (e.g. you try to pass a precision argument for a BOOLEAN type)
(2) Type is invalid (e.g. you try to change the DB type to PIZZA)
(3) Type options are invalid (e.g. you try to pass a abcd value for a precision option for a NUMERIC type)

These will all return a 400 status code, but having separate error codes for each of these will allow the frontend to distinguish the errors and handle them differently. e.g. (1) can be error code 1001, (2) can be error code 1002, (3) can be error code 1003.

[kgodey]

@dmos62 @mathemancer @pavish @seancolsen @silentninja Thanks for all your feedback. I've updated the spec, you can see the diff in the wiki page's History. Major changes:

• Updated to say that the API should always return a list.
• Updated to mention that we'll create a separate page for what error codes mean and link to it when done.
• Updated error spec to be the following:

{
    "message": "This is an error message.",
    "error_code": "2045",
    "field": "name",
    "details": {"failed_ids": [1, 2, 3, 4]}
}

Reasoning:

• I renamed details to message because it's more accurate.
• I repurposed details for @pavish's suggested meta key because it's for arbitrary extra details related to the error. The name meta didn't make sense to me for a field that's providing extra details.
• I left field as a top level value because I think it's going to be a common enough value that it makes sense for API users to rely on it being there.

---

Please leave feedback on this issue or unassign yourself if it looks good.

[mathemancer]

Most of my concerns have been covered, except that I think field should be an array (perhaps called fields). For example, uniqueness constraints can be multi-column, and you need to consider the whole constraint when reporting that a multicolumn uniqueness constraint has been violated. Suppose we have a constraint on some table indicating that the col1, col2 pair must be unique, and we already have:

 col1 | col2
------+------
  a   |  b

We can insert (c, b) into this table, even though b already exists in col2. Similarly, we can insert (a, c), even though a already exists in col1. However, we cannot insert a, b, since that violates our multi-column uniqueness constraint. We need to report both fields, but it's inappropriate to report them individually, since the individual columns are allowed to have duplicates.

Foreign key constraints can also be multi-column, of course, but it presents less of a problem.

[silentninja]

The reason I prefer to have field inside details is so that we could compose details as an object type based on the error code with each error code getting its own object type as each error will have its own set of requirements

[dmos62]

Seconded that field should be fields. Also, seconded that fields should be on the details subobject: it seems like a detail; especially since field will not make sense for all exceptions.

By the way, why not columns instead of fields? I think our column abstraction is strong enough to be relied upon.

[kgodey]

@mathemancer @silentninja @dmos62 I was thinking about field here as a single key mapping to a key provided in the request body. If an error spans more than a single field, then the field key would be null. field is also not the same as column, since it's not always referring to a table or a view record.

For example, imagine a request to rename and alter the type of a column via a PATCH request to /api/v0/columns/23/ with body:

{
   'name': 'Topping',
   'type': 'FOOD'
}

The error response could be:

[{
    "message": "There is already a column with the same name.",
    "error_code": "1010",
    "field": "name",
    "details": null,
}, {
    "message": "FOOD is not a valid type.",
    "error_code": "2001",
    "field": "type",
    "details": {
      "available_types": ["INTEGER", "DECIMAL", "VARCHAR"]
   }
}

---

Does that make sense and does that change your feedback at all? Perhaps it would be clearer if I renamed field to key? I was going with standard Django/DRF nomenclature but it might not be as clear since we're not using Django fields for a lot of this.

[mathemancer]

Aha, I was completely misunderstanding. So in my example with the multi-column constraint, the columns list would be in the details. If that's correct (i.e., that field will only be used to refer to a key in a request, and the columns for a multicolumn constraint issue would go in the details), then I think this looks good. We should probably add some documentation for that. As for the name itself, I defer to your expertise about whatever is most common for DRF errors, @kgodey .

[dmos62]

I had misunderstood what field meant too. The idea about fields (plural) still makes sense, since you might have constraints on what's a valid request that are based on the values of more than one request body field. At the same time, specifying which request fields are problematic can be awkward, since more of the code might need to be aware of the JSON. I'm a bit concerned that this can be hard to implement. I'd be interested in hearing what the frontend team thinks @seancolsen @pavish.

[seancolsen]

@kgodey's last message makes sense so I'm inclined to go with her approach. If we run into situations on the front end that are difficult we can re-evaluate as-needed. I'm confident that Kriti's spec is going to good enough for quite a while, and I'd like to get going on implementing it.

[kgodey]

The idea behind field being singular is so that we can display errors in forms if needed. Forms are a common way to interact with APIs – each input is generally tied to a single field in the request. Having a single field key makes it easy to parse and display errors next to the corresponding input. If an error is not related to any particular field (or if it's related to multiple fields), then it would be displayed in a more general location for errors.

As @seancolsen mentioned, we can always reevaluate the spec if needed. The most important thing at the moment is to have a consistent error standard.

[kgodey]

I'm closing this issue. I've opened a new one for implementation: #883