Skip to content

Commit

Permalink
@uppy/aws-s3: remove deprecated prepareUploadParts option (#5075)
Browse files Browse the repository at this point in the history
  • Loading branch information
@aduh95
aduh95 authored Apr 11, 2024
1 parent bfd5f5f commit f6734fe
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 213 deletions.
10 changes: 0 additions & 10 deletions e2e/clients/dashboard-aws-multipart/app.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,6 @@ const uppy = new Uppy()
limit: 2,
companionUrl: process.env.VITE_COMPANION_URL,
shouldUseMultipart: true,
// This way we can test that the user provided API still works
// as expected in the flow. We call the default internal function for this,
// otherwise we would have to run another server to pre-sign requests
// and we don't care about that, just that the flow works.
async prepareUploadParts (file, { uploadId, key, parts, signal }) {
const { number: partNumber, chunk: body } = parts[0]
const plugin = uppy.getPlugin('AwsS3Multipart')
const { url } = await plugin.signPart(file, { uploadId, key, partNumber, body, signal })
return { presignedUrls: { [partNumber]: url } }
},
})

// Keep this here to access uppy in tests
Expand Down
162 changes: 10 additions & 152 deletions packages/@uppy/aws-s3/src/index.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,159 +117,17 @@ describe('AwsS3Multipart', () => {
}),
completeMultipartUpload: vi.fn(async () => ({ location: 'test' })),
abortMultipartUpload: vi.fn(),
prepareUploadParts: vi.fn(
async (file, { parts }: { parts: { number: number }[] }) => {
const presignedUrls: Record<number, string> = {}
parts.forEach(({ number }) => {
presignedUrls[number] =
`https://bucket.s3.us-east-2.amazonaws.com/test/upload/multitest.dat?partNumber=${number}&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, headers: { 1: { 'Content-MD5': 'foo' } } }
},
),
signPart: vi.fn(async (file, { number }) => {
return {
url: `https://bucket.s3.us-east-2.amazonaws.com/test/upload/multitest.dat?partNumber=${number}&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`,
headers: number === 1 ? { 'Content-MD5': 'foo' } : undefined,
}
}),
listParts: undefined as any,
})
awsS3Multipart = core.getPlugin('AwsS3Multipart') as any
})

it('Calls the prepareUploadParts function totalChunks / limit times', async () => {
const scope = nock(
'https://bucket.s3.us-east-2.amazonaws.com',
).defaultReplyHeaders({
'access-control-allow-headers': '*',
'access-control-allow-method': 'PUT',
'access-control-allow-origin': '*',
'access-control-expose-headers': 'ETag, Content-MD5',
})
// 6MB file will give us 2 chunks, so there will be 2 PUT and 2 OPTIONS
// calls to the presigned URL from 1 prepareUploadParts calls
const fileSize = 5 * MB + 1 * MB

scope
.options((uri) =>
uri.includes('test/upload/multitest.dat?partNumber=1'),
)
.reply(function replyFn() {
expect(this.req.headers['access-control-request-headers']).toEqual(
'Content-MD5',
)
return [200, '']
})
scope
.options((uri) =>
uri.includes('test/upload/multitest.dat?partNumber=2'),
)
.reply(function replyFn() {
expect(
this.req.headers['access-control-request-headers'],
).toBeUndefined()
return [200, '']
})
scope
.put((uri) => uri.includes('test/upload/multitest.dat?partNumber=1'))
.reply(200, '', { ETag: 'test1' })
scope
.put((uri) => uri.includes('test/upload/multitest.dat?partNumber=2'))
.reply(200, '', { ETag: 'test2' })

core.addFile({
source: 'vi',
name: 'multitest.dat',
type: 'application/octet-stream',
data: new File([new Uint8Array(fileSize)], '', {
type: 'application/octet-stream',
}),
})

await core.upload()

expect(
(awsS3Multipart.opts as any).prepareUploadParts.mock.calls.length,
).toEqual(2)

scope.done()
})

