From bb9dbc83ddf4f2eb49e441eda072d87e2b15a0c1 Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Tue, 27 Jun 2023 14:20:54 -0400 Subject: [PATCH 1/9] Do not parse as JSON --- packages/connect/src/protocol-connect/validate-response.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/connect/src/protocol-connect/validate-response.ts b/packages/connect/src/protocol-connect/validate-response.ts index dfc8aa328..ded149310 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,15 @@ 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 (methodKind == MethodKind.Unary && parsedType && !parsedType.stream) { return { isUnaryError: true, unaryError: errorFromStatus }; } throw errorFromStatus; From 6ee442eb8f0ca53e09d36fabaf26b9a153380878 Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Tue, 27 Jun 2023 16:55:08 -0400 Subject: [PATCH 2/9] Validate --- packages/connect/src/protocol-connect/validate-response.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/connect/src/protocol-connect/validate-response.ts b/packages/connect/src/protocol-connect/validate-response.ts index ded149310..51af0bdf8 100644 --- a/packages/connect/src/protocol-connect/validate-response.ts +++ b/packages/connect/src/protocol-connect/validate-response.ts @@ -45,7 +45,7 @@ export function validateResponse( codeFromHttpStatus(status), headers ); - if (methodKind == MethodKind.Unary && parsedType && !parsedType.stream) { + if (methodKind == MethodKind.Unary && parsedType && !parsedType.binary) { return { isUnaryError: true, unaryError: errorFromStatus }; } throw errorFromStatus; From 2f71f63186d8cc3e55ead11380556af18fd191a5 Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Wed, 28 Jun 2023 12:41:07 -0400 Subject: [PATCH 3/9] Validate response fixes --- .../src/protocol-connect/content-type.spec.ts | 2 + .../src/protocol-connect/transport.spec.ts | 73 ++++++---- .../validate-response.spec.ts | 125 +++++++++++++++--- .../src/protocol-connect/validate-response.ts | 10 +- 4 files changed, 166 insertions(+), 44 deletions(-) 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..9ee90a50c 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,55 @@ 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); + // expect(ConnectError.from(e).message).toBe("[resource_exhausted] foo"); + // 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 +156,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..942558656 100644 --- a/packages/connect/src/protocol-connect/validate-response.spec.ts +++ b/packages/connect/src/protocol-connect/validate-response.spec.ts @@ -15,41 +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("validateResponse() Connect", 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( @@ -57,9 +70,81 @@ 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"); + }); + it("should throw error for content type application/csv", function () { + try { + validateResponse( + MethodKind.Unary, + 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, + 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("with streaming", function () { + it("should throw error for unary content type for streaming RPC", function () { + try { + validateResponse( + MethodKind.BiDiStreaming, + 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"' + ); + } + }); 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 51af0bdf8..7f86811a0 100644 --- a/packages/connect/src/protocol-connect/validate-response.ts +++ b/packages/connect/src/protocol-connect/validate-response.ts @@ -16,7 +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 { parseContentType } from "./content-type.js"; import { headerStreamEncoding, headerUnaryEncoding } from "./headers.js"; import type { Compression } from "../protocol/compression.js"; @@ -45,11 +45,19 @@ export function validateResponse( codeFromHttpStatus(status), headers ); + // 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; } + const isStream = methodKind != MethodKind.Unary; + if (!parsedType || parsedType.stream != isStream) { + throw new ConnectError( + `unexpected response content type "${mimeType ?? "?"}"`, + Code.InvalidArgument + ); + } return { isUnaryError: false }; } From 22efaaa2d4ae34857bf107de9caaf177f8eb933e Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Wed, 28 Jun 2023 12:49:18 -0400 Subject: [PATCH 4/9] Bench --- packages/connect-web-bench/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/connect-web-bench/README.md b/packages/connect-web-bench/README.md index afb399e2c..420165eb8 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 | 112,976 b | 49,590 b | 13,282 b | +| connect | 113,744 b | 49,948 b | 13,422 b | | grpc-web | 414,906 b | 301,127 b | 53,279 b | From f48ab48cb3b02f2a395c973d704f34875a5ac744 Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Wed, 28 Jun 2023 13:41:10 -0400 Subject: [PATCH 5/9] Spec cleanup --- packages/connect/src/protocol-connect/transport.spec.ts | 1 - packages/connect/src/protocol-connect/validate-response.spec.ts | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/connect/src/protocol-connect/transport.spec.ts b/packages/connect/src/protocol-connect/transport.spec.ts index 9ee90a50c..c8a3a40a9 100644 --- a/packages/connect/src/protocol-connect/transport.spec.ts +++ b/packages/connect/src/protocol-connect/transport.spec.ts @@ -135,7 +135,6 @@ describe("Connect transport", function () { fail("expected error"); } catch (e) { expect(e).toBeInstanceOf(ConnectError); - // expect(ConnectError.from(e).message).toBe("[resource_exhausted] foo"); // 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"); diff --git a/packages/connect/src/protocol-connect/validate-response.spec.ts b/packages/connect/src/protocol-connect/validate-response.spec.ts index 942558656..9e59ddb03 100644 --- a/packages/connect/src/protocol-connect/validate-response.spec.ts +++ b/packages/connect/src/protocol-connect/validate-response.spec.ts @@ -17,7 +17,7 @@ import { validateResponse } from "./validate-response.js"; import { ConnectError } from "../connect-error.js"; import { Code } from "../code.js"; -describe("validateResponse() Connect", function () { +describe("Connect validateResponse()", function () { describe("with unary", function () { it("should be successful for HTTP 200 with proper unary JSON content type", function () { const r = validateResponse( From 6d814451cda1206d63d5371aa4b3413d918c7845 Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Wed, 28 Jun 2023 13:42:13 -0400 Subject: [PATCH 6/9] Bench --- packages/connect-web-bench/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/connect-web-bench/README.md b/packages/connect-web-bench/README.md index 420165eb8..747d69660 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,744 b | 49,948 b | 13,422 b | +| connect | 113,888 b | 50,002 b | 13,440 b | | grpc-web | 414,906 b | 301,127 b | 53,279 b | From a48f095393a2a7a00a2428d97fc8b1e3a768324f Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Wed, 28 Jun 2023 13:47:05 -0400 Subject: [PATCH 7/9] Dont throw on unexpected content type --- .../validate-response.spec.ts | 51 ------------------- .../src/protocol-connect/validate-response.ts | 7 --- 2 files changed, 58 deletions(-) diff --git a/packages/connect/src/protocol-connect/validate-response.spec.ts b/packages/connect/src/protocol-connect/validate-response.spec.ts index 9e59ddb03..44de12b8b 100644 --- a/packages/connect/src/protocol-connect/validate-response.spec.ts +++ b/packages/connect/src/protocol-connect/validate-response.spec.ts @@ -82,59 +82,8 @@ describe("Connect validateResponse()", function () { expect(result.unaryError?.code).toBe(Code.InvalidArgument); expect(result.unaryError?.message).toBe("[invalid_argument] HTTP 400"); }); - it("should throw error for content type application/csv", function () { - try { - validateResponse( - MethodKind.Unary, - 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, - 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("with streaming", function () { - it("should throw error for unary content type for streaming RPC", function () { - try { - validateResponse( - MethodKind.BiDiStreaming, - 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"' - ); - } - }); it("should include headers as error metadata", function () { try { validateResponse( diff --git a/packages/connect/src/protocol-connect/validate-response.ts b/packages/connect/src/protocol-connect/validate-response.ts index 7f86811a0..f1db08f7e 100644 --- a/packages/connect/src/protocol-connect/validate-response.ts +++ b/packages/connect/src/protocol-connect/validate-response.ts @@ -51,13 +51,6 @@ export function validateResponse( } throw errorFromStatus; } - const isStream = methodKind != MethodKind.Unary; - if (!parsedType || parsedType.stream != isStream) { - throw new ConnectError( - `unexpected response content type "${mimeType ?? "?"}"`, - Code.InvalidArgument - ); - } return { isUnaryError: false }; } From 6de9274adadf72ca7ef60a886e05e0bc1d5e2601 Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Wed, 28 Jun 2023 13:47:35 -0400 Subject: [PATCH 8/9] bundle --- packages/connect-web-bench/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/connect-web-bench/README.md b/packages/connect-web-bench/README.md index 747d69660..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,888 b | 50,002 b | 13,440 b | +| connect | 113,630 b | 49,881 b | 13,372 b | | grpc-web | 414,906 b | 301,127 b | 53,279 b | From cd1f133910f67b98597ab4081e73c0103ea965a5 Mon Sep 17 00:00:00 2001 From: Steve Ayers Date: Wed, 28 Jun 2023 14:09:45 -0400 Subject: [PATCH 9/9] Added fail statement to test --- packages/connect/src/protocol-connect/validate-response.spec.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/connect/src/protocol-connect/validate-response.spec.ts b/packages/connect/src/protocol-connect/validate-response.spec.ts index 44de12b8b..73b2cb8fe 100644 --- a/packages/connect/src/protocol-connect/validate-response.spec.ts +++ b/packages/connect/src/protocol-connect/validate-response.spec.ts @@ -63,6 +63,7 @@ describe("Connect validateResponse()", function () { "Content-Type": "application/proto", }) ); + fail("expected error"); } catch (e) { expect(e).toBeInstanceOf(ConnectError); expect(ConnectError.from(e).message).toBe(