Public: anyone can read this; the crew can change it.
GET /api/v1/repos/:owner/:name/branches carries an ETag and answers a repeat with 304 until a ref moves #9160
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/9157-get-api-v1-repos-owner-name-branches-car → main 4 commits fork 1 thread · 1 unresolved
5 files
changed +146
−8
src/contract/openapi/routes/getApiV1ReposOwnerNameBranches.ts +2 −1
| @@ -5,7 +5,8 @@ export const route: Route = { | ||
| 5 | 5 | method: "get", |
| 6 | 6 | path: `${REPO}/branches`, |
| 7 | 7 | summary: |
| 8 | "List a repository's branches; answers { items: [{ name, oid, isDefault, ahead, behind, protected }], next_cursor }; ahead and behind are null past the comparison cap; limit 1 to 200, cursor from next_cursor; a private repository needs a token", | |
| 8 | "List a repository's branches; answers { items: [{ name, oid, isDefault, ahead, behind, protected }], next_cursor }; ahead and behind are null past the comparison cap; limit 1 to 200, cursor from next_cursor; a private repository needs a token; the 200 carries a weak etag of the cursor, limit, default branch and every ref's name and oid, and cache-control private, no-cache; a repeat sending If-None-Match gets 304 with no body until a branch is pushed, created or deleted", | |
| 9 | alsoStatus: [304], | |
| 9 | 10 | auth: false, |
| 10 | 11 | query: { limit: limitTo(LIST_LIMIT_MAX), cursor: "string" }, |
| 11 | 12 | refusals: { |
src/http/api.ts +13 −4
| @@ -1688,13 +1688,22 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 1688 | 1688 | // The cursor is the name of the last branch of the page before. |
| 1689 | 1689 | const start = cursor === null ? 0 : all.findIndex((r) => r.name === cursor) + 1; |
| 1690 | 1690 | if (start === 0 && cursor !== null) return undefined; |
| 1691 | // Ahead/behind rests on the default's tip, so the tag covers every ref. | |
| 1692 | const etag = weakEtag( | |
| 1693 | [limit, cursor ?? "", repo.defaultBranch, repo.protectDefault, ...all.map((r) => `${r.name}\0${r.oid}`)].join("\n"), | |
| 1694 | ); | |
| 1695 | const headers = { etag, "cache-control": "private, no-cache", vary: "authorization, cookie" }; | |
| 1696 | if (holds(ctx.request, etag)) return { notModified: new Response(null, { status: 304, headers }) }; | |
| 1691 | 1697 | const slice = all.slice(start, start + limit); |
| 1692 | const compared = await compareBranches(dir, repo.defaultBranch, slice, all.length); | |
| 1693 | return { branches: compared.branches, more: start + limit < all.length }; | |
| 1698 | return { slice, total: all.length, more: start + limit < all.length, headers }; | |
| 1694 | 1699 | }); |
| 1695 | 1700 | if (page === undefined) |
| 1696 | 1701 | return apiError("invalid_cursor", 422, "cursor is the next_cursor of a page before"); |
| 1697 | const items = page.branches.map((b) => ({ | |
| 1702 | if ("notModified" in page) return page.notModified; | |
| 1703 | const compared = await timePhaseAsync(ctx.request, "compare", () => | |
| 1704 | compareBranches(dir, repo.defaultBranch, page.slice, page.total), | |
| 1705 | ); | |
| 1706 | const items = compared.branches.map((b) => ({ | |
| 1698 | 1707 | name: b.name, |
| 1699 | 1708 | oid: b.oid, |
| 1700 | 1709 | isDefault: b.isDefault, |
| @@ -1704,7 +1713,7 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 1704 | 1713 | })); |
| 1705 | 1714 | const more = page.more; |
| 1706 | 1715 | return timePhase(ctx.request, "render", () => |
| 1707 | json({ items, next_cursor: more ? items[items.length - 1]!.name : null }), | |
| 1716 | json({ items, next_cursor: more ? items[items.length - 1]!.name : null }, 200, page.headers), | |
| 1708 | 1717 | ); |
| 1709 | 1718 | }); |
| 1710 | 1719 |
test/api-branches-timing.test.ts +2 −2
| @@ -53,10 +53,10 @@ async function phases(path: string): Promise<{ status: number; names: string[]; | ||
| 53 | 53 | return { status: res.status, names: names.slice(names.indexOf("app") + 1), body }; |
| 54 | 54 | } |
| 55 | 55 | |
| 56 | test("the branches list names session, git, render in order", async () => { | |
| 56 | test("the branches list names session, git, compare, render in order", async () => { | |
| 57 | 57 | const got = await phases("/api/v1/repos/alice/r/branches"); |
| 58 | 58 | expect(got.status).toBe(200); |
| 59 | expect(got.names).toEqual(["session", "git", "render"]); | |
| 59 | expect(got.names).toEqual(["session", "git", "compare", "render"]); | |
| 60 | 60 | expect(Array.isArray(JSON.parse(got.body).items)).toBe(true); |
| 61 | 61 | }); |
| 62 | 62 |
test/branches-deep.test.ts +1 −1
| @@ -69,7 +69,7 @@ async function gitMs(query: string): Promise<{ ms: number; items: { name: string | ||
| 69 | 69 | }); |
| 70 | 70 | const body = (await res.json()) as { items: { name: string; ahead: number | null }[] }; |
| 71 | 71 | expect(res.status).toBe(200); |
| 72 | const ms = Number(/(?:^|, )git;dur=([0-9.]+)/.exec(res.headers.get("server-timing") ?? "")?.[1]); | |
| 72 | const ms = Number(/(?:^|, )compare;dur=([0-9.]+)/.exec(res.headers.get("server-timing") ?? "")?.[1]); | |
| 73 | 73 | return { ms, items: body.items }; |
| 74 | 74 | } |
| 75 | 75 |
test/branches-etag.test.ts added +128
| @@ -0,0 +1,128 @@ | ||
| 1 | /** GET …/branches carries an ETag over every ref; a repeat is 304 until a ref moves (#9157). */ | |
| 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 { commitToBranch, createRepo } from "../src/actions/repos.ts"; | |
| 8 | import { createSession } from "../src/core/sessions.ts"; | |
| 9 | import type { Repo } from "../src/core/repos.ts"; | |
| 10 | import { createUser, type User } 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 repo: Repo; | |
| 21 | let alice: User; | |
| 22 | const cells = process.env["FORGE_CELLS"]; | |
| 23 | ||
| 24 | const commit = async (branch: string | undefined, file: string, content: string) => { | |
| 25 | const done = await commitToBranch(app, alice, repo, { | |
| 26 | ...(branch === undefined ? {} : { branch }), | |
| 27 | message: "change", | |
| 28 | files: [{ path: file, content }], | |
| 29 | }); | |
| 30 | if (!done.ok) throw new Error(JSON.stringify(done.error)); | |
| 31 | }; | |
| 32 | ||
| 33 | beforeAll(async () => { | |
| 34 | process.env["FORGE_CELLS"] = "off"; | |
| 35 | dataDir = await mkdtemp(join(tmpdir(), "forge-branches-etag-")); | |
| 36 | app = createApp({ dataDir, port: 0 }); | |
| 37 | servers = await serve(app); | |
| 38 | const made = await createUser(app.db, { | |
| 39 | origin: "seed", trusted: true, username: "alice", email: "[email protected]", | |
| 40 | password: "a-good-passphrase", adminIfFirst: true, | |
| 41 | }); | |
| 42 | if (!made.ok) throw new Error(String(made.error)); | |
| 43 | alice = made.value; | |
| 44 | cookie = `forge_session=${createSession(app.db, alice.id).token}`; | |
| 45 | const r = await createRepo(app, alice, { name: "r", visibility: "private" }); | |
| 46 | if (!r.ok) throw new Error(String(r.error)); | |
| 47 | repo = r.value; | |
| 48 | await commit(undefined, "forge.json", "{}"); | |
| 49 | await commit("topic", "t.txt", "x"); | |
| 50 | }, 120_000); | |
| 51 | ||
| 52 | afterAll(async () => { | |
| 53 | await servers.stop(); | |
| 54 | app.close(); | |
| 55 | if (cells === undefined) delete process.env["FORGE_CELLS"]; | |
| 56 | else process.env["FORGE_CELLS"] = cells; | |
| 57 | await rm(dataDir, { recursive: true, force: true }); | |
| 58 | }); | |
| 59 | ||
| 60 | const URL_ = "/api/v1/repos/alice/r/branches"; | |
| 61 | const get = (query = "", tag?: string, c: string | undefined = cookie) => | |
| 62 | fetch(`${servers.url}${URL_}${query}`, { | |
| 63 | headers: { ...(c ? { cookie: c } : {}), ...(tag === undefined ? {} : { "if-none-match": tag }) }, | |
| 64 | }); | |
| 65 | const tagOf = async (query = "") => { | |
| 66 | const res = await get(query); | |
| 67 | await res.text(); | |
| 68 | return res.headers.get("etag")!; | |
| 69 | }; | |
| 70 | ||
| 71 | test("200 carries a weak tag and private headers; a repeat is an empty 304", async () => { | |
| 72 | const first = await get(); | |
| 73 | expect(first.status).toBe(200); | |
| 74 | const tag = first.headers.get("etag")!; | |
| 75 | expect(tag).toMatch(/^W\//); | |
| 76 | expect(first.headers.get("cache-control")).toBe("private, no-cache"); | |
| 77 | expect(first.headers.get("vary")).toContain("authorization, cookie"); | |
| 78 | await first.text(); | |
| 79 | expect(first.headers.get("server-timing") ?? "").toContain("compare"); | |
| 80 | const again = await get("", tag); | |
| 81 | expect(again.status).toBe(304); | |
| 82 | expect(again.headers.get("etag")).toBe(tag); | |
| 83 | expect(await again.text()).toBe(""); | |
| 84 | expect(again.headers.get("server-timing") ?? "").not.toContain("compare"); | |
| 85 | }); | |
| 86 | ||
| 87 | test("a push to a branch, to the default, a new branch and a deleted branch each change the tag", async () => { | |
| 88 | let tag = await tagOf(); | |
| 89 | await commit("topic", "t2.txt", "y"); | |
| 90 | let next = await tagOf(); | |
| 91 | expect(next).not.toBe(tag); | |
| 92 | tag = next; | |
| 93 | // The page lists only one row, yet the default's move still changes it. | |
| 94 | const limited = await tagOf("?limit=1"); | |
| 95 | await commit(undefined, "m.txt", "z"); | |
| 96 | expect(await tagOf("?limit=1")).not.toBe(limited); | |
| 97 | tag = await tagOf(); | |
| 98 | await commit("extra", "e.txt", "e"); | |
| 99 | next = await tagOf(); | |
| 100 | expect(next).not.toBe(tag); | |
| 101 | tag = next; | |
| 102 | const del = await fetch(`${servers.url}${URL_}/extra`, { method: "DELETE", headers: { cookie, origin: servers.url } }); | |
| 103 | expect(del.status).toBeLessThan(300); | |
| 104 | const stale = await get("", tag); | |
| 105 | expect(stale.status).toBe(200); | |
| 106 | expect(stale.headers.get("etag")).not.toBe(tag); | |
| 107 | await stale.text(); | |
| 108 | }); | |
| 109 | ||
| 110 | test("limit and cursor tag their own page", async () => { | |
| 111 | const all = await tagOf(); | |
| 112 | const one = await tagOf("?limit=1"); | |
| 113 | const after = await tagOf("?limit=1&cursor=main"); | |
| 114 | expect(new Set([all, one, after]).size).toBe(3); | |
| 115 | }); | |
| 116 | ||
| 117 | test("a bad limit or cursor is a 422 and an unseen repository a 404, none tagged", async () => { | |
| 118 | for (const q of ["?limit=0", "?limit=x", "?cursor=nope", "?cursor="]) { | |
| 119 | const res = await get(q, "*"); | |
| 120 | expect(res.status).toBe(422); | |
| 121 | expect(res.headers.get("etag")).toBeNull(); | |
| 122 | await res.text(); | |
| 123 | } | |
| 124 | const hidden = await get("", "*", ""); | |
| 125 | expect(hidden.status).toBe(404); | |
| 126 | expect(hidden.headers.get("etag")).toBeNull(); | |
| 127 | await hidden.text(); | |
| 128 | }); |
new above
offline
Also assert the Server-Timing header here. The 200 should name the compare phase and the 304 should not. This is what box 2 asks for.