fix(protocols): fix header serde, handle unset union payloads

.changeset/witty-rings-do.md

@@ -0,0 +1,5 @@
+---
+"@smithy/smithy-client": minor
+---
+
+add quoteHeader function

packages/smithy-client/src/index.ts

@@ -18,7 +18,9 @@ export * from "./lazy-json";
 export * from "./NoOpLogger";
 export * from "./object-mapping";
 export * from "./parse-utils";
+export * from "./quote-header";
 export * from "./resolve-path";
 export * from "./ser-utils";
 export * from "./serde-json";
 export * from "./split-every";
+export * from "./split-header";

packages/smithy-client/src/quote-header.spec.ts

@@ -0,0 +1,19 @@
+import { quoteHeader } from "./quote-header";
+
+describe(quoteHeader.name, () => {
+  it("should not wrap header elements that don't include the delimiter or double quotes", () => {
+    expect(quoteHeader("bc")).toBe("bc");
+  });
+
+  it("should wrap header elements that include the delimiter", () => {
+    expect(quoteHeader("b,c")).toBe('"b,c"');
+  });
+
+  it("should wrap header elements that include double quotes", () => {
+    expect(quoteHeader(`"bc"`)).toBe('"\\"bc\\""');
+  });
+
+  it("should wrap header elements that include the delimiter and double quotes", () => {
+    expect(quoteHeader(`"b,c"`)).toBe('"\\"b,c\\""');
+  });
+});

packages/smithy-client/src/quote-header.ts

@@ -0,0 +1,11 @@
+/**
+ * @public
+ * @param part - header list element
+ * @returns quoted string if part contains delimiter.
+ */
+export function quoteHeader(part: string) {
+  if (part.includes(",") || part.includes('"')) {
+    part = `"${part.replace(/"/g, '\\"')}"`;
+  }
+  return part;
+}

packages/smithy-client/src/split-header.spec.ts

@@ -0,0 +1,15 @@
+import { splitHeader } from "./split-header";
+
+describe(splitHeader.name, () => {
+  it("should split a string by commas and trim only the comma delimited outer values", () => {
+    expect(splitHeader("abc")).toEqual(["abc"]);
+    expect(splitHeader("a,b,c")).toEqual(["a", "b", "c"]);
+    expect(splitHeader("a, b, c")).toEqual(["a", "b", "c"]);
+    expect(splitHeader("a , b , c")).toEqual(["a", "b", "c"]);
+    expect(splitHeader(`a , b , "  c  "`)).toEqual(["a", "b", "  c  "]);
+  });
+  it("should split a string by commas that are not in quotes, and remove outer quotes", () => {
+    expect(splitHeader('"b,c", "\\"def\\"", a')).toEqual(["b,c", '"def"', "a"]);
+    expect(splitHeader('"a,b,c", ""def"", "a,b ,c"')).toEqual(["a,b,c", '"def"', "a,b ,c"]);
+  });
+});

packages/smithy-client/src/split-header.ts

@@ -0,0 +1,42 @@
+/**
+ * @param value - header string value.
+ * @returns value split by commas that aren't in quotes.
+ */
+export const splitHeader = (value: string): string[] => {
+  const z = value.length;
+  const values = [];
+
+  let withinQuotes = false;
+  let prevChar = undefined;
+  let anchor = 0;
+
+  for (let i = 0; i < z; ++i) {
+    const char = value[i];
+    switch (char) {
+      case `"`:
+        if (prevChar !== "\\") {
+          withinQuotes = !withinQuotes;
+        }
+        break;
+      case ",":
+        if (!withinQuotes) {
+          values.push(value.slice(anchor, i));
+          anchor = i + 1;
+        }
+        break;
+      default:
+    }
+    prevChar = char;
+  }
+
+  values.push(value.slice(anchor));
+
+  return values.map((v) => {
+    v = v.trim();
+    const z = v.length;
+    if (v[0] === `"` && v[z - 1] === `"`) {
+      v = v.slice(1, z - 1);
+    }
+    return v.replace(/\\"/g, '"');
+  });
+};

private/smithy-rpcv2-cbor/test/functional/rpcv2cbor.spec.ts

@@ -697,7 +697,7 @@ it("no_input:Request", async () => {
     expect(r.headers["smithy-protocol"]).toBeDefined();
     expect(r.headers["smithy-protocol"]).toBe("rpc-v2-cbor");
 
-    expect(r.body).toBeFalsy();
+    expect(!r.body || r.body === `{}`).toBeTruthy();
   }
 });
 

smithy-typescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/HttpProtocolTestGenerator.java

@@ -71,6 +71,7 @@
 import software.amazon.smithy.utils.IoUtils;
 import software.amazon.smithy.utils.MapUtils;
 import software.amazon.smithy.utils.Pair;
