Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 8 additions & 8 deletions src/openapi/define-route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,14 @@ export function registerRouteSpec(registry: OpenAPIRegistry, options: RouteSpecO
}

const security = securityFor(options.auth);
// ONE `request` key, built once (#9705). This literal used to carry two: path parameters in the first,
// body and query in a conditional spread further down. A property arriving through a later spread
// replaces an earlier one of the same name -- and TypeScript does not flag it, because the duplicate is
// a spread rather than a literal duplicate key -- so any route with BOTH a templated segment and a body
// or query published no `parameters` at all. That is precisely what pathParameters() exists to prevent:
// a templated segment with no matching parameter is a schema-validation warning and leaves a generated
// client holding a URL it cannot fill. Latent only because no caller passed `request` yet; the first
// migrated POST /v1/repos/:owner/:repo/... would have lost owner and repo silently.
registry.registerPath({
method: options.method,
path: toSpecPath(options.path),
Expand All @@ -191,14 +199,6 @@ export function registerRouteSpec(registry: OpenAPIRegistry, options: RouteSpecO
summary: options.summary,
...(options.description ? { description: options.description } : {}),
...(security ? { security } : {}),
...(options.request?.body || options.request?.query
? {
request: {
...(options.request.body ? { body: { content: { "application/json": { schema: options.request.body } } } } : {}),
...(options.request.query ? { query: options.request.query } : {}),
},
}
: {}),
responses,
});
}
70 changes: 69 additions & 1 deletion test/unit/define-route.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { describe, expect, it } from "vitest";
import { Hono } from "hono";
import { OpenAPIRegistry, OpenApiGeneratorV3 } from "@asteasolutions/zod-to-openapi";
import { z } from "zod";
import { defineRoute, invalidRequestBody, registerRouteSpec } from "../../src/openapi/define-route";
import { defineRoute, invalidRequestBody, registerRouteSpec, type RouteSpecOptions } from "../../src/openapi/define-route";

type TestEnv = { Bindings: Record<string, unknown>; Variables: Record<string, unknown> };

Expand Down Expand Up @@ -172,6 +172,22 @@ describe("defineRoute", () => {
}
});

it("gives the ORB ingress its OWN schemes rather than the LoopOver pair", () => {
// The two auth levels that are not a LoopOver credential at all: an ORB-issued instance token, and a
// webhook that carries no bearer and is verified by a body signature. Publishing the generic pair for
// either would tell a client to send something the route does not accept.
const { app, registry } = build();
for (const [auth, path, id] of [
["orb", "/v1/orb/relay", "orbOp"],
["webhook", "/v1/orb/webhook", "webhookOp"],
] as const) {
defineRoute(app, registry, { method: "post", path, operationId: id, tags: ["t"], summary: "s", auth, responses: { 200: { description: "ok" } } }, (c) => c.json({}));
}
const paths = generate(registry).paths ?? {};
expect(paths["/v1/orb/relay"]?.post?.security).toEqual([{ OrbBearer: [] }]);
expect(paths["/v1/orb/webhook"]?.post?.security).toEqual([{ OrbWebhookSignature: [] }]);
});

it("emits responses with and without a schema", () => {
const { app, registry } = build();
defineRoute(app, registry, {
Expand Down Expand Up @@ -206,6 +222,58 @@ describe("registerRouteSpec", () => {
const operation = generate(registry).paths?.["/v1/spec-only/{id}"]?.post;
expect(operation?.operationId).toBe("specOnly");
expect(operation?.requestBody).toBeDefined();
// #9705: and its path parameter, which the duplicate `request` key used to erase. This route declares
// both a templated segment and a body, so before the fix `parameters` was absent entirely.
expect(operation?.parameters).toContainEqual(expect.objectContaining({ in: "path", name: "id", required: true }));
});

// #9705 regression suite. The defect was that a route carrying a templated path AND any request
// declaration published no `parameters` array -- silently, because the duplicate key arrived via a
// spread and TypeScript does not flag that. Each case below fails on the pre-fix emitter.
function specOnlyOperation(request: RouteSpecOptions["request"]) {
const registry = new OpenAPIRegistry();
registerRouteSpec(registry, {
method: "post",
path: "/v1/repos/:owner/:repo/thing",
operationId: "repoThing",
tags: ["ops"],
summary: "Repo thing",
auth: "token",
...(request ? { request } : {}),
responses: { 202: { description: "accepted" } },
});
return generate(registry).paths?.["/v1/repos/{owner}/{repo}/thing"]?.post;
}

function pathParameterNames(operation: ReturnType<typeof specOnlyOperation>): string[] {
return (operation?.parameters ?? []).filter((parameter) => "in" in parameter && parameter.in === "path").map((parameter) => ("name" in parameter ? parameter.name : ""));
}

it("keeps every path parameter alongside a request BODY", () => {
const operation = specOnlyOperation({ body: z.object({ a: z.string() }) });
expect(pathParameterNames(operation)).toEqual(["owner", "repo"]);
expect(operation?.requestBody).toBeDefined();
});

it("keeps every path parameter alongside a QUERY schema", () => {
const operation = specOnlyOperation({ query: z.object({ since: z.string() }) });
expect(pathParameterNames(operation)).toEqual(["owner", "repo"]);
expect(operation?.parameters).toContainEqual(expect.objectContaining({ in: "query", name: "since" }));
});

it("keeps path parameters, the body, and the query together in ONE operation", () => {
const operation = specOnlyOperation({ body: z.object({ a: z.string() }), query: z.object({ since: z.string() }) });
expect(pathParameterNames(operation)).toEqual(["owner", "repo"]);
expect(operation?.parameters).toContainEqual(expect.objectContaining({ in: "query", name: "since" }));
expect(operation?.requestBody).toBeDefined();
});

it("still emits path parameters for a route that declares no request at all", () => {
// The unchanged arm: every production entry today is response-only, and their operations must be
// byte-for-byte what they were.
const operation = specOnlyOperation(undefined);
expect(pathParameterNames(operation)).toEqual(["owner", "repo"]);
expect(operation?.requestBody).toBeUndefined();
});
});

Expand Down