Queries returning only errors are forced to be a 500

[derek-miller]

On commit b3ccce9 a query response containing only the errors field is forced to be a 500, however these errors could easily be coercion errors due to incorrect input types, thus a bad request and not a server error. I am wondering if there is a strong opinion to keep it this way or if we could improve this logic and potentially make it configurable? A lot of client code that makes graphQL requests often retry on 500 errors and in the case of coercion errors will never succeed.

[andfk]

+1 having an issue doing proper error handling in client-side as it always returns status 500 if the response must be non null.

[masiamj]

@andfk Can you elaborate on your solution? What version are you using now? How did it change?

Best!

[andfk]

hey @masiamj yeah. I think i may have to edit my comment as is wrong. I initially thought the error was coming from Apollo and it'll be solved by upgrading apollo-link-http anyway the issue still remains. What i ended doing as a temporal solution (i hope) was to remove the ! or new GraphQLNonNull() from the responses so the 500 is not returned if the response is empty when it has errors. For example a user error we throw manually not an unhandled expection or so.

Hope that helps and lets see how this issue goes. I personally like much more the previous approach.

[julkwiec]

I'm facing the same issue. As @derek-miller said, it's caused by these lines in the aforementioned merge request:

        if (response.statusCode === 200 && result && !result.data) {
          response.statusCode = 500;
        }

I'd love to have a possibility to change this behavior.

+1

[berstend]

Hardcoded 5xx errors made me a little sad, as this might confuse certain GraphQL clients.

This PR doesn't seem to be exhaustive or about to be merged and I also didn't feel like maintaining a fork.

I therefore resorted to the next best thing, hijacking the express send handler. 🐴

import { NextFunction, Request, Response } from "express";
import * as hijackResponse from "hijackresponse";

// Extend Express Response with hijack specific function
interface IHijackedResponse extends Response {
  unhijack: () => void;
}

/**
 * Stupid problems sometimes require stupid solutions.
 * Unfortunately `express-graphql` has hardcoded 4xx/5xx http status codes in certain error scenarios.
 * In addition they also finalize the response, so no other middleware shall prevail in their wake.
 *
 * It's best practice to always return 200 in GraphQL APIs and specify the error in the response,
 * as otherwise clients might choke on the response or unnecessarily retry stuff.
 * Also monitoring is improved by only throwing 5xx responses on unexpected server errors.
 *
 * This middleware will hijack the `res.send` method which gives us one last chance to modify
 * the response and normalize the response status codes.
 *
 * The only alternative to this would be to either fork or ditch `express-graphql`. ;-)
 */
export const responseHijack = (_: Request, originalRes: Response, next: NextFunction) => {
  hijackResponse(originalRes, (err: Error, res: IHijackedResponse) => {
    // In case we encounter a "real" non GraphQL server error we keep it untouched and move on.
    if (err) {
      res.unhijack();
      return next(err);
    }

    // We like our status code simple in GraphQL land
    // e.g. Apollo clients will retry on 5xx despite potentially not necessary.
    res.statusCode = 200;
    res.pipe(res);
  });
  // next() must be called explicitly, even when hijacking the response:
  next();
};

Usage:

import { responseHijack } from "./expressMiddleware/responseHijack";
app.use(responseHijack);

Please note: My inline comment is not meant to be snarky or condescending, I appreciate all open source work ❤️

[deerares]

If resolver returns only errors its incorrect set status to 500, it's may be bad request or forbidden etc

[jsonmaur]

Any update on this? Validation errors should definitely not be returning a 500.

[robatwilliams]

The lines before the one that sets 500:

.catch(error => {
        // If an error was caught, report the httpError status, or 500.
        response.statusCode = error.status || 500;
        return { errors: [error] };
      })

So if you add an extension that examines the result for errors, you can throw an error with a status property which will then be used as the response code. It will replace an errors currently in result.errors.

Also note that in an extension, the errors are GraphQL errors and they have an originalError property.

[jsonmaur]

@robatwilliams That kinda works, though the Typescript typings require the extensions function to return an object. Which means all responses will have an empty extensions object now. Also, it doesn't allow you to throw multiple errors if you have them, since that catch statement is assuming only one error.

[robatwilliams]

Implementation is fine with returning non-object, so typings need updating:

if (extensions && typeof extensions === 'object') {
  (result: any).extensions = extensions;
}

Yes, the single thrown error will replace any existing errors as I said. Agree it's not ideal, the approach might work for some.

[seeruk]

I think there should be a hook that we can add, similar to customFormatErrorFn. The main thing I wanted to do was log errors on my server, and it's not possible to do that without either ditching express-graphql or using something like hijackresponse. Not ideal workarounds, and it makes it less than ideal to use express-graphql in production.

[Aligertor]

+1

[heyaco]

Still waiting for a fix on this..