feat(client-s3): throw errors with 200 status code (#1657)

AllanZhengYP · web-flow · commit abed452caf9e · 2020-11-06T12:03:29.000-08:00
diff --git a/clients/client-s3/S3.spec.ts b/clients/client-s3/S3.spec.ts
@@ -1,8 +1,14 @@
 /// <reference types="mocha" />
-import { expect } from "chai";
-import { S3 } from "./S3";
-import { SerializeMiddleware, BuildMiddleware } from "@aws-sdk/types";
 import { HttpRequest } from "@aws-sdk/protocol-http";
+import { BuildMiddleware, SerializeMiddleware } from "@aws-sdk/types";
+import chai from "chai";
+import chaiAsPromised from "chai-as-promised";
+import { PassThrough } from "stream";
+
+import { S3 } from "./S3";
+
+chai.use(chaiAsPromised);
+const { expect } = chai;
 
 describe("endpoint", () => {
   it("users can override endpoint from client.", async () => {
@@ -93,3 +99,75 @@ describe("Accesspoint ARN", async () => {
     );
   });
 });
+
+describe("Throw 200 response", () => {
+  const response = {
+    statusCode: 200,
+    headers: {},
+    body: new PassThrough(),
+  };
+  const client = new S3({
+    region: "us-west-2",
+    requestHandler: {
+      handle: async () => ({
+        response,
+      }),
+    },
+  });
+  const errorBody = `<?xml version="1.0" encoding="UTF-8"?>
+    <Error>
+      <Code>InternalError</Code>
+      <Message>We encountered an internal error. Please try again.</Message>
+      <RequestId>656c76696e6727732072657175657374</RequestId>
+      <HostId>Uuag1LuByRx9e6j5Onimru9pO4ZVKnJ2Qz7/C1NPcfTWAtRPfTaOFg==</HostId>
+    </Error>`;
+  const params = {
+    Bucket: "bucket",
+    Key: "key",
+    CopySource: "source",
+  };
+
+  beforeEach(() => {
+    response.body = new PassThrough();
+  });
+
+  it("should throw if CopyObject() return with 200 and empty payload", async () => {
+    response.body.end("");
+    return expect(client.copyObject(params)).to.eventually.be.rejectedWith("S3 aborted request");
+  });
+
+  it("should throw if CopyObject() return with 200 and error preamble", async () => {
+    response.body.end(errorBody);
+    return expect(client.copyObject(params)).to.eventually.be.rejectedWith(
+      "We encountered an internal error. Please try again."
+    );
+  });
+
+  it("should throw if UploadPartCopy() return with 200 and empty payload", async () => {
+    response.body.end("");
+    return expect(client.uploadPartCopy({ ...params, UploadId: "id", PartNumber: 1 })).to.eventually.be.rejectedWith(
+      "S3 aborted request"
+    );
+  });
+
+  it("should throw if UploadPartCopy() return with 200 and error preamble", async () => {
+    response.body.end(errorBody);
+    return expect(client.uploadPartCopy({ ...params, UploadId: "id", PartNumber: 1 })).to.eventually.be.rejectedWith(
+      "We encountered an internal error. Please try again."
+    );
+  });
+
+  it("should throw if CompleteMultipartUpload() return with 200 and empty payload", async () => {
+    response.body.end("");
+    return expect(client.completeMultipartUpload({ ...params, UploadId: "id" })).to.eventually.be.rejectedWith(
+      "S3 aborted request"
+    );
+  });
+
+  it("should throw if CompleteMultipartUpload() return with 200 and error preamble", async () => {
+    response.body.end(errorBody);
+    return expect(client.completeMultipartUpload({ ...params, UploadId: "id" })).to.eventually.be.rejectedWith(
+      "We encountered an internal error. Please try again."
+    );
+  });
+});
diff --git a/clients/client-s3/commands/CompleteMultipartUploadCommand.ts b/clients/client-s3/commands/CompleteMultipartUploadCommand.ts
@@ -5,6 +5,7 @@ import {
   serializeAws_restXmlCompleteMultipartUploadCommand,
 } from "../protocols/Aws_restXml";
 import { getBucketEndpointPlugin } from "@aws-sdk/middleware-bucket-endpoint";
+import { getThrow200ExceptionsPlugin } from "@aws-sdk/middleware-sdk-s3";
 import { getSerdePlugin } from "@aws-sdk/middleware-serde";
 import { HttpRequest as __HttpRequest, HttpResponse as __HttpResponse } from "@aws-sdk/protocol-http";
 import { Command as $Command } from "@aws-sdk/smithy-client";
@@ -41,6 +42,7 @@ export class CompleteMultipartUploadCommand extends $Command<
     options?: __HttpHandlerOptions
   ): Handler<CompleteMultipartUploadCommandInput, CompleteMultipartUploadCommandOutput> {
     this.middlewareStack.use(getSerdePlugin(configuration, this.serialize, this.deserialize));
+    this.middlewareStack.use(getThrow200ExceptionsPlugin(configuration));
     this.middlewareStack.use(getBucketEndpointPlugin(configuration));
 
     const stack = clientStack.concat(this.middlewareStack);
diff --git a/clients/client-s3/commands/CopyObjectCommand.ts b/clients/client-s3/commands/CopyObjectCommand.ts
@@ -5,6 +5,7 @@ import {
   serializeAws_restXmlCopyObjectCommand,
 } from "../protocols/Aws_restXml";
 import { getBucketEndpointPlugin } from "@aws-sdk/middleware-bucket-endpoint";
+import { getThrow200ExceptionsPlugin } from "@aws-sdk/middleware-sdk-s3";
 import { getSerdePlugin } from "@aws-sdk/middleware-serde";
 import { getSsecPlugin } from "@aws-sdk/middleware-ssec";
 import { HttpRequest as __HttpRequest, HttpResponse as __HttpResponse } from "@aws-sdk/protocol-http";
@@ -42,6 +43,7 @@ export class CopyObjectCommand extends $Command<
     options?: __HttpHandlerOptions
   ): Handler<CopyObjectCommandInput, CopyObjectCommandOutput> {
     this.middlewareStack.use(getSerdePlugin(configuration, this.serialize, this.deserialize));
+    this.middlewareStack.use(getThrow200ExceptionsPlugin(configuration));
     this.middlewareStack.use(getSsecPlugin(configuration));
     this.middlewareStack.use(getBucketEndpointPlugin(configuration));
 
diff --git a/clients/client-s3/commands/UploadPartCopyCommand.ts b/clients/client-s3/commands/UploadPartCopyCommand.ts
@@ -5,6 +5,7 @@ import {
   serializeAws_restXmlUploadPartCopyCommand,
 } from "../protocols/Aws_restXml";
 import { getBucketEndpointPlugin } from "@aws-sdk/middleware-bucket-endpoint";
+import { getThrow200ExceptionsPlugin } from "@aws-sdk/middleware-sdk-s3";
 import { getSerdePlugin } from "@aws-sdk/middleware-serde";
 import { getSsecPlugin } from "@aws-sdk/middleware-ssec";
 import { HttpRequest as __HttpRequest, HttpResponse as __HttpResponse } from "@aws-sdk/protocol-http";
@@ -42,6 +43,7 @@ export class UploadPartCopyCommand extends $Command<
     options?: __HttpHandlerOptions
   ): Handler<UploadPartCopyCommandInput, UploadPartCopyCommandOutput> {
     this.middlewareStack.use(getSerdePlugin(configuration, this.serialize, this.deserialize));
+    this.middlewareStack.use(getThrow200ExceptionsPlugin(configuration));
     this.middlewareStack.use(getSsecPlugin(configuration));
     this.middlewareStack.use(getBucketEndpointPlugin(configuration));
 
diff --git a/codegen/smithy-aws-typescript-codegen/src/main/java/software/amazon/smithy/aws/typescript/codegen/AddS3Config.java b/codegen/smithy-aws-typescript-codegen/src/main/java/software/amazon/smithy/aws/typescript/codegen/AddS3Config.java
@@ -63,6 +63,12 @@ public final class AddS3Config implements TypeScriptIntegration {
         "ListBuckets"
     );
 
+    private static final Set<String> EXCEPTIONS_OF_200_OPERATIONS = SetUtils.of(
+        "CopyObject",
+        "UploadPartCopy",
+        "CompleteMultipartUpload"
+    );
+
     @Override
     public void addConfigInterfaceFields(TypeScriptSettings settings, Model model, SymbolProvider symbolProvider,
             TypeScriptWriter writer) {
@@ -118,6 +124,13 @@ public List<RuntimeClientPlugin> getClientPlugins() {
                                         HAS_MIDDLEWARE)
                         .servicePredicate((m, s) -> testServiceId(s))
                         .build(),
+                RuntimeClientPlugin.builder()
+                                .withConventions(AwsDependency.S3_MIDDLEWARE.dependency, "throw200Exceptions",
+                                        HAS_MIDDLEWARE)
+                                .operationPredicate(
+                                        (m, s, o) -> EXCEPTIONS_OF_200_OPERATIONS.contains(o.getId().getName())
+                                                && testServiceId(s))
+                                .build(),
                 RuntimeClientPlugin.builder()
                         .withConventions(AwsDependency.ADD_EXPECT_CONTINUE.dependency, "AddExpectContinue",
                                         HAS_MIDDLEWARE)
diff --git a/packages/middleware-sdk-s3/src/index.ts b/packages/middleware-sdk-s3/src/index.ts
@@ -1,2 +1,3 @@
 export * from "./validate-bucket-name";
 export * from "./use-regional-endpoint";
+export * from "./throw-200-exceptions";
diff --git a/packages/middleware-sdk-s3/src/throw-200-exceptions.spec.ts b/packages/middleware-sdk-s3/src/throw-200-exceptions.spec.ts
@@ -0,0 +1,72 @@
+import { HttpRequest, HttpResponse } from "@aws-sdk/protocol-http";
+
+import { throw200ExceptionsMiddleware } from "./throw-200-exceptions";
+
+describe("throw200ExceptionsMiddlewareOptions", () => {
+  const mockNextHandler = jest.fn();
+  const mockStreamCollector = jest.fn();
+  const mockUtf8Encoder = jest.fn();
+  const mockConfig = {
+    streamCollector: mockStreamCollector,
+    utf8Encoder: mockUtf8Encoder,
+  };
+
+  beforeEach(() => {
+    jest.clearAllMocks();
+  });
+
+  it("should throw if response body is empty", async () => {
+    expect.assertions(3);
+    mockStreamCollector.mockResolvedValue(Buffer.from(""));
+    mockUtf8Encoder.mockReturnValue("");
+    mockNextHandler.mockReturnValue({
+      response: new HttpResponse({
+        statusCode: 200,
+        headers: {},
+        body: "",
+      }),
+    });
+    const handler = throw200ExceptionsMiddleware(mockConfig)(mockNextHandler, {} as any);
+    try {
+      await handler({
+        input: {},
+        request: new HttpRequest({
+          hostname: "s3.us-east-1.amazonaws.com",
+        }),
+      });
+    } catch (e) {
+      expect(e).toBeDefined();
+      expect(e.name).toEqual("InternalError");
+      expect(e.message).toEqual("S3 aborted request");
+    }
+  });
+
+  it("should throw if response body contains Error tag", async () => {
+    const errorBody = `<?xml version="1.0" encoding="UTF-8"?>
+    <Error>
+      <Code>InternalError</Code>
+      <Message>We encountered an internal error. Please try again.</Message>
+      <RequestId>656c76696e6727732072657175657374</RequestId>
+      <HostId>Uuag1LuByRx9e6j5Onimru9pO4ZVKnJ2Qz7/C1NPcfTWAtRPfTaOFg==</HostId>
+    </Error>`;
+    mockStreamCollector.mockResolvedValue(Buffer.from(errorBody));
+    mockUtf8Encoder.mockReturnValue(errorBody);
+    mockNextHandler.mockReturnValue({
+      response: new HttpResponse({
+        statusCode: 200,
+        headers: {},
+        body: "",
+      }),
+    });
+    const handler = throw200ExceptionsMiddleware(mockConfig)(mockNextHandler, {} as any);
+    const { response } = await handler({
+      input: {},
+      request: new HttpRequest({
+        hostname: "s3.us-east-1.amazonaws.com",
+      }),
+    });
+    expect(HttpResponse.isInstance(response)).toBe(true);
+    // @ts-ignore
+    expect(response.statusCode).toBeGreaterThanOrEqual(400);
+  });
+});
diff --git a/packages/middleware-sdk-s3/src/throw-200-exceptions.ts b/packages/middleware-sdk-s3/src/throw-200-exceptions.ts
@@ -0,0 +1,72 @@
+import { HttpResponse } from "@aws-sdk/protocol-http";
+import { DeserializeMiddleware, Encoder, Pluggable, RelativeMiddlewareOptions, StreamCollector } from "@aws-sdk/types";
+
+type PreviouslyResolved = {
+  streamCollector: StreamCollector;
+  utf8Encoder: Encoder;
+};
+
+/**
+ * In case of an internal error/terminated connection, S3 operations may return 200 errors. CopyObject, UploadPartCopy,
+ * CompleteMultipartUpload may return empty payload or payload with only xml Preamble.
+ * @internal
+ */
+export const throw200ExceptionsMiddleware = (config: PreviouslyResolved): DeserializeMiddleware<any, any> => (
+  next
+) => async (args) => {
+  const result = await next(args);
+  const { response } = result;
+  if (!HttpResponse.isInstance(response)) return result;
+  const { statusCode, body } = response;
+  if (statusCode < 200 && statusCode >= 300) return result;
+
+  // Throw 2XX response that's either an error or has empty body.
+  const bodyBytes = await collectBody(body, config);
+  const bodyString = await collectBodyString(bodyBytes, config);
+  if (bodyBytes.length === 0) {
+    const err = new Error("S3 aborted request");
+    err.name = "InternalError";
+    throw err;
+  }
+  if (bodyString && bodyString.match("<Error>")) {
+    // Set the error code to 4XX so that error deserializer can parse them
+    response.statusCode = 400;
+  }
+
+  // Body stream is consumed and paused at this point. So replace the response.body to the collected bytes.
+  // So that the deserializer can consume the body as normal.
+  response.body = bodyBytes;
+  return result;
+};
+
+// Collect low-level response body stream to Uint8Array.
+const collectBody = (streamBody: any = new Uint8Array(), context: PreviouslyResolved): Promise<Uint8Array> => {
+  if (streamBody instanceof Uint8Array) {
+    return Promise.resolve(streamBody);
+  }
+  return context.streamCollector(streamBody) || Promise.resolve(new Uint8Array());
+};
+
+// Encode Uint8Array data into string with utf-8.
+const collectBodyString = (streamBody: any, context: PreviouslyResolved): Promise<string> =>
+  collectBody(streamBody, context).then((body) => context.utf8Encoder(body));
+
+/**
+ * @internal
+ */
+export const throw200ExceptionsMiddlewareOptions: RelativeMiddlewareOptions = {
+  relation: "after",
+  toMiddleware: "deserializerMiddleware",
+  tags: ["THROW_200_EXCEPTIONS", "S3"],
+  name: "throw200ExceptionsMiddleware",
+};
+
+/**
+ *
+ * @internal
+ */
+export const getThrow200ExceptionsPlugin = (config: PreviouslyResolved): Pluggable<any, any> => ({
+  applyToStack: (clientStack) => {
+    clientStack.addRelativeTo(throw200ExceptionsMiddleware(config), throw200ExceptionsMiddlewareOptions);
+  },
+});
diff --git a/packages/middleware-sdk-s3/src/use-regional-endpoint.ts b/packages/middleware-sdk-s3/src/use-regional-endpoint.ts
@@ -15,6 +15,9 @@ type PreviouslyResolved = {
   isCustomEndpoint: boolean;
 };
 
+/**
+ * @internal
+ */
 export const useRegionalEndpointMiddleware = (config: PreviouslyResolved): BuildMiddleware<any, any> => <
   Output extends MetadataBearer
 >(
@@ -30,12 +33,18 @@ export const useRegionalEndpointMiddleware = (config: PreviouslyResolved): Build
   return next({ ...args });
 };
 
+/**
+ * @internal
+ */
 export const useRegionalEndpointMiddlewareOptions: BuildHandlerOptions = {
   step: "build",
   tags: ["USE_REGIONAL_ENDPOINT", "S3"],
   name: "useRegionalEndpointMiddleware",
 };
 
+/**
+ * @internal
+ */
 export const getUseRegionalEndpointPlugin = (config: PreviouslyResolved): Pluggable<any, any> => ({
   applyToStack: (clientStack) => {
     clientStack.add(useRegionalEndpointMiddleware(config), useRegionalEndpointMiddlewareOptions);
diff --git a/packages/middleware-sdk-s3/src/validate-bucket-name.ts b/packages/middleware-sdk-s3/src/validate-bucket-name.ts
@@ -9,6 +9,9 @@ import {
 } from "@aws-sdk/types";
 import { validate as validateArn } from "@aws-sdk/util-arn-parser";
 
+/**
+ * @internal
+ */
 export function validateBucketNameMiddleware(): InitializeMiddleware<any, any> {
   return <Output extends MetadataBearer>(
     next: InitializeHandler<any, Output>
@@ -27,12 +30,18 @@ export function validateBucketNameMiddleware(): InitializeMiddleware<any, any> {
   };
 }
 
+/**
+ * @internal
+ */
 export const validateBucketNameMiddlewareOptions: InitializeHandlerOptions = {
   step: "initialize",
   tags: ["VALIDATE_BUCKET_NAME"],
   name: "validateBucketNameMiddleware",
 };
 
+/**
+ * @internal
+ */
 // eslint-disable-next-line @typescript-eslint/no-unused-vars
 export const getValidateBucketNamePlugin = (unused: any): Pluggable<any, any> => ({
   applyToStack: (clientStack) => {

Original file line number	Diff line number	Diff line change
`@@ -1,2 +1,3 @@`
`1`	`1`	`export * from "./validate-bucket-name";`
`2`	`2`	`export * from "./use-regional-endpoint";`
	`3`	`+export * from "./throw-200-exceptions";`