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

http: refactor to use validateHeaderName #46143

Merged

Conversation

deokjinkim
Copy link
Contributor

Remove duplicate implementation by using validateHeaderName.

Remove duplicate implementation by using validateHeaderName.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Jan 9, 2023
if (typeof field !== 'string' || !field || !checkIsHttpToken(field)) {
throw new ERR_INVALID_HTTP_TOKEN('Trailer name', field);
}
validateHeaderName(field);
Copy link
Contributor

Choose a reason for hiding this comment

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

This function uses 'header name' instead of 'trailer name' in the error message which is probably not as helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. Unless you make the error message customizable we cannot make this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand your concern. So added label parameter to validateHeaderName. PTAL.

To customize error message, add label parameter
@@ -628,9 +628,9 @@ function matchHeader(self, state, field, value) {
}
}

const validateHeaderName = hideStackFrames((name) => {
const validateHeaderName = hideStackFrames((name, label = 'Header name') => {
Copy link
Member

Choose a reason for hiding this comment

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

This does not work the way you want, as it will be undefined if the second parameter is explicitly passed as undefined. Move it to a || conditional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied your suggestion. Thank you for your help.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW it's not correct, the default value is used no matter if undefined is passed explicitly or not.

((param = 'default value') => console.log(param))(); // 'default value'
((param = 'default value') => console.log(param))(undefined); // 'default value'
((param = 'default value') => console.log(param))(null); // null

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 mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 11, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 11, 2023
@nodejs-github-bot
Copy link
Collaborator

doc/api/http.md Outdated Show resolved Hide resolved
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
@mcollina mcollina added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Jan 13, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2023
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 17, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 17, 2023
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 17, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 17, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/46143
✔  Done loading data for nodejs/node/pull/46143
----------------------------------- PR info ------------------------------------
Title      http: refactor to use `validateHeaderName` (#46143)
Author     Deokjin Kim  (@deokjinkim)
Branch     deokjinkim:230110_http_validate_header_name -> nodejs:main
Labels     http, author ready
Commits    4
 - http: refactor to use `validateHeaderName`
 - add `label` parameter to validateHeaderName
 - use `||` instead of default parameter
 - describe default value of label as string
Committers 2
 - Deokjin Kim 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/46143
Reviewed-By: Matteo Collina 
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46143
Reviewed-By: Matteo Collina 
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 09 Jan 2023 15:27:15 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/46143#pullrequestreview-1243743249
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/46143#pullrequestreview-1251349454
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-01-17T11:17:12Z: https://ci.nodejs.org/job/node-test-pull-request/49007/
- Querying data for job/node-test-pull-request/49007/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 46143
From https://github.com/nodejs/node
 * branch                  refs/pull/46143/merge -> FETCH_HEAD
✔  Fetched commits as 15d673dfbe02..254670133f08
--------------------------------------------------------------------------------
Auto-merging lib/_http_outgoing.js
[main b408cb431e] http: refactor to use `validateHeaderName`
 Author: Deokjin Kim 
 Date: Tue Jan 10 00:25:41 2023 +0900
 2 files changed, 1 insertion(+), 4 deletions(-)
Auto-merging doc/api/http.md
Auto-merging lib/_http_outgoing.js
[main d9ccab9f46] add `label` parameter to validateHeaderName
 Author: Deokjin Kim 
 Date: Tue Jan 10 08:52:34 2023 +0900
 3 files changed, 10 insertions(+), 4 deletions(-)
Auto-merging lib/_http_outgoing.js
[main 03b6d46f31] use `||` instead of default parameter
 Author: Deokjin Kim 
 Date: Tue Jan 10 23:05:34 2023 +0900
 1 file changed, 2 insertions(+), 2 deletions(-)
Auto-merging doc/api/http.md
[main 431d56ea02] describe default value of label as string
 Author: Deokjin Kim 
 Date: Thu Jan 12 08:11:46 2023 +0900
 1 file changed, 1 insertion(+), 1 deletion(-)
   ✔  Patches applied
There are 4 commits in the PR. Attempting autorebase.
Rebasing (2/8)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
http: refactor to use validateHeaderName

Remove duplicate implementation by using validateHeaderName.

PR-URL: #46143
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Antoine du Hamel duhamelantoine1995@gmail.com

[detached HEAD 4b16669dda] http: refactor to use validateHeaderName
Author: Deokjin Kim deokjin81.kim@gmail.com
Date: Tue Jan 10 00:25:41 2023 +0900
2 files changed, 1 insertion(+), 4 deletions(-)
Rebasing (3/8)
Rebasing (4/8)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
add label parameter to validateHeaderName

To customize error message, add label parameter

PR-URL: #46143
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Antoine du Hamel duhamelantoine1995@gmail.com

[detached HEAD 808c60209d] add label parameter to validateHeaderName
Author: Deokjin Kim deokjin81.kim@gmail.com
Date: Tue Jan 10 08:52:34 2023 +0900
3 files changed, 10 insertions(+), 4 deletions(-)
Rebasing (5/8)
Rebasing (6/8)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
use || instead of default parameter

PR-URL: #46143
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Antoine du Hamel duhamelantoine1995@gmail.com

[detached HEAD c796450c06] use || instead of default parameter
Author: Deokjin Kim deokjin81.kim@gmail.com
Date: Tue Jan 10 23:05:34 2023 +0900
1 file changed, 2 insertions(+), 2 deletions(-)
Rebasing (7/8)
Rebasing (8/8)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
describe default value of label as string

Co-authored-by: mscdex mscdex@users.noreply.github.com
PR-URL: #46143
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Antoine du Hamel duhamelantoine1995@gmail.com

[detached HEAD 72a5c8b667] describe default value of label as string
Author: Deokjin Kim deokjin81.kim@gmail.com
Date: Thu Jan 12 08:11:46 2023 +0900
1 file changed, 1 insertion(+), 1 deletion(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/3939661314

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 17, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 17, 2023
@nodejs-github-bot nodejs-github-bot merged commit f461a4c into nodejs:main Jan 17, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in f461a4c

RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Jan 17, 2023
Remove duplicate implementation by using validateHeaderName.

PR-URL: nodejs#46143
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
Remove duplicate implementation by using validateHeaderName.

PR-URL: #46143
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
Remove duplicate implementation by using validateHeaderName.

PR-URL: #46143
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
Remove duplicate implementation by using validateHeaderName.

PR-URL: #46143
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants