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: correct type definition for genReqId argument #4784

Merged
merged 8 commits into from
Jun 16, 2023
Merged
4 changes: 2 additions & 2 deletions docs/Reference/Server.md
Original file line number Diff line number Diff line change
Expand Up @@ -527,8 +527,8 @@ Defines the label used for the request identifier when logging the request.
### `genReqId`
<a id="factory-gen-request-id"></a>

Function for generating the request-id. It will receive the incoming request as
a parameter. This function is expected to be error-free.
Function for generating the request-id. It will receive _raw_ incoming
sergburn marked this conversation as resolved.
Show resolved Hide resolved
request as a parameter. This function is expected to be error-free.

+ Default: `value of 'request-id' header if provided or monotonically increasing
integers`
Expand Down
2 changes: 1 addition & 1 deletion fastify.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ declare namespace fastify {
requestIdHeader?: string | false,
requestIdLogLabel?: string;
jsonShorthand?: boolean;
genReqId?: <RequestGeneric extends RequestGenericInterface = RequestGenericInterface, TypeProvider extends FastifyTypeProvider = FastifyTypeProviderDefault>(req: FastifyRequest<RequestGeneric, RawServer, RawRequestDefaultExpression<RawServer>, FastifySchema, TypeProvider>) => string,
genReqId?: (req: RawRequestDefaultExpression<RawServer>) => string,
trustProxy?: boolean | string | string[] | number | TrustProxyFunction,
querystringParser?: (str: string) => { [key: string]: unknown },
/**
Expand Down
40 changes: 40 additions & 0 deletions test/genReqId.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,43 @@ test('Should accept a custom genReqId function', t => {
})
})
})

test('Custom genReqId function gets raw request as argument', t => {
t.plan(8)

const REQUEST_ID = 'REQ-1234'

const fastify = Fastify({
genReqId: function (req) {
t.notOk(req.id)
t.notOk(req.raw)
Copy link
Contributor

Choose a reason for hiding this comment

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

So raw is false or undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, this way is more specific?

   const fastify = Fastify({
     genReqId: function (req) {
-      t.notOk(req.id)
-      t.notOk(req.raw)
+      t.notOk('id' in req)
+      t.notOk('raw' in req)
+      t.ok(req instanceof Readable)

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, better

// http.IncomingMessage does have `rawHeaders` property, but FastifyRequest does not
const index = req.rawHeaders.indexOf('x-request-id')
const xReqId = req.rawHeaders[index + 1]
t.equal(xReqId, REQUEST_ID)
t.equal(req.headers['x-request-id'], REQUEST_ID)
return xReqId
}
})

fastify.get('/', (req, reply) => {
t.equal(req.id, REQUEST_ID)
reply.send({ id: req.id })
})

fastify.listen({ port: 0 }, err => {
t.error(err)
fastify.inject({
method: 'GET',
headers: {
'x-request-id': REQUEST_ID
},
url: 'http://localhost:' + fastify.server.address().port
}, (err, res) => {
t.error(err)
const payload = JSON.parse(res.payload)
t.equal(payload.id, REQUEST_ID)
fastify.close()
})
})
})
2 changes: 1 addition & 1 deletion test/types/fastify.test-d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ expectAssignable<FastifyInstance>(fastify({ serverFactory: () => http.createServ
expectAssignable<FastifyInstance>(fastify({ caseSensitive: true }))
expectAssignable<FastifyInstance>(fastify({ requestIdHeader: 'request-id' }))
expectAssignable<FastifyInstance>(fastify({ requestIdHeader: false }))
expectAssignable<FastifyInstance>(fastify({ genReqId: () => 'request-id' }))
expectAssignable<FastifyInstance>(fastify({ genReqId: (req) => req.rawHeaders[1] || 'foo' }))
Copy link
Contributor

Choose a reason for hiding this comment

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

The actual typing test needs to be something like this:

fastify({ genReqId: (req) => {
  expectType<RawRequestDefaultExpression>(req)
}})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about this?

expectAssignable<FastifyInstance>(fastify({ genReqId: (req) => {
  expectType<RawRequestDefaultExpression>(req)
  return 'foo'
} }))
expectAssignable<FastifyInstance<http.Server>>(fastify({ genReqId: (req) => {
  expectType<http.IncomingMessage>(req)
  return 'foo'
} }))
expectAssignable<FastifyInstance<http2.Http2Server>>(fastify({ http2: true, genReqId: (req) => {
  expectType<http2.Http2ServerRequest>(req)
  return 'foo'
} }))

Are the other two checks completely redundant?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I think they are unrelated to this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll remove them

expectAssignable<FastifyInstance>(fastify({ trustProxy: true }))
expectAssignable<FastifyInstance>(fastify({ querystringParser: () => ({ foo: 'bar' }) }))
expectAssignable<FastifyInstance>(fastify({ querystringParser: () => ({ foo: { bar: 'fuzz' } }) }))
Expand Down