Skip to content

Commit

Permalink
fix(sdk): fix race condition in bucket.tryget() and `bucket.trygetj…
Browse files Browse the repository at this point in the history 
…son()` for aws targets (#3655)

Closes #2821

*By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*.
  • Loading branch information
@garysassano
garysassano authored Aug 21, 2023
1 parent 02e2056 commit 1fa4e97
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 35 deletions.
73 changes: 41 additions & 32 deletions libs/wingsdk/src/shared-aws/bucket.inflight.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
GetPublicAccessBlockCommandOutput,
S3Client,
GetObjectOutput,
NoSuchKey,
} from "@aws-sdk/client-s3";
import { BucketDeleteOptions, IBucketClient } from "../cloud";
import { Duration, Json } from "../std";
Expand Down Expand Up @@ -70,34 +71,46 @@ export class BucketClient implements IBucketClient {
}

/**
* Get an object from the bucket
*
* @param key Key of the object
* @returns content of the object
* See https://github.com/aws/aws-sdk-js-v3/issues/1877
*/
public async get(key: string): Promise<string> {
// See https://github.com/aws/aws-sdk-js-v3/issues/1877
private async getObjectContent(key: string): Promise<string | undefined> {
const command = new GetObjectCommand({
Bucket: this.bucketName,
Key: key,
});
let resp: GetObjectOutput;

try {
resp = await this.s3Client.send(command);
const resp: GetObjectOutput = await this.s3Client.send(command);
const objectContent = resp.Body as Readable;
try {
return await consumers.text(objectContent);
} catch (e) {
throw new Error(
`Object content could not be read as text (key=${key}): ${
(e as Error).stack
})}`
);
}
} catch (e) {
throw new Error(
`Object does not exist (key=${key}): ${(e as Error).stack}`
);
if (e instanceof NoSuchKey) {
return undefined;
}
throw new Error((e as Error).stack);
}
try {
return await consumers.text(resp.Body as Readable);
} catch (e) {
throw new Error(
`Object contents could not be read as text (key=${key}): ${
(e as Error).stack
})}`
);
}

/**
* Get an object from the bucket
*
* @param key Key of the object
* @returns content of the object
*/
public async get(key: string): Promise<string> {
const objectContent = await this.getObjectContent(key);
if (objectContent !== undefined) {
return objectContent;
}
throw new Error(`Object does not exist (key=${key}).`);
}

/**
Expand All @@ -107,11 +120,7 @@ export class BucketClient implements IBucketClient {
* @returns content of the object
*/
public async tryGet(key: string): Promise<string | undefined> {
if (await this.exists(key)) {
return this.get(key);
}

return undefined;
return this.getObjectContent(key);
}

/**
Expand All @@ -131,11 +140,12 @@ export class BucketClient implements IBucketClient {
* @returns Json content of the object
*/
public async tryGetJson(key: string): Promise<Json | undefined> {
if (await this.exists(key)) {
return this.getJson(key);
const objectContent = await this.tryGet(key);
if (objectContent !== undefined) {
return JSON.parse(objectContent);
} else {
return undefined;
}

return undefined;
}

/**
Expand All @@ -158,13 +168,12 @@ export class BucketClient implements IBucketClient {

try {
await this.s3Client.send(command);
} catch (er) {
const error = er as any;
if (!mustExist && error.name === "NoSuchKey") {
} catch (e: any) {
if (!mustExist && e instanceof NoSuchKey) {
return;
}

throw new Error(`Unable to delete "${key}": ${error.message}`);
throw new Error(`Unable to delete "${key}": ${e.message}`);
}
}

Expand Down
41 changes: 38 additions & 3 deletions libs/wingsdk/test/shared-aws/bucket.inflight.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
ListObjectsV2Command,
PutObjectCommand,
S3Client,
NoSuchKey,
} from "@aws-sdk/client-s3";
import { SdkStream } from "@aws-sdk/types";
import { sdkStreamMixin } from "@aws-sdk/util-stream-node";
Expand Down Expand Up @@ -93,7 +94,7 @@ test("get a non-existent object from the bucket", async () => {
const VALUE = "VALUE";
s3Mock
.on(GetObjectCommand, { Bucket: BUCKET_NAME, Key: KEY })
.rejects(new Error("fake error"));
.rejects(new Error("Object does not exist"));

// WHEN
const client = new BucketClient(BUCKET_NAME);
Expand Down Expand Up @@ -372,7 +373,7 @@ test("tryGet a non-existent object from the bucket", async () => {
const VALUE = "VALUE";
s3Mock
.on(GetObjectCommand, { Bucket: BUCKET_NAME, Key: KEY })
.rejects(new Error("fake error"));
.rejects(new NoSuchKey({ message: "NoSuchKey error", $metadata: {} }));
s3Mock
.on(HeadObjectCommand, { Bucket: BUCKET_NAME, Key: KEY })
.rejects({ name: "NotFound" });
Expand All @@ -385,6 +386,23 @@ test("tryGet a non-existent object from the bucket", async () => {
expect(objectTryGet).toEqual(undefined);
});

test("tryGet object from the bucket throws an unknown error", async () => {
// GIVEN
const BUCKET_NAME = "BUCKET_NAME";
const KEY = "KEY";
const VALUE = "VALUE";
s3Mock
.on(GetObjectCommand, { Bucket: BUCKET_NAME, Key: KEY })
.rejects(new Error("unknown error"));
s3Mock.on(HeadObjectCommand, { Bucket: BUCKET_NAME, Key: KEY }).resolves({});

// WHEN
const client = new BucketClient(BUCKET_NAME);

// THEN
await expect(() => client.tryGet(KEY)).rejects.toThrowError(/unknown error/);
});

test("tryGetJson an existing object from the bucket", async () => {
// GIVEN
const BUCKET_NAME = "BUCKET_NAME";
Expand Down Expand Up @@ -418,7 +436,7 @@ test("tryGetJson a non-existent object from the bucket", async () => {
const VALUE = { msg: "Hello, World!" };
s3Mock
.on(GetObjectCommand, { Bucket: BUCKET_NAME, Key: KEY })
.rejects(new Error("fake error"));
.rejects(new NoSuchKey({ message: "NoSuchKey error", $metadata: {} }));
s3Mock
.on(HeadObjectCommand, { Bucket: BUCKET_NAME, Key: KEY })
.rejects({ name: "NotFound" });
Expand All @@ -431,6 +449,23 @@ test("tryGetJson a non-existent object from the bucket", async () => {
expect(objectTryGetJson).toEqual(undefined);
});

test("tryGetJson object from the bucket throws an unknown error", async () => {
// GIVEN
const BUCKET_NAME = "BUCKET_NAME";
const KEY = "KEY";
const VALUE = { msg: "Hello, World!" };
s3Mock
.on(GetObjectCommand, { Bucket: BUCKET_NAME, Key: KEY })
.rejects(new Error("unknown error"));
s3Mock.on(HeadObjectCommand, { Bucket: BUCKET_NAME, Key: KEY }).resolves({});

// WHEN
const client = new BucketClient(BUCKET_NAME);

// THEN
await expect(() => client.tryGet(KEY)).rejects.toThrowError(/unknown error/);
});

test("tryGetJson an existing non-Json object from the bucket", async () => {
// GIVEN
const BUCKET_NAME = "BUCKET_NAME";
Expand Down

0 comments on commit 1fa4e97

Please sign in to comment.