Should BAD_USER_INPUT raise a 400 error?

[IodizedGabe]

According to the docs, a failure to parse/validate a request should raise a 400.

This should imply that sending a request with required variables and no input should also raise a 400, because the request as a whole is technically invalid:

query ItemById($itemId: ID!) {
    item(itemId: $itemId) {
        name
    }
}

Input: {}

However, when upgrading from apollo-server-core: 3.11 => @apollo/server: 4.5.0, I noticed that this same request started returning 200 instead. Was this an intentional change, or a regression?

[trevor-scheer]

Fair question, there's maybe some nuance to the language here. Assuming you're referring to:

If Apollo Server can't parse the request into a legal GraphQL document and validate it against your schema, it responds with a 400 status code
The parse and validate here refer specifically to the document - does it parse (is it valid graphql) and does it validate against the schema (can we execute it).

Invalid variables actually surface during execution, mostly as an unfortunate detail of graphql-js's variable resolution being baked into execution (described at length here):

apollo-server/packages/server/src/requestPipeline.ts

Lines 447 to 468 in d7e9b97

	// The first thing that execution does is coerce the request's variables
	// to the types declared in the operation, which can lead to errors if
	// they are of the wrong type. It also makes sure that all non-null
	// variables are required and get non-null values. If any of these things
	// lead to errors, we change them into UserInputError so that their code
	// doesn't end up being INTERNAL_SERVER_ERROR, since these are client
	// errors. (But if the error already has a code, perhaps because the
	// original error was thrown from a custom scalar parseValue, we leave it
	// alone. We check that here instead of as part of
	// isBadUserInputGraphQLError since perhaps that function will one day be
	// changed to something we can get directly from graphql-js, but the
	// `code` check is AS-specific.)
	//
	// This is hacky! Hopefully graphql-js will give us a way to separate
	// variable resolution from execution later; see
	// https://github.com/graphql/graphql-js/issues/3169
	const resultErrors = result.errors?.map((e) => {
	if (isBadUserInputGraphQLError(e) && e.extensions?.code == null) {
	return new UserInputError(e);
	}
	return e;
	});

.

This case is falling awkwardly into the last bullet, and I can sympathize with any confusion on this one:

Otherwise, Apollo Server returns a 200 status code. This is essentially the case where the server can execute the GraphQL operation, and execution completes successfully (though this can still include resolver-specific errors)
...we should probably update that to include variable resolution failure to be more clear.

If that status code is important to you, you can configure it with a plugin. Take a look at the examples in the docs here and let me know if that's helpful.

[IodizedGabe]

Thanks for the confirmation.

Should this be raised as an error with graphql-js, then? Or is this a case of ambiguity in the spec?

[jessemyers]

you can configure it with a plugin

While this is true, doing so in our project would result in multiple configuration elements for error handling because we also need to implement a custom formatError function.

My take is that the current behavior is confusing and unnecessarily difficult to customize; would there be willingness to accept contributions that reduce the customization overhead?

[glasser]

I think @jessemyers and @IodizedGabe are right and this was an unintentional v4 regression that I introduced. (Whether the correct answer is "UserInputError should be a 400" or "the case where execute produces no data field (which is roughly the same as where AS proactively creates a UserInputError) should be a 400" is slightly less clear to me.) More at #7462 (comment)

[trevor-scheer]

Sorry for the confusion here, had to hash this one out with @glasser. Opened a PR to provide an opt-in mitigation here: #7465