From 3054f7b7aa6a790c454f12c61693d9fd5cf08556 Mon Sep 17 00:00:00 2001 From: Murderlon Date: Wed, 27 Jul 2022 11:48:18 +0200 Subject: [PATCH 1/2] Correctly handle errors for prepareUploadParts --- .../aws-s3-multipart/src/MultipartUploader.js | 31 ++++--- .../@uppy/aws-s3-multipart/src/index.test.js | 93 ++++++++++++------- 2 files changed, 79 insertions(+), 45 deletions(-) diff --git a/packages/@uppy/aws-s3-multipart/src/MultipartUploader.js b/packages/@uppy/aws-s3-multipart/src/MultipartUploader.js index b41f5e295a..6eecebb14b 100644 --- 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 index 39aecf9bd2..e357a9b4c4 100644 --- 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.toBeDefined() + + expect(awsS3Multipart.opts.prepareUploadParts.mock.calls.length).toEqual(2) + expect(mock.mock.calls.length).toEqual(1) + }) }) describe('dynamic companionHeader', () => { From 9d9c2108cf0341a14c47303064262c750cdb7aa0 Mon Sep 17 00:00:00 2001 From: Murderlon Date: Wed, 27 Jul 2022 17:28:03 +0200 Subject: [PATCH 2/2] Check value of rejects in test --- packages/@uppy/aws-s3-multipart/src/index.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@uppy/aws-s3-multipart/src/index.test.js b/packages/@uppy/aws-s3-multipart/src/index.test.js index e357a9b4c4..cf7e336ebf 100644 --- a/packages/@uppy/aws-s3-multipart/src/index.test.js +++ b/packages/@uppy/aws-s3-multipart/src/index.test.js @@ -261,7 +261,7 @@ describe('AwsS3Multipart', () => { }), }) - await expect(core.upload()).rejects.toBeDefined() + 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)