feat: unify error responses

[thomasheartman]

This PR implements the first version of a suggested unification (and documentation) of the errors that we return from the API today.

The goal is for this to be the first step towards the error type defined in this internal linear task.

The state of things today

As things stand, we currently have no (or very little) documentation of the errors that are returned from the API. We mention error codes, but never what the errors may contain.

Second, there is no specified format for errors, so what they return is arbitrary, and based on ... Who knows? As a result, we have multiple different errors returned by the API depending on what operation you're trying to do. What's more, with OpenAPI validation in the mix, it's absolutely possible for you to get two completely different error objects for operations to the same endpoint.

Third, the errors we do return are usually pretty vague and don't really provide any real help to the user. "You don't have the right permissions". Great. Well what permissions do I need? And how would I know? "BadDataError". Sick. Why is it bad?

... You get it.

What we want to achieve

The ultimate goal is for error messages to serve both humans and machines. When the user provides bad data, we should tell them what parts of the data are bad and what they can do to fix it. When they don't have the right permissions, we should tell them what permissions they need.

Additionally, it would be nice if we could provide an ID for each error instance, so that you (or an admin) can look through the logs and locate the incident.

What's included in this PR?

This PR does not aim to implement everything above. It's not intended to magically fix everything. Its goal is to implement the necessary breaking changes, so that they can be included in v5. Changing error messages is a slightly grayer area than changing APIs directly, but changing the format is definitely something I'd consider breaking.

So this PR:

• defines a minimal version of the error type defined in the API error definition linear task.
• aims to catch all errors we return today and wrap them in the error type
• updates tests to match the new expectations.

An important point: because we are cutting v5 very soon and because work for this wasn't started until last week, the code here isn't necessarily very polished. But it doesn't need to be. The internals can be as messy as we want, as long as the API surface is stable.

That said, I'm very open to feedback about design and code completeness, etc, but this has intentionally been done quickly.

Please also see my inline comments on the changes for more specific details.

Proposed follow-ups

As mentioned, this is the first step to implementing the error type. The public API error type only exposes id, name, and message. This is barely any more than most of the previous messages, but they are now all using the same format. Any additional properties, such as suggestion, help, documentationLink etc can be added as features without breaking the current format. This is an intentional limitation of this PR.

Regarding additional properties: there are some error responses that must contain extra properties. Some of these are documented in the types of the new error constructor, but not all. This includes path and type properties on 401 errors, errors on validation errors, and more.

Also, because it was put together quickly, I don't yet know exactly how we (as developers) would prefer to use these new error messages within the code, so the internal API (the new type, name, etc), is just a suggestion. This can evolve naturally over time if (based on feedback and experience) without changing the public API.

Error types:

We have ValidationError, BadDataError, and BadRequestError today. Do we really need all of these? I think we all of these are essentially the same thing?

Returning multiple errors

Most of the time when we return errors today, we only return a single error (even if many things are wrong). AJV, the OpenAPI integration we use does have a setting that allows it to return all errors in a request instead of a single one. I suggest we turn that on, but that we do it in a separate PR (because it updates a number of other snapshots).

I've suggested we use the following format as a base to work off of:

type: ErrorDesecription = { description: string }

{
  errors: [ErrorDescription, ...ErrorDescription[]] // a non-empty list of descriptors
}

That gives json that looks a bit like this:

{
  "name": "BadDataError",
  "message": "Something went wrong. Check the `errors` property for more information."
  "errors": [{
    "description": "The .params property must be an object. You provided an array.",
  }]
}

However, we already have used details and message in some places, which would look like this.

{
  "name": "BadDataError",
  "message": "Something went wrong. Check the `errors` property for more information."
  "details": [{
    "message": "The .params property must be an object. You provided an array.",
  }]
}

I propose changing that to errors and descriptions, but am willing to discuss it. I think the term description is a better descriptor of what's contained in each separate item, but I don't have very strong feelings regarding errors vs details. Let me know if you have any thoughts.

Failing tests

As of right now, there are a number of failing tests. I'm working on fixing them, but would like to get this reviewed as soon as possible to agree on the direction.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
unleash-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 25, 2023 1:36pm
unleash-monorepo-frontend	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 25, 2023 1:36pm

[github-actions]

After enabling strictNullChecks this PR would be increasing the number of null check errors from 332 to 333.
Make sure your branch is up-to-date with main and check the diff in the console output to pinpoint the offending files.

[thomasheartman]

@kwasniew Thanks a lot for a very thorough and thoroughly helpful review! 🙏🏼 I think I've addressed all your comments now except for the one about unit testing the error messages for openapi validation errors. I'll have a look at that now.

In the meantime, there's a few comments I haven't resolved because I would like some more input from you before I do. They are:

• Password mismatch error
• additional error parameters
• relaxing checks
• random id creation in constructor

Please resolve them if you think they're OK.

As for your general comment on error object properties:

TBH I preferred our old naming with error having details+messages. Error with errors inside feels weird. But hey, this is naming so it's very personal.

Yeah, I see what you mean. error.errors isn't great. I'm happy to leave it at details (I do like that), but I still prefer description over message. Would that compromise work for you?