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

feat: @defer support #898

Open
wants to merge 20 commits into
base: next
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
cee50b7
added @defer support for requests with multipart/mixed; deferSpec=202…
igrlk Oct 24, 2022
4ad38b4
reverted mercurius version bump, moved @defer tests to their own file
igrlk Oct 26, 2022
08ee6d2
remove 'Mercurius' from the error message about wrong accept header, …
igrlk Oct 26, 2022
35fdb02
bump graphql to 17.0.0-alpha.2
igrlk Oct 26, 2022
d41fff6
moved missing multipart accept header error into errors.js
igrlk Oct 26, 2022
274c5bc
added opts.defer: boolean to enable @defer directive
igrlk Oct 26, 2022
3e3ca77
use @fastify/accepts instead of Negotiator package
igrlk Oct 26, 2022
2b3c25a
add @defer test with undici.fetch
igrlk Oct 26, 2022
369457e
Add space between merged SDLs to fix merging errors (#899)
Igloczek Nov 2, 2022
780c668
Explicitly say in the docs that JIT is disabled by default (#901)
igrlk Nov 2, 2022
b4d70fc
feat: add types for object in graphiql configuration (#907)
codeflyer Nov 2, 2022
e27e5b0
Prevent parsing schema exceptions when importing directives (#900)
Igloczek Nov 2, 2022
6933420
Bumped v11.3.0
mcollina Nov 2, 2022
d1d8485
Removed canUseIncrementalExecution check
igrlk Nov 2, 2022
2d599a7
Merge branch 'mercurius-js:master' into defer-directive-support
igrlk Nov 2, 2022
ef73394
Merge branch 'defer-directive-support' of github.com:igrlk/mercurius …
igrlk Nov 2, 2022
640300f
Throw an error if JIT is used together with defer, update the docs
igrlk Nov 2, 2022
8b7e9b2
Throw an error if JIT is used together with defer, update the docs
igrlk Nov 2, 2022
7a84f17
Merge branch 'defer-directive-support' of github.com:igrlk/mercurius …
igrlk Nov 2, 2022
093d0df
Merge branch 'next' into defer-directive-support
igrlk Nov 2, 2022
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
92 changes: 89 additions & 3 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict'

const Negotiator = require('negotiator')
const fp = require('fastify-plugin')
let LRU = require('tiny-lru')
const routes = require('./lib/routes')
Expand All @@ -18,10 +19,10 @@ const {
extendSchema,
validate,
validateSchema,
specifiedRules,
execute
specifiedRules
} = require('graphql')
const { buildExecutionContext } = require('graphql/execution/execute')
const { Readable } = require('stream')
const queryDepth = require('./lib/queryDepth')
const buildFederationSchema = require('./lib/federation')
const { initGateway } = require('./lib/gateway')
Expand All @@ -47,6 +48,7 @@ const {
preExecutionHandler,
onResolutionHandler
} = require('./lib/handlers')
const { executeGraphql, MEDIA_TYPES } = require('./lib/util')

// Required for module bundlers
// istanbul ignore next
Expand Down Expand Up @@ -546,7 +548,7 @@ const plugin = fp(async function (app, opts) {
return maybeFormatErrors(execution, context)
}

const execution = await execute({
const execution = await executeGraphql({
schema: modifiedSchema || fastifyGraphQl.schema,
document: modifiedDocument || document,
rootValue: root,
Expand All @@ -555,9 +557,93 @@ const plugin = fp(async function (app, opts) {
operationName
})

/* istanbul ignore next */
if (execution.initialResult) {
const acceptHeader = reply.request.raw.headers.accept

if (
!(
acceptHeader &&
new Negotiator({
headers: { accept: acceptHeader }
}).mediaType([
// mediaType() will return the first one that matches, so if the client
// doesn't include the deferSpec parameter it will match this one here,
// which isn't good enough.
MEDIA_TYPES.MULTIPART_MIXED_NO_DEFER_SPEC,
MEDIA_TYPES.MULTIPART_MIXED_EXPERIMENTAL
]) === MEDIA_TYPES.MULTIPART_MIXED_EXPERIMENTAL
)
mcollina marked this conversation as resolved.
Show resolved Hide resolved
) {
// The client ran an operation that would yield multiple parts, but didn't
// specify `accept: multipart/mixed`. We return an error.
throw new Error(
Copy link
Collaborator

Choose a reason for hiding this comment

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

'Server received an operation that uses incremental delivery ' +
'(@defer or @stream), but the client does not accept multipart/mixed ' +
'HTTP responses. To enable incremental delivery support, add the HTTP ' +
"header 'Accept: multipart/mixed; deferSpec=20220824'."
)
}
mcollina marked this conversation as resolved.
Show resolved Hide resolved

reply.header('content-type', 'multipart/mixed; boundary="-"; deferSpec=20220824')

return Readable.from(
mcollina marked this conversation as resolved.
Show resolved Hide resolved
writeMultipartBody(
execution.initialResult,
execution.subsequentResults
)
)
}

return maybeFormatErrors(execution, context)
}

/* istanbul ignore next */
async function * writeMultipartBody (initialResult, subsequentResults) {
yield `\r\n---\r\ncontent-type: application/json; charset=utf-8\r\n\r\n${JSON.stringify(
orderInitialIncrementalExecutionResultFields(initialResult)
)}\r\n---${initialResult.hasNext ? '' : '--'}\r\n`

for await (const result of subsequentResults) {
yield `content-type: application/json; charset=utf-8\r\n\r\n${JSON.stringify(
orderSubsequentIncrementalExecutionResultFields(result)
)}\r\n---${result.hasNext ? '' : '--'}\r\n`
}
}

/* istanbul ignore next */
function orderInitialIncrementalExecutionResultFields (result) {
return {
hasNext: result.hasNext,
errors: result.errors,
data: result.data,
incremental: orderIncrementalResultFields(result.incremental),
extensions: result.extensions
}
}

/* istanbul ignore next */
function orderSubsequentIncrementalExecutionResultFields (result) {
return {
hasNext: result.hasNext,
incremental: orderIncrementalResultFields(result.incremental),
extensions: result.extensions
}
}

/* istanbul ignore next */
function orderIncrementalResultFields (incremental) {
return incremental?.map((i) => ({
hasNext: i.hasNext,
errors: i.errors,
path: i.path,
label: i.label,
data: i.data,
items: i.items,
extensions: i.extensions
}))
}

async function maybeFormatErrors (execution, context) {
execution = addErrorsToExecutionResult(execution, context.errors)

Expand Down
14 changes: 8 additions & 6 deletions lib/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,14 @@ function toGraphQLError (err) {

const gqlError = new GraphQLError(
err.message,
err.nodes,
err.source,
err.positions,
err.path,
err,
err.extensions
{
nodes: err.nodes,
source: err.source,
positions: err.positions,
path: err.path,
originalError: err,
extensions: err.extensions
}
mcollina marked this conversation as resolved.
Show resolved Hide resolved
)

gqlError.locations = err.locations
Expand Down
24 changes: 23 additions & 1 deletion lib/util.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
'use strict'

const { execute } = require('graphql')
const { experimentalExecuteIncrementally } = require('graphql/execution')

function hasDirective (directiveName, node) {
if (!node.directives || node.directives.length < 1) {
return false
Expand All @@ -23,7 +26,26 @@ function hasExtensionDirective (node) {
}
}

const canUseIncrementalExecution = !!require('graphql/execution').experimentalExecuteIncrementally
Copy link
Collaborator

Choose a reason for hiding this comment

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

not needed, this is always true now.


// istanbul ignore next
function executeGraphql (args) {
if (canUseIncrementalExecution) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should happen only if defer is enabled.

return experimentalExecuteIncrementally(args)
}

return execute(args)
}

const MEDIA_TYPES = {
MULTIPART_MIXED_NO_DEFER_SPEC: 'multipart/mixed',
MULTIPART_MIXED_EXPERIMENTAL: 'multipart/mixed; deferSpec=20220824'
}

module.exports = {
hasDirective,
hasExtensionDirective
hasExtensionDirective,
canUseIncrementalExecution,
executeGraphql,
MEDIA_TYPES
}
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
"graphql": "^16.0.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we are not testing this behavior in CI, right? We should. The peerDependency might not be correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could try adding CI tests that install graphql@17 if you can point me to the right direction - how and where you add them?

Also, what do you mean by peerDependency not correct? It didn't change in my PR, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

How would an end user use defer? As you can see we depend upon graphql.

Copy link
Contributor Author

@igrlk igrlk Oct 26, 2022

Choose a reason for hiding this comment

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

I see. So the Apollo Server, as a reference, didn't update graphql version and they will only do that when graphql@17 is released. Until that, it's the change that is implemented and tested on CI, but not available to the end users since it's still in alpha

That being said, the idea of mine was to do the same here - start supporting it but not allow users to use it until graphql@17 is officially released

In the future, when graphql@17 is live, we will update the mercurius dependency and everyone will be able to use defer

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm happy to cut a next branch for you, so we can land this with the alpha version of graphql-js.

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good to me! so i finish the feedback first and then you create a next release branch?

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I incorporated the feedback and going to see what's going on there with JIT compiler when I have some time

"graphql-jit": "^0.7.3",
"mqemitter": "^5.0.0",
"negotiator": "^0.6.3",
"p-map": "^4.0.0",
"readable-stream": "^4.0.0",
"safe-stable-stringify": "^2.3.0",
Expand Down
146 changes: 146 additions & 0 deletions test/defer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
const { test } = require('tap')
const Fastify = require('fastify')
const mercurius = require('../index')
const { canUseIncrementalExecution } = require('../lib/util')

if (canUseIncrementalExecution) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This if should not be needed anymore.

const schema = `
directive @defer(
if: Boolean! = true
label: String
) on FRAGMENT_SPREAD | INLINE_FRAGMENT
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this directive added automatically by mercurius?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would do this behind a defer: true option, set to false by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @mcollina I've added opts.defer, but I'm not quite sure if we really need that, since we decided to cut this off to a new release branch where we assume @defer will be available to everyone.

I can remove opts.defer and just leave its directive definition so that we always have this functionality with graphql@17.

What do you think?


type Query {
allProducts: [Product!]!
}

type Product {
delivery: DeliveryEstimates!
sku: String!
id: ID!
}

type DeliveryEstimates {
estimatedDelivery: String!
fastestDelivery: String!
}`

const allProducts = new Array(1).fill(0).map((_, index) => ({
id: `${index}`,
sku: 'sku'
}))

const resolvers = {
Query: {
allProducts: () => {
return allProducts
}
},
Product: {
delivery: () => {
return {
estimatedDelivery: '25.01.2000',
fastestDelivery: '25.01.2000'
}
}
}
}

const query = `
query deferVariation {
allProducts {
delivery {
...MyFragment @defer
}
sku
id
}
}

fragment MyFragment on DeliveryEstimates {
estimatedDelivery
fastestDelivery
}
`

const wrongAcceptValues = [
'',
'application/json',
'multipart/mixed',
'multipart/mixed; deferSpec=12345'
]

for (const accept of wrongAcceptValues) {
test('errors with @defer when used with wrong "accept" header', async t => {
const app = Fastify()
await app.register(mercurius, { schema, resolvers, graphiql: true })

const res = await app.inject({
method: 'POST',
url: '/graphql',
headers: {
'content-type': 'application/json',
accept
},
body: JSON.stringify({ query })
})

t.match(res, {
statusCode: 200,
body: JSON.stringify({
data: null,
errors: [{
message: "Server received an operation that uses incremental delivery (@defer or @stream), but the client does not accept multipart/mixed HTTP responses. To enable incremental delivery support, add the HTTP header 'Accept: multipart/mixed; deferSpec=20220824'."
}]
})
})

await app.close()
t.end()
})
}

const correctAcceptValues = [
'multipart/mixed; deferSpec=20220824',
'multipart/mixed; deferSpec=20220824, application/json',
'application/json, multipart/mixed; deferSpec=20220824'
]

for (const accept of correctAcceptValues) {
test('works with @defer when used with correct "accept" header', async t => {
const app = Fastify()
await app.register(mercurius, { schema, resolvers, graphiql: true })

const res = await app.inject({
method: 'POST',
url: '/graphql',
headers: {
'content-type': 'application/json',
accept
},
body: JSON.stringify({ query })
})

t.match(res, {
statusCode: 200,
headers: {
'content-type': 'multipart/mixed; boundary="-"; deferSpec=20220824'
},
body: `\r
---\r
content-type: application/json; charset=utf-8\r
\r
{"hasNext":true,"data":{"allProducts":[{"delivery":{},"sku":"sku","id":"0"}]}}\r
---\r
content-type: application/json; charset=utf-8\r
\r
{"hasNext":false,"incremental":[{"path":["allProducts",0,"delivery"],"data":{"estimatedDelivery":"25.01.2000","fastestDelivery":"25.01.2000"}}]}\r
-----\r
`
})

await app.close()
t.end()
})
}
}