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 regression in handling badly formed JSON #678

Merged
merged 1 commit into from Dec 1, 2021
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Fix regression in handling badly formed JSON
  • Loading branch information
mcollina committed Dec 1, 2021
commit 732b2f895312da8deadd7b173dcd2d141d54b223
17 changes: 15 additions & 2 deletions lib/routes.js
Expand Up @@ -135,14 +135,27 @@ module.exports = async function (app, opts) {
const errorFormatter = typeof opts.errorFormatter === 'function' ? opts.errorFormatter : defaultErrorFormatter

if (typeof opts.errorHandler === 'function') {
app.setErrorHandler(opts.errorHandler)
app.setErrorHandler((error, request, reply) => {
const errorHandler = opts.errorHandler
if (!request[kRequestContext]) {
// Generate the context for this request
request[kRequestContext] = { reply, app }
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for missing this.

@mcollina
should not we try to generate full context, like in other places?

    if (contextFn) {
      request[kRequestContext] = await contextFn(request, reply)
      Object.assign(request[kRequestContext], { reply, app })
    } else {
      request[kRequestContext] = { reply, app }
    }

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 just focused on fixing it asap. Open a fresh PR in case.

}

return errorHandler(error, request, reply)
})
} else if (opts.errorHandler === true || opts.errorHandler === undefined) {
app.setErrorHandler((error, request, reply) => {
if (!request[kRequestContext]) {
// Generate the context for this request
request[kRequestContext] = { reply, app }
}

const { statusCode, response } = errorFormatter(
error,
request[kRequestContext]
)
reply.code(statusCode).send(response)
return reply.code(statusCode).send(response)
})
}
const contextFn = opts.context
Expand Down
87 changes: 87 additions & 0 deletions test/errors.js
Expand Up @@ -767,3 +767,90 @@ test('errors - should override statusCode to 200 if the data is present', async

t.equal(res.statusCode, 200)
})

test('bad json', async (t) => {
const schema = `
type Query {
successful: String
}
`

const resolvers = {
Query: {
successful () {
t.fail('Should not be called')
return 'Runs OK'
}
}
}

const app = Fastify()

app.register(GQL, {
schema,
resolvers
})

await app.ready()

const res = await app.inject({
method: 'POST',
headers: {
'content-type': 'application/json'
},
body: 'this is not a json',
url: '/graphql'
})

t.equal(res.statusCode, 400)
t.same(res.json(),
{ data: null, errors: [{ message: 'Unexpected token h in JSON at position 1' }] }
)
})

test('bad json with custom error handler', async (t) => {
t.plan(3)
const schema = `
type Query {
successful: String
}
`

const resolvers = {
Query: {
successful () {
t.fail('Should not be called')
return 'Runs OK'
}
}
}

const app = Fastify()

app.register(GQL, {
schema,
resolvers,
errorHandler: (_, request, reply) => {
t.pass('custom error handler called')
reply.code(400).send({
is: 'error'
})
}
})

await app.ready()

const res = await app.inject({
method: 'POST',
headers: {
'content-type': 'application/json'
},
body: 'this is not a json',
url: '/graphql'
})

t.equal(res.statusCode, 400)
t.same(res.json(), {
is: 'error'
})
})