-
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -155,6 +155,23 @@ class BaseUpload { | |
}) | ||
} | ||
|
||
static convertToStringHeaderValue(val) { | ||
if (val === null || val === undefined) { | ||
return '' | ||
} else if (Array.isArray(val)) { | ||
return val.join(',') | ||
} else if ( | ||
typeof val === 'string' || | ||
typeof val === 'number' || | ||
typeof val === 'boolean' || | ||
typeof val === 'symbol' | ||
) { | ||
return val.toString() | ||
} else if (typeof val === 'object') { | ||
return Object.values(val).join(',') | ||
} | ||
} | ||
|
||
findPreviousUploads() { | ||
return this.options | ||
.fingerprint(this.file, this.options) | ||
|
@@ -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 commentThe 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. |
||
} | ||
|
||
return this._sendRequest(req, null) | ||
|
@@ -579,15 +596,15 @@ class BaseUpload { | |
const req = this._openRequest('POST', this.options.endpoint) | ||
|
||
if (this.options.uploadLengthDeferred) { | ||
req.setHeader('Upload-Defer-Length', 1) | ||
req.setHeader('Upload-Defer-Length', '1') | ||
} 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 commentThe 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 |
||
} | ||
|
||
// 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)) | ||
} | ||
|
||
let promise | ||
|
@@ -754,7 +771,7 @@ class BaseUpload { | |
req = this._openRequest('PATCH', this.url) | ||
} | ||
|
||
req.setHeader('Upload-Offset', this._offset) | ||
req.setHeader('Upload-Offset', BaseUpload.convertToStringHeaderValue(this._offset)) | ||
const promise = this._addChunkToRequest(req) | ||
|
||
promise | ||
|
@@ -810,7 +827,7 @@ class BaseUpload { | |
// upload size and can tell the tus server. | ||
if (this.options.uploadLengthDeferred && done) { | ||
this._size = this._offset + valueSize | ||
req.setHeader('Upload-Length', this._size) | ||
req.setHeader('Upload-Length', BaseUpload.convertToStringHeaderValue(this._size)) | ||
} | ||
|
||
// The specified uploadSize might not match the actual amount of data that a source | ||
|
@@ -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 commentThe 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. |
||
} | ||
|
||
if (options.addRequestId) { | ||
|
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.