-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -56,7 +54,9 @@ describe('SchemaLink', () => { | |
}); | ||
observable.subscribe({ | ||
next, | ||
error: error => expect(false), | ||
error: () => { | ||
throw new Error('Received error') | ||
}, | ||
complete: () => { | ||
expect(next).toHaveBeenCalledTimes(1); | ||
done(); | ||
|
@@ -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(); | ||
}, | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 => { | ||
|
@@ -101,7 +104,9 @@ describe('SchemaLink', () => { | |
}); | ||
observable.subscribe( | ||
next, | ||
error => expect(false), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
@@ -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(); | ||
}); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
}); |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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!