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

Respect Accept headers in a more strict way #1516

Merged
merged 4 commits into from
Aug 1, 2022
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
5 changes: 5 additions & 0 deletions .changeset/more-accept-headers.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'graphql-yoga': major
---

Now it is possible to decide the returned `Content-Type` by specifying the `Accept` header. So if `Accept` header has `text/event-stream` without `application/json`, Yoga respects that returns `text/event-stream` instead of `application/json`.
2 changes: 1 addition & 1 deletion e2e/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ export async function assertQuery(
const response = await fetch(endpoint, {
method: 'POST',
headers: {
accept: 'applications/json',
accept: 'application/json',
'content-type': 'application/json',
},
body: JSON.stringify({
Expand Down
56 changes: 56 additions & 0 deletions packages/graphql-yoga/__tests__/node.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -621,6 +621,7 @@ describe('Incremental Delivery', () => {
body: formData,
})

expect(response.status).toBe(200)
const body = await response.json()

expect(body.errors).toBeUndefined()
Expand Down Expand Up @@ -1481,3 +1482,58 @@ describe('404 Handling', () => {
expect(body).toEqual('Do you really like em?')
})
})

describe('Respect Accept headers', () => {
const yoga = createYoga({
schema,
})
const server = createServer(yoga)
let port: number
let url: string
beforeAll((done) => {
port = Math.floor(Math.random() * 100) + 4000
url = `http://localhost:${port}/graphql`
server.listen(port, done)
})
afterAll(() => {
server.close()
})
Comment on lines +1486 to +1500
Copy link
Collaborator

@n1ru4l n1ru4l Aug 1, 2022

Choose a reason for hiding this comment

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

I think these tests do not necessarily require running a Node HTTP server, but we can change it later on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it is safer to test stream requests and responses via the really HTTP

it('should force the server return event stream even if the result is not', async () => {
const response = await fetch(`${url}?query=query{ping}`, {
headers: {
Accept: 'text/event-stream',
},
})
expect(response.headers.get('content-type')).toEqual('text/event-stream')
const iterator = response.body![Symbol.asyncIterator]()
const { value } = await iterator.next()
const valueStr = Buffer.from(value).toString('utf-8')
expect(valueStr).toContain(
`data: ${JSON.stringify({ data: { ping: 'pong' } })}`,
)
})
it('should force the server return multipart even if the result is not', async () => {
const response = await fetch(`${url}?query=query{ping}`, {
headers: {
Accept: 'multipart/mixed',
},
})
expect(response.headers.get('content-type')).toEqual(
'multipart/mixed; boundary="-"',
)
const iterator = response.body![Symbol.asyncIterator]()
const { value } = await iterator.next()
const valueStr = Buffer.from(value).toString('utf-8')
expect(valueStr).toContain(`Content-Type: application/json; charset=utf-8`)
expect(valueStr).toContain(`Content-Length: 24`)
expect(valueStr).toContain(`${JSON.stringify({ data: { ping: 'pong' } })}`)
})
it('should not allow to return if the result is an async iterable and accept is just json', async () => {
const response = await fetch(`${url}?query=subscription{counter}`, {
headers: {
Accept: 'application/json',
},
})
expect(response.status).toEqual(406)
})
Comment on lines +1531 to +1538
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmmmm, here it could actually be debatable whether it is okay for the client to only wait for the first event and then ditch the subscription 🤔

})
30 changes: 19 additions & 11 deletions packages/graphql-yoga/src/plugins/resultProcessor/multipart.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,15 @@
import { isAsyncIterable } from '@envelop/core'
import { ExecutionResult } from 'graphql'
import { ExecutionPatchResult, FetchAPI } from '../../types.js'
import { FetchAPI } from '../../types.js'
import { ResultProcessorInput } from '../types.js'

export function isMultipartResult(
request: Request,
result: ResultProcessorInput,
): result is AsyncIterable<ExecutionPatchResult> {
return (
isAsyncIterable(result) &&
!!request.headers.get('accept')?.includes('multipart/mixed')
)
export function isMultipartResult(request: Request): boolean {
// There should be an explicit accept header for this result type
return !!request.headers.get('accept')?.includes('multipart/mixed')
}

export function processMultipartResult(
executionPatchResultIterable: AsyncIterable<ExecutionPatchResult>,
result: ResultProcessorInput,
fetchAPI: FetchAPI,
): Response {
const headersInit: HeadersInit = {
Expand All @@ -33,7 +28,20 @@ export function processMultipartResult(

const readableStream = new fetchAPI.ReadableStream({
start(controller) {
iterator = executionPatchResultIterable[Symbol.asyncIterator]()
if (isAsyncIterable(result)) {
iterator = result[Symbol.asyncIterator]()
} else {
let finished = false
iterator = {
next: () => {
if (finished) {
return Promise.resolve({ done: true, value: null })
}
finished = true
return Promise.resolve({ done: false, value: result })
},
}
}
controller.enqueue(textEncoder.encode(`---`))
},
async pull(controller) {
Expand Down
28 changes: 18 additions & 10 deletions packages/graphql-yoga/src/plugins/resultProcessor/push.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,13 @@ import { ExecutionResult } from 'graphql'
import { FetchAPI } from '../../types.js'
import { ResultProcessorInput } from '../types.js'

export function isPushResult(
request: Request,
result: ResultProcessorInput,
): result is AsyncIterable<ExecutionResult> {
return (
isAsyncIterable(result) &&
!!request.headers.get('accept')?.includes('text/event-stream')
)
export function isPushResult(request: Request): boolean {
// There should be an explicit accept header for this result type
return !!request.headers.get('accept')?.includes('text/event-stream')
}

export function processPushResult(
result: AsyncIterable<ExecutionResult>,
result: ResultProcessorInput,
fetchAPI: FetchAPI,
): Response {
const headersInit: HeadersInit = {
Expand All @@ -33,7 +28,20 @@ export function processPushResult(
const textEncoder = new fetchAPI.TextEncoder()
const readableStream = new fetchAPI.ReadableStream({
start() {
iterator = result[Symbol.asyncIterator]()
if (isAsyncIterable(result)) {
iterator = result[Symbol.asyncIterator]()
} else {
let finished = false
iterator = {
next: () => {
if (finished) {
return Promise.resolve({ done: true, value: null })
}
finished = true
return Promise.resolve({ done: false, value: result })
},
}
}
},
async pull(controller) {
const { done, value } = await iterator.next()
Expand Down
31 changes: 26 additions & 5 deletions packages/graphql-yoga/src/plugins/resultProcessor/regular.ts
Original file line number Diff line number Diff line change
@@ -1,24 +1,45 @@
import { isAsyncIterable } from '@graphql-tools/utils'
import { ExecutionResult } from 'graphql'
import { FetchAPI } from '../../types.js'
import { ResultProcessorInput } from '../types.js'

const acceptHeaderByResult = new WeakMap<ResultProcessorInput, string>()

export function isRegularResult(
request: Request,
result: ResultProcessorInput,
): result is ExecutionResult {
return !isAsyncIterable(result)
): boolean {
if (!isAsyncIterable(result)) {
const acceptHeader = request.headers.get('accept')
if (acceptHeader && !acceptHeader.includes('*/*')) {
if (acceptHeader.includes('application/json')) {
acceptHeaderByResult.set(result, 'application/json')
return true
}
if (acceptHeader.includes('application/graphql+json')) {
acceptHeaderByResult.set(result, 'application/graphql+json')
return true
}
// If there is an accept header but this processer doesn't support, reject
return false
}
// If there is no header, assume it's a regular result per spec
acceptHeaderByResult.set(result, 'application/json')
return true
}
// If it is not an async iterable, it's not a regular result
return false
}

export function processRegularResult(
executionResult: ExecutionResult,
executionResult: ResultProcessorInput,
fetchAPI: FetchAPI,
): Response {
const textEncoder = new fetchAPI.TextEncoder()
const responseBody = JSON.stringify(executionResult)
const decodedString = textEncoder.encode(responseBody)
const contentType = acceptHeaderByResult.get(executionResult)
const headersInit: HeadersInit = {
'Content-Type': 'application/json',
'Content-Type': contentType || 'application/json',
'Content-Length': decodedString.byteLength.toString(),
}
const responseInit: ResponseInit = {
Expand Down
18 changes: 9 additions & 9 deletions packages/graphql-yoga/src/plugins/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,21 +70,21 @@ export type OnResultProcess = (
payload: OnResultProcessEventPayload,
) => PromiseOrValue<void>

export type ResultProcessorInput = PromiseOrValue<
ExecutionResult | AsyncIterable<ExecutionResult | ExecutionPatchResult>
>
export type ResultProcessorInput =
| ExecutionResult
| AsyncIterable<ExecutionResult>
| AsyncIterable<ExecutionPatchResult>

export type ResultProcessor<
TResult extends ResultProcessorInput = ResultProcessorInput,
> = (result: TResult, fetchAPI: FetchAPI) => PromiseOrValue<Response>
export type ResultProcessor = (
result: ResultProcessorInput,
fetchAPI: FetchAPI,
) => PromiseOrValue<Response>

export interface OnResultProcessEventPayload {
request: Request
result: ResultProcessorInput
resultProcessor?: ResultProcessor
setResultProcessor<TResult extends ResultProcessorInput>(
resultProcessor: ResultProcessor<TResult>,
): void
setResultProcessor(resultProcessor: ResultProcessor): void
}

export type OnResponseHook<TServerContext> = (
Expand Down
14 changes: 6 additions & 8 deletions packages/graphql-yoga/src/plugins/useResultProcessor.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
import { Plugin, ResultProcessor, ResultProcessorInput } from './types.js'

export interface ResultProcessorPluginOptions<
TResult extends ResultProcessorInput,
> {
processResult: ResultProcessor<TResult>
match?(request: Request, result: ResultProcessorInput): result is TResult
export interface ResultProcessorPluginOptions {
processResult: ResultProcessor
match?(request: Request, result: ResultProcessorInput): boolean
}

export function useResultProcessor<
TResult extends ResultProcessorInput = ResultProcessorInput,
>(options: ResultProcessorPluginOptions<TResult>): Plugin {
export function useResultProcessor(
options: ResultProcessorPluginOptions,
): Plugin {
const matchFn = options.match || (() => true)
return {
onResultProcess({ request, result, setResultProcessor }) {
Expand Down
6 changes: 2 additions & 4 deletions packages/graphql-yoga/src/processRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export async function processResult({
*/
onResultProcessHooks: OnResultProcess[]
}) {
let resultProcessor: ResultProcessor<any> | undefined
let resultProcessor: ResultProcessor | undefined

for (const onResultProcessHook of onResultProcessHooks) {
await onResultProcessHook({
Expand Down Expand Up @@ -79,7 +79,5 @@ export async function processRequest<TContext>({
: enveloped.execute

// Get the result to be processed
const result = await executeFn(executionArgs)

return result
return executeFn(executionArgs)
}
8 changes: 4 additions & 4 deletions packages/graphql-yoga/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -387,16 +387,16 @@ export class YogaServer<
}),
// Middlewares after the GraphQL execution
useResultProcessor({
match: isRegularResult,
processResult: processRegularResult,
match: isMultipartResult,
processResult: processMultipartResult,
}),
useResultProcessor({
match: isPushResult,
processResult: processPushResult,
}),
useResultProcessor({
match: isMultipartResult,
processResult: processMultipartResult,
match: isRegularResult,
processResult: processRegularResult,
}),
...(options?.plugins ?? []),

Expand Down