Allow HTTP 204 response in the gRPC-web client (#656)

packages/connect-web-bench/README.md

@@ -10,5 +10,5 @@ it like a web server would usually do.
 
 | code generator | bundle size        | minified               | compressed           |
 |----------------|-------------------:|-----------------------:|---------------------:|
-| connect        | 112,762 b | 49,287 b | 13,236 b |
+| connect        | 111,912 b | 48,912 b | 13,145 b |
 | grpc-web       | 414,906 b    | 301,127 b    | 53,279 b |

packages/connect-web/src/connect-transport.ts

@@ -186,7 +186,6 @@ export function createConnectTransport(
           });
           const { isUnaryError, unaryError } = validateResponse(
             method.kind,
-            useBinaryFormat,
             response.status,
             response.headers
           );
@@ -302,12 +301,7 @@ export function createConnectTransport(
             signal: req.signal,
             body: await createRequestBody(req.message),
           });
-          validateResponse(
-            method.kind,
-            useBinaryFormat,
-            fRes.status,
-            fRes.headers
-          );
+          validateResponse(method.kind, fRes.status, fRes.headers);
           if (fRes.body === null) {
             throw "missing response body";
           }

packages/connect-web/src/grpc-web-transport.ts

@@ -162,7 +162,7 @@ export function createGrpcWebTransport(
             signal: req.signal,
             body: encodeEnvelope(0, serialize(req.message)),
           });
-          validateResponse(useBinaryFormat, response.status, response.headers);
+          validateResponse(response.status, response.headers);
           if (!response.body) {
             throw "missing response body";
           }
@@ -313,11 +313,7 @@ export function createGrpcWebTransport(
             signal: req.signal,
             body: await createRequestBody(req.message),
           });
-          const { foundStatus } = validateResponse(
-            useBinaryFormat,
-            fRes.status,
-            fRes.headers
-          );
+          const { foundStatus } = validateResponse(fRes.status, fRes.headers);
           if (!fRes.body) {
             throw "missing response body";
           }

packages/connect/src/protocol-connect/transport.spec.ts

@@ -36,7 +36,6 @@ import {
   contentTypeStreamProto,
   contentTypeUnaryProto,
 } from "./content-type.js";
