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

[heyaco]

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.

[proehlen]

@acao Since pull request #696 has been reverted in #736, can this issue be re-opened? I could log a new issue but the discussion here is useful history.

[acao]

@proehlen the best place for this discussion would be: https://github.com/graphql/graphql-over-http

this is where the whole spec for HTTP error codes is decided on. if the HTTP spec changes, we can update this reference implementation!

[MatthiasKunnen]

The question I have is how #696 violates the spec?

[acao]

@MatthiasKunnen this was @IvanGoncharov's resolution on slack:

I disagree with https://github.com/graphql/express-graphql/commit/43ba6061388b9a1fc0119dc8e909c7a4391c70e6 We discussed it bunch of times and consensus is that we should return non-2xx code for case where data is null and 2xx for cases where data contains some data https://github.com/graphql/graphql-over-http/blob/master/spec/GraphQLOverHTTP.md#status-codes

[proehlen]

@acao thanks for the link. I don't feel confident enought to raise a new issue there. I've hacked around it using @crazyx13th 's solution in the mean time but if I could just make a couple of observations here before I move on:

In general, I'm not sure http status code is an approriate mechanism for indicating problems with queries that managed to resolve (even if aborted by the api for whatever reason). For one thing, the query can have nested/multiple errors and be partially successful, partially unsucessful. One blanket http status code doesn't really cover the multitude of scenarios.

Also, 500 is obviously not appropriate for many or even most errors, causes problems for some users, and the 500 code itself specifically is not actually prescribed by the spec. Pull request #696 would have allowed me to raise a custom error object with a status code in my api and then set the http response accordingly. It would have then been my responsibility to ensure I was compliant with the spec - ie returning a 4xx or 5xx status as appropriate. Thanks again for your time.

[MatthiasKunnen]

I agree with @proehlen, I also wished to use handleRuntimeQueryErrorFn to add error info in the response headers. It might be true that you could use the function in a way that violates the spec but its existence and multiple usages for it, don't.

[acao]

personally, I think it makes sense too.

we can improve the HTTP transport spec error codes as much as we want, but users will almost always have edge cases or different needs altogether.

following the spec by default is good enough for me and for a reference implementation, and it doesn't add any performance debt or almost any maintenance burden to add this.

@danielrearden what is your take on this? @IvanGoncharov are you willing to revisit this decision?

[derek-miller]

I think it should be revisited and implemented in such a way that it cannot violate the spec if desired. If the change were to supply a function that returns the status code in error situations it could enforce spec compliance vs the custom response function proposed. As shown above, you can always hack around it and violate the spec. The library should do its best to maintain compliance while not blocking the user. Returning 500 on any error is arguably worse than violating a spec imo.

[MatthiasKunnen]

I think it should be revisited and implemented in such a way that it cannot violate the spec if desired.

Forcing spec compliance could be done but I don't see much advantage in that. After all, as you said, you can just hack your way around it.

IMO, the most important thing is that the default setting does not violate the spec. What the user then decides to do with the function is their business.