Skip to content
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.

Sign up for GitHub

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

Jump to bottom

@uppy/aws-s3: remove deprecated prepareUploadParts option #5075

Merged
merged 5 commits into from
Apr 11, 2024
Merged

@uppy/aws-s3: remove deprecated prepareUploadParts option #5075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4a8f72f
@aws-s3: remove deprecated `prepareUploadParts` option
aduh95 Apr 11, 2024
9c1135a
fixup
aduh95 Apr 11, 2024
4af53a9
Update packages/@uppy/aws-s3/src/index.test.ts
aduh95 Apr 11, 2024
b4217a2
Merge branch '4.x' into aws-deprecated-byebye
aduh95 Apr 11, 2024
e4078a8
fixup
aduh95 Apr 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions e2e/clients/dashboard-aws-multipart/app.js
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading