-
Notifications
You must be signed in to change notification settings - Fork 564
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
Integrate graphql-http, refactor and comply with spec (v3) #1589
Changes from 23 commits
dcef943
f3d4f14
9f2b4bc
969f2be
2b8e6cb
f9da15c
71881e2
9202f7d
06ec6c2
06f4ddb
c14302c
7f202b8
aab65d2
62e817a
48af902
5e07f27
f0220ca
4ff16f9
84aadff
64d589c
03b9b21
76e770b
39fbe6f
e064869
96841e6
ac26e6a
d909bab
352c572
1e9a658
d968a31
19cc9a8
7e371db
1dae527
5e85f85
547c356
2a9c48c
db3aca3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -166,7 +166,11 @@ describe('accept header', () => { | |
}, | ||
}, | ||
) | ||
expect(response.status).toEqual(406) | ||
expect(response.status).toEqual(200) | ||
const body = await response.json() | ||
expect(body.errors).toEqual([ | ||
{ message: 'Subscriptions are not supported' }, | ||
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. Message is not clear for me. It is not supported because 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 as above, when accepting 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 with above, we don't accept 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. #1589 (comment). I know that as per the HTTP spec 406 is obvious, but I am trying to follow the GQL over HTTP spec... I can improve the error message, it is indeed vague. |
||
]) | ||
}) | ||
|
||
it('server rejects request for AsyncIterable source (defer/stream) when client only accepts application/json', async () => { | ||
|
@@ -202,6 +206,10 @@ describe('accept header', () => { | |
accept: 'application/json', | ||
}, | ||
}) | ||
expect(response.status).toEqual(406) | ||
expect(response.status).toEqual(200) | ||
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. 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 as above, when accepting |
||
const body = await response.json() | ||
expect(body.errors).toEqual([ | ||
{ message: 'Subscriptions are not supported' }, | ||
]) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -145,12 +145,12 @@ describe('error masking', () => { | |
} | ||
}) | ||
|
||
it('non GraphQLError raised in onRequestParse is masked with the correct status code 500', async () => { | ||
it('non GraphQLError raised in onPrepare is masked with the correct status code 500', async () => { | ||
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. onPrepare looks too generic. onRequestParse is getting triggered after 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.
Name seemed like a right fit IMHO; but debatable, I agree. |
||
const yoga = createYoga({ | ||
schema: createTestSchema(), | ||
plugins: [ | ||
{ | ||
onRequestParse() { | ||
onPrepare() { | ||
throw new Error('Some random error!') | ||
}, | ||
}, | ||
|
@@ -160,6 +160,9 @@ describe('error masking', () => { | |
|
||
const response = await yoga.fetch('http://yoga/graphql', { | ||
method: 'POST', | ||
headers: { | ||
'content-type': 'application/json', | ||
}, | ||
body: JSON.stringify({ query: '{ hi hello }' }), | ||
}) | ||
|
||
|
@@ -186,15 +189,14 @@ describe('error masking', () => { | |
}) | ||
const body = JSON.parse(await response.text()) | ||
expect(body).toMatchInlineSnapshot(` | ||
Object { | ||
"data": null, | ||
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. I think we should always have 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. 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 I found this one, which I think we should cover as well. |
||
"errors": Array [ | ||
Object { | ||
"message": "I like turtles", | ||
}, | ||
], | ||
} | ||
`) | ||
Object { | ||
"errors": Array [ | ||
Object { | ||
"message": "I like turtles", | ||
}, | ||
], | ||
} | ||
`) | ||
}) | ||
|
||
it('error thrown within context factory is masked', async () => { | ||
|
@@ -213,15 +215,14 @@ describe('error masking', () => { | |
}) | ||
const body = JSON.parse(await response.text()) | ||
expect(body).toMatchInlineSnapshot(` | ||
Object { | ||
"data": null, | ||
"errors": Array [ | ||
Object { | ||
"message": "Unexpected error.", | ||
}, | ||
], | ||
} | ||
`) | ||
Object { | ||
"errors": Array [ | ||
Object { | ||
"message": "Unexpected error.", | ||
}, | ||
], | ||
} | ||
`) | ||
}) | ||
|
||
it('GraphQLError thrown within context factory with error masking is not masked', async () => { | ||
|
@@ -240,15 +241,14 @@ describe('error masking', () => { | |
}) | ||
const body = JSON.parse(await response.text()) | ||
expect(body).toMatchInlineSnapshot(` | ||
Object { | ||
"data": null, | ||
"errors": Array [ | ||
Object { | ||
"message": "I like turtles", | ||
}, | ||
], | ||
} | ||
`) | ||
Object { | ||
"errors": Array [ | ||
Object { | ||
"message": "I like turtles", | ||
}, | ||
], | ||
} | ||
`) | ||
}) | ||
|
||
it('GraphQLError thrown within context factory has error extensions exposed on the response', async () => { | ||
|
@@ -277,7 +277,6 @@ describe('error masking', () => { | |
}) | ||
const body = JSON.parse(await response.text()) | ||
expect(body).toStrictEqual({ | ||
data: null, | ||
errors: [ | ||
{ | ||
message: 'I like turtles', | ||
|
@@ -287,7 +286,7 @@ describe('error masking', () => { | |
}, | ||
], | ||
}) | ||
expect(response.status).toEqual(200) | ||
expect(response.status).toEqual(400) | ||
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. Why 400? 4xx(Bad Request) should be thrown by the client's mistake, no? 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. I made a change to default to 200/400 when throwing instances of |
||
}) | ||
|
||
it('parse error is not masked', async () => { | ||
|
@@ -314,7 +313,6 @@ describe('error masking', () => { | |
const body = JSON.parse(await response.text()) | ||
|
||
expect(body).toEqual({ | ||
data: null, | ||
errors: [ | ||
{ | ||
locations: [ | ||
|
@@ -353,7 +351,6 @@ describe('error masking', () => { | |
const body = JSON.parse(await response.text()) | ||
|
||
expect(body).toEqual({ | ||
data: null, | ||
errors: [ | ||
{ | ||
locations: [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,32 +1,25 @@ | ||
import { GraphQLError } from 'graphql' | ||
import { createSchema, createYoga } from 'graphql-yoga' | ||
import { createYoga } from 'graphql-yoga' | ||
|
||
describe('GraphQLError.extensions.http', () => { | ||
it('sets correct status code and headers for thrown GraphQLError in a resolver', async () => { | ||
it('sets correct status code and headers for thrown GraphQLError in plugins', async () => { | ||
const yoga = createYoga({ | ||
schema: createSchema({ | ||
typeDefs: /* GraphQL */ ` | ||
type Query { | ||
a: String | ||
} | ||
`, | ||
resolvers: { | ||
Query: { | ||
a() { | ||
throw new GraphQLError('A', { | ||
extensions: { | ||
http: { | ||
status: 401, | ||
headers: { | ||
'www-authenticate': 'Bearer', | ||
}, | ||
plugins: [ | ||
{ | ||
onPrepare() { | ||
throw new GraphQLError('A', { | ||
extensions: { | ||
http: { | ||
status: 401, | ||
headers: { | ||
'www-authenticate': 'Bearer', | ||
}, | ||
}, | ||
}) | ||
}, | ||
}, | ||
}) | ||
}, | ||
}, | ||
}), | ||
], | ||
logging: false, | ||
}) | ||
|
||
|
@@ -39,120 +32,4 @@ describe('GraphQLError.extensions.http', () => { | |
expect(response.status).toBe(401) | ||
expect(response.headers.get('www-authenticate')).toBe('Bearer') | ||
}) | ||
|
||
it('picks the highest status code and headers for GraphQLErrors thrown within multiple resolvers', async () => { | ||
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. We have this because we want to give the user to control the response parameters. Also we benefit from this in our plugins in the execution level. 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. Throwing errors from field resolvers will nullify only that field, for example: query { a } and resolver for { data: { a: null }, errors: [{ message: 'Woah!' }] } As per the spec, if the |
||
const yoga = createYoga({ | ||
schema: { | ||
typeDefs: /* GraphQL */ ` | ||
type Query { | ||
a: String | ||
b: String | ||
} | ||
`, | ||
resolvers: { | ||
Query: { | ||
a: () => { | ||
throw new GraphQLError('A', { | ||
extensions: { | ||
http: { | ||
status: 401, | ||
}, | ||
}, | ||
}) | ||
}, | ||
b: () => { | ||
throw new GraphQLError('B', { | ||
extensions: { | ||
http: { | ||
status: 503, | ||
}, | ||
}, | ||
}) | ||
}, | ||
}, | ||
}, | ||
}, | ||
}) | ||
|
||
const response = await yoga.fetch('http://yoga/graphql', { | ||
method: 'POST', | ||
headers: { 'content-type': 'application/json' }, | ||
body: JSON.stringify({ query: '{ a b }' }), | ||
}) | ||
|
||
expect(response.status).toBe(503) | ||
|
||
const body = JSON.parse(await response.text()) | ||
expect(body).toMatchObject({ | ||
data: { | ||
a: null, | ||
b: null, | ||
}, | ||
errors: [ | ||
{ | ||
message: 'A', | ||
path: ['a'], | ||
}, | ||
{ | ||
message: 'B', | ||
path: ['b'], | ||
}, | ||
], | ||
}) | ||
}) | ||
|
||
it('picks the header of the last GraphQLError when multiple errors are thrown within multiple resolvers', async () => { | ||
const yoga = createYoga({ | ||
schema: { | ||
typeDefs: /* GraphQL */ ` | ||
type Query { | ||
a: String | ||
b: String | ||
} | ||
`, | ||
resolvers: { | ||
Query: { | ||
a: () => { | ||
throw new GraphQLError('', { | ||
extensions: { | ||
http: { | ||
headers: { | ||
'x-foo': 'A', | ||
}, | ||
}, | ||
}, | ||
}) | ||
}, | ||
b: () => { | ||
throw new GraphQLError('DB is not available', { | ||
extensions: { | ||
http: { | ||
headers: { | ||
'x-foo': 'B', | ||
}, | ||
}, | ||
}, | ||
}) | ||
}, | ||
}, | ||
}, | ||
}, | ||
}) | ||
|
||
let response = await yoga.fetch('http://yoga/graphql', { | ||
method: 'POST', | ||
headers: { 'content-type': 'application/json' }, | ||
body: JSON.stringify({ query: '{ a b }' }), | ||
}) | ||
|
||
expect(response.headers.get('x-foo')).toBe('B') | ||
|
||
response = await yoga.fetch('http://yoga/graphql', { | ||
method: 'POST', | ||
headers: { 'content-type': 'application/json' }, | ||
body: JSON.stringify({ query: '{ b a }' }), | ||
}) | ||
|
||
expect(response.headers.get('x-foo')).toBe('A') | ||
}) | ||
}) |
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.
This is per HTTP specification not GraphQL over HTTP, because this has nothing to do with GraphQL over HTTP since this validation is about the contract between client and server.
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.
Even though, GraphQL over HTTP says we should return
406
in this case as I understand;https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md#body
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.
Because when accepting
application/json
the server should not use 4xx and 5xx (see spec).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.
But we don't accept
application/json
, do we?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.
We do accept it, or do you mean we don't accept it in this situation? The server is set up now to respect the accept headers, if the client requests
application/json
and makes an invalid operation - the server will error out respecting the accept requirements.