diff --git a/packages/connect-web-bench/README.md b/packages/connect-web-bench/README.md index b009865d9..f0e545f20 100644 --- a/packages/connect-web-bench/README.md +++ b/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 | diff --git a/packages/connect/src/protocol-connect/content-type.spec.ts b/packages/connect/src/protocol-connect/content-type.spec.ts index 90cb1ccba..95f454f94 100644 --- a/packages/connect/src/protocol-connect/content-type.spec.ts +++ 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, diff --git a/packages/connect/src/protocol-connect/transport.spec.ts b/packages/connect/src/protocol-connect/transport.spec.ts index 6a44d237a..c8a3a40a9 100644 --- a/packages/connect/src/protocol-connect/transport.spec.ts +++ b/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 { - request.signal?.addEventListener( - "abort", - () => (httpRequestAborted = true) + const getUnaryTransport = (opts: { contentType: string }) => { + return createTransport({ + httpClient( + request: UniversalClientRequest + ): Promise { + 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(); diff --git a/packages/connect/src/protocol-connect/validate-response.spec.ts b/packages/connect/src/protocol-connect/validate-response.spec.ts index e7e985258..73b2cb8fe 100644 --- a/packages/connect/src/protocol-connect/validate-response.spec.ts +++ b/packages/connect/src/protocol-connect/validate-response.spec.ts @@ -15,40 +15,54 @@ 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()); + validateResponse( + MethodKind.Unary, + 400, + new Headers({ + "Content-Type": "application/proto", + }) + ); fail("expected error"); } catch (e) { expect(e).toBeInstanceOf(ConnectError); @@ -57,9 +71,30 @@ describe("Connect validateResponse()", function () { ); } }); + 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); diff --git a/packages/connect/src/protocol-connect/validate-response.ts b/packages/connect/src/protocol-connect/validate-response.ts index dfc8aa328..f1db08f7e 100644 --- a/packages/connect/src/protocol-connect/validate-response.ts +++ b/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;