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

add option to disable/ignore request-id header #4193

Merged
merged 5 commits into from Aug 11, 2022

Conversation

philippviereck
Copy link
Contributor

@philippviereck philippviereck commented Aug 10, 2022

fixes #4192

This way it should not break anything, just allow for requestIdHeader to be ignored by setting it explicitly to false

  const fastify = Fastify({
    requestIdHeader: false,
  })

(I am not sure if I updated all those config validation and typescript files correctly.)

Checklist

Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

Could you update the documentation as well?

then it LGTM 👍🏽

@philippviereck
Copy link
Contributor Author

@Eomm Yeah, I can probably do that. (update the docs)

So, the current PR implementation does not break anything, and undefined still defaults to 'request-id'.
In my opinion this is unexpected behaviour. I think it should be false by default and you should opt-in rather than opt-out.

Also, setting requestIdHeader to true instead of a string will result, in the same behaviour as false because
false | string is expected, should this be handled specifically or is it the users fault?

PS: here is why:

 const fastify = Fastify({
    requestIdHeader: true,
  })

const requestIdHeader = (options.requestIdHeader === false) ? false : (options.requestIdHeader || defaultInitOptions.requestIdHeader) 
// => requestIdHeader = true

...
const id = requestIdHeader === false ? genReqId(req) : (req.headers[requestIdHeader] || genReqId(req))
// req.headers[true] is undefined, so genReqId(req) will be used

generated by running 'node build/build-validation.js'
after change on 'build-validation.js'
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Good work, could you add this new option to the docs?

Adding documentation for opt-out of 'requestHeaderId'
@philippviereck philippviereck deleted the ignore_request_id_header branch August 11, 2022 10:42
@philippviereck philippviereck restored the ignore_request_id_header branch August 11, 2022 10:58
@philippviereck
Copy link
Contributor Author

philippviereck commented Aug 11, 2022

sorry, didn't mean to do that. I updated the docs

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@Eomm Eomm merged commit 4e79725 into fastify:main Aug 11, 2022
philippviereck added a commit to philippviereck/fastify that referenced this pull request Aug 11, 2022
feat: disable/ignore request-id header (fastify#4193)
@philippviereck philippviereck deleted the ignore_request_id_header branch August 11, 2022 13:49
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to disable 'request-id' header
3 participants