it('Calls prepareUploadParts with a Math.ceil(limit / 2) minimum, instead of one at a time for the remaining chunks after the first limit batch', async () => {
const scope = nock(
'https://bucket.s3.us-east-2.amazonaws.com',
).defaultReplyHeaders({
'access-control-allow-headers': '*',
'access-control-allow-method': 'PUT',
'access-control-allow-origin': '*',
'access-control-expose-headers': 'ETag',
})
// 50MB file will give us 10 chunks, so there will be 10 PUT and 10 OPTIONS
// calls to the presigned URL from 3 prepareUploadParts calls
//
// The first prepareUploadParts call will be for 5 parts, the second
// will be for 3 parts, the third will be for 2 parts.
const fileSize = 50 * MB

scope
.options((uri) => uri.includes('test/upload/multitest.dat'))
.reply(200, '')
scope
.put((uri) => uri.includes('test/upload/multitest.dat'))
.reply(200, '', { ETag: 'test' })
scope.persist()

core.addFile({
source: 'vi',
name: 'multitest.dat',
type: 'application/octet-stream',
data: new File([new Uint8Array(fileSize)], '', {
type: 'application/octet-stream',
}),
})

await core.upload()

function validatePartData(
{ parts }: { parts: { number: number; chunk: unknown }[] },
expected: number[],
) {
expect(parts.map((part) => part.number)).toEqual(expected)

for (const part of parts) {
expect(part.chunk).toBeDefined()
}
}

expect(
(awsS3Multipart.opts as any).prepareUploadParts.mock.calls.length,
).toEqual(10)

validatePartData(
(awsS3Multipart.opts as any).prepareUploadParts.mock.calls[0][1],
[1],
)
validatePartData(
(awsS3Multipart.opts as any).prepareUploadParts.mock.calls[1][1],
[2],
)
validatePartData(
(awsS3Multipart.opts as any).prepareUploadParts.mock.calls[2][1],
[3],
)

const completeCall = (awsS3Multipart.opts as any).completeMultipartUpload
.mock.calls[0][1]

expect(completeCall.parts).toEqual([
{ ETag: 'test', PartNumber: 1 },
{ ETag: 'test', PartNumber: 2 },
{ ETag: 'test', PartNumber: 3 },
{ ETag: 'test', PartNumber: 4 },
{ ETag: 'test', PartNumber: 5 },
{ ETag: 'test', PartNumber: 6 },
{ ETag: 'test', PartNumber: 7 },
{ ETag: 'test', PartNumber: 8 },
{ ETag: 'test', PartNumber: 9 },
{ ETag: 'test', PartNumber: 10 },
])
})

it('Keeps chunks marked as busy through retries until they complete', async () => {
const scope = nock(
'https://bucket.s3.us-east-2.amazonaws.com',
Expand Down Expand Up @@ -364,9 +222,9 @@ describe('AwsS3Multipart', () => {
}
}

expect(
(awsS3Multipart.opts as any).prepareUploadParts.mock.calls.length,
).toEqual(10)
expect((awsS3Multipart.opts as any).signPart.mock.calls.length).toEqual(
10,
)
})
})

Expand Down Expand Up @@ -623,7 +481,7 @@ describe('AwsS3Multipart', () => {

await core.upload()
expect(createMultipartUpload).toHaveBeenCalled()
expect(signPart).toHaveBeenCalledTimes(10)
expect(signPart).toHaveBeenCalledTimes(11)
expect(completeMultipartUpload).toHaveBeenCalled()
})

Expand Down
53 changes: 2 additions & 51 deletions packages/@uppy/aws-s3/src/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,25 +197,6 @@ type AWSS3MultipartWithoutCompanionMandatorySignPart<
opts: SignPartOptions,
) => MaybePromise<AwsS3UploadParameters>
}
/** @deprecated Use signPart instead */
type AWSS3MultipartWithoutCompanionMandatoryPrepareUploadParts<
M extends Meta,
B extends Body,
> = {
/** @deprecated Use signPart instead */
prepareUploadParts: (
file: UppyFile<M, B>,
partData: {
uploadId: string
key: string
parts: [{ number: number; chunk: Blob }]
signal?: AbortSignal
},
) => MaybePromise<{
presignedUrls: Record<number, string>
headers?: Record<number, Record<string, string>>
}>
}
type AWSS3MultipartWithoutCompanionMandatory<M extends Meta, B extends Body> = {
getChunkSize?: (file: UppyFile<M, B>) => number
createMultipartUpload: (file: UppyFile<M, B>) => MaybePromise<UploadResult>
Expand All @@ -236,10 +217,7 @@ type AWSS3MultipartWithoutCompanionMandatory<M extends Meta, B extends Body> = {
signal: AbortSignal
},
) => MaybePromise<{ location?: string }>
} & (
| AWSS3MultipartWithoutCompanionMandatorySignPart<M, B>
| AWSS3MultipartWithoutCompanionMandatoryPrepareUploadParts<M, B>
)
} & AWSS3MultipartWithoutCompanionMandatorySignPart<M, B>

type AWSS3MultipartWithoutCompanion<
M extends Meta,
Expand Down Expand Up @@ -387,33 +365,6 @@ export default class AwsS3Multipart<
)
}
}
if (
(opts as AWSS3MultipartWithoutCompanionMandatoryPrepareUploadParts<M, B>)
?.prepareUploadParts != null &&
(opts as AWSS3MultipartWithoutCompanionMandatorySignPart<M, B>)
.signPart == null
) {
this.opts.signPart = async (
file: UppyFile<M, B>,
{ uploadId, key, partNumber, body, signal }: SignPartOptions,
) => {
const { presignedUrls, headers } = await (
opts as AWSS3MultipartWithoutCompanionMandatoryPrepareUploadParts<
M,
B
>
).prepareUploadParts(file, {
uploadId,
key,
parts: [{ number: partNumber, chunk: body }],
signal,
})
return {
url: presignedUrls?.[partNumber],
headers: headers?.[partNumber],
}
}
}

/**
* Simultaneous upload limiting is shared across all uploads with this plugin.
Expand Down Expand Up @@ -729,7 +680,7 @@ export default class AwsS3Multipart<
;(error as any).source = { status: 403 }
reject(error)
})
xhr.addEventListener('load', (ev) => {
xhr.addEventListener('load', () => {
cleanup()

if (
Expand Down

0 comments on commit f6734fe

Please sign in to comment.