Public: anyone can read this; the crew can change it.
GET /api/v1/repos/:owner/:name/issues carries an ETag and answers a repeat with 304 when the list it would send has not changed #9120
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/9117-get-api-v1-repos-owner-name-issues-carri → main 2 commits fork
3 files
changed +95
−2
src/contract/openapi/routes/getApiV1ReposOwnerNameIssues.ts +1 −1
| @@ -1,3 +1,3 @@ | ||
| 1 | 1 | import { LIST_QUERY, REPO, type Route } from "../shared.ts"; |
| 2 | 2 | |
| 3 | export const route: Route = { method: "get", path: `${REPO}/issues`, summary: "List a repository's issues; fields=brief leaves out bodies and comments, any other fields value is a 422 invalid_fields", auth: false, query: { ...LIST_QUERY, fields: "string" }, refusals: { invalid_limit: "limit is not a whole number from 1 to 200 (422)", invalid_cursor: "cursor is not the next_cursor of a previous page (422)", invalid_state: "state is not open, closed or all (422)" } }; | |
| 3 | export const route: Route = { method: "get", path: `${REPO}/issues`, summary: "List a repository's issues; fields=brief leaves out bodies and comments, any other fields value is a 422 invalid_fields; 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", auth: false, alsoStatus: [304], query: { ...LIST_QUERY, fields: "string" }, refusals: { invalid_limit: "limit is not a whole number from 1 to 200 (422)", invalid_cursor: "cursor is not the next_cursor of a previous page (422)", invalid_state: "state is not open, closed or all (422)" } }; |
src/http/api/issues.ts +8 −1
| @@ -5,6 +5,7 @@ | ||
| 5 | 5 | * (#7450); the rules live in `actions/issues.ts` and its neighbours. |
| 6 | 6 | */ |
| 7 | 7 | |
| 8 | import { holds, weakEtag } from "../etag.ts"; | |
| 8 | 9 | import { ISSUE_TEAMS_SAY, nameTeams } from "../../actions/issue_teams.ts"; |
| 9 | 10 | import { confirmIssue, dependencyWords, linkIssue, unlinkIssue } from "../../actions/dependencies.ts"; |
| 10 | 11 | import { |
| @@ -150,9 +151,15 @@ export function issueRoutes(router: Router<Handler>): void { | ||
| 150 | 151 | const items = timePhase(ctx.request, "labels", () => withLabelsMany(ctx, listed)); |
| 151 | 152 | return timePhase(ctx.request, "render", () => { |
| 152 | 153 | const page = paged(items, query.limit, (i) => i.number); |
| 153 | return json( | |
| 154 | const text = JSON.stringify( | |
| 154 | 155 | fields === "brief" ? { ...page, items: page.items.map(briefIssue) } : page, |
| 155 | 156 | ); |
| 157 | // Scoped by who asks (sealed issues), so private to the reader; the tag | |
| 158 | // 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 } }); | |
| 156 | 163 | }); |
| 157 | 164 | }); |
| 158 | 165 |
test/issues-etag.test.ts added +86
| @@ -0,0 +1,86 @@ | ||
| 1 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 2 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 3 | import { tmpdir } from "node:os"; | |
| 4 | import { join } from "node:path"; | |
| 5 | import { createApp, type App } from "../src/app.ts"; | |
| 6 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 7 | import { seedAdmin } from "./helpers/admin.ts"; | |
| 8 | ||
| 9 | describe("issues list ETag", () => { | |
| 10 | let dataDir: string; | |
| 11 | let app: App; | |
| 12 | let servers: Servers; | |
| 13 | let base: string; | |
| 14 | let token = ""; | |
| 15 | ||
| 16 | const send = (path: string, method = "GET", bodyObj?: unknown, headers: Record<string, string> = {}, auth = true) => | |
| 17 | fetch(`${base}${path}`, { | |
| 18 | method, | |
| 19 | headers: { | |
| 20 | ...(auth ? { authorization: `Bearer ${token}` } : {}), | |
| 21 | "content-type": "application/json", | |
| 22 | ...headers, | |
| 23 | }, | |
| 24 | ...(bodyObj === undefined ? {} : { body: JSON.stringify(bodyObj) }), | |
| 25 | }); | |
| 26 | const LIST = "/api/v1/repos/dev/lib/issues"; | |
| 27 | ||
| 28 | beforeAll(async () => { | |
| 29 | dataDir = await mkdtemp(join(tmpdir(), "forge-issues-etag-")); | |
| 30 | app = createApp({ dataDir, port: 0 }); | |
| 31 | servers = await serve(app); | |
| 32 | base = servers.url; | |
| 33 | await seedAdmin(app, { username: "dev", email: "[email protected]", password: "etag-pass-1" }); | |
| 34 | token = await fetch(`${base}/api/v1/tokens`, { | |
| 35 | method: "POST", | |
| 36 | headers: { "content-type": "application/json" }, | |
| 37 | body: JSON.stringify({ adminConfirm: true, username: "dev", password: "etag-pass-1", name: "c", scope: "rw" }), | |
| 38 | }) | |
| 39 | .then((r) => r.json()) | |
| 40 | .then((j) => (j as { token: string }).token); | |
| 41 | await send("/api/v1/repos", "POST", { name: "lib", visibility: "public" }); | |
| 42 | await send(LIST, "POST", { title: "First", body: "b" }); | |
| 43 | }); | |
| 44 | ||
| 45 | afterAll(async () => { | |
| 46 | await servers.stop(); | |
| 47 | await rm(dataDir, { recursive: true, force: true }); | |
| 48 | }); | |
| 49 | ||
| 50 | test("200, then 304 on a repeat, then 200 with a new tag after a change", async () => { | |
| 51 | const first = await send(LIST); | |
| 52 | expect(first.status).toBe(200); | |
| 53 | const tag = first.headers.get("etag")!; | |
| 54 | expect(tag).toMatch(/^W\//); | |
| 55 | expect(first.headers.get("cache-control")).toBe("private, no-cache"); | |
| 56 | expect(first.headers.get("vary")).toContain("authorization, cookie"); | |
| 57 | await first.text(); | |
| 58 | ||
| 59 | const again = await send(LIST, "GET", undefined, { "if-none-match": tag }); | |
| 60 | expect(again.status).toBe(304); | |
| 61 | expect(again.headers.get("etag")).toBe(tag); | |
| 62 | expect(await again.text()).toBe(""); | |
| 63 | ||
| 64 | await send(LIST, "POST", { title: "Second", body: "b" }); | |
| 65 | const changed = await send(LIST, "GET", undefined, { "if-none-match": tag }); | |
| 66 | expect(changed.status).toBe(200); | |
| 67 | expect(changed.headers.get("etag")).not.toBe(tag); | |
| 68 | expect(((await changed.json()) as { items: unknown[] }).items.length).toBe(2); | |
| 69 | }); | |
| 70 | ||
| 71 | test("fields=brief and limit tag their own page", async () => { | |
| 72 | const full = (await send(LIST)).headers.get("etag"); | |
| 73 | const brief = (await send(`${LIST}?fields=brief`)).headers.get("etag"); | |
| 74 | const one = (await send(`${LIST}?limit=1`)).headers.get("etag"); | |
| 75 | expect(new Set([full, brief, one]).size).toBe(3); | |
| 76 | }); | |
| 77 | ||
| 78 | test("a signed-out reader and a writer who sees a sealed issue get different tags", async () => { | |
| 79 | const before = (await send(LIST, "GET", undefined, {}, false)).headers.get("etag"); | |
| 80 | await send(LIST, "POST", { title: "Sealed", body: "x", sealed: true }); | |
| 81 | const out = await send(LIST, "GET", undefined, {}, false); | |
| 82 | const writer = await send(LIST); | |
| 83 | expect(out.headers.get("etag")).toBe(before!); | |
| 84 | expect(writer.headers.get("etag")).not.toBe(out.headers.get("etag")); | |
| 85 | }); | |
| 86 | }); |
new above
offline