Correctly handle errors for prepareUploadParts (#3912)

- Rename `#prepareUploadParts` to `#prepareUploadPartsRetryable` - Call `this.#onError` instead of `return` to correctly handle error - Add a test for it
transloadit · Aug 3, 2022 · 2785885 · 2785885
1 parent 6f3380b
commit 2785885
Show file tree

Hide file tree

Showing 2 changed files with 79 additions and 45 deletions.
diff --git a/packages/@uppy/aws-s3-multipart/src/MultipartUploader.js b/packages/@uppy/aws-s3-multipart/src/MultipartUploader.js
@@ -198,19 +198,22 @@ class MultipartUploader {
 
     if (chunkIndexes.length === 0) return
 
-    return this.#prepareUploadParts(chunkIndexes).then((result) => {
-      const { presignedUrls, headers } = result
-
-      chunkIndexes.forEach((index) => {
-        const partNumber = index + 1
-        const prePreparedPart = { url: presignedUrls[partNumber], headers: headers?.[partNumber] }
-        this.#uploadPartRetryable(index, prePreparedPart).then(() => {
-          this.#uploadParts()
-        }, (err) => {
-          this.#onError(err)
-        })
-      })
-    })
+    this.#prepareUploadPartsRetryable(chunkIndexes).then(
+      ({ presignedUrls, headers }) => {
+        for (const index of chunkIndexes) {
+          const partNumber = index + 1
+          const prePreparedPart = {
+            url: presignedUrls[partNumber],
+            headers: headers?.[partNumber],
+          }
+          this.#uploadPartRetryable(index, prePreparedPart).then(
+            () => this.#uploadParts(),
+            (err) => this.#onError(err),
+          )
+        }
+      },
+      (err) => this.#onError(err),
+    )
   }
 
   #retryable ({ before, attempt, after }) {
@@ -247,7 +250,7 @@ class MultipartUploader {
     })
   }
 
-  async #prepareUploadParts (chunkIndexes) {
+  async #prepareUploadPartsRetryable (chunkIndexes) {
     chunkIndexes.forEach((i) => {
       this.chunkState[i].busy = true
     })

diff --git a/packages/@uppy/aws-s3-multipart/src/index.test.js b/packages/@uppy/aws-s3-multipart/src/index.test.js
@@ -185,41 +185,41 @@ describe('AwsS3Multipart', () => {
   })
 
   describe('MultipartUploader', () => {
-    let core
-    let awsS3Multipart
+    const createMultipartUpload = jest.fn(() => {
+      return {
+        uploadId: '6aeb1980f3fc7ce0b5454d25b71992',
+        key: 'test/upload/multitest.dat',
+      }
+    })
 
-    beforeEach(() => {
-      core = new Core()
-      core.use(AwsS3Multipart, {
-        createMultipartUpload: jest.fn(() => {
-          return {
-            uploadId: '6aeb1980f3fc7ce0b5454d25b71992',
-            key: 'test/upload/multitest.dat',
-          }
-        }),
-        completeMultipartUpload: jest.fn(async () => ({ location: 'test' })),
-        abortMultipartUpload: jest.fn(),
-        prepareUploadParts: jest
-          .fn(async () => {
-            const presignedUrls = {}
-            const possiblePartNumbers = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
-
-            possiblePartNumbers.forEach((partNumber) => {
-              presignedUrls[
-                partNumber
-              ] = `https://bucket.s3.us-east-2.amazonaws.com/test/upload/multitest.dat?partNumber=${partNumber}&uploadId=6aeb1980f3fc7ce0b5454d25b71992&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIATEST%2F20210729%2Fus-east-2%2Fs3%2Faws4_request&X-Amz-Date=20210729T014044Z&X-Amz-Expires=600&X-Amz-SignedHeaders=host&X-Amz-Signature=test`
-            })
-
-            return { presignedUrls }
-          })
-          // This runs first and only once
-          // eslint-disable-next-line prefer-promise-reject-errors
-          .mockImplementationOnce(() => Promise.reject({ source: { status: 500 } })),
+    const prepareUploadParts = jest
+      .fn(async () => {
+        const presignedUrls = {}
+        const possiblePartNumbers = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
+
+        possiblePartNumbers.forEach((partNumber) => {
+          presignedUrls[
+            partNumber
+          ] = `https://bucket.s3.us-east-2.amazonaws.com/test/upload/multitest.dat?partNumber=${partNumber}&uploadId=6aeb1980f3fc7ce0b5454d25b71992&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIATEST%2F20210729%2Fus-east-2%2Fs3%2Faws4_request&X-Amz-Date=20210729T014044Z&X-Amz-Expires=600&X-Amz-SignedHeaders=host&X-Amz-Signature=test`
+        })
+
+        return { presignedUrls }
       })
-      awsS3Multipart = core.getPlugin('AwsS3Multipart')
-    })
+
+    afterEach(() => jest.clearAllMocks())
 
     it('retries prepareUploadParts when it fails once', async () => {
+      const core = new Core()
+        .use(AwsS3Multipart, {
+          createMultipartUpload,
+          completeMultipartUpload: jest.fn(async () => ({ location: 'test' })),
+          abortMultipartUpload: jest.fn(),
+          prepareUploadParts:
+          prepareUploadParts
+          // eslint-disable-next-line prefer-promise-reject-errors
+            .mockImplementationOnce(() => Promise.reject({ source: { status: 500 } })),
+        })
+      const awsS3Multipart = core.getPlugin('AwsS3Multipart')
       const fileSize = 5 * MB + 1 * MB
 
       core.addFile({
@@ -235,6 +235,37 @@ describe('AwsS3Multipart', () => {
 
       expect(awsS3Multipart.opts.prepareUploadParts.mock.calls.length).toEqual(2)
     })
+
+    it('calls `upload-error` when prepareUploadParts fails after all retries', async () => {
+      const core = new Core()
+        .use(AwsS3Multipart, {
+          retryDelays: [100],
+          createMultipartUpload,
+          completeMultipartUpload: jest.fn(async () => ({ location: 'test' })),
+          abortMultipartUpload: jest.fn(),
+          prepareUploadParts: prepareUploadParts
+            // eslint-disable-next-line prefer-promise-reject-errors
+            .mockImplementation(() => Promise.reject({ source: { status: 500 } })),
+        })
+      const awsS3Multipart = core.getPlugin('AwsS3Multipart')
+      const fileSize = 5 * MB + 1 * MB
+      const mock = jest.fn()
+      core.on('upload-error', mock)
+
+      core.addFile({
+        source: 'jest',
+        name: 'multitest.dat',
+        type: 'application/octet-stream',
+        data: new File([new Uint8Array(fileSize)], {
+          type: 'application/octet-stream',
+        }),
+      })
+
+      await expect(core.upload()).rejects.toEqual({ source: { status: 500 } })
+
+      expect(awsS3Multipart.opts.prepareUploadParts.mock.calls.length).toEqual(2)
+      expect(mock.mock.calls.length).toEqual(1)
+    })
   })
 
   describe('dynamic companionHeader', () => {