jaydenseric / graphql-api-koa

GraphQL execution and error handling middleware written from scratch for Koa.
https://npm.im/graphql-api-koa
MIT License
52 stars 10 forks source link

Add possibility to reformat error in errorHandler #10

Open vladshcherbin opened 4 years ago

vladshcherbin commented 4 years ago

It would be nice to be able to reformat error in errorHandler. It's useful when you need to modify error before sending to client, for example:

The api might be as simple as:

function formatError(error) {
  // do something with error
}

errorHandler(formatError)

I keep hitting this actually simple cases where I just want to format my errors before sending them to client and current errorHandler is too restrictive in what I can do. However, I'm trying to avoid writing custom middleware instead of errorHandler and struggling to find solutions.

@jaydenseric help 🙏

jaydenseric commented 4 years ago

It depends what you are trying to do. You can either try to throw errors the way you need at the original place the error is thrown, try to customize errors before they get to the errorHandler middleware, or customize the response after all that has happened.

Here is an example of the latter:

// …
async (ctx, next) => {
  await next();
  if (ctx.response.body?.errors)
    ctx.response.body.errors.forEach((error) => {
      // Add an extension…
      if (!error.extensions) error.extensions = {};
      error.extensions.foo = 'foo';

      // Reword messages…
      switch (error.message) {
        case 'Internal Server Error':
          error.message = 'Something is wrong on our end. Sorry!';
      }
    });
},
errorHandler(),
// …

The upside to this is you can let the errorHandler do most of the work, and you can be sure that you are having final say what is going to the client.

The downside is that while iterating the errors, you don't have the original error instances to work with - only what is intended to be exposed to the client.

An idea if you need to customize the errors before they pass through the errorHandler middleware, you could do something like this:

// …
errorHandler(),
async (ctx, next) => {
  try {
    await next();
  } catch (error) {
    // Customize the error…
    // Then throw it again.
    throw error;
  }
},
// …

A few potential gotchas with that:

Regarding your first use case for a format error API:

translate Internal server error message to other language

You could probably get away with the switch example I shared above; but if you needed to internationalise dynamic error message coming from resolvers that were deliberately exposed to the client then another approach would be better. You have to deliberately mark the error for exposure to the client in resolvers anyway, so at that time you could be creating the translated message. No custom Koa middleware needed.

Regarding your second use case:

catch internal db error, expose it with a nice message

Those sorts of errors IMO are best customized at the point of failure. I do things like this in my resolver code:

try {
  response = await fetchTimeout(
    `https://blockchain.com/tobtc?currency=${currencyCode}&value=1`
  );
} catch ({ message }) {
  throw new Error(
    `Failed to fetch Bitcoin price for currency code “${currencyCode}” using Blockchain: ${message}`
  );
}

Note that I'm making a nice message for my API error log, but deliberately these sorts of resolver errors should not be exposed to the client — the errorHandler middleware will mask them with an Internal Server Error message. What goes wrong in your backend is none of their business. In development, you should look at your API process to see errors logging; don't rely on looking at errors on the client side to debug problems with your server.

I could see how maybe a formatError option for the errorHandler middleware would make some edge-case situations easier, it would accept a function:

function formatError(errorOriginal, errorExpose) {
  // Return the object to be JSON stringified in the response body `errors` array.
  // `errorExpose` is what would have been exposed by default.
  return errorExpose
}

Given the above discussion, do you really need it?

vladshcherbin commented 4 years ago

It depends what you are trying to do. You can either try to throw errors the way you need at the original place the error is thrown 1️⃣ , try to customize errors before they get to the errorHandler middleware 2️⃣ , or customize the response after all that has happened 3️⃣ .

Given 3 solutions, numbered, here is what I have:

In my project I tried to use 1️⃣ , used custom errors with expose and extensions inside and it worked for errors which I can control this way.

However, there are other errors, which are thrown not by me and which don't use my custom errors:

