Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add server support for Connect HTTP GET requests #624

Merged
merged 26 commits into from
May 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
d67ef1d
Don't omit search from :path in h2 client
jchadwick-buf May 5, 2023
8fc21a4
Add client support for HTTP GET requests
jchadwick-buf May 4, 2023
7881503
Add transport options for connect-node tests
jchadwick-buf May 5, 2023
bcc2ea7
Add cacheable_unary test for web + node
jchadwick-buf May 5, 2023
a115e83
connect-node: Don't include query in path routing
jchadwick-buf May 8, 2023
0a44ea6
Handle negotiation for requests sans Content-Type
jchadwick-buf May 8, 2023
19c07c2
Add query parameter utilities for Connect Get
jchadwick-buf May 8, 2023
1ac50db
protocol-connect: Add GET support to handler
jchadwick-buf May 8, 2023
a5f8276
Update tests to test Connect Get for Node servers
jchadwick-buf May 8, 2023
2babeb3
Remove support for binary percent-encoding
jchadwick-buf May 9, 2023
dd5df6d
Add comment regarding query string splitting
jchadwick-buf May 9, 2023
5203736
Use version constant in client.
jchadwick-buf May 9, 2023
b76dc31
Use onHeader instead of interceptors
jchadwick-buf May 9, 2023
069f83c
Better test condition for cacheable unary test
jchadwick-buf May 9, 2023
aaafb45
Merge branch 'main' of https://github.com/bufbuild/connect-es into jc…
jchadwick-buf May 9, 2023
85fff63
Merge branch 'jchadwick/unary-get-support' of https://github.com/bufb…
jchadwick-buf May 10, 2023
1214000
Merge redundant import declarations
jchadwick-buf May 10, 2023
bfc8817
Merge branch 'main' of https://github.com/bufbuild/connect-es into jc…
jchadwick-buf May 10, 2023
965b56e
Add requestMethod, protocolName to context
jchadwick-buf May 11, 2023
76d8f41
Add test for protocolName
jchadwick-buf May 11, 2023
c80d4c8
Merge branch 'main' of https://github.com/bufbuild/connect-es into jc…
jchadwick-buf May 11, 2023
b9b59e1
Apply suggestion to improve handler code
jchadwick-buf May 12, 2023
57ee8aa
Clean imports in protocol-connect/handler-factory
jchadwick-buf May 12, 2023
c336699
Clean up after adding suggestion/reorganize import
jchadwick-buf May 12, 2023
8cfee9a
Merge branch 'main' of https://github.com/bufbuild/connect-es into jc…
jchadwick-buf May 12, 2023
13ffba5
Add more tests for GET support (#629)
timostamm May 12, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/connect-express/src/express-connect-middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ export function expressConnectMiddleware(
res: express.Response,
next: express.NextFunction
) {
const uHandler = paths.get(req.url);
// Strip the query parameter when matching paths.
const uHandler = paths.get(req.url.split("?", 2)[0]);
jchadwick-buf marked this conversation as resolved.
Show resolved Hide resolved
if (!uHandler) {
return next();
}
Expand Down
3 changes: 2 additions & 1 deletion packages/connect-next/src/connect-nextjs-adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ export function nextJsApiRouter(options: NextJsApiRouterOptions): ApiRoute {
}

async function handler(req: NextApiRequest, res: NextApiResponse) {
const requestPath = req.url ?? "";
// Strip the query parameter when matching paths.
const requestPath = req.url?.split("?", 2)[0] ?? "";
const uHandler = paths.get(requestPath);
if (!uHandler) {
res.writeHead(404);
Expand Down
106 changes: 30 additions & 76 deletions packages/connect-node-test/src/crosstest/cacheable_unary.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,83 +28,37 @@ describe("cacheable_unary", function () {
const servers = createTestServers();
beforeAll(async () => await servers.start());

servers.describeTransportsExcluding(
// connect-node servers currently do not support HTTP GET.
[
"@bufbuild/connect-node (Connect, binary, http, gzip) against @bufbuild/connect-express (h1)",
"@bufbuild/connect-node (Connect, binary, http, gzip) against @bufbuild/connect-fastify (h2c)",
"@bufbuild/connect-node (Connect, binary, http, gzip) against @bufbuild/connect-node (h1)",
"@bufbuild/connect-node (Connect, binary, http) against @bufbuild/connect-node (h1)",
"@bufbuild/connect-node (Connect, binary, http2, gzip) against @bufbuild/connect-node (h2c)",
"@bufbuild/connect-node (Connect, binary, https) against @bufbuild/connect-node (h1 + tls)",
"@bufbuild/connect-node (Connect, JSON, http, gzip) against @bufbuild/connect-express (h1)",
"@bufbuild/connect-node (Connect, JSON, http, gzip) against @bufbuild/connect-fastify (h2c)",
"@bufbuild/connect-node (Connect, JSON, http, gzip) against @bufbuild/connect-node (h1)",
"@bufbuild/connect-node (Connect, JSON, http) against @bufbuild/connect-node (h1)",
"@bufbuild/connect-node (Connect, JSON, http2, gzip) against @bufbuild/connect-node (h2c)",
"@bufbuild/connect-node (Connect, JSON, https) against @bufbuild/connect-node (h1 + tls)",
"@bufbuild/connect-node (gRPC-web, binary, http, gzip against @bufbuild/connect-fastify (h2c)",
"@bufbuild/connect-node (gRPC-web, binary, http, gzip) against @bufbuild/connect-express (h1)",
"@bufbuild/connect-node (gRPC-web, binary, http, gzip) against @bufbuild/connect-node (h1)",
"@bufbuild/connect-node (gRPC-web, binary, http) against @bufbuild/connect-node (h1)",
"@bufbuild/connect-node (gRPC-web, binary, http2, gzip) against @bufbuild/connect-node (h2c)",
"@bufbuild/connect-node (gRPC-web, binary, http2) against @bufbuild/connect-node (h2c)",
"@bufbuild/connect-node (gRPC-web, binary, https) against @bufbuild/connect-node (h1 + tls)",
"@bufbuild/connect-node (gRPC-web, JSON, http, gzip) against @bufbuild/connect-express (h1)",
"@bufbuild/connect-node (gRPC-web, JSON, http, gzip) against @bufbuild/connect-fastify (h2c)",
"@bufbuild/connect-node (gRPC-web, JSON, http, gzip) against @bufbuild/connect-node (h1)",
"@bufbuild/connect-node (gRPC-web, JSON, http) against @bufbuild/connect-node (h1)",
"@bufbuild/connect-node (gRPC-web, JSON, http2, gzip) against @bufbuild/connect-node (h2c)",
"@bufbuild/connect-node (gRPC-web, JSON, http2) against @bufbuild/connect-node (h2c)",
"@bufbuild/connect-node (gRPC-web, JSON, https) against @bufbuild/connect-node (h1 + tls)",
"@bufbuild/connect-node (gRPC, binary, http, gzip) against @bufbuild/connect-express (h1)",
"@bufbuild/connect-node (gRPC, binary, http, gzip) against @bufbuild/connect-fastify (h2c)",
"@bufbuild/connect-node (gRPC, binary, http, gzip) against @bufbuild/connect-node (h1)",
"@bufbuild/connect-node (gRPC, binary, http) against @bufbuild/connect-node (h1)",
"@bufbuild/connect-node (gRPC, binary, http2, gzip) against @bufbuild/connect-node (h2c)",
"@bufbuild/connect-node (gRPC, binary, http2) against @bufbuild/connect-node (h2)",
"@bufbuild/connect-node (gRPC, binary, http2) against @bufbuild/connect-node (h2c)",
"@bufbuild/connect-node (gRPC, binary, https) against @bufbuild/connect-node (h1 + tls)",
"@bufbuild/connect-node (gRPC, JSON, http, gzip) against @bufbuild/connect-express (h1)",
"@bufbuild/connect-node (gRPC, JSON, http, gzip) against @bufbuild/connect-fastify (h2c)",
"@bufbuild/connect-node (gRPC, JSON, http, gzip) against @bufbuild/connect-node (h1)",
"@bufbuild/connect-node (gRPC, JSON, http) against @bufbuild/connect-node (h1)",
"@bufbuild/connect-node (gRPC, JSON, http2, gzip) against @bufbuild/connect-node (h2c)",
"@bufbuild/connect-node (gRPC, JSON, http2) against @bufbuild/connect-node (h2c)",
"@bufbuild/connect-node (gRPC, JSON, https) against @bufbuild/connect-node (h1 + tls)",
],
(transportFactory) => {
const request = new SimpleRequest({
responseSize: 1024,
payload: {
body: new Uint8Array(1024).fill(0),
},
});
it("with promise client", async function () {
const transport = transportFactory({ useHttpGet: true });
const client = createPromiseClient(TestService, transport);
const response = await client.cacheableUnaryCall(request, {
onHeader: ensureGetRequest,
});
expect(response.payload).toBeDefined();
expect(response.payload?.body.length).toEqual(request.responseSize);
servers.describeTransports((transportFactory) => {
const request = new SimpleRequest({
responseSize: 1024,
payload: {
body: new Uint8Array(1024).fill(0),
},
});
it("with promise client", async function () {
const transport = transportFactory({ useHttpGet: true });
const client = createPromiseClient(TestService, transport);
const response = await client.cacheableUnaryCall(request, {
onHeader: ensureGetRequest,
});
it("with callback client", function (done) {
const transport = transportFactory({ useHttpGet: true });
const client = createCallbackClient(TestService, transport);
client.cacheableUnaryCall(
request,
(err, response) => {
expect(err).toBeUndefined();
expect(response.payload).toBeDefined();
expect(response.payload?.body.length).toEqual(request.responseSize);
done();
},
{ onHeader: ensureGetRequest }
);
});
}
);
expect(response.payload).toBeDefined();
expect(response.payload?.body.length).toEqual(request.responseSize);
});
it("with callback client", function (done) {
const transport = transportFactory({ useHttpGet: true });
const client = createCallbackClient(TestService, transport);
client.cacheableUnaryCall(
request,
(err, response) => {
expect(err).toBeUndefined();
expect(response.payload).toBeDefined();
expect(response.payload?.body.length).toEqual(request.responseSize);
done();
},
{ onHeader: ensureGetRequest }
);
});
});

afterAll(async () => await servers.stop());
});
87 changes: 87 additions & 0 deletions packages/connect-node-test/src/extra/protocol-name.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// Copyright 2021-2023 Buf Technologies, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import { createCallbackClient, createPromiseClient } from "@bufbuild/connect";
import type { Transport } from "@bufbuild/connect";
import { TestService } from "../gen/grpc/testing/test_connect.js";
import { SimpleRequest } from "../gen/grpc/testing/messages_pb.js";
import { createTestServers } from "../helpers/testserver.js";

function ensureProtocolName(expectedName: string) {
return (header: Headers) => {
expect(header.get("request-protocol")).toEqual(expectedName);
};
}

function testForProtocol(expectedProtocolName: string) {
return (transportFactory: () => Transport) => {
const request = new SimpleRequest({
responseSize: 1024,
payload: {
body: new Uint8Array(1024).fill(0),
},
});
it("with promise client", async function () {
const transport = transportFactory();
const client = createPromiseClient(TestService, transport);
const response = await client.unaryCall(request, {
onHeader: ensureProtocolName(expectedProtocolName),
});
expect(response.payload).toBeDefined();
expect(response.payload?.body.length).toEqual(request.responseSize);
});
it("with callback client", function (done) {
const transport = transportFactory();
const client = createCallbackClient(TestService, transport);
client.unaryCall(
request,
(err, response) => {
expect(err).toBeUndefined();
expect(response.payload).toBeDefined();
expect(response.payload?.body.length).toEqual(request.responseSize);
done();
},
{ onHeader: ensureProtocolName(expectedProtocolName) }
);
});
};
}

describe("protocolName", function () {
const servers = createTestServers();
beforeAll(async () => await servers.start());

servers.describeTransportsOnly(
[
"@bufbuild/connect-node (gRPC, binary, http2) against @bufbuild/connect-node (h2)",
],
testForProtocol("grpc")
);

servers.describeTransportsOnly(
[
"@bufbuild/connect-node (gRPC-web, binary, http2) against @bufbuild/connect-node (h2c)",
],
testForProtocol("grpc-web")
);

servers.describeTransportsOnly(
[
"@bufbuild/connect-node (Connect, binary, http2, gzip) against @bufbuild/connect-node (h2c)",
],
testForProtocol("connect")
);

afterAll(async () => await servers.stop());
});
8 changes: 6 additions & 2 deletions packages/connect-node-test/src/helpers/test-routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ const testService: ServiceImpl<typeof TestService> = {
context.responseHeader,
context.responseTrailer
);
context.responseHeader.set("request-protocol", context.protocolName);
maybeRaiseError(request.responseStatus);
return {
payload: interop.makeServerPayload(
Expand All @@ -57,8 +58,11 @@ const testService: ServiceImpl<typeof TestService> = {
]);
},

cacheableUnaryCall(/*request*/) {
throw new ConnectError("TODO", Code.Unimplemented);
cacheableUnaryCall(request, context) {
if (context.requestMethod == "GET") {
context.responseHeader.set("get-request", "true");
}
return this.unaryCall(request, context);
},

async *streamingOutputCall(request, context) {
Expand Down
3 changes: 2 additions & 1 deletion packages/connect-node/src/connect-node-adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ export function connectNodeAdapter(
req: NodeServerRequest,
res: NodeServerResponse
): void {
const uHandler = paths.get(req.url ?? "");
// Strip the query parameter when matching paths.
const uHandler = paths.get(req.url?.split("?", 2)[0] ?? "");
if (!uHandler) {
(options.fallback ?? fallback)(req, res);
return;
Expand Down
71 changes: 31 additions & 40 deletions packages/connect-web-test/src/crosstest/cacheable_unary.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

import { createCallbackClient, createPromiseClient } from "@bufbuild/connect";
import { TestService } from "../gen/grpc/testing/test_connect.js";
import { describeTransportsExcluding } from "../helpers/crosstestserver.js";
import { describeTransports } from "../helpers/crosstestserver.js";
import { SimpleRequest } from "../gen/grpc/testing/messages_pb.js";

function ensureGetRequest(header: Headers) {
Expand All @@ -25,44 +25,35 @@ function ensureGetRequest(header: Headers) {
}

describe("cacheable_unary", function () {
describeTransportsExcluding(
// connect-node servers currently do not support HTTP GET.
[
"@bufbuild/connect-web (gRPC-web, binary) gRPC-web against @bufbuild/connect-node (h1)",
"@bufbuild/connect-web (gRPC-web, JSON) gRPC-web against @bufbuild/connect-node (h1)",
"@bufbuild/connect-web (Connect, binary) against @bufbuild/connect-node (h1)",
"@bufbuild/connect-web (Connect, JSON) against @bufbuild/connect-node (h1)",
],
(transportFactory) => {
const request = new SimpleRequest({
responseSize: 1,
payload: {
body: new Uint8Array(1).fill(0),
},
});
it("with promise client", async function () {
const transport = transportFactory({ useHttpGet: true });
const client = createPromiseClient(TestService, transport);
const response = await client.cacheableUnaryCall(request, {
onHeader: ensureGetRequest,
});
expect(response.payload).toBeDefined();
expect(response.payload?.body.length).toEqual(request.responseSize);
describeTransports((transportFactory) => {
const request = new SimpleRequest({
responseSize: 1,
payload: {
body: new Uint8Array(1).fill(0),
},
});
it("with promise client", async function () {
const transport = transportFactory({ useHttpGet: true });
const client = createPromiseClient(TestService, transport);
const response = await client.cacheableUnaryCall(request, {
onHeader: ensureGetRequest,
});
it("with callback client", function (done) {
const transport = transportFactory({ useHttpGet: true });
const client = createCallbackClient(TestService, transport);
client.cacheableUnaryCall(
request,
(err, response) => {
expect(err).toBeUndefined();
expect(response.payload).toBeDefined();
expect(response.payload?.body.length).toEqual(request.responseSize);
done();
},
{ onHeader: ensureGetRequest }
);
});
}
);
expect(response.payload).toBeDefined();
expect(response.payload?.body.length).toEqual(request.responseSize);
});
it("with callback client", function (done) {
const transport = transportFactory({ useHttpGet: true });
const client = createCallbackClient(TestService, transport);
client.cacheableUnaryCall(
request,
(err, response) => {
expect(err).toBeUndefined();
expect(response.payload).toBeDefined();
expect(response.payload?.body.length).toEqual(request.responseSize);
done();
},
{ onHeader: ensureGetRequest }
);
});
});
});
17 changes: 16 additions & 1 deletion packages/connect/src/implementation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,12 @@ export interface HandlerContext {
*/
readonly signal: AbortSignal;

/**
* HTTP method of incoming request, usually "POST", but "GET" in the case of
* Connect Get.
*/
readonly requestMethod: string;

/**
* Incoming request headers.
*/
Expand All @@ -95,6 +101,11 @@ export interface HandlerContext {
* Outgoing response trailers.
*/
readonly responseTrailer: Headers;

/**
* Name of the RPC protocol in use; one of "connect", "grpc" or "grpc-web".
*/
readonly protocolName: string;
}

/**
Expand All @@ -104,17 +115,21 @@ export function createHandlerContext(
spec: { service: ServiceType; method: MethodInfo },
deadline: AbortSignal | undefined, // TODO
signal: AbortSignal,
requestMethod: string,
requestHeader: HeadersInit,
responseHeader: HeadersInit,
responseTrailer: HeadersInit
responseTrailer: HeadersInit,
protocolName: string
): HandlerContext {
return {
method: spec.method,
service: spec.service,
signal: createLinkedAbortController(deadline, signal).signal,
requestMethod,
requestHeader: new Headers(requestHeader),
responseHeader: new Headers(responseHeader),
responseTrailer: new Headers(responseTrailer),
protocolName,
};
}

Expand Down
Loading