feat: access handler name add properties to req route options #4470

cesarvspr · 2022-12-17T20:47:48Z

Closes #4439

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
[] documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

Eomm

The request.routeOptions should be immutable test is failing, it could be due to the new warning

lib/request.js

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>

Signed-off-by: cesarvspr <vinicius_spr@hotmail.com>

climba03003 · 2022-12-20T04:41:26Z

lib/request.js

@@ -171,6 +172,7 @@ Object.defineProperties(Request.prototype, {
      const routeLimit = context._parserOptions.limit
      const serverLimit = context.server.initialConfig.bodyLimit
      const version = context.server.hasConstraintStrategy('version') ? this.raw.headers['accept-version'] : undefined
+      const { handler, config } = context


You are taking the reference here and config is an object.
When you deep frozen the object, altering config in else where will be break.

The getter should be immutable, but the hidden value should be allowed to modify by the core.

Signed-off-by: cesarvspr <vinicius_spr@hotmail.com>

Eomm · 2023-06-05T12:21:35Z

Two comments to be addressed:

then we can land this

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>

Songkeys · 2023-08-06T11:03:56Z

What can we do to move this PR forward?

Currently my logging is full of this:

Uzlopak · 2023-08-06T11:06:23Z

Is this even related to this PR?

Cant you patch your code accordingly?

SimenB · 2023-08-06T11:07:47Z

What can we do to move this PR forward?

From my understanding, only a doc update is missing. So you can probably suggest changes here or PR the fork.

Songkeys · 2023-08-06T11:09:22Z

Is this even related to this PR?

Cant you patch your code accordingly?

I think this is related to another issue for the opentelemetry which is blocked by this PR? open-telemetry/opentelemetry-js-contrib#1275

…rties-to-req-route-options

Uzlopak · 2023-08-06T11:13:39Z

I think @SimenB wanted to actually say, that this PR would also add more warnings to the opentelemetry issue.

Uzlopak · 2023-08-06T11:18:44Z

To fix the issue in opentelemetry, you would need to patch

https://github.com/open-telemetry/opentelemetry-js-contrib/blob/154b30bc98ff0b848348faa18197eb7f11dcbdc4/plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts#L263

There I think you would need to change it to:

      const handlerName = (((request as any).routeConfig || {})?.name || '').substr(6);

Songkeys · 2023-08-06T11:19:33Z

Just took another look at this PR- yes you're right. Sorry about this. Guess there's some work needed to do on the opentelemetry side then.

Edit: thank you for the patching instruction above! I'll try.

…rties-to-req-route-options

lib/request.js

Uzlopak · 2023-08-21T13:00:16Z

I fixed the remaining remarks. It should be ready for merge.

@Eomm PTAL

docs/Reference/Request.md

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>

Eomm

We will need to check these warnings across the repos

Thanks GH code search

…rties-to-req-route-options

mcollina

lgtm

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [fastify](https://www.fastify.dev/) ([source](https://togithub.com/fastify/fastify)) | [`4.22.2` -> `4.23.0`](https://renovatebot.com/diffs/npm/fastify/4.22.2/4.23.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/fastify/4.23.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/fastify/4.23.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/fastify/4.22.2/4.23.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/fastify/4.22.2/4.23.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>fastify/fastify (fastify)</summary> ### [`v4.23.0`](https://togithub.com/fastify/fastify/releases/tag/v4.23.0) [Compare Source](https://togithub.com/fastify/fastify/compare/v4.22.2...v4.23.0) #### What's Changed - test: reduce output of reqIdGenFactory.test.js by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#5012 - feat: Add onListen Hook by [@BrendenInhelder](https://togithub.com/BrendenInhelder) in [fastify/fastify#4899 - docs(readme): avoid line breaks in documentation links by [@antoineneff](https://togithub.com/antoineneff) in [fastify/fastify#5014 - chore(examples): added curly braces to conditions for consistency by [@turnerran](https://togithub.com/turnerran) in [fastify/fastify#5015 - feat: access handler name add properties to req route options by [@cesarvspr](https://togithub.com/cesarvspr) in [fastify/fastify#4470 - chore: Bump the dev-dependencies group with 1 update by [@dependabot](https://togithub.com/dependabot) in [fastify/fastify#5018 - docs: minor improvements by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#5019 - chore(deps): replace tiny-lru with toad-cache by [@kibertoad](https://togithub.com/kibertoad) in [fastify/fastify#4668 - docs(ecosystem.md): Fix wrong entry for ecosystem.md by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#5021 - perf: use `node:` prefix to bypass require.cache call for builtins by [@Fdawgs](https://togithub.com/Fdawgs) in [fastify/fastify#5026 - docs(typo): mistype of monkeypatch by [@Menkveld-24](https://togithub.com/Menkveld-24) in [fastify/fastify#5027 - chore: change website to .dev instead of .io by [@Eomm](https://togithub.com/Eomm) in [fastify/fastify#5028 - chore: add gurgunday and uzlopak as contributors by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#5029 - chore: add a citgm command to customize what we run in CITGM by [@mcollina](https://togithub.com/mcollina) in [fastify/fastify#5030 #### New Contributors - [@BrendenInhelder](https://togithub.com/BrendenInhelder) made their first contribution in [fastify/fastify#4899 - [@antoineneff](https://togithub.com/antoineneff) made their first contribution in [fastify/fastify#5014 - [@turnerran](https://togithub.com/turnerran) made their first contribution in [fastify/fastify#5015 - [@Menkveld-24](https://togithub.com/Menkveld-24) made their first contribution in [fastify/fastify#5027 **Full Changelog**: fastify/fastify@v4.22.2...v4.23.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/tomacheese/telcheck).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [fastify](https://www.fastify.dev/) ([source](https://togithub.com/fastify/fastify)) | [`4.22.2` -> `4.23.0`](https://renovatebot.com/diffs/npm/fastify/4.22.2/4.23.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/fastify/4.23.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/fastify/4.23.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/fastify/4.22.2/4.23.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/fastify/4.22.2/4.23.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>fastify/fastify (fastify)</summary> ### [`v4.23.0`](https://togithub.com/fastify/fastify/releases/tag/v4.23.0) [Compare Source](https://togithub.com/fastify/fastify/compare/v4.22.2...v4.23.0) ##### What's Changed - test: reduce output of reqIdGenFactory.test.js by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#5012 - feat: Add onListen Hook by [@BrendenInhelder](https://togithub.com/BrendenInhelder) in [fastify/fastify#4899 - docs(readme): avoid line breaks in documentation links by [@antoineneff](https://togithub.com/antoineneff) in [fastify/fastify#5014 - chore(examples): added curly braces to conditions for consistency by [@turnerran](https://togithub.com/turnerran) in [fastify/fastify#5015 - feat: access handler name add properties to req route options by [@cesarvspr](https://togithub.com/cesarvspr) in [fastify/fastify#4470 - chore: Bump the dev-dependencies group with 1 update by [@dependabot](https://togithub.com/dependabot) in [fastify/fastify#5018 - docs: minor improvements by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#5019 - chore(deps): replace tiny-lru with toad-cache by [@kibertoad](https://togithub.com/kibertoad) in [fastify/fastify#4668 - docs(ecosystem.md): Fix wrong entry for ecosystem.md by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#5021 - perf: use `node:` prefix to bypass require.cache call for builtins by [@Fdawgs](https://togithub.com/Fdawgs) in [fastify/fastify#5026 - docs(typo): mistype of monkeypatch by [@Menkveld-24](https://togithub.com/Menkveld-24) in [fastify/fastify#5027 - chore: change website to .dev instead of .io by [@Eomm](https://togithub.com/Eomm) in [fastify/fastify#5028 - chore: add gurgunday and uzlopak as contributors by [@Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#5029 - chore: add a citgm command to customize what we run in CITGM by [@mcollina](https://togithub.com/mcollina) in [fastify/fastify#5030 ##### New Contributors - [@BrendenInhelder](https://togithub.com/BrendenInhelder) made their first contribution in [fastify/fastify#4899 - [@antoineneff](https://togithub.com/antoineneff) made their first contribution in [fastify/fastify#5014 - [@turnerran](https://togithub.com/turnerran) made their first contribution in [fastify/fastify#5015 - [@Menkveld-24](https://togithub.com/Menkveld-24) made their first contribution in [fastify/fastify#5027 **Full Changelog**: fastify/fastify@v4.22.2...v4.23.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/redwoodjs/redwood).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

cesarvspr added 3 commits December 15, 2022 02:47

fix: 'Error' is not assignable to parameter of type 'null'

b55808e

chore: add handler, schema and config to req.routeOptions

c8b3e19

rollback not needed change

8adce4d

cesarvspr changed the title ~~(#4439) access handler name add properties to req route options~~ (#4439) access handler name add properties to req route options (WIP) Dec 17, 2022

cesarvspr marked this pull request as draft December 17, 2022 20:50

Eomm reviewed Dec 19, 2022

View reviewed changes

lib/request.js Outdated Show resolved Hide resolved

cesarvspr and others added 2 commits December 19, 2022 16:59

Update lib/request.js

59236db

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>

chore: use deepFreeze

e617fb8

Signed-off-by: cesarvspr <vinicius_spr@hotmail.com>

climba03003 reviewed Dec 20, 2022

View reviewed changes

cesarvspr added 6 commits December 24, 2022 00:38

chore: use Object.defineProperty

7b13dd8

Signed-off-by: cesarvspr <vinicius_spr@hotmail.com>

chore: use defineProperties

5012a19

Signed-off-by: cesarvspr <vinicius_spr@hotmail.com>

chore: add tests

e3b72dc

Signed-off-by: cesarvspr <vinicius_spr@hotmail.com>

fix: test count exceeds plan

61dcac5

Signed-off-by: cesarvspr <vinicius_spr@hotmail.com>

fix: test count exceeds plan

b8e10f8

Signed-off-by: cesarvspr <vinicius_spr@hotmail.com>

fix: add TODO

9cf1318

Signed-off-by: cesarvspr <vinicius_spr@hotmail.com>

cesarvspr marked this pull request as ready for review December 24, 2022 06:57

cesarvspr changed the title ~~(#4439) access handler name add properties to req route options (WIP)~~ (#4439) access handler name add properties to req route options Dec 24, 2022

cesarvspr requested review from Eomm and climba03003 and removed request for Eomm and climba03003 December 24, 2022 07:00

Fdawgs changed the title ~~(#4439) access handler name add properties to req route options~~ feat: access handler name add properties to req route options Jul 2, 2023

Update test/request.deprecated.test.js

3a64cef

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>

Merge branch 'main' into (fastify#4439)-Access-handler-name-add-prope…

5eb061b

…rties-to-req-route-options

Merge branch 'main' into (fastify#4439)-Access-handler-name-add-prope…

c13caa1

…rties-to-req-route-options

Uzlopak reviewed Aug 21, 2023

View reviewed changes

lib/request.js Outdated Show resolved Hide resolved

Uzlopak reviewed Aug 21, 2023

View reviewed changes

lib/request.js Outdated Show resolved Hide resolved

Uzlopak added 2 commits August 21, 2023 14:44

Apply suggestions from code review

3e3d05c

add documentation

8e5602e

Uzlopak requested review from rahmanvivasoft and mcollina August 21, 2023 13:28

add missing documentation

1c1901d

Eomm reviewed Aug 23, 2023

View reviewed changes

docs/Reference/Request.md Outdated Show resolved Hide resolved

Update docs/Reference/Request.md

232cc3e

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>

Eomm approved these changes Aug 24, 2023

View reviewed changes

Merge branch 'main' into (fastify#4439)-Access-handler-name-add-prope…

53ecf4a

…rties-to-req-route-options

mcollina approved these changes Sep 4, 2023

View reviewed changes

mcollina merged commit 59050e5 into fastify:main Sep 4, 2023
28 checks passed

PixnBits mentioned this pull request Sep 18, 2023

Enable OpenTelemetry logging in v6 americanexpress/one-app#1097

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: access handler name add properties to req route options #4470

feat: access handler name add properties to req route options #4470

cesarvspr commented Dec 17, 2022 •

edited by Fdawgs

Eomm left a comment

climba03003 Dec 20, 2022 •

edited

cesarvspr Dec 24, 2022 •

edited

Eomm commented Jun 5, 2023

Songkeys commented Aug 6, 2023

Uzlopak commented Aug 6, 2023

SimenB commented Aug 6, 2023

Songkeys commented Aug 6, 2023

Uzlopak commented Aug 6, 2023

Uzlopak commented Aug 6, 2023

Songkeys commented Aug 6, 2023 •

edited

Uzlopak commented Aug 21, 2023 •

edited

Eomm left a comment

mcollina left a comment

feat: access handler name add properties to req route options #4470

feat: access handler name add properties to req route options #4470

Conversation

cesarvspr commented Dec 17, 2022 • edited by Fdawgs

Checklist

Eomm left a comment

Choose a reason for hiding this comment

climba03003 Dec 20, 2022 • edited

Choose a reason for hiding this comment

cesarvspr Dec 24, 2022 • edited

Choose a reason for hiding this comment

Eomm commented Jun 5, 2023

Songkeys commented Aug 6, 2023

Uzlopak commented Aug 6, 2023

SimenB commented Aug 6, 2023

Songkeys commented Aug 6, 2023

Uzlopak commented Aug 6, 2023

Uzlopak commented Aug 6, 2023

Songkeys commented Aug 6, 2023 • edited

Uzlopak commented Aug 21, 2023 • edited

Eomm left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

cesarvspr commented Dec 17, 2022 •

edited by Fdawgs

climba03003 Dec 20, 2022 •

edited

cesarvspr Dec 24, 2022 •

edited

Songkeys commented Aug 6, 2023 •

edited

Uzlopak commented Aug 21, 2023 •

edited