Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apollo-server-core: use UserInputError for variable coercion errors #5091

Merged
merged 4 commits into from
Apr 8, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ The version headers in this history reflect the versions of Apollo Server itself
- `apollo-server-cache-redis`: New `BaseRedisCache` class which takes an `ioredis`-compatible Redis client as an argument. The existing classes `RedisCache` and `RedisClusterCache` (which pass their arguments to `ioredis` constructors) are now implemented in terms of this class. This allows you to use any of the `ioredis` constructor forms rather than just the ones recognized by our classes. This also fixes a long-standing bug where the Redis cache implementations returned a number from `delete()`; it now returns a number, matching what the `KeyValueCache` interface and the TypeScript types expect. [PR #5034](https://github.com/apollographql/apollo-server/pull/5034) [PR #5088](https://github.com/apollographql/apollo-server/pull/5088) [Issue #4870](https://github.com/apollographql/apollo-server/issues/4870) [Issue #5006](https://github.com/apollographql/apollo-server/issues/5006)
- `apollo-server-core`: Fix type for `formatResponse` function. It never is called with a `null` argument, and is allowed to return `null`. [Issue #5009](https://github.com/apollographql/apollo-server/issues/5009) [PR #5089](https://github.com/apollographql/apollo-server/pull/5089)
- `apollo-server-lambda`: Fix regression in v2.21.2 where thrown errors were replaced by throwing the JS Error class itself. [PR #5085](https://github.com/apollographql/apollo-server/pull/5085)
- `apollo-server-core`: If a client sends a variable of the wrong type, this is now reported as an error with an `extensions.code` of `BAD_USER_INPUT` rather than `INTERNAL_SERVER_ERROR`. [PR #5091](https://github.com/apollographql/apollo-server/pull/5091) [Issue #3498](https://github.com/apollographql/apollo-server/issues/3498)

## v2.22.2

Expand Down
29 changes: 25 additions & 4 deletions packages/apollo-server-core/src/requestPipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
PersistedQueryNotSupportedError,
PersistedQueryNotFoundError,
formatApolloErrors,
UserInputError,
} from 'apollo-server-errors';
import {
GraphQLRequest,
Expand Down Expand Up @@ -451,16 +452,36 @@ export async function processGraphQLRequest<TContext>(
requestContext as GraphQLRequestContextExecutionDidStart<TContext>,
);

if (result.errors) {
await didEncounterErrors(result.errors);
// 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. We change any such errors into
// UserInputError so that their code doesn't end up being
// INTERNAL_SERVER_ERROR, since these are client errors.
const resultErrors = result.errors?.map((e) => {
if (
e.nodes?.length === 1 &&
e.nodes[0].kind === 'VariableDefinition' &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use graphql-js's Kind.VARIABLE_DEFINITION for this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, although TS actually does require the string constant here to be one of the possible valid strings (ie, typos won't compile).

e.message.startsWith(
`Variable "$${e.nodes[0].variable.name.value}" got invalid value `,
)
) {
return fromGraphQLError(e, {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why codecov thinks this is uncovered. There's a test!

errorClass: UserInputError,
});
}
return e;
});

if (resultErrors) {
await didEncounterErrors(resultErrors);
}

response = {
...result,
errors: result.errors ? formatErrors(result.errors) : undefined,
errors: resultErrors ? formatErrors(resultErrors) : undefined,
};

executionDispatcher.reverseInvokeHookSync("executionDidEnd");
executionDispatcher.reverseInvokeHookSync('executionDidEnd');
} catch (executionError) {
executionDispatcher.reverseInvokeHookSync("executionDidEnd", executionError);
return await sendErrorResponse(executionError);
Expand Down
2 changes: 1 addition & 1 deletion packages/apollo-server-errors/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ export function toApolloError(

export interface ErrorOptions {
code?: string;
errorClass?: typeof ApolloError;
errorClass?: new (message: string) => ApolloError;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine, maybe a comment here that explains, though I also think it's pretty self-explanatory if you look at it for a second.

FWIW, I tried to improve the type here a bit with no luck - I tried to make this "condition" into an interface that UserInputError could implement but TS wasn't having it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment. FWIW I find this type to be easier to understand than typeof ApolloError anyway. I think the lesson for the future is to just take functions rather than class names/constructors though... easy enough to pass (s) => new MyClass(s)...

}

export function fromGraphQLError(error: GraphQLError, options?: ErrorOptions) {
Expand Down
21 changes: 21 additions & 0 deletions packages/apollo-server-integration-testsuite/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,27 @@ export function testApolloServer<AS extends ApolloServerBase>(
});
});

it('variable coercion errors', async () => {
const { url: uri } = await createApolloServer({
typeDefs: gql`
type Query {
hello(x: String): String
}
`,
});

const apolloFetch = createApolloFetch({ uri });

const result = await apolloFetch({
query: `query ($x:String) {hello(x:$x)}`,
variables: { x: 2 },
});
expect(result.data).toBeUndefined();
expect(result.errors).toBeDefined();
expect(result.errors[0].message).toMatch(/got invalid value 2; Expected type String/);
expect(result.errors[0].extensions.code).toBe('BAD_USER_INPUT');
});

describe('schema creation', () => {
it('accepts typeDefs and resolvers', async () => {
const typeDefs = gql`
Expand Down