⚠️ Errors thrown by database - NotFoundError, UniqueViolationError, etc.

They don't have expose inside so their output is Internal Server Error. I would like to check their instanceof, add expose and nice messages so client can see actual error like Item was not found.

From proposed solutions:

1️⃣ - can't use, don't control them 2️⃣ - can use, in middleware before errorHandler 3️⃣ - can't use, need original error instance

The winner is 2️⃣ , middleware before errorHandler where I can catch all errors and format them the way I need.

⚠️ Default error returned by errorHandler

Default message from source is hardcoded Internal Server Error. I need to translate it to other language and return to client.

From proposed solutions:

1️⃣ - can't use, don't control it 2️⃣ - can't use, it's thrown in errorHandler 3️⃣ - can use, in middleware after errorHandler

The winner here is 3️⃣ , middleware after errorHandler where I can format processed errors the way I need.

So from this 2 cases the result is something like this:

.formatErrorsAfterErrorHandler()
.errorHandler()
.formatErrorsBeforeErrorHandler()

To me it looks unbelievably complicated. A single error middleware should handle all of this - catch errors, format them and return to the client.

Given the above discussion, do you really need it?

All issues I had previously (https://github.com/jaydenseric/graphql-api-koa/issues/4, https://github.com/jaydenseric/graphql-api-koa/issues/8) are actually about formatting errors.

I've tried to move from apollo-server monster to graphql-api-koa at least 2 times already and this error thing is always on my way. It gives me nightmares, I hate it to the point I drop moving attempts. I even wrote and asked about this in Twitter about a year ago.

I used formatError there which gave me error with originalError inside. I checked it with instanceof, formatted type, message, custom fields and returned to client. This was a single place to handle all errors related for graphql. I never had any issues or questions about it.

Here I have to dance with exposing errors using expose, 3 middlewares to format error 🤯 With formatError functionality I won't even need to use expose because I can check error later and decide - should I show it or just return Internal server error.

I don't want to write and use 3 middlewares and try to avoid rewriting errorHandler as I want to rely on using default error handler out of the box.

So the answer is yes, I really need this more than anything in graphql server world 🙏

jaydenseric commented 4 years ago

I'm not following why you can't control your database errors in resolvers - can't you wrap your query code in a try catch? If the queries are happening via abstractions like data loaders, you can still update the abstraction to throw an appropriate error.

vladshcherbin commented 4 years ago

I can use try catch for database error in resolvers but don't do this because I'll need to use this wrapper in every resolver. I can avoid this by just throwing errors in resolvers and handle them later in centralized graphql error handler.

jaydenseric commented 4 years ago

You can make repetitive DB query error handling code DRY with helper functions. The best place to get the error message right is at the location of the error. You will have clean, sensible errors right from the source that way instead of cryptic DB errors flying out of your resolvers. It's good to self contain things too, instead of having the final errors determined elsewhere in your project via configuration.

Part of the graphql-api-koa design philosophy is that it should be as close to zero config as possible; nice and composable functional code FTW.

vladshcherbin commented 4 years ago

Even with this DRY helper functions I will still need to use them in every graphql resolver. 😞

I've tried to follow graphql-api-koa design philosophy by using expose and extension properties. It feels like I'm constantly adding more and more difficulties.

I love and prefer things simplicity and was taught by apollo to handle how errors are finally formatted and returned to client outside resolvers and it felt simple and natural. I didn't need any wrappers, helper functions, extra middlewares, expose properties, etc.

I want this simplicity in graphql-api-koa, be it alternativeErrorHandler or formatError function, just anything. For any other apollo server user it'll also be a blessing to use familiar way of handling things.

🙏

vladshcherbin commented 4 years ago

This also doesn't handle case like above where I need to set custom default error message instead of Internal Server Error.

You may even want to customize error produced by formatError function from graphql which is used inside. I believe it is also currently not possible.

All this cases can be handled by using just a plain function which executes lastly, has access to original error and returns final error which client will see.

vladshcherbin commented 4 years ago

@jaydenseric any update to this?

I kinda need this in my app badly 😢

jaydenseric commented 4 years ago

I thought a fair bit about customising the language of the Internal Server Error message, but I'm not sure this is a reasonable requirement considering that this message purposefully matches the message Koa itself (same as Express) responds with if there is an error. These error messages are not intended to be viewed by public users, and in theory if you put anything risky in resolvers in a try-catch, no one will see the default masked error message. Considering that JavaScript itself uses English keywords for syntax (GraphQL too for that matter), I'm not sure there is a need to internationalize error messages only developers will see.

I love and prefer things simplicity and was taught by apollo to handle how errors are finally formatted and returned to client outside resolvers and it felt simple and natural.

For any other apollo server user it'll also be a blessing to use familiar way of handling things.

I've published several GraphQL server and client related packages for which Apollo has a direct alternative, and almost everytime ex-Apollo users mistake different for "wrong", and push for API changes to be more like what is familiar to them. Similarity to Apollo APIs is not a design goal for my packages. It would be easy to say "yes!" to everyone and bend the API to grow users, but I'm not motivated by popularity. Everything I build is primarily motivated by technical excellence; I use these packages in my own projects.

For example, Apollo users coming to graphql-react get wierded out that there is no central config on the GraphQL client where you set a single GraphQL endpoint URL. But that is a good thing! You can query any API you like in components, something very hard/impossible to do with Apollo. Apollo looks simple at first, but they have multiple kilobytes of buggy complexity to deal with a lot of the problems of their API design. Take normalized cache for example.

Probably the biggest Apollo-ism is their push to get people to define their schema on the backend using GraphQL SDL strings, instead of composing it using the regular GraphQL.js API. Don't get me started on that!

All my APIs use databases, and I have never had a problem with the way error messages currently work.

I can see that there may be edge-cases beyond how I use graphql-api-koa that I'm not very familiar with, that is why I haven't closed this issue. I'm just not sure yet that your use cases are an edge-case, or that they should be worked on as a priority.

jaydenseric commented 4 years ago

I want to elaborate a bit on simplicity, at the risk of sounding argumentative. In every measurable way Apollo Server is not "simpler".

graphql-api-koa only has 2 exports! errorHandler, and execute. Very easy to learn.

Here are all the exports of apollo-server-koa:

[
  'GraphQLUpload',
  'GraphQLExtension',
  'gql',
  'ApolloError',
  'toApolloError',
  'SyntaxError',
  'ValidationError',
  'AuthenticationError',
  'ForbiddenError',
  'UserInputError',
  'defaultPlaygroundOptions',
  'makeExecutableSchema',
  'addCatchUndefinedToSchema',
  'addErrorLoggingToSchema',
  'addResolveFunctionsToSchema',
  'addSchemaLevelResolveFunction',
  'assertResolveFunctionsPresent',
  'attachDirectiveResolvers',
  'attachConnectorsToContext',
  'buildSchemaFromTypeDefinitions',
  'chainResolvers',
  'checkForResolveTypeResolver',
  'concatenateTypeDefs',
  'decorateWithLogger',
  'extendResolversFromInterfaces',
  'extractExtensionDefinitions',
  'forEachField',
  'SchemaError',
  'mockServer',
  'addMockFunctionsToSchema',
  'MockList',
  'makeRemoteExecutableSchema',
  'defaultCreateRemoteResolver',
  'introspectSchema',
  'mergeSchemas',
  'delegateToSchema',
  'defaultMergedResolver',
  'transformSchema',
  'AddArgumentsAsVariables',
  'CheckResultAndHandleErrors',
  'ReplaceFieldWithFragment',
  'AddTypenameToAbstract',
  'FilterToSchema',
  'RenameTypes',
  'FilterTypes',
  'TransformRootFields',
  'RenameRootFields',
  'FilterRootFields',
  'ExpandAbstractTypes',
  'ExtractField',
  'WrapQuery',
  'SchemaDirectiveVisitor',
  'PubSubEngine',
  'PubSub',
  'withFilter',
  'ApolloServer'
]

At least 7 jump out as related to errors alone. APIs that need to be documented, learned and maintained.

vladshcherbin commented 4 years ago

I disagree with you on internalization. An app can be in language different from English and it's quite common to return errors in language of the app, frontend developers can then use this messages to show to end user.

I understand what you write about Apollo, I've been using Apollo packages for a long time (and their Meteor framework before too), know how many bugs are hidden behind their simplicity and personally won't use or suggest any of their packages.

Even though there are only two exports, errorHandler doesn't solve any of my edge cases and thoughts about copy-pasting its code with added functionality, keeping up with source version updates make me so sad.

Apps have different requirements and unfortunately many of them can't be structured and used same way you do. The feature I ask for covers so many edge cases, Apollo is not the only one who has it - I checked a few other packages and all of them have it: koa-graphql, express-graphql, hapi-graphql.

I absolutely love your packages and enjoy using them but here I strongly believe this is not an added unnecessary complexity, it is really useful and needed for so many things.

vladshcherbin commented 2 years ago

A small update:

I've being using graphql-api-koa in ~5 production apps during past year as well as a few hobby projects. What I've being doing in all of this apps is during project start, copy-paste errorHandler code and add formatError handler there 😅

It becomes kinda custom errorHandler which is inside almost 100% identical to graphql-api-koa one. I'm actually very happy with how it's working, exactly the way I'd love to. During all this time I from time to time check this issue for any updates to reduce my copy-paste project boilerplate 😄

@jaydenseric first, huge thank you for the library man, really appreciate the simplicity and absolutely love it ❤️ I've re-read carefully your proposed solutions today and I'm wondering if your opinion on formatError addition has changed during this (almost 2) years?

jaydenseric commented 2 years ago

Thanks for the update; glad to see you've been finding it useful. Your feedback based on solid experience is appreciated :)

I've re-read carefully your proposed solutions today and I'm wondering if your opinion on formatError addition has changed during this (almost 2) years?

I've been working full time for over a year now doing R&D to pivot from Node.js, centralised npm and bloated packages designed around build steps to Deno, decentralised CDN's and standard import maps, buildless, with type safety via TypeScript JSDoc comments.

I had to first figure out the best way to make universal ESM packages with type safety that work for both Node.js and Deno, then had to one-by-one republish all of my dev tools, and then use those updated dev tools to republish all of the raw ingredients needed to work on a from-scratch web application framework for Deno (graphql-react, react-waterfall-render, etc.). After a lot of work, in the last few weeks I was able to publish v1 of the buildless React web application framework for Deno: https://github.com/jaydenseric/ruck (https://ruck.tech).

Now I am updating all of the web apps I maintain to switch from Node.js and Next.js deployed with Vercel to Deno and Ruck, deployed with Fly.io. Once that is done, I can turn my attention back to GraphQL APIs because I want to be able to build them using Deno as well. I haven't actually had the chance to work much in GraphQL API backends in the last 2 years due to all this R&D work, and my last contract before this was working on an app that was powered by an Elixir GraphQL backend so I couldn't work directly on it or use any of my JS GraphQL backend stuff. The GraphQL APIs for my own apps have been pretty reliable and didn't need a lot of maintenance.

I'm not 100% sure what the dream GraphQL API backend in Deno would look like; could it be universal for both Node.js and Deno or will that be too difficult, or compromise ergonomics? It probably doesn't make sense to work with Koa anymore. Hopefully this work won't be blocked by GraphQL.js not providing real ESM or using Node.js API's which is problematic for Deno. I was hoping that by the time I was working on the backend again GraphQL.js would have improved their ESM game.

A year working pro-bono on open source has smashed my savings so I have to take up contract work for money very soon, so it might have to be after that that I can immerse in the Deno GraphQL API problem space.