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

Fix schema link to return errors for unknown queries #7094

Merged
merged 1 commit into from
Oct 1, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
59 changes: 41 additions & 18 deletions src/link/schema/__tests__/schemaLink.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@ type Query {
const schema = makeExecutableSchema({ typeDefs });

describe('SchemaLink', () => {
const mockError = { throws: new TypeError('mock me') };

it('raises warning if called with concat', () => {
const link = new SchemaLink({ schema });
const _warn = console.warn;
Expand Down Expand Up @@ -56,7 +54,9 @@ describe('SchemaLink', () => {
});
observable.subscribe({
next,
error: error => expect(false),
Copy link
Contributor Author

@amannn amannn Sep 29, 2020

Choose a reason for hiding this comment

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

expect(false) doesn't fail the test. I've replaced this with throwing an error that would really fail the test now.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for noticing this and for going out of your way to improve it!

error: () => {
throw new Error('Received error')
},
complete: () => {
expect(next).toHaveBeenCalledTimes(1);
done();
Expand All @@ -65,23 +65,26 @@ describe('SchemaLink', () => {
});

it('calls error when fetch fails', done => {
const badSchema = makeExecutableSchema({ typeDefs });

const link = new SchemaLink({ schema: badSchema });
const schema = makeExecutableSchema({
typeDefs,
resolvers: {
Query: {
sampleQuery() {
throw new Error('Unauthorized');
}
}
}
});
const link = new SchemaLink({ schema });
const observable = execute(link, {
query: sampleQuery,
});
observable.subscribe(
result => expect(false),
error => {
expect(error).toEqual(mockError.throws);
done();
},
() => {
expect(false);
done();
},
);
Copy link
Contributor Author

@amannn amannn Sep 29, 2020

Choose a reason for hiding this comment

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

Also this test didn't really assert for anything. The error handler was never called.

observable.subscribe(result => {
expect(result.errors).toBeTruthy()
expect(result.errors!.length).toBe(1)
expect(result.errors![0].message).toMatch(/Unauthorized/)
done();
});
});

it('supports query which is executed synchronously', done => {
Expand All @@ -101,7 +104,9 @@ describe('SchemaLink', () => {
});
observable.subscribe(
next,
error => expect(false),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here as mentioned above.

() => {
throw new Error('Received error')
},
() => {
expect(next).toHaveBeenCalledTimes(1);
done();
Expand Down Expand Up @@ -192,4 +197,22 @@ describe('SchemaLink', () => {
},
);
});

it('reports errors for unknown queries', done => {
const schema = makeExecutableSchema({typeDefs})
const link = new SchemaLink({ schema });
const observable = execute(link, {
query: gql`
query {
unknown
}
`
});
observable.subscribe(result => {
expect(result.errors).toBeTruthy()
expect(result.errors!.length).toBe(1)
expect(result.errors![0].message).toMatch(/Cannot query field "unknown"/)
done();
});
});
Copy link
Contributor Author

@amannn amannn Sep 29, 2020

Choose a reason for hiding this comment

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

This is the new test I've added that now passes.

Previously Apollo would return with an empty result object and loading status 7 – "ready". This can lead to unexpected states in apps that use the schema link.

});
24 changes: 16 additions & 8 deletions src/link/schema/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { execute } from 'graphql/execution/execute';
import { validate } from 'graphql/validation/validate';
import { GraphQLSchema } from 'graphql/type/schema';

import { ApolloLink, Operation, FetchResult } from '../core';
Expand Down Expand Up @@ -48,14 +49,21 @@ export class SchemaLink extends ApolloLink {
? this.context(operation)
: this.context
)
).then(context => execute(
this.schema,
operation.query,
this.rootValue,
context,
operation.variables,
operation.operationName,
)).then(data => {
).then(context => {
const validationErrors = validate(this.schema, operation.query);
if (validationErrors.length > 0) {
return { errors: validationErrors };
}

return execute(
this.schema,
operation.query,
this.rootValue,
context,
operation.variables,
operation.operationName,
)
benjamn marked this conversation as resolved.
Show resolved Hide resolved
}).then(data => {
if (!observer.closed) {
observer.next(data);
observer.complete();
Expand Down