chore(protocol-http): guidance for HttpRequest cloning (#1333)

Author: kuhe

.changeset/khaki-seahorses-cough.md

@@ -0,0 +1,5 @@
+---
+"@smithy/protocol-http": patch
+---
+
+add guidance for HttpRequest cloning

packages/protocol-http/src/httpRequest.spec.ts

@@ -0,0 +1,57 @@
+import { HttpRequest, IHttpRequest } from "./httpRequest";
+
+describe("HttpRequest", () => {
+  const httpRequest: IHttpRequest = {
+    headers: {
+      hKey: "header-value",
+    },
+    query: {
+      qKey: "query-value",
+    },
+    method: "GET",
+    protocol: "https",
+    hostname: "localhost",
+    path: "/",
+    body: [],
+  };
+
+  it("should statically clone with deep-cloned headers/query but shallow cloned body", () => {
+    const httpRequest: IHttpRequest = {
+      headers: {
+        hKey: "header-value",
+      },
+      query: {
+        qKey: "query-value",
+      },
+      method: "GET",
+      protocol: "https",
+      hostname: "localhost",
+      path: "/",
+      body: [],
+    };
+
+    const clone1 = HttpRequest.clone(httpRequest);
+    const clone2 = HttpRequest.clone(httpRequest);
+
+    expect(new HttpRequest(httpRequest)).toEqual(clone1);
+    expect(clone1).toEqual(clone2);
+
+    expect(clone1.query).not.toBe(clone2.query);
+    expect(clone1.headers).not.toBe(clone2.headers);
+    expect(clone1.body).toBe(clone2.body);
+  });
+
+  it("should maintain a deprecated instance clone method", () => {
+    const httpRequestInstance = new HttpRequest(httpRequest);
+
+    const clone1 = httpRequestInstance.clone();
+    const clone2 = httpRequestInstance.clone();
+
+    expect(httpRequestInstance).toEqual(clone1);
+    expect(clone1).toEqual(clone2);
+
+    expect(clone1.query).not.toBe(clone2.query);
+    expect(clone1.headers).not.toBe(clone2.headers);
+    expect(clone1.body).toBe(clone2.body);
+  });
+});

packages/protocol-http/src/httpRequest.ts

@@ -3,8 +3,24 @@ import { HeaderBag, HttpMessage, HttpRequest as IHttpRequest, QueryParameterBag,
 
 type HttpRequestOptions = Partial<HttpMessage> & Partial<URI> & { method?: string };
 
+/**
+ * Use the distinct IHttpRequest interface from @smithy/types instead.
+ * This should not be used due to
+ * overlapping with the concrete class' name.
+ *
+ * This is not marked deprecated since that would mark the concrete class
+ * deprecated as well.
+ */
 export interface HttpRequest extends IHttpRequest {}
 
+/**
+ * @public
+ */
+export { IHttpRequest };
+
+/**
+ * @public
+ */
 export class HttpRequest implements HttpMessage, URI {
   public method: string;
   public protocol: string;
@@ -18,7 +34,7 @@ export class HttpRequest implements HttpMessage, URI {
   public fragment?: string;
   public body?: any;
 
-  constructor(options: HttpRequestOptions) {
+  public constructor(options: HttpRequestOptions) {
     this.method = options.method || "GET";
     this.hostname = options.hostname || "localhost";
     this.port = options.port;
@@ -36,9 +52,31 @@ export class HttpRequest implements HttpMessage, URI {
     this.fragment = options.fragment;
   }
 
-  static isInstance(request: unknown): request is HttpRequest {
-    //determine if request is a valid httpRequest
-    if (!request) return false;
+  /**
+   * Note: this does not deep-clone the body.
+   */
+  public static clone(request: IHttpRequest) {
+    const cloned = new HttpRequest({
+      ...request,
+      headers: { ...request.headers },
+    });
+    if (cloned.query) {
+      cloned.query = cloneQuery(cloned.query);
+    }
+    return cloned;
+  }
+
+  /**
+   * This method only actually asserts that request is the interface {@link IHttpRequest},
+   * and not necessarily this concrete class. Left in place for API stability.
+   *
+   * Do not call instance methods on the input of this function, and
+   * do not assume it has the HttpRequest prototype.
+   */
+  public static isInstance(request: unknown): request is HttpRequest {
+    if (!request) {
+      return false;
+    }
     const req: any = request;
     return (
       "method" in req &&
@@ -50,13 +88,13 @@ export class HttpRequest implements HttpMessage, URI {
     );
   }
 
-  clone(): HttpRequest {
-    const cloned = new HttpRequest({
-      ...this,
-      headers: { ...this.headers },
-    });
-    if (cloned.query) cloned.query = cloneQuery(cloned.query);
-    return cloned;
+  /**
+   * @deprecated use static HttpRequest.clone(request) instead. It's not safe to call
+   * this method because {@link HttpRequest.isInstance} incorrectly
+   * asserts that IHttpRequest (interface) objects are of type HttpRequest (class).
+   */
+  public clone(): HttpRequest {
+    return HttpRequest.clone(this);
   }
 }