Skip to content

test: http2 util passing array in te header#16246

Closed
trivikr wants to merge 1 commit intonodejs:masterfrom
trivikr:test-http2-util-headers-list
Closed

test: http2 util passing array in te header#16246
trivikr wants to merge 1 commit intonodejs:masterfrom
trivikr:test-http2-util-headers-list

Conversation

@trivikr
Copy link
Member

@trivikr trivikr commented Oct 17, 2017

This adds a test case in which array with two values is passed to param value in isIllegalConnectionSpecificHeader:

node/lib/internal/http2/util.js

Line 363 in 212de3c

const val = Array.isArray(value) ? value.join(', ') : value;

Refs: #14985

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test, http2

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 17, 2017
@mscdex mscdex added the http2 Issues or PRs related to the http2 subsystem. label Oct 17, 2017
mcollina
mcollina approved these changes Oct 17, 2017
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

@mcollina
Copy link
Member

mcollina commented Oct 17, 2017

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

@apapirovski
Copy link
Contributor

apapirovski commented Oct 17, 2017 •
edited
Loading

Just a note for future reference if someone wants to take it on, the pathway this is testing could actually be refactored a bit:

node/lib/internal/http2/util.js

Lines 363 to 364 in ff747e3

const val = Array.isArray(value) ? value.join(', ') : value;
return val !== 'trailers';

value will never be an array with length < 2 because of

node/lib/internal/http2/util.js

Lines 405 to 416 in ff747e3

const isArray = Array.isArray(value);
if (isArray) {
switch (value.length) {
case 0:
continue;
case 1:
value = String(value[0]);
break;
default:
if (kSingleValueHeaders.has(key))
return new errors.Error('ERR_HTTP2_HEADER_SINGLE_VALUE', key);
}

so it could just be rewritten as:

return value !== 'trailers';

@trivikr
Copy link
Member Author

trivikr commented Oct 17, 2017

Thanks @apapirovski, I've updated the commit to add toString() to cover both cases as follows:

return value.toString() !== 'trailers';

Commit is at trivikr@596df98
However, I'm not able to see it in this PR https://github.com/nodejs/node/pull/16246/files
Could this be because CI is running on older commit?

@trivikr trivikr force-pushed the test-http2-util-headers-list branch from 7b2c4ef to 596df98 Compare October 17, 2017 16:22
apapirovski
apapirovski reviewed Oct 17, 2017
View reviewed changes
lib/internal/http2/util.js Outdated
Copy link
Contributor

@apapirovski apapirovski Oct 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

toString() isn't necessary here (and is bad for perf). If an array slips through then it won't match anyway (because it has at least two items in it). Should just be value !== 'trailers'.

Copy link
Member Author

@trivikr trivikr Oct 17, 2017

Choose a reason for hiding this comment

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

removed toString() in the updated commit

@trivikr
test: http2 util passing array in te header
9a03c2a 
This adds a test case in which array with two values is passed to
param value in isIllegalConnectionSpecificHeader

Refs: nodejs#14985
@trivikr trivikr force-pushed the test-http2-util-headers-list branch from 596df98 to 9a03c2a Compare October 17, 2017 17:59
apapirovski
apapirovski approved these changes Oct 17, 2017
View reviewed changes
Copy link
Contributor

@apapirovski apapirovski left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 👍

@apapirovski
Copy link
Contributor

apapirovski commented Oct 18, 2017

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

apapirovski pushed a commit to apapirovski/node that referenced this pull request Oct 19, 2017
@trivikr @apapirovski
http2: simplify te header validation, add test
1fd662c 
This simplifies validation of te header and adds a test case
in which array with two values is passed to param value in
isIllegalConnectionSpecificHeader

PR-URL: nodejs#16246
Refs: nodejs#14985
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@apapirovski
Copy link
Contributor

apapirovski commented Oct 19, 2017

Landed in 1fd662c

@trivikr trivikr mentioned this pull request Oct 19, 2017
Closed
@trivikr trivikr deleted the test-http2-util-headers-list branch October 22, 2017 21:56
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
@trivikr @MylesBorins
http2: simplify te header validation, add test
25ef372 
This simplifies validation of te header and adds a test case
in which array with two values is passed to param value in
isIllegalConnectionSpecificHeader

PR-URL: #16246
Refs: #14985
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@trivikr @addaleax
http2: simplify te header validation, add test
e851661 
This simplifies validation of te header and adds a test case
in which array with two values is passed to param value in
isIllegalConnectionSpecificHeader

PR-URL: nodejs/node#16246
Refs: nodejs/node#14985
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@trivikr @addaleax
http2: simplify te header validation, add test
49cbad1 
This simplifies validation of te header and adds a test case
in which array with two values is passed to param value in
isIllegalConnectionSpecificHeader

PR-URL: nodejs/node#16246
Refs: nodejs/node#14985
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
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

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

+1 more reviewer

@apapirovski apapirovski apapirovski approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

http2 Issues or PRs related to the http2 subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@trivikr @mcollina @apapirovski @jasnell @cjihrig @mscdex @MylesBorins @nodejs-github-bot