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 context loss when async storage is instantiated early in the request lifecycle #12

Merged
merged 17 commits into from
Aug 14, 2020
Merged
Show file tree
Hide file tree
Changes from 9 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
2 changes: 1 addition & 1 deletion .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
"prettier"
],
"parserOptions": {
"ecmaVersion": 2015,
"ecmaVersion": 2017,
"sourceType": "module"
},
"rules": {
Expand Down
6 changes: 4 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,17 @@ const { fastifyRequestContextPlugin } = require('fastify-request-context')
const fastify = require('fastify');

fastify.register(fastifyRequestContextPlugin, {
hook: 'preValidation',
defaultStoreValues: {
user: { id: 'system' }
}
});
```

This plugin accepts option named `defaultStoreValues`.
This plugin accepts options `hook` and `defaultStoreValues`.

`defaultStoreValues` set initial values for the store (that can be later overwritten during request execution if needed). This is an optional parameter.
* `hook` allows you to specify to which lifecycle hook (or multiple hooks) should request context initialization be bound. Make sure that you are not trying to access the request context earlier in the lifecycle than you initialize it. Default value is `preValidation`.
* `defaultStoreValues` sets initial values for the store (that can be later overwritten during request execution if needed). This is an optional parameter.

From there you can set a context in another hook, route, or method that is within scope.

Expand Down
14 changes: 14 additions & 0 deletions index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,22 @@ export type RequestContext = {
set: <T>(key: string, value: T) => void
}

export type Hook =
| 'preValidation'
| 'preHandler'
| 'preSerialization'
| 'onSend'
| 'onResponse'
| 'onTimeout'
| 'onError'
| 'onRoute'
| 'onRegister'
| 'onReady'
| 'onClose'

export type RequestContextOptions = {
defaultStoreValues?: Record<string, any>
hook?: Hook | Hook[]
Copy link
Member

Choose a reason for hiding this comment

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

I think it is wrong since this doesn't work:

fastify.addHook(['onResponse', 'preHandler'], ()=>{}) // throw new FST_ERR_HOOK_INVALID_TYPE()

***continue

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't see much point in having multiple hooks, so kept just one.

}

declare module 'fastify' {
Expand Down
4 changes: 4 additions & 0 deletions index.test-d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ expectAssignable<RequestContextOptions>({})
expectAssignable<RequestContextOptions>({
defaultStoreValues: { a: 'dummy' },
})
expectAssignable<RequestContextOptions>({
hook: 'preValidation',
defaultStoreValues: { a: 'dummy' },
})

expectType<RequestContext>(app.requestContext)

Expand Down
2 changes: 1 addition & 1 deletion lib/requestContextPlugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ function plugin(fastify, opts, next) {
fastify.decorate('requestContext', requestContext)
fastify.decorateRequest('requestContext', requestContext)

fastify.addHook('onRequest', (req, res, done) => {
fastify.addHook(opts.hook || 'preValidation', (req, res, done) => {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be a semver major this change?

***continue
if the opts.hook is an array, it won't works, so we need to iterate over the array.
Why we should add the same hook on multiple lifecycle events?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, will have to do major version bump.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully there is a way to fix losing context from onRequest stage, though, but worst case scenario, initting ASL only on preValidation upwards can be made canonic.

als.runWith(() => {
done()
}, opts.defaultStoreValues)
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
"eslint-plugin-prettier": "^3.1.4",
"jest": "^26.4.0",
"prettier": "^2.0.5",
"superagent": "^6.0.0",
"tsd": "^0.13.1",
"typescript": "3.9.7"
},
Expand Down
215 changes: 215 additions & 0 deletions test/requestContextPlugin.e2e.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,215 @@
const fastify = require('fastify')
const request = require('superagent')
const { fastifyRequestContextPlugin } = require('../lib/requestContextPlugin')
const { TestService } = require('./internal/testService')

async function initAppPostWithPrevalidation(endpoint) {
Copy link
Member

Choose a reason for hiding this comment

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

this function is duplicated in test/requestContextPlugin.spec.js (except ready vs listen)

Could you use once?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, need to do a couple of deduplication passes. Will do.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Eomm Extracted this part, probably can extract some more.

const app = fastify({ logger: true })
app.register(fastifyRequestContextPlugin, { hook: 'preValidation' })

const preValidationFn = (req, reply, done) => {
const requestId = Number.parseInt(req.body.requestId)
req.requestContext.set('testKey', `testValue${requestId}`)
done()
}

app.route({
url: '/',
method: ['GET', 'POST'],
preValidation: preValidationFn,
handler: endpoint,
})

await app.listen(0)
return app
}

async function initAppPostWithAllPlugins(endpoint, requestHook) {
const app = fastify({ logger: true })
app.register(fastifyRequestContextPlugin, { hook: requestHook })

app.addHook('onRequest', (req, reply, done) => {
req.requestContext.set('onRequest', 'dummy')
done()
})

app.addHook('preParsing', (req, reply, payload, done) => {
req.requestContext.set('preParsing', 'dummy')
done(null, payload)
})

app.addHook('preValidation', (req, reply, done) => {
const requestId = Number.parseInt(req.body.requestId)
req.requestContext.set('preValidation', requestId)
req.requestContext.set('testKey', `testValue${requestId}`)
done()
})

app.addHook('preHandler', (req, reply, done) => {
const requestId = Number.parseInt(req.body.requestId)
req.requestContext.set('preHandler', requestId)
done()
})

app.route({
url: '/',
method: ['GET', 'POST'],
handler: endpoint,
})

await app.listen(0)
return app
}

describe('requestContextPlugin E2E', () => {
let app
afterEach(() => {
return app.close()
})

it('correctly preserves values set in prevalidation phase within single POST request', () => {
expect.assertions(2)

let testService
let responseCounter = 0
return new Promise((resolveResponsePromise) => {
const promiseRequest2 = new Promise((resolveRequest2Promise) => {
const promiseRequest1 = new Promise((resolveRequest1Promise) => {
Copy link
Member

Choose a reason for hiding this comment

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

please use async functions instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you sure that would work as expected? In this case we want to able to have flexibility to control when each promise resolves specifically, and I don't think that is possible with async functions.

Copy link
Member

Choose a reason for hiding this comment

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

oh ok.

const route = (req) => {
const requestId = req.requestContext.get('testKey')

function prepareReply() {
return testService.processRequest(requestId.replace('testValue', '')).then(() => {
const storedValue = req.requestContext.get('testKey')
return Promise.resolve({ storedValue })
})
}

// We don't want to read values until both requests wrote their values to see if there is a racing condition
if (requestId === 'testValue1') {
resolveRequest1Promise()
return promiseRequest2.then(prepareReply)
}

if (requestId === 'testValue2') {
resolveRequest2Promise()
return promiseRequest1.then(prepareReply)
}

throw new Error(`Unexpected requestId: ${requestId}`)
}

initAppPostWithPrevalidation(route).then((_app) => {
app = _app
testService = new TestService(app)
const { address, port } = app.server.address()
const url = `${address}:${port}`
const response1Promise = request('POST', url)
.send({ requestId: 1 })
.then((response) => {
expect(response.body.storedValue).toBe('testValue1')
responseCounter++
if (responseCounter === 2) {
resolveResponsePromise()
}
})

const response2Promise = request('POST', url)
.send({ requestId: 2 })
.then((response) => {
expect(response.body.storedValue).toBe('testValue2')
responseCounter++
if (responseCounter === 2) {
resolveResponsePromise()
}
})

return Promise.all([response1Promise, response2Promise])
})
})

return promiseRequest1
})

return promiseRequest2
})
})

it('correctly preserves values set in multiple phases within single POST request', () => {
expect.assertions(10)

let testService
let responseCounter = 0
return new Promise((resolveResponsePromise) => {
const promiseRequest2 = new Promise((resolveRequest2Promise) => {
const promiseRequest1 = new Promise((resolveRequest1Promise) => {
const route = (req) => {
const onRequestValue = req.requestContext.get('onRequest')
const preParsingValue = req.requestContext.get('preParsing')
const preValidationValue = req.requestContext.get('preValidation')
const preHandlerValue = req.requestContext.get('preHandler')

expect(onRequestValue).toBe(undefined)
expect(preParsingValue).toBe(undefined)
expect(preValidationValue).toEqual(expect.any(Number))
expect(preHandlerValue).toEqual(expect.any(Number))

const requestId = `testValue${preHandlerValue}`

function prepareReply() {
return testService.processRequest(requestId.replace('testValue', '')).then(() => {
const storedValue = req.requestContext.get('preValidation')
return Promise.resolve({ storedValue: `testValue${storedValue}` })
})
}

// We don't want to read values until both requests wrote their values to see if there is a racing condition
if (requestId === 'testValue1') {
resolveRequest1Promise()
return promiseRequest2.then(prepareReply)
}

if (requestId === 'testValue2') {
resolveRequest2Promise()
return promiseRequest1.then(prepareReply)
}

throw new Error(`Unexpected requestId: ${requestId}`)
}

initAppPostWithAllPlugins(route, 'preValidation').then((_app) => {
app = _app
testService = new TestService(app)
const { address, port } = app.server.address()
const url = `${address}:${port}`
const response1Promise = request('POST', url)
.send({ requestId: 1 })
.then((response) => {
expect(response.body.storedValue).toBe('testValue1')
responseCounter++
if (responseCounter === 2) {
resolveResponsePromise()
}
})

const response2Promise = request('POST', url)
.send({ requestId: 2 })
.then((response) => {
expect(response.body.storedValue).toBe('testValue2')
responseCounter++
if (responseCounter === 2) {
resolveResponsePromise()
}
})

return Promise.all([response1Promise, response2Promise])
})
})

return promiseRequest1
})

return promiseRequest2
})
})
})
92 changes: 92 additions & 0 deletions test/requestContextPlugin.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,26 @@ function initAppPost(endpoint) {
return app.ready()
}

function initAppPostWithPrevalidation(endpoint) {
const app = fastify({ logger: true })
app.register(fastifyRequestContextPlugin, { hook: 'preValidation' })

const preValidationFn = (req, reply, done) => {
const requestId = Number.parseInt(req.body.requestId)
req.requestContext.set('testKey', `testValue${requestId}`)
done()
}

app.route({
url: '/',
method: ['GET', 'POST'],
preValidation: preValidationFn,
handler: endpoint,
})

return app.ready()
}

describe('requestContextPlugin', () => {
let app
afterEach(() => {
Expand Down Expand Up @@ -171,4 +191,76 @@ describe('requestContextPlugin', () => {
return promiseRequest2
})
})

it('correctly preserves values set in prevalidation phase within single POST request', () => {
expect.assertions(2)

let testService
let responseCounter = 0
return new Promise((resolveResponsePromise) => {
const promiseRequest2 = new Promise((resolveRequest2Promise) => {
const promiseRequest1 = new Promise((resolveRequest1Promise) => {
const route = (req, reply) => {
const requestId = req.requestContext.get('testKey')

function prepareReply() {
testService.processRequest(requestId.replace('testValue', '')).then(() => {
const storedValue = req.requestContext.get('testKey')
reply.status(204).send({
storedValue,
})
})
}

// We don't want to read values until both requests wrote their values to see if there is a racing condition
if (requestId === 'testValue1') {
resolveRequest1Promise()
return promiseRequest2.then(prepareReply)
}

if (requestId === 'testValue2') {
resolveRequest2Promise()
return promiseRequest1.then(prepareReply)
}
}

initAppPostWithPrevalidation(route).then((_app) => {
app = _app
testService = new TestService(app)
const response1Promise = app
.inject()
.post('/')
.body({ requestId: 1 })
.end()
.then((response) => {
expect(response.json().storedValue).toBe('testValue1')
responseCounter++
if (responseCounter === 2) {
resolveResponsePromise()
}
})

const response2Promise = app
.inject()
.post('/')
.body({ requestId: 2 })
.end()
.then((response) => {
expect(response.json().storedValue).toBe('testValue2')
responseCounter++
if (responseCounter === 2) {
resolveResponsePromise()
}
})

return Promise.all([response1Promise, response2Promise])
})
})

return promiseRequest1
})

return promiseRequest2
})
})
})