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

Validate and throws a custom error when attempting to register invalid hook functions #4332

Merged
merged 6 commits into from Oct 17, 2022
Merged

Validate and throws a custom error when attempting to register invalid hook functions #4332

merged 6 commits into from Oct 17, 2022

Conversation

jhhom
Copy link
Contributor

@jhhom jhhom commented Oct 11, 2022

Handles this issue, a hook is invalid if it is not of type function because it needs to bind with the fastify instance.

This PR will add validation for the hook functions passed in the options argument of route registration functions.

Checklist

Copy link
Contributor

@cesarvspr cesarvspr left a comment

Choose a reason for hiding this comment

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

lgtm

lib/route.js Outdated Show resolved Hide resolved
lib/route.js Outdated Show resolved Hide resolved
Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

Thanks for the pr!
Can you use/create a custom error like we are doing for every other Fastify custom error? See here.
Thanks!

Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

It seems that we have this error already:

fastify/lib/errors.js

Lines 98 to 103 in acba25a

FST_ERR_HOOK_INVALID_HANDLER: createError(
'FST_ERR_HOOK_INVALID_HANDLER',
'The hook callback must be a function',
500,
TypeError
),

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

@jhhom
Copy link
Contributor Author

jhhom commented Oct 14, 2022

@delvedor I think the error message I added (e.g "onSend hook should be a function, instead got undefined") can be more informative than the error message in the existing error "The hook callback must be a function", it also provides the following additional information which may helps in debugging:

  • which hook is invalid
  • the actual type passed

In my latest commit, I modified the error message of the existing error instead.
Just that I'm not sure if modifying the error message of the existing error might have any backwards-compatibility issues. I think its probably not an issue because error checking should be done using the error code.

@delvedor
Copy link
Member

I see your point, then I'll suggest to update the existing error message :)

Just that I'm not sure if modifying the error message of the existing error might have any backwards-compatibility issues. I think its probably not an issue because error checking should be done using the error code.

This is a valid point, but we have error codes for a reason. Error messages are the human-friendly version of them which is subjected to change.

@jhhom
Copy link
Contributor Author

jhhom commented Oct 15, 2022

I see your point, then I'll suggest to update the existing error message :)

the existing error message is updated in my last commit 👍

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

@mcollina
Copy link
Member

@delvedor PTAL

@Eomm Eomm added the bugfix Issue or PR that should land as semver patch label Oct 15, 2022
Copy link
Contributor

@cesarvspr cesarvspr left a comment

Choose a reason for hiding this comment

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

Nice

Copy link
Member

@delvedor delvedor 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 d6e0543 into fastify:main Oct 17, 2022
@jhhom jhhom deleted the throw-error-on-invalid-hook-functions branch October 17, 2022 09:31
@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 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bugfix Issue or PR that should land as semver patch
Projects
None yet
7 participants