Skip to content

Commit

Permalink
refactor(sdkv3): migrate S3 Client (#6721)
Browse files Browse the repository at this point in the history
## Problem
This client is tightly coupled to some implementations, making this
migration more difficult than usual. Additionally, v3 handles streams
differently than v2. Also, some v2 features in the SDK are completely
redone. For example, pre-signing a url:
https://aws.amazon.com/blogs/developer/generate-presigned-url-modular-aws-sdk-javascript/.

## Solution
- refactor or delete most of the tests in
https://github.com/aws/aws-toolkit-vscode/blob/1dbaab43d3533cc35b8bcd705860c7199bc98264/packages/core/src/test/shared/clients/defaultS3Client.test.ts
- Most of them don't add real value. They simply assert the SDK was
called, or that errors propagate. These unit tests should be reserved
for testing the logic built on top of these requests, not the requests
themselves.
- For tests that do add value, refactor to avoid stubbing the SDK (Ex.
`listFiles` refactor).
- Throw an explicit error when we reach a code path that is not
web-supported.
- Migrate to new pre-signing logic, described here:
https://aws.amazon.com/blogs/developer/generate-presigned-url-modular-aws-sdk-javascript/
- Simplify list buckets since region is now added in response by
default.
- Unify Bucket type definitions. There originally was the S3 `Bucket`
type exported by the SDK, our own `Bucket`, and `DefaultBucket`.
`Bucket` was derived from `DefaultBucket`, but this made reading type
signatures ambiguous at first glance. These are all now under S3Bucket.

## Verification
To verify this migration didn't break anything I manually tested the
following through the toolkit:
- Create buckets (in console and toolkit) in three different regions,
ensure they display in proper regions.
- Create folders (nested as well). 
- Upload a file, view and edit the file through explorer. 
- Download the file, and verify my changes were saved through S3. 
- Generate a presigned url for objects in my bucket. 
- Delete a file, bucket, folder. 
- Verify copied ARN, name, and Path are correct through selection. 

## Future Work
- Downloading and uploading a file now supports web streams, which means
that `GetObject` response body is now a
`StreamingBlobPayloadOutputTypes` from `@smithy/types`. This is a
general type for web/node streams, therefore we must cast to Node's
`Readable` type. Ideally we would support working with the abstract
stream directly and avoiding the cast. The approach is outlined here:
https://docs.aws.amazon.com/code-library/latest/ug/s3_example_s3_Scenario_UsingLargeFiles_section.html.
- E2E tests for uploading and downloading a file. 
- Remove mocks from
https://github.com/aws/aws-toolkit-vscode/blob/1a313e4029930e1da48558f6c7b84f48036672e9/packages/core/src/test/awsService/s3/commands/uploadFile.test.ts.
Mocked tests are extremely expensive to maintain because they inherently
rely on implementation details.
- There are three listBuckets-related methods that all do slightly
different things. These should live in a single method, `listBuckets`
and it should return an `AsyncCollection` for proper pagination support.
- Reduce stub usage in the tests to avoid coupling to implementation.
The entire test suite for the wrapper stubbed S3 v2 directly, making it
very difficult to refactor the tests.
---

- Treat all work as PUBLIC. Private `feature/x` branches will not be
squash-merged at release time.
- Your code changes must meet the guidelines in
[CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines).
- License: I confirm that my contribution is made under the terms of the
Apache 2.0 license.

---------

Co-authored-by: aws-toolkit-automation <[email protected]>
  • Loading branch information
Hweinstock and aws-toolkit-automation authored Mar 6, 2025
1 parent da0b811 commit 997b3b7
Show file tree
Hide file tree
Showing 45 changed files with 1,664 additions and 1,135 deletions.
1,252 changes: 1,138 additions & 114 deletions package-lock.json

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions packages/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,8 @@
"@aws-sdk/client-docdb-elastic": "<3.696.0",
"@aws-sdk/client-ec2": "<3.696.0",
"@aws-sdk/client-iam": "<3.696.0",
"@aws-sdk/client-s3": "<3.696.0",
"@aws-sdk/lib-storage": "<3.696.0",
"@aws-sdk/client-lambda": "<3.696.0",
"@aws-sdk/client-ssm": "<3.696.0",
"@aws-sdk/client-sso": "<3.696.0",
Expand All @@ -514,6 +516,7 @@
"@aws-sdk/protocol-http": "<3.696.0",
"@aws-sdk/smithy-client": "<3.696.0",
"@aws-sdk/util-arn-parser": "<3.696.0",
"@aws-sdk/s3-request-presigner": "<3.696.0",
"@aws/mynah-ui": "^4.23.1",
"@gerhobbelt/gitignore-parser": "^0.2.0-9",
"@iarna/toml": "^2.2.5",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
PolicyChecksUiClick,
ValidatePolicyFindingType,
} from './constants'
import { DefaultS3Client, parseS3Uri } from '../../../shared/clients/s3Client'
import { S3Client, parseS3Uri } from '../../../shared/clients/s3'
import { ExpiredTokenException } from '@aws-sdk/client-sso-oidc'
import { ChildProcess } from '../../../shared/utilities/processUtils'