+import software.amazon.smithy.utils.SetUtils;
 import software.amazon.smithy.utils.SmithyInternalApi;
 
 /**
@@ -91,6 +92,9 @@ public final class HttpProtocolTestGenerator implements Runnable {
 
     private static final Logger LOGGER = Logger.getLogger(HttpProtocolTestGenerator.class.getName());
     private static final String TEST_CASE_FILE_TEMPLATE = "test/functional/%s.spec.ts";
+    private static final Set<String> IGNORE_COMMA_SPACING = SetUtils.of(
+        "content-encoding"
+    );
 
     private final TypeScriptSettings settings;
     private final Model model;
@@ -517,7 +521,17 @@ private void writeHttpHeaderAssertions(HttpMessageTestCase testCase) {
         testCase.getHeaders().forEach((header, value) -> {
             header = header.toLowerCase();
             writer.write("expect(r.headers[$S]).toBeDefined();", header);
-            writer.write("expect(r.headers[$S]).toBe($S);", header, value);
+            if (IGNORE_COMMA_SPACING.contains(header) && value.contains(",")) {
+                writer.write("""
+                    expect(
+                      r.headers[$S].replaceAll(", ", ",")
+                    ).toBe(
+                      $S.replaceAll(", ", ",")
+                    );
+                    """, header, value);
+            } else {
+                writer.write("expect(r.headers[$S]).toBe($S);", header, value);
+            }
         });
         writer.write("");
     }
@@ -531,9 +545,11 @@ private void writeHttpHostAssertion(HttpRequestTestCase testCase) {
     }
 
     private void writeHttpBodyAssertions(String body, String mediaType, boolean isClientTest) {
-        // If we expect an empty body, expect it to be falsy.
         if (body.isEmpty()) {
-            writer.write("expect(r.body).toBeFalsy();");
+            // If we expect an empty body, expect it to be falsy.
+            // Or, for JSON an empty object represents an empty body.
+            // mediaType is often UNKNOWN here.
+            writer.write("expect(!r.body || r.body === `{}`).toBeTruthy();");
             return;
         }
 

smithy-typescript-codegen/src/main/java/software/amazon/smithy/typescript/codegen/integration/HttpBindingProtocolGenerator.java

@@ -1385,6 +1385,11 @@ private String getCollectionInputParam(
 
         switch (bindingType) {
             case HEADER:
+                if (collectionTarget.isStringShape()) {
+                    context.getWriter().addImport(
+                        "quoteHeader", "__quoteHeader", TypeScriptDependency.AWS_SMITHY_CLIENT);
+                    return iteratedParam + ".map(__quoteHeader).join(', ')";
+                }
                 return iteratedParam + ".join(', ')";
             case QUERY:
             case QUERY_PARAMS:
@@ -2464,18 +2469,31 @@ private HttpBinding readPayload(
                     + "= __expectObject(await parseBody(output.body, context));");
         } else if (target instanceof UnionShape) {
             // If payload is a Union, then we need to parse the string into JavaScript object.
-            importUnionDeserializer(writer);
             writer.write("const data: Record<string, any> | undefined "
-                    + "= __expectUnion(await parseBody(output.body, context));");
+                    + "= await parseBody(output.body, context);");
         } else if (target instanceof StringShape || target instanceof DocumentShape) {
             // If payload is String or Document, we need to collect body and convert binary to string.
             writer.write("const data: any = await collectBodyString(output.body, context);");
         } else {
             throw new CodegenException(String.format("Unexpected shape type bound to payload: `%s`",
                     target.getType()));
         }
-        writer.write("contents.$L = $L;", binding.getMemberName(), getOutputValue(context,
+
+        if (target instanceof UnionShape) {
+            writer.openBlock(
+                "if (Object.keys(data ?? {}).length) {",
+                "}",
+                () -> {
+                    importUnionDeserializer(writer);
+                    writer.write("contents.$L = __expectUnion($L);", binding.getMemberName(), getOutputValue(context,
+                        Location.PAYLOAD, "data", binding.getMember(), target));
+                }
+            );
+        } else {
+            writer.write("contents.$L = $L;", binding.getMemberName(), getOutputValue(context,
                 Location.PAYLOAD, "data", binding.getMember(), target));
+        }
+
         return binding;
     }
 
@@ -2716,7 +2734,8 @@ private String getCollectionOutputParam(
             case HEADER:
                 dataSource = "(" + dataSource + " || \"\")";
                 // Split these values on commas.
-                outputParam = dataSource + ".split(',')";
+                context.getWriter().addImport("splitHeader", "__splitHeader", TypeScriptDependency.AWS_SMITHY_CLIENT);
+                outputParam = "__splitHeader(" + dataSource + ")";
 
                 // Headers that have HTTP_DATE formatted timestamps already contain a ","
                 // in their formatted entry, so split on every other "," instead.