Skip to content

http: correctly translate HTTP method #52701

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

http: correctly translate HTTP method #52701

wants to merge 1 commit into from

Conversation

ShogunPanda
Copy link
Contributor

This PR correctly maps the HTTP method reported by llhttp (which is an index of an array) to the right array.

Fixes #51562.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http Issues or PRs related to the http subsystem. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. needs-ci PRs that need a full CI run. labels Apr 26, 2024
@ShogunPanda ShogunPanda mentioned this pull request Apr 26, 2024
Closed
mcollina
mcollina approved these changes Apr 26, 2024
View reviewed changes
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

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 26, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 26, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58729/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58742/

@bricss
Copy link
Contributor

bricss commented Apr 27, 2024

I wonder if HTTP_KNOWN_METHODS(V) should be extended as well to make constants consistent?

@mcollina
Copy link
Member

@ShogunPanda C++ linting is failing

@ShogunPanda ShogunPanda force-pushed the http-search-all-methods branch from 99b3d19 to 71d3c02 Compare May 3, 2024 11:35
@ShogunPanda
Copy link
Contributor Author

@mcollina Fixed.

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 3, 2024
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels May 3, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 3, 2024

Failed to start CI
   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/8938415767

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58892/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58906/

jasnell pushed a commit that referenced this pull request May 4, 2024
@ShogunPanda @jasnell
http: correctly translate HTTP method
65c8380 
PR-URL: #52701
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented May 4, 2024

Landed in 65c8380

@jasnell jasnell closed this May 4, 2024
@ShogunPanda ShogunPanda deleted the http-search-all-methods branch May 5, 2024 05:32
Ch3nYuY pushed a commit to Ch3nYuY/node that referenced this pull request May 8, 2024
@ShogunPanda
http: correctly translate HTTP method
70a4c31 
PR-URL: nodejs#52701
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request May 8, 2024
@ShogunPanda @targos
http: correctly translate HTTP method
ad9c4bd 
PR-URL: #52701
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request May 13, 2024
Merged
eliphazbouye pushed a commit to eliphazbouye/node that referenced this pull request Jun 20, 2024
@ShogunPanda @eliphazbouye
http: correctly translate HTTP method
8726a30 
PR-URL: nodejs#52701
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@ShogunPanda @bmeck
http: correctly translate HTTP method
953f356 
PR-URL: nodejs#52701
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos added dont-land-on-v18.x dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels Sep 21, 2024
@kapouer
Copy link
Contributor

kapouer commented May 19, 2025 •
edited
Loading

@jasnell Shouldn't this be backported to 20.x, since it now includes a more recent llhttp ?

@marco-ippolito marco-ippolito removed the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Jun 5, 2025
marco-ippolito pushed a commit that referenced this pull request Jun 5, 2025
@ShogunPanda @marco-ippolito
http: correctly translate HTTP method
9623f48 
PR-URL: #52701
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jun 5, 2025
@ShogunPanda @marco-ippolito
http: correctly translate HTTP method
8c77f73 
PR-URL: #52701
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@marco-ippolito marco-ippolito mentioned this pull request Jun 5, 2025
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 10, 2025
@ShogunPanda @marco-ippolito
http: correctly translate HTTP method
8670ed4 
PR-URL: #52701
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jun 11, 2025
@ShogunPanda @marco-ippolito
http: correctly translate HTTP method
0556f54 
PR-URL: #52701
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
n0v1
n0v1 reviewed Jun 26, 2025
View reviewed changes
test/parallel/test-http-server-method.query.js
Copy link

Choose a reason for hiding this comment

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

nit: Is the dot between method and query intended in the file name or should it be a hyphen instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@panva panva panva approved these changes

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@ronag ronag ronag approved these changes

@marco-ippolito marco-ippolito marco-ippolito approved these changes

+1 more reviewer

@n0v1 n0v1 n0v1 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for 'QUERY' method