Disregard non-JSON error response bodies

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        | 113,120 b | 49,644 b | 13,266 b |
+| connect        | 113,630 b | 49,881 b | 13,372 b |
 | grpc-web       | 414,906 b    | 301,127 b    | 53,279 b |

packages/connect/src/protocol-connect/content-type.spec.ts

@@ -22,6 +22,8 @@ import {
 
 describe("parseContentType()", function () {
   it("should parse", function () {
+    expect(parseContentType("text/plain")).toBeUndefined();
+    expect(parseContentType("text/plain; charset=utf-8")).toBeUndefined();
     expect(parseContentType(contentTypeUnaryJson)).toEqual({
       stream: false,
       binary: false,

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

@@ -37,6 +37,7 @@ import { Code } from "../code.js";
 import {
   contentTypeStreamProto,
   contentTypeUnaryProto,
+  contentTypeUnaryJson,
 } from "./content-type.js";
 import { errorToJsonBytes } from "./error-json.js";
 import { createHandlerFactory } from "./handler-factory.js";
@@ -94,31 +95,54 @@ describe("Connect transport", function () {
   describe("against server responding with an error", function () {
     describe("for unary", function () {
       let httpRequestAborted = false;
-      const t = createTransport({
-        httpClient(
-          request: UniversalClientRequest
-        ): Promise<UniversalClientResponse> {
-          request.signal?.addEventListener(
-            "abort",
-            () => (httpRequestAborted = true)
+      const getUnaryTransport = (opts: { contentType: string }) => {
+        return createTransport({
+          httpClient(
+            request: UniversalClientRequest
+          ): Promise<UniversalClientResponse> {
+            request.signal?.addEventListener(
+              "abort",
+              () => (httpRequestAborted = true)
+            );
+            return Promise.resolve({
+              status: 429,
+              header: new Headers({
+                "Content-Type": opts.contentType,
+              }),
+              body: createAsyncIterable([
+                errorToJsonBytes(
+                  new ConnectError("foo", Code.ResourceExhausted),
+                  {}
+                ),
+              ]),
+              trailer: new Headers(),
+            });
+          },
+          ...defaultTransportOptions,
+        });
+      };
+      it("should cancel the HTTP request and not parse the body", async function () {
+        const t = getUnaryTransport({ contentType: contentTypeUnaryProto });
+        try {
+          await t.unary(
+            TestService,
+            TestService.methods.unary,
+            undefined,
+            undefined,
+            undefined,
+            {}
           );
-          return Promise.resolve({
-            status: 429,
-            header: new Headers({
-              "Content-Type": contentTypeUnaryProto,
-            }),
-            body: createAsyncIterable([
-              errorToJsonBytes(
-                new ConnectError("foo", Code.ResourceExhausted),
-                {}
-              ),
-            ]),
-            trailer: new Headers(),
-          });
-        },
-        ...defaultTransportOptions,
+          fail("expected error");
+        } catch (e) {
+          expect(e).toBeInstanceOf(ConnectError);
+          // Body should not be parsed because the Content-Type response header is not application/json
+          expect(ConnectError.from(e).code).toBe(Code.Unavailable);
+          expect(ConnectError.from(e).message).toBe("[unavailable] HTTP 429");
+        }
+        expect(httpRequestAborted).toBeTrue();
       });
-      it("should cancel the HTTP request", async function () {
+      it("should cancel the HTTP request and parse the body", async function () {
+        const t = getUnaryTransport({ contentType: contentTypeUnaryJson });
         try {
           await t.unary(
             TestService,
@@ -131,6 +155,8 @@ describe("Connect transport", function () {
           fail("expected error");
         } catch (e) {
           expect(e).toBeInstanceOf(ConnectError);
+          // Body should be parsed because the Content-Type response header is application/json
+          expect(ConnectError.from(e).code).toBe(Code.ResourceExhausted);
           expect(ConnectError.from(e).message).toBe("[resource_exhausted] foo");
         }
         expect(httpRequestAborted).toBeTrue();

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

@@ -15,51 +15,85 @@
 import { MethodKind } from "@bufbuild/protobuf";
 import { validateResponse } from "./validate-response.js";
 import { ConnectError } from "../connect-error.js";
+import { Code } from "../code.js";
 
 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());
+    it("should be successful for HTTP 200 with proper unary JSON content type", function () {
+      const r = validateResponse(
+        MethodKind.Unary,
+        200,
+        new Headers({ "Content-Type": "application/json" })
+      );
       expect(r.isUnaryError).toBeFalse();
       expect(r.unaryError).toBeUndefined();
     });
     it("should return error for HTTP 204", function () {
-      const r = validateResponse(methodKind, 204, new Headers());
+      const r = validateResponse(
+        MethodKind.Unary,
+        204,
+        new Headers({ "Content-Type": "application/json" })
+      );
       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" }));
+      const r = validateResponse(
+        MethodKind.Unary,
+        204,
+        new Headers({ "Content-Type": "application/json", Foo: "Bar" })
+      );
       expect(r.unaryError?.metadata.get("Foo")).toBe("Bar");
     });
-  });
-  describe("with streaming", function () {
-    const methodKind = MethodKind.ServerStreaming;
-    it("should be successful for HTTP 200", function () {
-      const r = validateResponse(methodKind, 200, new Headers());
+    it("should be successful for HTTP 200 with proper unary proto content type", function () {
+      const r = validateResponse(
+        MethodKind.Unary,
+        200,
+        new Headers({ "Content-Type": "application/proto" })
+      );
       expect(r.isUnaryError).toBeFalse();
       expect(r.unaryError).toBeUndefined();
     });
-    it("should throw error for HTTP error status", function () {
+    it("should throw error for HTTP error status with binary response body", function () {
       try {
-        validateResponse(methodKind, 400, new Headers());
-        fail("expected error");
+        validateResponse(
+          MethodKind.Unary,
+          400,
+          new Headers({
+            "Content-Type": "application/proto",
+          })
+        );
       } catch (e) {
         expect(e).toBeInstanceOf(ConnectError);
         expect(ConnectError.from(e).message).toBe(
           "[invalid_argument] HTTP 400"
         );
       }
     });
+    it("should return an error for HTTP error status if content type is JSON", function () {
+      const result = validateResponse(
+        MethodKind.Unary,
+        400,
+        new Headers({
+          "Content-Type": "application/json",
+        })
+      );
+      expect(result.isUnaryError).toBeTrue();
+      expect(result.unaryError?.code).toBe(Code.InvalidArgument);
+      expect(result.unaryError?.message).toBe("[invalid_argument] HTTP 400");
+    });
+  });
+  describe("with streaming", function () {
     it("should include headers as error metadata", function () {
       try {
-        validateResponse(methodKind, 400, new Headers({ Foo: "Bar" }));
+        validateResponse(
+          MethodKind.BiDiStreaming,
+          400,
+          new Headers({
+            "Content-Type": "application/connect+proto",
+            Foo: "Bar",
+          })
+        );
         fail("expected error");
       } catch (e) {
         expect(e).toBeInstanceOf(ConnectError);

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

@@ -16,6 +16,7 @@ import { MethodKind } from "@bufbuild/protobuf";
 import { Code } from "../code.js";
 import { codeFromHttpStatus } from "./http-status.js";
 import { ConnectError } from "../connect-error.js";
+import { parseContentType } from "./content-type.js";
 import { headerStreamEncoding, headerUnaryEncoding } from "./headers.js";
 import type { Compression } from "../protocol/compression.js";
 
@@ -36,13 +37,16 @@ export function validateResponse(
 ):
   | { 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),
       headers
     );
-    if (methodKind == MethodKind.Unary) {
+    // If parsedType is defined and it is not binary, then this is a unary JSON response
+    if (methodKind == MethodKind.Unary && parsedType && !parsedType.binary) {
       return { isUnaryError: true, unaryError: errorFromStatus };
     }
     throw errorFromStatus;