-
Notifications
You must be signed in to change notification settings - Fork 315
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
Only pass strings to HttpRequest#setHeader
#709
Only pass strings to HttpRequest#setHeader
#709
Conversation
DraganDanicic
commented
Aug 23, 2024
•
edited
Loading
edited
- Numbers passed as header-values are casted to string type.
- Tests adjusted in a way that expected header values are now strings.
Thank you for the PR! All of these should be addressed once we get #683 merged, which moves the code base to TypeScript. While working on it, I also discovered that we call setHeader sometimes with a number, which is not ideal. But we can also get this PR faster merged without waiting on the TypeScript PR. |
lib/upload.js
Outdated
@@ -378,7 +395,7 @@ class BaseUpload { | |||
// Add metadata if values have been added | |||
const metadata = encodeMetadata(this.options.metadata) | |||
if (metadata !== '') { | |||
req.setHeader('Upload-Metadata', metadata) | |||
req.setHeader('Upload-Metadata', BaseUpload.convertToStringHeaderValue(metadata)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
encodeMetadata always returns a string, so we don't need a conversion here.
lib/upload.js
Outdated
} else { | ||
req.setHeader('Upload-Length', this._size) | ||
req.setHeader('Upload-Length', BaseUpload.convertToStringHeaderValue(this._size)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most violations come from Upload-Length and Upload-Offset as you already found out. We can just use ${this._size}
or similar here to convert the numbers into strings without the additional conversion method.
lib/upload.js
Outdated
@@ -973,7 +990,7 @@ function openRequest(method, url, options) { | |||
const headers = options.headers || {} | |||
|
|||
for (const [name, value] of Object.entries(headers)) { | |||
req.setHeader(name, value) | |||
req.setHeader(name, BaseUpload.convertToStringHeaderValue(value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should magically convert non-strings into strings here, especially since this is technically a breaking change. It's the user responsibility to pass proper values in here that match what the documentation mentions.
We can log a warning if the value is not a string and then convert the warning into an error in the next major release.
lib/upload.js
Outdated
@@ -155,6 +155,23 @@ class BaseUpload { | |||
}) | |||
} | |||
|
|||
static convertToStringHeaderValue(val) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The helper method is a good idea, but not really needed. I would like to avoid a large helper method like this that magically converts everything somehow into a string and instead use dedicated methods where we need to convert things into string.
I agree on your points. Shall we then go with explicit conversion at-the-spot (this -> |
Yes, that would be my preferred solution. |
I have adjusted the code, and it should now meet the requirements. Please, have a look. |
HttpRequest#setHeader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR!