Public: anyone can read this; the crew can change it.
GET /api/v1/repos/:owner/:name/pulls/:number carries an ETag and answers a repeat with 304 until the pull's head, checks, review or state change #9146
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/9142-get-api-v1-repos-owner-name-pulls-number → main 2 commits fork
3 files
changed +105
−2
src/contract/openapi/routes/getApiV1ReposOwnerNamePullsNumber.ts +1 −1
| @@ -1,3 +1,3 @@ | ||
| 1 | 1 | import { REPO, type Route } from "../shared.ts"; |
| 2 | 2 | |
| 3 | export const route: Route = { method: "get", path: `${REPO}/pulls/{number}`, summary: "Read one pull with its diff and review gate (diff=0 leaves the diff out); `check` is the word for the head's check: green, red, running or none", auth: false, query: { diff: "string" }, reply: { check: "string" } }; | |
| 3 | export const route: Route = { method: "get", path: `${REPO}/pulls/{number}`, summary: "Read one pull with its diff and review gate (diff=0 leaves the diff out); `check` is the word for the head's check: green, red, running or none; the 200 carries a weak etag of the exact body, cache-control private, no-cache and vary authorization, cookie, and a repeat sending If-None-Match gets 304 with no body and the same etag until the head, a check verdict, a review or the state changes", auth: false, alsoStatus: [304], query: { diff: "string" }, reply: { check: "string" } }; |
src/http/api.ts +6 −1
| @@ -2520,7 +2520,7 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 2520 | 2520 | ); |
| 2521 | 2521 | return timePhase(ctx.request, "render", () => { |
| 2522 | 2522 | const hold = getHold(ctx.app.db, found.pull.id); |
| 2523 | return json({ | |
| 2523 | const text = JSON.stringify({ | |
| 2524 | 2524 | ...withLabels(ctx, found.pull), |
| 2525 | 2525 | check: checkWord(checkSummary(ctx.app.db, found.pull.id, status.headOid).run ?? undefined, status.headOid), |
| 2526 | 2526 | checkStale: checkStaleNow, |
| @@ -2531,6 +2531,11 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 2531 | 2531 | ...(diff === undefined ? {} : { diff }), |
| 2532 | 2532 | hold: hold === undefined ? null : holdJson(hold), |
| 2533 | 2533 | }); |
| 2534 | // Tagged by the exact bytes sent, after access was checked: a new head, verdict, review or merge is never hidden behind a 304. | |
| 2535 | const etag = weakEtag(text); | |
| 2536 | const headers = { etag, "cache-control": "private, no-cache", vary: "authorization, cookie" }; | |
| 2537 | if (holds(ctx.request, etag)) return new Response(null, { status: 304, headers }); | |
| 2538 | return new Response(text, { headers: { "content-type": "application/json", ...headers } }); | |
| 2534 | 2539 | }); |
| 2535 | 2540 | }); |
| 2536 | 2541 |
test/pull-etag.test.ts added +98
| @@ -0,0 +1,98 @@ | ||
| 1 | /** GET …/pulls/:number carries an ETag of the exact body; a repeat is 304 until the pull changes (#9142). */ | |
| 2 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 3 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 4 | import { tmpdir } from "node:os"; | |
| 5 | import { join } from "node:path"; | |
| 6 | import { createApp, type App } from "../src/app.ts"; | |
| 7 | import { openPull } from "../src/actions/pulls.ts"; | |
| 8 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; | |
| 9 | import { createSession } from "../src/core/sessions.ts"; | |
| 10 | import { createUser } from "../src/core/users.ts"; | |
| 11 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 12 | import { quietLogs } from "./helpers/globals.ts"; | |
| 13 | ||
| 14 | quietLogs(); | |
| 15 | ||
| 16 | let dataDir: string; | |
| 17 | let app: App; | |
| 18 | let servers: Servers; | |
| 19 | let cookie = ""; | |
| 20 | let stranger = ""; | |
| 21 | let number = 0; | |
| 22 | let repoId = ""; | |
| 23 | let issueId = ""; | |
| 24 | const cells = process.env["FORGE_CELLS"]; | |
| 25 | ||
| 26 | beforeAll(async () => { | |
| 27 | process.env["FORGE_CELLS"] = "off"; | |
| 28 | dataDir = await mkdtemp(join(tmpdir(), "forge-pull-etag-")); | |
| 29 | app = createApp({ dataDir, port: 0 }); | |
| 30 | servers = await serve(app); | |
| 31 | const made = await createUser(app.db, { | |
| 32 | origin: "seed", trusted: true, username: "alice", email: "[email protected]", | |
| 33 | password: "a-good-passphrase", adminIfFirst: true, | |
| 34 | }); | |
| 35 | if (!made.ok) throw new Error(String(made.error)); | |
| 36 | const alice = made.value; | |
| 37 | cookie = `forge_session=${createSession(app.db, alice.id).token}`; | |
| 38 | const other = await createUser(app.db, { | |
| 39 | origin: "seed", trusted: true, username: "bob", email: "[email protected]", password: "a-good-passphrase", | |
| 40 | }); | |
| 41 | if (!other.ok) throw new Error(String(other.error)); | |
| 42 | stranger = `forge_session=${createSession(app.db, other.value.id).token}`; | |
| 43 | const repo = await createRepo(app, alice, { name: "r", visibility: "private" }); | |
| 44 | if (!repo.ok) throw new Error(String(repo.error)); | |
| 45 | repoId = repo.value.id; | |
| 46 | for (const [branch, file] of [[undefined, "forge.json"], ["topic", "t.txt"]] as const) { | |
| 47 | const done = await commitToBranch(app, alice, repo.value, { | |
| 48 | ...(branch === undefined ? {} : { branch }), | |
| 49 | message: "change", | |
| 50 | files: [{ path: file, content: file === "forge.json" ? "{}" : "x" }], | |
| 51 | }); | |
| 52 | if (!done.ok) throw new Error(JSON.stringify(done.error)); | |
| 53 | } | |
| 54 | const pull = await openPull(app, alice, repo.value, { title: "t", body: "", baseBranch: "main", headBranch: "topic" }); | |
| 55 | if (!pull.ok) throw new Error(JSON.stringify(pull.error)); | |
| 56 | number = pull.value.number; | |
| 57 | issueId = pull.value.id; | |
| 58 | }, 120_000); | |
| 59 | ||
| 60 | afterAll(async () => { | |
| 61 | await servers.stop(); | |
| 62 | app.close(); | |
| 63 | if (cells === undefined) delete process.env["FORGE_CELLS"]; | |
| 64 | else process.env["FORGE_CELLS"] = cells; | |
| 65 | await rm(dataDir, { recursive: true, force: true }); | |
| 66 | }); | |
| 67 | ||
| 68 | const get = (c: string, tag?: string) => | |
| 69 | fetch(`${servers.url}/api/v1/repos/alice/r/pulls/${number}`, { | |
| 70 | headers: { cookie: c, ...(tag === undefined ? {} : { "if-none-match": tag }) }, | |
| 71 | }); | |
| 72 | ||
| 73 | test("200 with a tag, 304 on a repeat, 200 with a new tag after a check verdict, 404 for a stranger", async () => { | |
| 74 | const first = await get(cookie); | |
| 75 | expect(first.status).toBe(200); | |
| 76 | const tag = first.headers.get("etag")!; | |
| 77 | expect(tag).toMatch(/^W\//); | |
| 78 | expect(first.headers.get("cache-control")).toBe("private, no-cache"); | |
| 79 | expect(first.headers.get("vary")).toContain("authorization, cookie"); | |
| 80 | const body = (await first.json()) as { status: { headOid: string } }; | |
| 81 | ||
| 82 | const again = await get(cookie, tag); | |
| 83 | expect(again.status).toBe(304); | |
| 84 | expect(again.headers.get("etag")).toBe(tag); | |
| 85 | expect(await again.text()).toBe(""); | |
| 86 | ||
| 87 | app.db | |
| 88 | .query("INSERT INTO run (id, repo_id, kind, issue_id, commit_oid, state) VALUES (?, ?, 'check', ?, ?, 'ok')") | |
| 89 | .run(`zzzz-${crypto.randomUUID()}`, repoId, issueId, body.status.headOid); | |
| 90 | const changed = await get(cookie, tag); | |
| 91 | expect(changed.status).toBe(200); | |
| 92 | expect(changed.headers.get("etag")).not.toBe(tag); | |
| 93 | await changed.text(); | |
| 94 | ||
| 95 | const denied = await get(stranger, tag); | |
| 96 | expect(denied.status).toBe(404); | |
| 97 | expect(denied.headers.get("etag")).toBeNull(); | |
| 98 | }); |
new above
offline