-import type { Transport } from "../transport.js";
 import { errorToJsonBytes } from "./error-json.js";
 
 const TestService = {
@@ -87,70 +86,6 @@ describe("Connect transport", function () {
     useBinaryFormat: true,
     writeMaxBytes: 0xffffff,
   };
-  describe("against server responding with unexpected content type", function () {
-    let httpRequestAborted = false;
-    let transport: Transport = null as unknown as Transport;
-    beforeEach(function () {
-      httpRequestAborted = false;
-      transport = createTransport({
-        httpClient(
-          request: UniversalClientRequest
-        ): Promise<UniversalClientResponse> {
-          request.signal?.addEventListener(
-            "abort",
-            () => (httpRequestAborted = true)
-          );
-          return Promise.resolve({
-            status: 200,
-            header: new Headers({
-              "Content-Type": "application/csv",
-            }),
-            body: createAsyncIterable([]),
-            trailer: new Headers(),
-          });
-        },
-        ...defaultOptions,
-      });
-    });
-    it("should cancel the HTTP request for unary", async function () {
-      try {
-        await transport.unary(
-          TestService,
-          TestService.methods.unary,
-          undefined,
-          undefined,
-          undefined,
-          {}
-        );
-        fail("expected error");
-      } catch (e) {
-        expect(e).toBeInstanceOf(ConnectError);
-        expect(ConnectError.from(e).message).toBe(
-          '[invalid_argument] unexpected response content type "application/csv"'
-        );
-      }
-      expect(httpRequestAborted).toBeTrue();
-    });
-    it("should cancel the HTTP request for server-streaming", async function () {
-      try {
-        await transport.stream(
-          TestService,
-          TestService.methods.unary,
-          undefined,
-          undefined,
-          undefined,
-          createAsyncIterable([])
-        );
-        fail("expected error");
-      } catch (e) {
-        expect(e).toBeInstanceOf(ConnectError);
-        expect(ConnectError.from(e).message).toBe(
-          '[invalid_argument] unexpected response content type "application/csv"'
-        );
-      }
-      expect(httpRequestAborted).toBeTrue();
-    });
-  });
   describe("against server responding with an error", function () {
     describe("for unary", function () {
       let httpRequestAborted = false;

packages/connect/src/protocol-connect/transport.ts

@@ -134,7 +134,6 @@ export function createTransport(opt: CommonTransportOptions): Transport {
           const { compression, isUnaryError, unaryError } =
             validateResponseWithCompression(
               method.kind,
-              opt.useBinaryFormat,
               opt.acceptCompression,
               universalResponse.status,
               universalResponse.header
@@ -243,7 +242,6 @@ export function createTransport(opt: CommonTransportOptions): Transport {
           });
           const { compression } = validateResponseWithCompression(
             method.kind,
-            opt.useBinaryFormat,
             opt.acceptCompression,
             uRes.status,
             uRes.header

packages/connect/src/protocol-connect/validate-response.spec.ts

@@ -13,74 +13,58 @@
 // limitations under the License.
 
 import { MethodKind } from "@bufbuild/protobuf";
-import { ConnectError } from "../connect-error.js";
 import { validateResponse } from "./validate-response.js";
+import { ConnectError } from "../connect-error.js";
 
-describe("validateResponse() Connect", function () {
-  it("should return unary error for HTTP error status", function () {
-    const r = validateResponse(
-      MethodKind.Unary,
-      false,
-      400,
-      new Headers({
-        "Content-Type": "application/proto",
-      })
-    );
-    expect(r.isUnaryError).toBeTrue();
-    expect(r.unaryError?.message).toBe("[invalid_argument] HTTP 400");
-  });
-  it("should throw error for content type application/csv", function () {
-    try {
-      validateResponse(
-        MethodKind.Unary,
-        false,
-        200,
-        new Headers({
-          "Content-Type": "application/csv",
-        })
-      );
-      fail("expected error");
-    } catch (e) {
-      expect(e).toBeInstanceOf(ConnectError);
-      expect(ConnectError.from(e).message).toBe(
-        '[invalid_argument] unexpected response content type "application/csv"'
-      );
-    }
-  });
-  it("should throw error for streaming content type for unary RPC", function () {
-    try {
-      validateResponse(
-        MethodKind.Unary,
-        false,
-        200,
-        new Headers({
-          "Content-Type": "application/connect+proto",
-        })
-      );
-      fail("expected error");
-    } catch (e) {
-      expect(e).toBeInstanceOf(ConnectError);
-      expect(ConnectError.from(e).message).toBe(
-        '[invalid_argument] unexpected response content type "application/connect+proto"'
-      );
-    }
+describe("Connect validateResponse()", function () {
+  describe("with unary", function () {
+    const methodKind = MethodKind.Unary;
+    it("should be successful for HTTP 200", function () {
+      const r = validateResponse(methodKind, 200, new Headers());
+      expect(r.isUnaryError).toBeFalse();
+      expect(r.unaryError).toBeUndefined();
+    });
+    it("should return error for HTTP 204", function () {
+      const r = validateResponse(methodKind, 204, new Headers());
+      expect(r.isUnaryError).toBeTrue();
+      expect(r.unaryError?.message).toBe("[unknown] HTTP 204");
+    });
+    it("should return error for HTTP error status", function () {
+      const r = validateResponse(methodKind, 400, new Headers());
+      expect(r.isUnaryError).toBeTrue();
+      expect(r.unaryError?.message).toBe("[invalid_argument] HTTP 400");
+    });
+    it("should include headers as error metadata", function () {
+      const r = validateResponse(methodKind, 204, new Headers({ Foo: "Bar" }));
+      expect(r.unaryError?.metadata.get("Foo")).toBe("Bar");
+    });
   });
-  it("should throw error for unary content type for streaming RPC", function () {
-    try {
-      validateResponse(
-        MethodKind.BiDiStreaming,
-        false,
-        200,
-        new Headers({
-          "Content-Type": "application/proto",
-        })
-      );
-      fail("expected error");
-    } catch (e) {
-      expect(e).toBeInstanceOf(ConnectError);
-      expect(ConnectError.from(e).message).toBe(
-        '[invalid_argument] unexpected response content type "application/proto"'
-      );
-    }
+  describe("with streaming", function () {
+    const methodKind = MethodKind.ServerStreaming;
+    it("should be successful for HTTP 200", function () {
+      const r = validateResponse(methodKind, 200, new Headers());
+      expect(r.isUnaryError).toBeFalse();
+      expect(r.unaryError).toBeUndefined();
+    });
+    it("should throw error for HTTP error status", function () {
+      try {
+        validateResponse(methodKind, 400, new Headers());
+        fail("expected error");
+      } catch (e) {
+        expect(e).toBeInstanceOf(ConnectError);
+        expect(ConnectError.from(e).message).toBe(
+          "[invalid_argument] HTTP 400"
+        );
+      }
+    });
+    it("should include headers as error metadata", function () {
+      try {
+        validateResponse(methodKind, 400, new Headers({ Foo: "Bar" }));
+        fail("expected error");
+      } catch (e) {
+        expect(e).toBeInstanceOf(ConnectError);
+        expect(ConnectError.from(e).metadata.get("Foo")).toBe("Bar");
+      }
+    });
   });
 });

packages/connect/src/protocol-connect/validate-response.ts

@@ -14,7 +14,6 @@
 
 import { MethodKind } from "@bufbuild/protobuf";
 import { Code } from "../code.js";
-import { parseContentType } from "./content-type.js";
 import { codeFromHttpStatus } from "./http-status.js";
 import { ConnectError } from "../connect-error.js";
 import { headerStreamEncoding, headerUnaryEncoding } from "./headers.js";
@@ -32,35 +31,22 @@ import type { Compression } from "../protocol/compression.js";
  */
 export function validateResponse(
   methodKind: MethodKind,
-  useBinaryFormat: boolean,
   status: number,
   headers: Headers
 ):
   | { isUnaryError: false; unaryError?: undefined }
   | { isUnaryError: true; unaryError: ConnectError } {
-  const mimeType = headers.get("Content-Type");
-  const parsedType = parseContentType(mimeType);
   if (status !== 200) {
     const errorFromStatus = new ConnectError(
       `HTTP ${status}`,
-      codeFromHttpStatus(status)
+      codeFromHttpStatus(status),
+      headers
     );
-    if (methodKind == MethodKind.Unary && parsedType && !parsedType.stream) {
+    if (methodKind == MethodKind.Unary) {
       return { isUnaryError: true, unaryError: errorFromStatus };
     }
     throw errorFromStatus;
   }
-  const isStream = methodKind != MethodKind.Unary;
-  if (
-    !parsedType ||
-    parsedType.binary != useBinaryFormat ||
-    parsedType.stream != isStream
-  ) {
-    throw new ConnectError(
-      `unexpected response content type "${mimeType ?? "?"}"`,
-      Code.InvalidArgument
-    );
-  }
   return { isUnaryError: false };
 }
 
@@ -73,7 +59,6 @@ export function validateResponse(
  */
 export function validateResponseWithCompression(
   methodKind: MethodKind,
-  useBinaryFormat: boolean,
   acceptCompression: Compression[],
   status: number,
   headers: Headers
@@ -89,12 +74,13 @@ export function validateResponseWithCompression(
     if (!compression) {
       throw new ConnectError(
         `unsupported response encoding "${encoding}"`,
-        Code.InvalidArgument
+        Code.InvalidArgument,
+        headers
       );
     }
   }
   return {
     compression,
-    ...validateResponse(methodKind, useBinaryFormat, status, headers),
+    ...validateResponse(methodKind, status, headers),
   };
 }