trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns14g rEventsFlows

GET /api/v1/repos/:owner/:name/issues/:number/comments carries an ETag and answers a repeat with 304 until a comment is added or edited #9134

Merged scoutcrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

scout/forge:scout/9130-get-api-v1-repos-owner-name-issues-numbe → main 2 commits fork

  • Merged. scout/9130-get-api-v1-repos-owner-name-issues-numbe was merged into main as 8454241.

Review

Sign in to review this change.
3 files changed +103 −13
src/contract/openapi/routes/getApiV1ReposOwnerNameIssuesNumberComments.ts +1 −1
@@ -1,3 +1,3 @@
11 import { REPO, type Route, limitTo } from "../shared.ts";
22
3 export const route: Route = { method: "get", path: `${REPO}/issues/{number}/comments`, summary: "Page through an issue's or pull's comments, oldest first: {comments, next}; limit 1 to 100 (default 50), after a comment id; cursor (an issue's commentsNext) answers the page of older comments before it, next being the cursor for older still", auth: false, query: { after: "string", cursor: "string", limit: limitTo(100) } };
3 export const route: Route = { method: "get", path: `${REPO}/issues/{number}/comments`, summary: "Page through an issue's or pull's comments, oldest first: {comments, next}; limit 1 to 100 (default 50), after a comment id; cursor (an issue's commentsNext) answers the page of older comments before it, next being the cursor for older still; the 200 carries a weak etag of the page and cache-control private, no-cache (it varies by reader), and a repeat sending If-None-Match gets 304 with no body once access is checked", auth: false, alsoStatus: [304], query: { after: "string", cursor: "string", limit: limitTo(100) } };
src/http/api/issues.ts +27 −12
@@ -99,6 +99,18 @@ export async function labelsHandler(ctx: Ctx): Promise<Response> {
9999 return json(set.value);
100100 }
101101
102 /**
103 * A reader-scoped JSON answer tagged by its exact bytes: a repeat holding the
104 * tag gets 304. Callers check access first, so a 304 confirms nothing to
105 * someone who may not read it.
106 */
107 function tagged(request: Request, text: string): Response {
108 const etag = weakEtag(text);
109 const headers = { etag, "cache-control": "private, no-cache", vary: "authorization, cookie" };
110 if (holds(request, etag)) return new Response(null, { status: 304, headers });
111 return new Response(text, { headers: { "content-type": "application/json", ...headers } });
112 }
113
102114 export function issueRoutes(router: Router<Handler>): void {
103115 router.add(
104116 "PUT",
@@ -156,10 +168,7 @@ export function issueRoutes(router: Router<Handler>): void {
156168 );
157169 // Scoped by who asks (sealed issues), so private to the reader; the tag
158170 // is of the exact bytes sent, so each page, state and fields value has its own.
159 const etag = weakEtag(text);
160 const headers = { etag, "cache-control": "private, no-cache", vary: "authorization, cookie" };
161 if (holds(ctx.request, etag)) return new Response(null, { status: 304, headers });
162 return new Response(text, { headers: { "content-type": "application/json", ...headers } });
171 return tagged(ctx.request, text);
163172 });
164173 });
165174
@@ -275,10 +284,13 @@ export function issueRoutes(router: Router<Handler>): void {
275284 );
276285 const page = older.slice(older.length - Math.min(limit, older.length));
277286 return timePhase(ctx.request, "render", () =>
278 json({
279 comments: readerMarked(ctx, found.repo.id, page),
280 next: older.length > limit ? page[0]!.id : null,
281 }),
287 tagged(
288 ctx.request,
289 JSON.stringify({
290 comments: readerMarked(ctx, found.repo.id, page),
291 next: older.length > limit ? page[0]!.id : null,
292 }),
293 ),
282294 );
283295 }
284296 const { rows, page } = timePhase(ctx.request, "comments", () => {
@@ -289,10 +301,13 @@ export function issueRoutes(router: Router<Handler>): void {
289301 return { rows: read, page: read.slice(0, limit) };
290302 });
291303 return timePhase(ctx.request, "render", () =>
292 json({
293 comments: readerMarked(ctx, found.repo.id, page),
294 next: rows.length > limit ? page[page.length - 1]!.id : null,
295 }),
304 tagged(
305 ctx.request,
306 JSON.stringify({
307 comments: readerMarked(ctx, found.repo.id, page),
308 next: rows.length > limit ? page[page.length - 1]!.id : null,
309 }),
310 ),
296311 );
297312 });
298313
test/issues-etag.test.ts +75
@@ -84,3 +84,78 @@ describe("issues list ETag", () => {
8484 expect(writer.headers.get("etag")).not.toBe(out.headers.get("etag"));
8585 });
8686 });
87
88 describe("issue comments ETag", () => {
89 let dataDir: string;
90 let app: App;
91 let servers: Servers;
92 let base: string;
93 let token = "";
94 const send = (path: string, method = "GET", bodyObj?: unknown, headers: Record<string, string> = {}, auth = true) =>
95 fetch(`${base}${path}`, {
96 method,
97 headers: {
98 ...(auth ? { authorization: `Bearer ${token}` } : {}),
99 "content-type": "application/json",
100 ...headers,
101 },
102 ...(bodyObj === undefined ? {} : { body: JSON.stringify(bodyObj) }),
103 });
104
105 beforeAll(async () => {
106 dataDir = await mkdtemp(join(tmpdir(), "forge-comments-etag-"));
107 app = createApp({ dataDir, port: 0 });
108 servers = await serve(app);
109 base = servers.url;
110 await seedAdmin(app, { username: "dev", email: "[email protected]", password: "etag-pass-1" });
111 token = await fetch(`${base}/api/v1/tokens`, {
112 method: "POST",
113 headers: { "content-type": "application/json" },
114 body: JSON.stringify({ adminConfirm: true, username: "dev", password: "etag-pass-1", name: "c", scope: "rw" }),
115 })
116 .then((r) => r.json())
117 .then((j) => (j as { token: string }).token);
118 await send("/api/v1/repos", "POST", { name: "lib", visibility: "public" });
119 await send("/api/v1/repos/dev/lib/issues", "POST", { title: "T", body: "b" });
120 await send("/api/v1/repos/dev/lib/issues", "POST", { title: "Hidden", body: "x", sealed: true });
121 await send("/api/v1/repos/dev/lib/issues/1/comments", "POST", { body: "one" });
122 await send("/api/v1/repos/dev/lib/issues/2/comments", "POST", { body: "secret" });
123 });
124
125 afterAll(async () => {
126 await servers.stop();
127 await rm(dataDir, { recursive: true, force: true });
128 });
129
130 test("200, 304, then 200 with a new tag after a comment", async () => {
131 const path = "/api/v1/repos/dev/lib/issues/1/comments";
132 const first = await send(path);
133 expect(first.status).toBe(200);
134 const tag = first.headers.get("etag")!;
135 expect(tag).toMatch(/^W\//);
136 expect(first.headers.get("cache-control")).toBe("private, no-cache");
137 expect(first.headers.get("vary")).toContain("authorization, cookie");
138 await first.text();
139 const again = await send(path, "GET", undefined, { "if-none-match": tag });
140 expect(again.status).toBe(304);
141 expect(again.headers.get("etag")).toBe(tag);
142 expect(await again.text()).toBe("");
143 await send(path, "POST", { body: "two" });
144 const changed = await send(path, "GET", undefined, { "if-none-match": tag });
145 expect(changed.status).toBe(200);
146 expect(changed.headers.get("etag")).not.toBe(tag);
147 const cursorTag = (await send(`${path}?limit=1`)).headers.get("etag");
148 expect(cursorTag).not.toBe(changed.headers.get("etag"));
149 });
150
151 test("a stranger is refused a sealed thread whatever tag they hold", async () => {
152 const path = "/api/v1/repos/dev/lib/issues/2/comments";
153 const own = (await send(path)).headers.get("etag")!;
154 const out = await send(path, "GET", undefined, { "if-none-match": own }, false);
155 expect(out.status).not.toBe(304);
156 expect(out.status).not.toBe(200);
157 const star = await send(path, "GET", undefined, { "if-none-match": "*" }, false);
158 expect(star.status).not.toBe(304);
159 expect(star.status).not.toBe(200);
160 });
161 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs