fix(lib-storage): add validation for partCount and contentLength for multipart upload #7363
Conversation
… multipart upload
smilkuri
changed the title
smilkuri
changed the title
| }); | ||
| } | ||
|
|
||
| private __calculatePartSize(targetPartSizeBytes: number, minPartSize: number): number { |
There was a problem hiding this comment.
The variable name targetPartSizeBytes is inaccurate because it is not a part size. It is the size of the entire object.
There's no need for the second arg since it is only ever called with Upload.MIN_PART_SIZE.
But beyond that, there's no need for this function since it's only called once.
this.partSize = Math.max(
Upload.MIN_PART_SIZE,
Math.floor((this.totalBytes ?? 0) / this.MAX_PARTS)
);| this.abortController = options.abortController ?? new AbortController(); | ||
|
|
||
| this.partSize = this.__calculatePartSize(this.totalBytes ?? 0, Upload.MIN_PART_SIZE); | ||
| this.expectedPartsCount = this.totalBytes ? Math.ceil(this.totalBytes / this.partSize) : 1; |
There was a problem hiding this comment.
I get the sense that this is a check for whether totalBytes was able to be calculated and not whether or not it is the numeric value 0.
In that case, it would be clearer to state the check as this.totalBytes !== undefined ?. However, with a zero byte input, the expected part count is indeed 1, so perhaps no change is needed.
But, why would expectedPartsCount be 1 if totalBytes could not be calculated? It could be a stream of an unknown byte length. In this case, both totalBytes and expectedPartCount should be undefined, since they would not be knowable.
|
|
||
| private uploadedParts: CompletedPart[] = []; | ||
| private uploadEnqueuedPartsCount = 0; | ||
| private expectedPartsCount = 0; |
There was a problem hiding this comment.
it should not be assigned a value here, since 0 is never possible and a value is always assigned in the constructor.
But since my comments below understand this value could be impossible to calculate, in which case it should be undefined. Therefore the type should be private expectedPartsCount?: number;.
|
|
||
| let result; | ||
| if (this.isMultiPart) { | ||
| if (this.totalBytes && this.uploadedParts.length !== this.expectedPartsCount) { |
There was a problem hiding this comment.
totalBytes can be zero and expectedPartsCount would be 1 in that case.
This should instead be
const { expectedPartsCount, uploadedParts } = this;
if (expectedPartsCount !== undefined
&& uploadedParts.length !== expectedPartsCount) {| if (this.isMultiPart) { | ||
| if (this.totalBytes && this.uploadedParts.length !== this.expectedPartsCount) { | ||
| throw new Error( | ||
| `Expected ${this.expectedPartsCount} number of parts but uploaded only ${this.uploadedParts.length} parts` |
There was a problem hiding this comment.
| `Expected ${this.expectedPartsCount} number of parts but uploaded only ${this.uploadedParts.length} parts` | |
| `Expected ${this.expectedPartsCount} part(s) but uploaded ${this.uploadedParts.length} part(s).` |
Can't say "only" because the check is for inequality. The uploaded parts count could be higher.
| return Math.max(minPartSize, calculatedPartSize); | ||
| } | ||
|
|
||
| private __validateUploadPart(dataPart: RawDataPart, partSize: number): void { |
There was a problem hiding this comment.
why is partSize an argument to this method when it is only ever the same instance variable this.partSize?
| } | ||
|
|
||
| private __validateUploadPart(dataPart: RawDataPart, partSize: number): void { | ||
| const actualPartSize = byteLength(dataPart.data) || 0; |
There was a problem hiding this comment.
the fallback value of undefined should be used, since real data may be zero bytes.
| return; | ||
| } | ||
|
|
||
| if (actualPartSize === 0) { |
There was a problem hiding this comment.
| if (actualPartSize === 0) { | |
| if (actualPartSize === undefined) { |
| } | ||
|
|
||
| if (actualPartSize === 0) { | ||
| throw new Error(`Content length is missing on the data for part number ${dataPart.partNumber}`); |
There was a problem hiding this comment.
This statement implies the response header is missing, since content length usually refers to the header of that name.
But it's not related to that header. This error branch is only due to programming error on our part, right? The error message should be developer-facing, e.g.
Error: a dataPart was generated without a measurable data chunk size.
| return; | ||
| } | ||
|
|
||
| this.__validateUploadPart(dataPart, this.partSize); |
There was a problem hiding this comment.
this should be moved closer to UploadPartCommand to prevent any changes happening to dataPart between the validation and sending out the data
Issue
Internal JS-6196
Description
Testing
Locally
Additional context
Add any other context about the PR here.
Checklist
*.integ.spec.ts).@publictag and enable doc generation on the package?By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.