Make each error class have to define its own status code. This makes
it easier for developers to see which code an error corresponds to and
means less jumping back and forth between files. In other words:
improved locality.
Unfortunately, the long switch needs to stay in case we get errors
thrown that aren't of the Unleash Error type, but we can move it to
the `fromLegacyError` file instead.
Tradeoff analysis by @kwasniew:
+ I like the locality of error to code reasoning
- now HTTP leaks to the non-HTTP code that throws those errors e.g. application services
If we had other delivery mechanisms other than HTTP then it wouldn't make sense to couple error codes to one protocol (HTTP). But since we're mostly doing web it may not be a problem.
@thomasheartman's response:
This is a good point and something I hadn't considered. The same data was always available on those errors (by using the same property), I've just made the declaration local to each error instead of something that the parent class handles. The idea was to make it easier to create new error classes with their corresponding error codes. Because the errors are intended to be API errors (or at least, I've always considered them to be that), I think that makes sense.
Taking your comment into consideration, I still think it's the right thing to do, but I'm not bullish about it. We could always walk it back later if we find that it's not appropriate. The old code is still available and we could easily enough roll back this change if we find that we want to decouple it later.
In some of the places we used `NoAccessError` for permissions, other
places we used it for a more generic 403 error with a different
message. This refactoring splits the error type into two distinct
types instead to make the error messages more consistent.
This PR aims to handle the increased log alarm volume seen by the SREs.
It appears that we get a large number of alarms because a client
disconnects early from the front-end API. These errors are then
converted into 500s because of missing handling. These errors appear to
be caused by the `http-errors` library in a dependency.
We also introduced a log line whenever we see errors now a while back,
and I don't think we need this logging (I was also the one who
introduced it).
The changes in this PR are specifically:
- When converting from arbitrary errors, give `BadRequestError` a 400
status code, not a 500.
- Add a dependency on `http-errors` (which is already a transitive
dependency because of the body parser) and use that to check whether an
error is an http-error.
- If an error is an http error, then propagate it to the user with the
status code and message.
- Remove warning logs when an error occurs. This was introduced to make
it easier to correlate an API error and the logs, but the system hasn't
been set up for that (yet?), so it's just noise now.
- When logging errors as errors, only do that if their status code would
be 500.
This PR improves our handling of internal Joi errors, to make them more
sensible to the end user. It does that by providing a better description
of the errors and by telling the user what they value they provided was.
Previous conversion:
```json
{
"id": "705a8dc0-1198-4894-9015-f1e5b9992b48",
"name": "BadDataError",
"message": "\"value\" must contain at least 1 items",
"details": [
{
"message": "\"value\" must contain at least 1 items",
"description": "\"value\" must contain at least 1 items"
}
]
}
```
New conversion:
```json
{
"id": "87fb4715-cbdd-48bb-b4d7-d354e7d97380",
"name": "BadDataError",
"message": "Request validation failed: your request body contains invalid data. Refer to the `details` list for more information.",
"details": [
{
"description": "\"value\" must contain at least 1 items. You provided [].",
"message": "\"value\" must contain at least 1 items. You provided []."
}
]
}
```
## Restructuring
This PR moves some code out of `unleash-error.ts` and into a new file.
The purpose of this is twofold:
1. avoid circular dependencies (we need imports from both UnleashError
and BadDataError)
2. carve out a clearer separation of concerns, keeping `unleash-error` a
little more focused.
This PR attempts to improve the error handling introduced in #3607.
## About the changes
## **tl;dr:**
- Make `UnleashError` constructor protected
- Make all custom errors inherit from `UnleashError`.
- Add tests to ensure that all special error cases include their
relevant data
- Remove `PasswordMismatchError` and `BadRequestError`. These don't
exist.
- Add a few new error types: `ContentTypeError`, `NotImplementedError`,
`UnauthorizedError`
- Remove the `...rest` parameter from error constructor
- Add an unexported `GenericUnleashError` class
- Move OpenAPI conversion function to `BadDataError` clas
- Remove explicit `Error.captureStackTrace`. This is done automatically.
- Extract `getPropFromString` function and add tests
### **In a more verbose fashion**
The main thing is that all our internal errors now inherit
from`UnleashError`. This allows us to simplify the `UnleashError`
constructor and error handling in general while still giving us the
extra benefits we added to that class. However, it _does_ also mean that
I've had to update **all** existing error classes.
The constructor for `UnleashError` is now protected and all places that
called that constructor directly have been updated. Because the base
error isn't available anymore, I've added three new errors to cover use
cases that we didn't already have covered: `NotImplementedError`,
`UnauthorizedError`, `ContentTypeError`. This is to stay consistent in
how we report errors to the user.
There is also an internal class, `GenericUnleashError` that inherits
from the base error. This class is only used in conversions for cases
where we don't know what the error is. It is not exported.
In making all the errors inherit, I've also removed the `...rest`
parameter from the `UnleashError` constructor. We don't need this
anymore.
Following on from the fixes with missing properties in #3638, I have
added tests for all errors that contain extra data.
Some of the error names that were originally used when creating the list
don't exist in the backend. `BadRequestError` and
`PasswordMismatchError` have been removed.
The `BadDataError` class now contains the conversion code for OpenAPI
validation errors. In doing so, I extracted and tested the
`getPropFromString` function.
### Main files
Due to the nature of the changes, there's a lot of files to look at. So
to make it easier to know where to turn your attention:
The changes in `api-error.ts` contain the main changes: protected
constructor, removal of OpenAPI conversion (moved into `BadDataError`.
`api-error.test.ts` contains tests to make sure that errors work as
expected.
Aside from `get-prop-from-string.ts` and the tests, everything else is
just the required updates to go through with the changes.
## Discussion points
I've gone for inheritance of the Error type over composition. This is in
large part because throwing actual Error instances instead of just
objects is preferable (because they collect stack traces, for instance).
However, it's quite possible that we could solve the same thing in a
more elegant fashion using composition.
## For later / suggestions for further improvements
The `api-error` files still contain a lot of code. I think it might be
beneficial to break each Error into a separate folder that includes the
error, its tests, and its schema (if required). It would help decouple
it a bit.
We don't currently expose the schema anywhere, so it's not available in
the openapi spec. We should look at exposing it too.
Finally, it would be good to go through each individual error message
and update each one to be as helpful as possible.
