Express-graphql: 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.

[ghost]

+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

[starkana]

Still waiting for a fix on this..

[coockoo]

I've made a simple rewriter for my app using on-headers package.

const onHeaders = require('on-headers');

function graphqlStatusCodeRewriter(req, res, next) {
  const handleHeaders = () => {
    res.statusCode = res.statusCode === 500 ? 400 : res.statusCode;
  };
  onHeaders(res, handleHeaders);
  next();
};

// Include before your express-graphql middleware
app.use('/graphql', graphqlStatusCodeRewriter);
app.use('/graphql', graphqlHTTP({ schema }));

When it encounters a 500 error it replaces it with 400.

[seeruk]

@coockoo I guess that might work if all of your data is static and in-memory, but if there's anything where an actual internal error can occur, you'll just be signalling to your consumers that it's their fault, not a (hopefully temporary) issue on your end. This would be pretty confusing.

I think you need the actual error to be able to handle it properly, like the hijack response approach.

[coockoo]

@seeruk As for now, I can see that hijackresponse calls the callback only in one case with the null as an error, so it doesn't really solve this problem, as if (err) always returns false.

And of course, all of these solutions are temporary and must be replaced with possibility to customize status codes by express-graphql itself.

[timscott]

Any guidance here? I would be happy to make a PR to either:

1. Look for statusCode on the error (which can be added in formatError)
2. Hard code resolver errors as 200, and let extensions tell the client what kind of error(s) happened

Option #2 is how the other graphql servers I've worked with do it. Either option is preferable to hard coded 500.

[DethAriel]

completely agreeing with @berstend on this:

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.

While I understand that express-graphql might choose to follow a different paradigm, I am a strong believer that supporting industry best practices is beneficial to the ecosystem.

It seems like there has been no real progress on this issue. Given the ~5.3k ⭐️ marks on this project, I (hopefully with a bunch of other people as well) would like to understand if this issue is up for a fix consideration, or whether alternative solutions should be sought.

And, while I'm here - thx for creating and maintaining this!! OSS can be a true PITA, and I appreciate every damn minute you folks are putting into this. Keep up the good work, and LMK if help would be appreciated with this one

[crazyx13th]

my typescript solution / workaround:

app.post('/',
    jwtAuth,
    graphqlHTTPOptions200,
    graphqlHTTPError200,
    graphqlHTTP({
        schema: makeExecutableSchema({typeDefs: [DIRECTIVES, SCHEMEA], resolvers: schemaResolvers}),
        graphiql: false,
    }))

function graphqlHTTPError200(request: Request, response: Response, next: NextFunction): void
{
    const defaultWrite = response.write
    const defaultEnd = response.end
    const defaultWriteHead = response.writeHead
    const chunks: any[] = []
    let isGqlError: boolean = false

    response.write = (...chunk: any): any =>
    {
        chunks.push(Buffer.from(chunk[0]))
        defaultWrite.apply(response, chunk)
    }

    response.end = (...chunk: any) =>
    {
        if (chunk[0]) chunks.push(Buffer.from(chunk[0]))
        isGqlError = !!Buffer.concat(chunks).toString('utf8').match(/"errors":\[/)
        defaultEnd.apply(response, chunk)
    }

    response.writeHead = (statusCode: number) =>
    {
        return defaultWriteHead.apply(response, isGqlError ? [200] : [statusCode])
    }

    next()
}

[MatthiasKunnen]

I have opened a PR, #696, to address this issue. Any feedback is welcome.