Skip to content

Get all headers 3992 - #170

Closed
iankronquist wants to merge 2 commits into
nodejs:v0.12from
iankronquist:getAllHeaders-3992
Closed

Get all headers 3992#170
iankronquist wants to merge 2 commits into
nodejs:v0.12from
iankronquist:getAllHeaders-3992

Conversation

@iankronquist

Copy link
Copy Markdown
Contributor

Thanks @chrisdickinson for the pointer to this bug.

@iankronquist

Copy link
Copy Markdown
ContributorAuthor

Github doesn't automatically link to issues in the node.js repo. Here's a link to the issue in question:
nodejs/node-v0.x-archive#3992

Comment threaddoc/api/http.markdown Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think there might be two too many newlines here.

@iankronquist

Copy link
Copy Markdown
ContributorAuthor

@chrisdickinson Fixed documentation formatting

Comment threadlib/_http_outgoing.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be nice to return a copy of headers instead of the full object -- something like util.extend({}, this._headers).

@iankronquist

Copy link
Copy Markdown
ContributorAuthor

@chrisdickinson Fixed according to your advice.

Comment threaddoc/api/http.markdown Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should probably be something like:

Returns the headers which have already been queued but not yet sent to the client.

Test a getAllHeaders function. It should return an object representing
the headers in a request.
@iankronquist

Copy link
Copy Markdown
ContributorAuthor

I made changes according to @phpnode's and @bnoordhuis' comments. I then rebased to keep up with v0.12. r?

@chrisdickinson

Copy link
Copy Markdown
Contributor

Apologies for dragging my feet on this a bit: this LGTM. If there are no objections I'll merge tomorrow afternoon (by 5PM PST).

@iankronquist

Copy link
Copy Markdown
ContributorAuthor

@chrisdickinson No worries, I'm glad I could pitch in.

@iankronquist

Copy link
Copy Markdown
ContributorAuthor

@chrisdickinson Can this be merged?

@iankronquist

Copy link
Copy Markdown
ContributorAuthor

ping @chrisdickinson

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for the late comment, but wouldn't this omit the added-by-default headers Date and Connection?

@iankronquist

Copy link
Copy Markdown
ContributorAuthor

@chrisdickinson updated according to your comments.

@iankronquist

Copy link
Copy Markdown
ContributorAuthor

ping @chrisdickinson

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

varheaders;varrando=Math.random();varexpected=util._extend({},{'X-Random-Thing': rando,/* other values */});varserver=http.createServer(function(req,res){res.setHeader('X-Random-Thing',rando);headers=res.getAllHeaders();res.end('hello');assert.strictEqual(res.getAllHeaders(),null);});server.listen(common.PORT,function(){http.get({port: common.PORT},function(resp){assert.deepEqual(response.headers,expected);server.close();});});

@rvagg

Copy link
Copy Markdown
Member

@chrisdickinson do you require further action on this? Is your last comment a request for a test case along the lines of your code block? @iankronquist if this was to be merged it'd probably need to be a single commit so you might want to squash. The license block should also be removed from the test file and it now needs to go in to test/parallel/.

@chrisdickinson

Copy link
Copy Markdown
Contributor

Yes, that last comment is the test case that should be added and made to pass for this to be able to go in. Re: commits: I don't mind squashing those myself on merge.

@iankronquist

Copy link
Copy Markdown
ContributorAuthor

@rvagg I'll go ahead and do that.

Users should not have to access private attributes like res._headers in
order to get all the headers in a request. Define a method getAllHeaders
to return an object which is a copy of re._headers
Fixes issue nodejs#3992
@iankronquist

Copy link
Copy Markdown
ContributorAuthor

... and squashed like a bug. Let me know if you need anything else cleaned up.

eti-p-doray pushed a commit to eti-p-doray/node that referenced this pull request May 28, 2024
syg pushed a commit to syg/node that referenced this pull request Jun 20, 2024
eti-p-doray pushed a commit to eti-p-doray/node that referenced this pull request Aug 28, 2024
syg pushed a commit to syg/node that referenced this pull request May 5, 2025
bakkot pushed a commit to bakkot/node that referenced this pull request May 11, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

httpIssues or PRs related to the http subsystem.semver-minorPRs that contain new features and should be released in the next minor version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@iankronquist@chrisdickinson@rvagg@Fishrock123@vkurchatkin@bnoordhuis@phpnode@piscisaureus@brendanashworth