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

inject doesn't like the new HTTP QUERY Method #309

Closed
2 tasks done
andokai opened this issue Oct 25, 2024 · 2 comments · Fixed by #310
Closed
2 tasks done

inject doesn't like the new HTTP QUERY Method #309

andokai opened this issue Oct 25, 2024 · 2 comments · Fixed by #310

Comments

@andokai
Copy link
Contributor

andokai commented Oct 25, 2024

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Fastify version

5.0.0

Plugin version

No response

Node.js version

22.10

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

15.0.1

Description

With the PR added to find-my-way delvedor/find-my-way#380, Fastify can now support the QUERY method. However, testing the route using inject throws an error as the method is not in the hard coded list here.

if(!((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((data18 === "ACL") || (data18 === "BIND")) || (data18 === "CHECKOUT")) || (data18 === "CONNECT")) || (data18 === "COPY")) || (data18 === "DELETE")) || (data18 === "GET")) || (data18 === "HEAD")) || (data18 === "LINK")) || (data18 === "LOCK")) || (data18 === "M-SEARCH")) || (data18 === "MERGE")) || (data18 === "MKACTIVITY")) || (data18 === "MKCALENDAR")) || (data18 === "MKCOL")) || (data18 === "MOVE")) || (data18 === "NOTIFY")) || (data18 === "OPTIONS")) || (data18 === "PATCH")) || (data18 === "POST")) || (data18 === "PROPFIND")) || (data18 === "PROPPATCH")) || (data18 === "PURGE")) || (data18 === "PUT")) || (data18 === "REBIND")) || (data18 === "REPORT")) || (data18 === "SEARCH")) || (data18 === "SOURCE")) || (data18 === "SUBSCRIBE")) || (data18 === "TRACE")) || (data18 === "UNBIND")) || (data18 === "UNLINK")) || (data18 === "UNLOCK")) || (data18 === "UNSUBSCRIBE")) || (data18 === "acl")) || (data18 === "bind")) || (data18 === "checkout")) || (data18 === "connect")) || (data18 === "copy")) || (data18 === "delete")) || (data18 === "get")) || (data18 === "head")) || (data18 === "link")) || (data18 === "lock")) || (data18 === "m-search")) || (data18 === "merge")) || (data18 === "mkactivity")) || (data18 === "mkcalendar")) || (data18 === "mkcol")) || (data18 === "move")) || (data18 === "notify")) || (data18 === "options")) || (data18 === "patch")) || (data18 === "post")) || (data18 === "propfind")) || (data18 === "proppatch")) || (data18 === "purge")) || (data18 === "put")) || (data18 === "rebind")) || (data18 === "report")) || (data18 === "search")) || (data18 === "source")) || (data18 === "subscribe")) || (data18 === "trace")) || (data18 === "unbind")) || (data18 === "unlink")) || (data18 === "unlock")) || (data18 === "unsubscribe"))){

If I add QUERY to that line then the tests succeed. I'm happy to submit a PR with this change but wanted to confirm that there wasn't anything else that I need to bear in mind.

For example there's an array of what appear to be the most common HTTP methods here.

const httpMethods = [

Should I be adding 'query' to that array?

Link to code that reproduces the bug

No response

Expected Behavior

No response

@Eomm
Copy link
Member

Eomm commented Oct 25, 2024

Should I be adding 'query' to that array?

No, that code does not affect light-my-request/lib/config-validator.js, it generates the req.query() method shortcut (that we are missing and still it is an issue)

The real issue is here:

method: { type: 'string', enum: http.METHODS.concat(http.METHODS.map(toLowerCase)) },

We should:

  1. force the light-my-request http method list
  2. AND/OR force the script to throw if a maintainer build the config-validator.js file using nodejs != 22

@mcollina
Copy link
Member

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants