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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix preParsing is undefined will error #4359

Closed
wants to merge 1 commit into from

Conversation

fuergaosi233
Copy link

if request[kRouteContext].preParsing is error, the framework will error.
image

Checklist

I don't think this change needs testing 馃

if `request[kRouteContext].preParsing` is error, the framework will error.
@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 19, 2022

Well we thought we fixed that issue already. So please provide a test.

@fuergaosi233
Copy link
Author

I can only find this problem in other projects, but I don't know what causes this to be undefined, so should I provide you with my local project?

@leandroandrade
Copy link
Contributor

If you provide a simple execution example will help to investigate this issue.

@metcoder95
Copy link
Member

metcoder95 commented Oct 21, 2022

Would be extremely helpful to have a reproduction from your side. I tried with a very basic case and the behavior was expected, see the following snippet:

const fastify = require('./fastify')({ logger: true })

fastify.get(
  '/undefined',
  {
    preParsing: undefined // same as none hook registered
  },
  async (request, reply) => {
    reply.send('hello world')
  }
)

fastify.get(
  '/null',
  {
    preParsing: null // throws FST_ERR_HOOK_INVALID_HANDLER
  },
  async (request, reply) => {
    reply.send('hello world')
  }
)

fastify
  .inject({
    method: 'GET',
    url: '/undefined'
  })
  .then(res => {
    console.log(res.raw.req.url)
    console.log(res.body) // hello world
  })

Though, I was able to spot an error when trying to register lifecycle/application hooks by using Fastify#addHook method, as we're not checking non-existent values, and goes directly against the Object#constructor method, see the following snippet:

const fastify = require('./fastify')({ logger: true })

fastify.addHook('onReady', null)
fastify.addHook('onResponse', undefined)

Any of those throws the following error:

/Users/metcoder/Documents/Workspace/MetCoder/Fastify/fastify.js:577
      if (fn.constructor.name === 'AsyncFunction' && fn.length === 3) {
             ^

TypeError: Cannot read properties of null (reading 'constructor')
    at Object.addHook (/Users/metcoder/Documents/Workspace/MetCoder/Fastify/fastify.js:577:14)
    at Object.<anonymous> (/Users/metcoder/Documents/Workspace/MetCoder/Fastify/playground.js:4:9)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
    at node:internal/main/run_main_module:17:47

I'll write a proper error reproduction + tests to open a PR for the latest error

@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 Oct 24, 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.

None yet

5 participants