Expand Down Expand Up @@ -842,7 +842,7 @@ export async function _readCustomChecksFile(input: string): Promise<string> {
} else {
try {
const [region, bucket, key] = parseS3Uri(input)
const s3Client = new DefaultS3Client(region)
const s3Client = new S3Client(region)
const resp = await s3Client.getObject({ bucketName: bucket, key })
// Lint warning: this may evaluate to '[object Object]'. @typescript-eslint/no-base-to-string
// eslint-disable-next-line @typescript-eslint/no-base-to-string
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { defaultPartition } from '../../../../shared/regions/regionProvider'
import { Lambda, APIGateway } from 'aws-sdk'
import { LambdaNode } from '../../../../lambda/explorer/lambdaNodes'
import { LambdaFunctionNode } from '../../../../lambda/explorer/lambdaFunctionNode'
import { DefaultS3Client, DefaultBucket } from '../../../../shared/clients/s3Client'
import { S3Client, toBucket } from '../../../../shared/clients/s3'
import { S3Node } from '../../../../awsService/s3/explorer/s3Nodes'
import { S3BucketNode } from '../../../../awsService/s3/explorer/s3BucketNode'
import { ApiGatewayNode } from '../../../../awsService/apigateway/explorer/apiGatewayNodes'
Expand Down Expand Up @@ -100,13 +100,9 @@ export async function generateDeployedNode(
break
}
case s3BucketType: {
const s3Client = new DefaultS3Client(regionCode)
const s3Client = new S3Client(regionCode)
const s3Node = new S3Node(s3Client)
const s3Bucket = new DefaultBucket({
partitionId: partitionId,
region: regionCode,
name: deployedResource.PhysicalResourceId,
})
const s3Bucket = toBucket(deployedResource.PhysicalResourceId, regionCode, partitionId)
newDeployedResource = new S3BucketNode(s3Bucket, s3Node, s3Client)
break
}
Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/awsService/s3/activation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { VirtualFileSystem } from '../../shared/virtualFilesystem'
import { Commands } from '../../shared/vscode/commands2'

import * as nls from 'vscode-nls'
import { DefaultS3Client } from '../../shared/clients/s3Client'
import { S3Client } from '../../shared/clients/s3'
import { TreeNode } from '../../shared/treeview/resourceTreeDataProvider'
import { getSourceNode } from '../../shared/utilities/treeNodeUtils'
const localize = nls.loadMessageBundle()
Expand All @@ -37,7 +37,7 @@ export async function activate(ctx: ExtContext): Promise<void> {
const fs = new VirtualFileSystem(
localize('AWS.s3.fileViewer.genericError', 'Unable to open S3 file, try reopening from the explorer')
)
const manager = new S3FileViewerManager((region) => new DefaultS3Client(region), fs)
const manager = new S3FileViewerManager((region) => new S3Client(region), fs)

ctx.extensionContext.subscriptions.push(manager)
ctx.extensionContext.subscriptions.push(
Expand All @@ -64,7 +64,7 @@ export async function activate(ctx: ExtContext): Promise<void> {
if (!node) {
const awsContext = ctx.awsContext
const regionCode = awsContext.getCredentialDefaultRegion()
const s3Client = new DefaultS3Client(regionCode)
const s3Client = new S3Client(regionCode)
const document = vscode.window.activeTextEditor?.document.uri
await uploadFileCommand(s3Client, document)
} else {
Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/awsService/s3/commands/createFolder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/

import * as vscode from 'vscode'
import { DEFAULT_DELIMITER } from '../../../shared/clients/s3Client'
import { DEFAULT_DELIMITER } from '../../../shared/clients/s3'
import { getLogger } from '../../../shared/logger/logger'
import { S3BucketNode } from '../explorer/s3BucketNode'
import { S3FolderNode } from '../explorer/s3FolderNode'
Expand Down Expand Up @@ -40,10 +40,10 @@ export async function createFolderCommand(node: S3BucketNode | S3FolderNode): Pr
}

const path = node.path + folderName + DEFAULT_DELIMITER
getLogger().info(`Creating folder "${path}" in bucket '${node.bucket.name}'`)
getLogger().info(`Creating folder "${path}" in bucket '${node.bucket.Name}'`)

const { folder } = await node
.createFolder({ path, bucketName: node.bucket.name })
.createFolder({ path, bucketName: node.bucket.Name })
.catch((e) => {
const message = localize(
'AWS.s3.createFolder.error.general',
Expand Down
10 changes: 5 additions & 5 deletions packages/core/src/awsService/s3/commands/deleteBucket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,23 +30,23 @@ export async function deleteBucketCommand(node: S3BucketNode): Promise<void> {
getLogger().debug('DeleteBucket called for %O', node)

await telemetry.s3_deleteBucket.run(async () => {
const isConfirmed = await showConfirmationDialog(node.bucket.name)
const isConfirmed = await showConfirmationDialog(node.bucket.Name)
if (!isConfirmed) {
throw new CancellationError('user')
}

getLogger().info(`Deleting bucket: ${node.bucket.name}`)
getLogger().info(`Deleting bucket: ${node.bucket.Name}`)
await deleteWithProgress(node)
.catch((e) => {
const message = localize(
'AWS.s3.deleteBucket.error.general',
'Failed to delete bucket {0}',
node.bucket.name
node.bucket.Name
)
throw ToolkitError.chain(e, message)
})
.finally(() => refreshNode(node.parent))
getLogger().info(`deleted bucket: ${node.bucket.name}`)
getLogger().info(`deleted bucket: ${node.bucket.Name}`)
})
}

Expand All @@ -65,7 +65,7 @@ async function deleteWithProgress(node: S3BucketNode): Promise<void> {
return vscode.window.withProgress(
{
location: vscode.ProgressLocation.Notification,
title: localize('AWS.s3.deleteBucket.progressTitle', 'Deleting {0}...', node.bucket.name),
title: localize('AWS.s3.deleteBucket.progressTitle', 'Deleting {0}...', node.bucket.Name),
},
() => {
return node.deleteBucket()
Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/awsService/s3/commands/downloadFileAs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { readablePath } from '../util'
import { progressReporter } from '../progressReporter'
import { localize } from '../../../shared/utilities/vsCodeUtils'
import { showOutputMessage } from '../../../shared/utilities/messages'
import { DefaultS3Client, S3Client } from '../../../shared/clients/s3Client'
import { S3Client } from '../../../shared/clients/s3'
import { Timeout, CancellationError } from '../../../shared/utilities/timeoutUtils'
import { ToolkitError } from '../../../shared/errors'
import { streamToBuffer, streamToFile } from '../../../shared/utilities/streamUtilities'
Expand Down Expand Up @@ -55,7 +55,7 @@ async function downloadS3File(
file: S3File,
options?: FileOptions | BufferOptions
): Promise<Buffer | void> {
const downloadStream = await client.downloadFileStream(file.bucket.name, file.key)
const downloadStream = await client.downloadFileStream(file.bucket.Name, file.key)
const result = options?.saveLocation
? streamToFile(downloadStream, options.saveLocation)
: streamToBuffer(downloadStream, file.sizeBytes)
Expand Down Expand Up @@ -86,7 +86,7 @@ async function downloadS3File(
export async function downloadFile(file: S3File, options: FileOptions): Promise<void>
export async function downloadFile(file: S3File, options?: BufferOptions): Promise<Buffer>
export async function downloadFile(file: S3File, options?: FileOptions | BufferOptions): Promise<Buffer | void> {
const client = options?.client ?? new DefaultS3Client(file.bucket.region)
const client = options?.client ?? new S3Client(file.bucket.BucketRegion)

return downloadS3File(client, file, options).catch((err) => {
const message = localize('AWS.s3.downloadFile.error.general', 'Failed to download file {0}', file.name)
Expand Down
7 changes: 3 additions & 4 deletions packages/core/src/awsService/s3/commands/presignedURL.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { SignedUrlRequest } from '../../../shared/clients/s3Client'
import { SignedUrlRequest } from '../../../shared/clients/s3'
import { copyToClipboard } from '../../../shared/utilities/messages'
import { S3FileNode } from '../explorer/s3FileNode'
import * as vscode from 'vscode'
Expand All @@ -19,13 +19,12 @@ export async function presignedURLCommand(node: S3FileNode): Promise<void> {
const validTime = await promptTime(node.file.key)
const s3Client = node.s3
const request: SignedUrlRequest = {
bucketName: node.bucket.name,
bucketName: node.bucket.Name,
key: node.file.key,
time: validTime * 60,
operation: 'getObject',
}

const url = await s3Client.getSignedUrl(request).catch((e) => {
const url = await s3Client.getSignedUrlForObject(request).catch((e) => {
throw ToolkitError.chain(
e,
'Error creating the presigned URL. Make sure you have access to the requested file.'
Expand Down
30 changes: 15 additions & 15 deletions packages/core/src/awsService/s3/commands/uploadFile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,14 @@ import * as path from 'path'
import * as mime from 'mime-types'
import * as vscode from 'vscode'
import { statSync } from 'fs' // eslint-disable-line no-restricted-imports
import { S3 } from 'aws-sdk'
import { getLogger } from '../../../shared/logger/logger'
import { S3Node } from '../explorer/s3Nodes'
import { readablePath } from '../util'
import { localize } from '../../../shared/utilities/vsCodeUtils'
import { showOutputMessage } from '../../../shared/utilities/messages'
import { createQuickPick, promptUser, verifySinglePickerOutput } from '../../../shared/ui/picker'
import { addCodiconToString } from '../../../shared/utilities/textUtilities'
import { Bucket, Folder, S3Client } from '../../../shared/clients/s3Client'
import { S3Bucket, Folder, S3Client } from '../../../shared/clients/s3'
import { createBucketCommand } from './createBucket'
import { S3BucketNode } from '../explorer/s3BucketNode'
import { S3FolderNode } from '../explorer/s3FolderNode'
Expand All @@ -24,6 +23,7 @@ import { CancellationError } from '../../../shared/utilities/timeoutUtils'
import { progressReporter } from '../progressReporter'
import globals from '../../../shared/extensionGlobals'
import { telemetry } from '../../../shared/telemetry/telemetry'
import { Upload } from '@aws-sdk/lib-storage'

export interface FileSizeBytes {
/**
Expand All @@ -38,7 +38,7 @@ interface UploadRequest {
fileLocation: vscode.Uri
fileSizeBytes: number
s3Client: S3Client
ongoingUpload?: S3.ManagedUpload
ongoingUpload?: Upload
}

/**
Expand Down Expand Up @@ -97,7 +97,7 @@ export async function uploadFileCommand(
uploadRequests.push(
...filesToUpload.map((file) => {
const key = node!.path + path.basename(file.fsPath)
return fileToUploadRequest(node!.bucket.name, key, file)
return fileToUploadRequest(node!.bucket.Name, key, file)
})
)
if (node instanceof S3FolderNode) {
Expand Down Expand Up @@ -281,15 +281,15 @@ async function uploadBatchOfFiles(

token.onCancellationRequested((e) => {
if (uploadRequests[requestIdx].ongoingUpload) {
uploadRequests[requestIdx].ongoingUpload?.abort()
void uploadRequests[requestIdx].ongoingUpload?.abort()
}
return failedRequests
})

while (!token.isCancellationRequested && requestIdx < uploadRequests.length) {
const request = uploadRequests[requestIdx]
const fileName = path.basename(request.key)
const destinationPath = readablePath({ bucket: { name: request.bucketName }, path: request.key })
const destinationPath = readablePath({ bucket: { Name: request.bucketName }, path: request.key })
showOutputMessage(
localize('AWS.s3.uploadFile.startUpload', 'Uploading file {0} to {1}', fileName, destinationPath),
outputChannel
Expand Down Expand Up @@ -377,23 +377,23 @@ async function uploadWithProgress(

const cancelled = new Promise<void>((_, reject) => {
token.onCancellationRequested((e) => {
currentStream.abort()
void currentStream.abort()
reject(new CancellationError('user'))
})
})

await Promise.race([currentStream.promise(), cancelled])
await Promise.race([currentStream.done(), cancelled])

return (request.ongoingUpload = undefined)
}

export interface BucketQuickPickItem extends vscode.QuickPickItem {
bucket: S3.Bucket | undefined
bucket: (Partial<S3Bucket> & { Name: string }) | undefined
folder?: Folder | undefined
}

interface SavedFolder {
bucket: Bucket
bucket: S3Bucket
folder: Folder
}

Expand All @@ -411,9 +411,9 @@ export async function promptUserForBucket(
promptUserFunction = promptUser,
createBucket = createBucketCommand
): Promise<BucketQuickPickItem | 'cancel' | 'back'> {
let allBuckets: S3.Bucket[]
let allBuckets: S3Bucket[]
try {
allBuckets = await s3client.listAllBuckets()
allBuckets = (await s3client.listBuckets()).buckets
} catch (e) {
getLogger().error('Failed to list buckets from client %O', e)
void vscode.window.showErrorMessage(
Expand All @@ -424,7 +424,7 @@ export async function promptUserForBucket(

const s3Buckets = allBuckets.filter((bucket) => {
return bucket && bucket.Name
}) as S3.Bucket[]
})

const createNewBucket: BucketQuickPickItem = {
label: localize('AWS.command.s3.createBucket', 'Create new bucket'),
Expand All @@ -443,7 +443,7 @@ export async function promptUserForBucket(
lastFolderItem = {
label: lastTouchedFolder.folder.name,
description: '(last opened S3 folder)',
bucket: { Name: lastTouchedFolder.bucket.name },
bucket: { Name: lastTouchedFolder.bucket.Name },
folder: lastTouchedFolder.folder,
}
}
Expand All @@ -454,7 +454,7 @@ export async function promptUserForBucket(
lastUploadedFolderItem = {
label: lastUploadedToFolder.folder.name,
description: '(last uploaded-to S3 folder)',
bucket: { Name: lastUploadedToFolder.bucket.name },
bucket: { Name: lastUploadedToFolder.bucket.Name },
folder: lastUploadedToFolder.folder,
}
}
Expand Down
18 changes: 9 additions & 9 deletions packages/core/src/awsService/s3/explorer/s3BucketNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import * as vscode from 'vscode'
import { ChildNodePage } from '../../../awsexplorer/childNodeLoader'
import { Bucket, CreateFolderRequest, CreateFolderResponse, S3Client } from '../../../shared/clients/s3Client'
import { S3Bucket, CreateFolderRequest, CreateFolderResponse, S3Client } from '../../../shared/clients/s3'

import { AWSResourceNode } from '../../../shared/treeview/nodes/awsResourceNode'
import { AWSTreeNodeBase } from '../../../shared/treeview/nodes/awsTreeNodeBase'
Expand All @@ -30,13 +30,13 @@ export class S3BucketNode extends AWSTreeNodeBase implements AWSResourceNode, Lo
private readonly childLoader = new ChildNodeLoader(this, (token) => this.loadPage(token))

public constructor(
public readonly bucket: Bucket,
public readonly bucket: S3Bucket,
public readonly parent: S3Node,
public readonly s3: S3Client,
protected readonly settings: ClassToInterfaceType<Settings> = Settings.instance
) {
super(bucket.name, vscode.TreeItemCollapsibleState.Collapsed)
this.tooltip = bucket.name
super(bucket.Name, vscode.TreeItemCollapsibleState.Collapsed)
this.tooltip = bucket.Name
this.iconPath = getIcon('aws-s3-bucket')
this.contextValue = 'awsS3BucketNode'
}
Expand Down Expand Up @@ -64,7 +64,7 @@ export class S3BucketNode extends AWSTreeNodeBase implements AWSResourceNode, Lo
private async loadPage(continuationToken: string | undefined): Promise<ChildNodePage<S3FolderNode | S3FileNode>> {
getLogger().debug(`Loading page for %O using continuationToken %s`, this, continuationToken)
const response = await this.s3.listFiles({
bucketName: this.bucket.name,
bucketName: this.bucket.Name,
continuationToken,
maxResults: this.getMaxItemsPerPage(),
})
Expand All @@ -90,15 +90,15 @@ export class S3BucketNode extends AWSTreeNodeBase implements AWSResourceNode, Lo
* See {@link S3Client.deleteBucket}.
*/
public async deleteBucket(): Promise<void> {
await this.s3.deleteBucket({ bucketName: this.bucket.name })
await this.s3.deleteBucket({ bucketName: this.bucket.Name })
}

public get arn(): string {
return this.bucket.arn
return this.bucket.Arn
}

public get name(): string {
return this.bucket.name
return this.bucket.Name
}

public get path(): string {
Expand All @@ -107,7 +107,7 @@ export class S3BucketNode extends AWSTreeNodeBase implements AWSResourceNode, Lo
}

public [inspect.custom](): string {
return `S3BucketNode (bucket=${this.bucket.name})`
return `S3BucketNode (bucket=${this.bucket.Name})`
}

private getMaxItemsPerPage(): number | undefined {
Expand Down
Loading

0 comments on commit 997b3b7

Please sign in to comment.