http: refactor to use `validateHeaderName` by deokjinkim · Pull Request #46143 · nodejs/node

deokjinkim · 2023-01-09T15:27:15Z

Remove duplicate implementation by using validateHeaderName.

nodejs-github-bot · 2023-01-09T15:27:20Z

Review requested:

@nodejs/http
@nodejs/net

mscdex · 2023-01-09T16:06:37Z

lib/_http_outgoing.js

-    if (typeof field !== 'string' || !field || !checkIsHttpToken(field)) {
-      throw new ERR_INVALID_HTTP_TOKEN('Trailer name', field);
-    }
+    validateHeaderName(field);


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

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

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

To customize error message, add label parameter

mcollina · 2023-01-10T08:57:30Z

lib/_http_outgoing.js

 }

-const validateHeaderName = hideStackFrames((name) => {
+const validateHeaderName = hideStackFrames((name, label = 'Header name') => {


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.

Applied your suggestion. Thank you for your help.

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

mcollina

lgtm

nodejs-github-bot · 2023-01-11T11:36:37Z

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

doc/api/http.md

Co-authored-by: mscdex <mscdex@users.noreply.github.com>

nodejs-github-bot · 2023-01-13T15:34:09Z

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

nodejs-github-bot · 2023-01-17T11:17:12Z

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

nodejs-github-bot · 2023-01-17T13:24:04Z

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

nodejs-github-bot · 2023-01-17T13:40:40Z

Landed in f461a4c

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>

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>

http: refactor to use validateHeaderName

d8ab874

Remove duplicate implementation by using validateHeaderName.

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

mscdex reviewed Jan 9, 2023

View reviewed changes

add label parameter to validateHeaderName

a23f8f6

To customize error message, add label parameter

mcollina reviewed Jan 10, 2023

View reviewed changes

use || instead of default parameter

b1681f7

mcollina approved these changes Jan 11, 2023

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 11, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 11, 2023

mscdex reviewed Jan 11, 2023

View reviewed changes

doc/api/http.md Outdated Show resolved Hide resolved

describe default value of label as string

2546701

Co-authored-by: mscdex <mscdex@users.noreply.github.com>

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 bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2023

aduh95 approved these changes Jan 17, 2023

View reviewed changes

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 bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 17, 2023

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 17, 2023

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 removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 17, 2023

nodejs-github-bot merged commit f461a4c into nodejs:main Jan 17, 2023

RafaelGSS mentioned this pull request Jan 20, 2023

v19.5.0 proposal #46286

Merged

juanarbol mentioned this pull request Jan 28, 2023

V18.14.0 proposal #46396

Merged

Uh oh!

Conversation

deokjinkim commented Jan 9, 2023

Uh oh!

nodejs-github-bot commented Jan 9, 2023

Uh oh!

mscdex Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

ShogunPanda Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

deokjinkim Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

mcollina Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

deokjinkim Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

aduh95 Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jan 11, 2023

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 13, 2023

Uh oh!

nodejs-github-bot commented Jan 17, 2023

Uh oh!

nodejs-github-bot commented Jan 17, 2023

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

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

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

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

Successfully rebased and updated refs/heads/main.

Uh oh!

nodejs-github-bot commented Jan 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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

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

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

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