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: add setDefaultHeaders option to http.request #56112

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pimterry
Copy link
Member

@pimterry pimterry commented Dec 2, 2024

When sending an HTTP request, for some use cases you want full control of the request headers: controlling the raw format directly and setting only the headers provided, with no other headers automatically included whatsoever.

That is not currently possible, because:

  • The only way to disable various default headers like Connection & Transfer-Encoding is to call req.removeHeader for each of them individually on a request instance, before the headers are sent. Otherwise these are set automatically, even if you pass a raw header array.
  • The only way to set raw headers (i.e. passing a header array instead of an object, to directly control order, casing & duplicate handling etc) is by passing them upfront in the http.request() method, and when you do so the headers are passed to _storeHeader and serialized into _header immediately, meaning you can't call removeHeader afterwards.

That means these two use cases (removing default headers & setting raw header formats) are mutually exclusive, which isn't great since they're closely related.

This PR fixes that by adding a setDefaultHeaders option to http.request, defaulting to true. If you pass setDefaultHeaders: false then you take responsibility for providing all required headers and Node gets out of the way and gives you full control.

With this, you can now control all headers, in raw format, with:

http.request({
  /* ... */
  setDefaultHeaders: false,
  headers: [/* raw header array */]
})

Alternative ideas I've avoided, but might be interesting:

  • Adding separate setContentLength, setTransferEncoding etc parameters, analogous to the existing setHost option. Results in too many options, and I think in the case where this is necessary you almost always want to control all headers. In fact, I think if this PR is merged we could consider deprecating setHost entirely, and suggesting users migrate to this option instead (I expect most setHost: false cases will want this too) but I haven't looked into that for now, we can wait and see how this is used instead.
  • Extending setHeaders to allow setting raw headers after initial request construction, to handle this use case with just removeHeader + setHeaders calls. This would require that either setHeaders immediately serialize the raw headers to _headers in this case (blocking any further changes like an implicit flushHeaders, but only in the raw header argument case, with potentially weird differences between setHeaders behaviour for requests vs responses), or some fairly major refactoring to change all internal HTTP header representation to support incrementally adding raw headers (all current raw header APIs - writeHead and http.request - always serialize & fix the headers immediately). The former is quite an awkward API, the latter is messy and risky, and I'd rather not.
  • Adding a new request.writeHeaders method analogous to response.writeHead. This works, but it's yet another headers API with subtly different behaviour that would really only exist for this one use case.
  • Removing implicit headers automatically if you provide a full set of raw headers in http.request - this seems sensible to me as an API, but it's a huge breaking change that isn't really practical imo.

@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 Dec 2, 2024
This makes it possible to disable the various default headers directly
from the constructor. While this is possible for many use cases by
manually calling removeHeader on the request object instead, when
passing a raw header array to the request constructor the headers are
serialized and prepared to send immediately, and removeHeader cannot
subsequently be used.

With this change, it's now possible to 100% control sent request
headers by passing 'setDefaultHeaders: false' and a raw headers array to
http.request.
Copy link

codecov bot commented Dec 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.00%. Comparing base (d5d1e80) to head (950b50b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56112      +/-   ##
==========================================
- Coverage   88.00%   88.00%   -0.01%     
==========================================
  Files         656      656              
  Lines      189002   189008       +6     
  Branches    36003    35997       -6     
==========================================
+ Hits       166335   166340       +5     
+ Misses      15838    15830       -8     
- Partials     6829     6838       +9     
Files with missing lines Coverage Δ
lib/_http_client.js 97.99% <100.00%> (+0.01%) ⬆️

... and 25 files with indirect coverage changes

@pimterry pimterry added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 2, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 2, 2024
@nodejs-github-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants