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

http2: pass incoming set-cookie header as array #21360

Closed
wants to merge 1 commit into from
Closed

http2: pass incoming set-cookie header as array #21360

wants to merge 1 commit into from

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Jun 15, 2018

Incoming set-cookie headers should be passed to user as array like in http module - even for single occurrence.

Besides improving compatibility between http and http2 it avoids the need to check if the type is an array or not in user code.

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

Regarding doc: If this PR is accepted #21296 should be updated accordingly - or doc needs to be adapted in this PR if #21296 landed meanwhile.

@ryzokuken
Copy link
Contributor

/cc @nodejs/http2

@ryzokuken ryzokuken added the http2 Issues or PRs related to the http2 subsystem. label Jun 16, 2018
@ryzokuken
Copy link
Contributor

@maclover7 @nodejs/github-bot any idea why neither the CI nor labelling script in the bot was triggered for this one? It seems rather strange.

@joyeecheung
Copy link
Member

@ryzokuken The bot is down now: nodejs/build#1353

@BridgeAR
Copy link
Member

@nodejs/http2 PTAL

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

LGTM but I would prefer if we could just tack this test on to one of the existing headers tests rather than creating a new one for such a small behaviour.

@Flarna
Copy link
Member Author

Flarna commented Jun 19, 2018

@apapirovski ok, will try to move the test. I have chosen the easiest way to reproduce this 😁
Done: Removed new test, added checks in existing test-http2-util-headers-list.js

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

LGTM with a small nit.

// Tests the internal utility function that is used to prepare headers
// to pass to the internal binding layer.
// Tests the internal utility functions that are used to prepare headers
// to pass to the internal binding layer and to build a header object
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a dot at the end of this sentence, please?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do that soon. As #21296 landed I have to update doc for set-cookie also.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@apapirovski
Copy link
Member

@apapirovski apapirovski added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 25, 2018
@apapirovski
Copy link
Member

Weird failures last time. CI: https://ci.nodejs.org/job/node-test-pull-request/15606/

@Flarna
Copy link
Member Author

Flarna commented Jun 25, 2018

@apapirovski Looks like CI got confused because I did a git rebase. It tries to checkout commit 05c0f78b79cb7682c51e49af57341baa3f8d94db which was the last commit before the rebase but it should checkout ada2076d4ef919bc6d2981c39174c28a826fa971.

Applies at least to some of the CI jobs.

@apapirovski
Copy link
Member

Was there a merge commit by any chance? Weird that this is happening. Could you maybe squash?

Incoming set-cookie headers should be passed to user as array like in
http module.

Besides improving compatibility between http and http2 it avoids the
need to check if the type is an array or not in user code.
@Flarna
Copy link
Member Author

Flarna commented Jun 26, 2018

No, it was a "normal" rebase. Rebased again and squashed.

@Flarna
Copy link
Member Author

Flarna commented Jul 3, 2018

Is there anything I should add here?

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 Jul 5, 2018

@mcollina
Copy link
Member

mcollina commented Jul 5, 2018

Landed in d4966a1

@mcollina mcollina closed this Jul 5, 2018
mcollina pushed a commit that referenced this pull request Jul 5, 2018
Incoming set-cookie headers should be passed to user as array like in
http module.

Besides improving compatibility between http and http2 it avoids the
need to check if the type is an array or not in user code.

PR-URL: #21360
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@Flarna Flarna deleted the http2_set-cookie_array branch July 5, 2018 13:45
targos pushed a commit that referenced this pull request Jul 6, 2018
Incoming set-cookie headers should be passed to user as array like in
http module.

Besides improving compatibility between http and http2 it avoids the
need to check if the type is an array or not in user code.

PR-URL: #21360
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@targos targos mentioned this pull request Jul 17, 2018
kjin pushed a commit to kjin/node that referenced this pull request Aug 23, 2018
Incoming set-cookie headers should be passed to user as array like in
http module.

Besides improving compatibility between http and http2 it avoids the
need to check if the type is an array or not in user code.

PR-URL: nodejs#21360
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Sep 25, 2018
Incoming set-cookie headers should be passed to user as array like in
http module.

Besides improving compatibility between http and http2 it avoids the
need to check if the type is an array or not in user code.

PR-URL: nodejs#21360
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
Incoming set-cookie headers should be passed to user as array like in
http module.

Besides improving compatibility between http and http2 it avoids the
need to check if the type is an array or not in user code.

PR-URL: nodejs#21360
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
Incoming set-cookie headers should be passed to user as array like in
http module.

Besides improving compatibility between http and http2 it avoids the
need to check if the type is an array or not in user code.

Backport-PR-URL: #22850
PR-URL: #21360
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
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. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants