Public: anyone can read this; the crew can change it.
GET /api/v1/repos carries an ETag and answers a repeat with 304 when no repository it lists has changed #9122
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/9115-get-api-v1-repos-carries-an-etag-and-ans → main 5 commits fork 1 thread · 1 unresolved
5 files
changed +222
−8
src/contract/openapi/routes/getApiV1Repos.ts +1 −1
| @@ -1,4 +1,4 @@ | ||
| 1 | 1 | import { type Route, limitTo } from "../shared.ts"; |
| 2 | 2 | import { LIST_LIMIT_MAX } from "../../../core/issues.ts"; |
| 3 | 3 | |
| 4 | export const route: Route = { method: "get", path: "/api/v1/repos", summary: "List public repositories", auth: false, query: { limit: limitTo(LIST_LIMIT_MAX), cursor: "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)" } }; | |
| 4 | export const route: Route = { method: "get", path: "/api/v1/repos", summary: "List repositories the viewer can see; tagged with a weak ETag, and a repeat with If-None-Match gets 304 until a listed repository changes", auth: false, alsoStatus: [304], query: { limit: limitTo(LIST_LIMIT_MAX), cursor: "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)" } }; |
src/core/repos.ts +60 −4
| @@ -822,11 +822,11 @@ const VISIBLE = REPO_VISIBLE; | ||
| 822 | 822 | const newestFirst = (options: ListReposOptions): string => |
| 823 | 823 | `${options.appsFirst === true ? "EXISTS (SELECT 1 FROM app WHERE app.repo_id = repo.id) DESC, " : ""}repo.id DESC`; |
| 824 | 824 | |
| 825 | export function listReposVisibleTo( | |
| 826 | db: Database, | |
| 825 | /** The WHERE and parameters a visible-to listing reads by; the page and its tag share them. */ | |
| 826 | function visibleClause( | |
| 827 | 827 | viewer: User | undefined, |
| 828 | options: ListReposOptions = {}, | |
| 829 | ): Repo[] { | |
| 828 | options: ListReposOptions, | |
| 829 | ): { where: string; params: (string | number)[] } { | |
| 830 | 830 | const params: (string | number)[] = [viewer?.id ?? ""]; |
| 831 | 831 | let where = viewer ? VISIBLE : PUBLIC_VISIBLE; |
| 832 | 832 | if (options.query !== undefined && options.query.trim() !== "") { |
| @@ -842,6 +842,15 @@ export function listReposVisibleTo( | ||
| 842 | 842 | if (options.withoutArchived === true) where += " AND repo.archived_at IS NULL"; |
| 843 | 843 | if (options.archivedOnly === true) where += " AND repo.archived_at IS NOT NULL"; |
| 844 | 844 | params.push(pageLimit(options.limit)); |
| 845 | return { where, params }; | |
| 846 | } | |
| 847 | ||
| 848 | export function listReposVisibleTo( | |
| 849 | db: Database, | |
| 850 | viewer: User | undefined, | |
| 851 | options: ListReposOptions = {}, | |
| 852 | ): Repo[] { | |
| 853 | const { where, params } = visibleClause(viewer, options); | |
| 845 | 854 | const rows = db |
| 846 | 855 | .query<RepoRow, (string | number)[]>( |
| 847 | 856 | `${SELECT} WHERE ${where} ORDER BY ${newestFirst(options)} LIMIT ?${params.length}`, |
| @@ -851,6 +860,53 @@ export function listReposVisibleTo( | ||
| 851 | 860 | return withCounts(db, rows); |
| 852 | 861 | } |
| 853 | 862 | |
| 863 | /** The columns `toRepo` shows; `issue_seq` is not one, since a sealed report moves it. */ | |
| 864 | const SHOWN = [ | |
| 865 | "repo.id", "repo.owner_id", "user.username", "repo.name", "repo.visibility", "repo.description", | |
| 866 | "repo.default_branch", "repo.protect_default", "repo.require_review", "repo.require_checks", | |
| 867 | "repo.autonomy", "repo.autonomy_direction", "repo.autonomy_ceiling_usd", "repo.autonomy_slots", | |
| 868 | "repo.planner_slots", "repo.reviewer_slots", "repo.polish_share", "repo.autonomy_by", | |
| 869 | "repo.crew_outsiders", "repo.decisions_by", "repo.plans_need_nod", "repo.direction_issue_id", | |
| 870 | "repo.fork_parent_id", "repo.sealed_for", "repo.archived_at", "repo.origin_id", | |
| 871 | "repo.origin_full_name", "repo.origin_html_url", "repo.last_fetch_at", "repo.fetch_error", | |
| 872 | "repo.origin_detached_at", | |
| 873 | ].join(", "); | |
| 874 | ||
| 875 | /** | |
| 876 | * A marker that moves whenever the page `listReposVisibleTo` would answer | |
| 877 | * could differ, without building the page: one query that digests the shown | |
| 878 | * columns of the page's rows inside SQLite, the newest push of each, and the | |
| 879 | * open counts. Sealed reports are in neither the counts nor any event kind | |
| 880 | * read, so filing one moves no viewer's marker (`countsForRepos`). A viewer | |
| 881 | * sees only rows they may read, so two viewers who see different pages get | |
| 882 | * different markers. | |
| 883 | */ | |
| 884 | export function visibleReposMarker( | |
| 885 | db: Database, | |
| 886 | viewer: User | undefined, | |
| 887 | options: ListReposOptions = {}, | |
| 888 | ): string { | |
| 889 | const { where, params } = visibleClause(viewer, options); | |
| 890 | const page = `SELECT repo.id AS id, json_array(${SHOWN}) AS shown FROM repo | |
| 891 | JOIN user ON user.id = repo.owner_id | |
| 892 | WHERE ${where} ORDER BY ${newestFirst(options)} LIMIT ?${params.length}`; | |
| 893 | const row = db | |
| 894 | .query<{ shown: string | null; pushes: string | null; counts: string | null }, (string | number)[]>( | |
| 895 | `WITH page AS (${page}) | |
| 896 | SELECT | |
| 897 | (SELECT group_concat(shown, char(10)) FROM page) AS shown, | |
| 898 | (SELECT group_concat(repo_id || ':' || last, ',') FROM | |
| 899 | (SELECT repo_id, MAX(id) AS last FROM event | |
| 900 | WHERE kind = 'push' AND repo_id IN (SELECT id FROM page) GROUP BY repo_id ORDER BY repo_id)) AS pushes, | |
| 901 | (SELECT group_concat(repo_id || ':' || kind || ':' || n, ',') FROM | |
| 902 | (SELECT repo_id, kind, COUNT(*) AS n FROM issue | |
| 903 | WHERE state = 'open' AND sealed = 0 AND repo_id IN (SELECT id FROM page) | |
| 904 | GROUP BY repo_id, kind ORDER BY repo_id, kind)) AS counts`, | |
| 905 | ) | |
| 906 | .get(...params); | |
| 907 | return Bun.hash(`${row?.shown ?? ""}|${row?.pushes ?? ""}|${row?.counts ?? ""}`).toString(36); | |
| 908 | } | |
| 909 | ||
| 854 | 910 | /** |
| 855 | 911 | * Repos this viewer may write: their own, or an org's through a team with |
| 856 | 912 | * write or admin access — `REPO_WRITABLE`, the SQL twin of `canWrite`, for a |
src/http/api.ts +12 −1
| @@ -158,6 +158,7 @@ import { | ||
| 158 | 158 | getRepo, |
| 159 | 159 | listReposByOwner, |
| 160 | 160 | listReposVisibleTo, |
| 161 | visibleReposMarker, | |
| 161 | 162 | type Repo, |
| 162 | 163 | type RepoCounts, |
| 163 | 164 | canWrite, |
| @@ -1483,8 +1484,18 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 1483 | 1484 | notePhase(ctx.request, "session", takeSessionTime(ctx.request)); |
| 1484 | 1485 | const query = repoQuery(ctx); |
| 1485 | 1486 | if (query instanceof Response) return query; |
| 1487 | // A poller holding the tag gets 304 after the marker alone: the rows are | |
| 1488 | // not built, counted or serialised (#9115). | |
| 1489 | const marker = timePhase(ctx.request, "tag", () => visibleReposMarker(ctx.app.db, ctx.auth.user, query)); | |
| 1490 | const viewer = ctx.auth.user === undefined ? "anon" : `u${Bun.hash(ctx.auth.user.id).toString(36)}`; | |
| 1491 | const headers = { | |
| 1492 | etag: `W/"repos-${Bun.hash(`${viewer}|${query.limit}|${query.cursor ?? ""}|${marker}`).toString(36)}"`, | |
| 1493 | "cache-control": "private, no-cache", | |
| 1494 | vary: "authorization, cookie", | |
| 1495 | }; | |
| 1496 | if (holds(ctx.request, headers.etag)) return new Response(null, { status: 304, headers }); | |
| 1486 | 1497 | const items = timePhase(ctx.request, "repos", () => listReposVisibleTo(ctx.app.db, ctx.auth.user, query)); |
| 1487 | return timePhase(ctx.request, "render", () => json(paged(items, query.limit, (r) => r.id))); | |
| 1498 | return timePhase(ctx.request, "render", () => json(paged(items, query.limit, (r) => r.id), 200, headers)); | |
| 1488 | 1499 | }); |
| 1489 | 1500 | |
| 1490 | 1501 | router.add("POST", "/api/v1/repos", async (ctx) => { |
test/api-publicrepos-timing.test.ts +2 −2
| @@ -53,9 +53,9 @@ 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 repositories list names session, repos, render in order", async () => { | |
| 56 | test("the repositories list names session, tag, repos, render in order", async () => { | |
| 57 | 57 | const got = await phases("/api/v1/repos"); |
| 58 | 58 | expect(got.status).toBe(200); |
| 59 | expect(got.names).toEqual(["session", "repos", "render"]); | |
| 59 | expect(got.names).toEqual(["session", "tag", "repos", "render"]); | |
| 60 | 60 | expect(JSON.parse(got.body)).toBeTruthy(); |
| 61 | 61 | }); |
test/repos-etag.test.ts added +147
| @@ -0,0 +1,147 @@ | ||
| 1 | /** GET /api/v1/repos is tagged and answers a repeat 304 until a listed repository changes (#9115). */ | |
| 2 | import { afterAll, beforeAll, describe, 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 { createRepo } from "../src/actions/repos.ts"; | |
| 8 | import { createToken } from "../src/core/tokens.ts"; | |
| 9 | import { createUser, type User } from "../src/core/users.ts"; | |
| 10 | import { createIssue } from "../src/core/issues.ts"; | |
| 11 | import { updateRepoSettings, type Repo } from "../src/core/repos.ts"; | |
| 12 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 13 | import { promote } from "./helpers/members.ts"; | |
| 14 | ||
| 15 | let dataDir: string; | |
| 16 | let app: App; | |
| 17 | let servers: Servers; | |
| 18 | let alice: User; | |
| 19 | let bob: User; | |
| 20 | let tokens: Record<string, string>; | |
| 21 | let open: Repo; | |
| 22 | ||
| 23 | const get = (who: string | undefined, etag?: string, path = "/api/v1/repos") => | |
| 24 | fetch(`${servers.url}${path}`, { | |
| 25 | headers: { | |
| 26 | ...(who === undefined ? {} : { authorization: `Bearer ${tokens[who]}` }), | |
| 27 | ...(etag === undefined ? {} : { "if-none-match": etag }), | |
| 28 | }, | |
| 29 | }); | |
| 30 | const tagOf = async (who: string | undefined, path?: string) => { | |
| 31 | const res = await get(who, undefined, path); | |
| 32 | await res.arrayBuffer(); | |
| 33 | expect(res.status).toBe(200); | |
| 34 | return res.headers.get("etag")!; | |
| 35 | }; | |
| 36 | ||
| 37 | beforeAll(async () => { | |
| 38 | dataDir = await mkdtemp(join(tmpdir(), "forge-repos-etag-")); | |
| 39 | app = createApp({ dataDir, port: 0 }); | |
| 40 | servers = await serve(app); | |
| 41 | const mk = async (username: string, first = false) => { | |
| 42 | const r = await createUser(app.db, { | |
| 43 | origin: "seed", trusted: true, username, allowReserved: true, | |
| 44 | email: `${username}@forge.local`, password: "a-good-passphrase", adminIfFirst: first, | |
| 45 | }); | |
| 46 | if (!r.ok) throw new Error(r.error); | |
| 47 | promote(app.db, r.value); | |
| 48 | return r.value; | |
| 49 | }; | |
| 50 | await mk("root", true); | |
| 51 | alice = await mk("alice"); | |
| 52 | bob = await mk("bob"); | |
| 53 | tokens = { | |
| 54 | alice: createToken(app.db, alice.id, "t", "rw").token, | |
| 55 | bob: createToken(app.db, bob.id, "t", "rw").token, | |
| 56 | }; | |
| 57 | const o = await createRepo(app, alice, { name: "open", visibility: "public" }); | |
| 58 | const s = await createRepo(app, alice, { name: "secret", visibility: "private" }); | |
| 59 | const b = await createRepo(app, bob, { name: "bobs", visibility: "private" }); | |
| 60 | if (!o.ok || !s.ok || !b.ok) throw new Error("repos"); | |
| 61 | open = o.value; | |
| 62 | }); | |
| 63 | ||
| 64 | afterAll(async () => { | |
| 65 | servers.stop(); | |
| 66 | app.close(); | |
| 67 | await rm(dataDir, { recursive: true, force: true }); | |
| 68 | }); | |
| 69 | ||
| 70 | describe("GET /api/v1/repos etag", () => { | |
| 71 | test("carries a weak tag, private no-cache and vary", async () => { | |
| 72 | const res = await get(undefined); | |
| 73 | await res.arrayBuffer(); | |
| 74 | expect(res.headers.get("etag")).toMatch(/^W\/"/); | |
| 75 | expect(res.headers.get("cache-control")).toBe("private, no-cache"); | |
| 76 | expect(res.headers.get("vary")).toContain("authorization, cookie"); | |
| 77 | }); | |
| 78 | ||
| 79 | test("a repeat is a 304 with no body", async () => { | |
| 80 | const tag = await tagOf("alice"); | |
| 81 | const res = await get("alice", tag); | |
| 82 | expect(res.status).toBe(304); | |
| 83 | expect(await res.text()).toBe(""); | |
| 84 | expect(res.headers.get("etag")).toBe(tag); | |
| 85 | }); | |
| 86 | ||
| 87 | test("the cursor and limit are part of the tag", async () => { | |
| 88 | const a = await tagOf("alice"); | |
| 89 | expect(await tagOf("alice", "/api/v1/repos?limit=1")).not.toBe(a); | |
| 90 | }); | |
| 91 | ||
| 92 | test("a new repository makes the next answer a 200 with a new tag", async () => { | |
| 93 | const tag = await tagOf("alice"); | |
| 94 | const made = await createRepo(app, alice, { name: "fresh", visibility: "public" }); | |
| 95 | if (!made.ok) throw new Error("repo"); | |
| 96 | const res = await get("alice", tag); | |
| 97 | expect(res.status).toBe(200); | |
| 98 | expect(res.headers.get("etag")).not.toBe(tag); | |
| 99 | expect(JSON.stringify(await res.json())).toContain("fresh"); | |
| 100 | }); | |
| 101 | ||
| 102 | test("a push changes the tag", async () => { | |
| 103 | const tag = await tagOf(undefined); | |
| 104 | app.bus.append({ kind: "push", repoId: open.id, actorId: alice.id, payload: {} }); | |
| 105 | expect(await tagOf(undefined)).not.toBe(tag); | |
| 106 | }); | |
| 107 | ||
| 108 | test("a rename changes the tag", async () => { | |
| 109 | const tag = await tagOf(undefined); | |
| 110 | app.db.query("UPDATE repo SET name = 'opened' WHERE id = ?1").run(open.id); | |
| 111 | expect(await tagOf(undefined)).not.toBe(tag); | |
| 112 | }); | |
| 113 | ||
| 114 | test("turning a listed repository private changes an anonymous viewer's tag", async () => { | |
| 115 | const tag = await tagOf(undefined); | |
| 116 | updateRepoSettings(app.db, open, { visibility: "private" }); | |
| 117 | const res = await get(undefined, tag); | |
| 118 | expect(res.status).toBe(200); | |
| 119 | expect(res.headers.get("etag")).not.toBe(tag); | |
| 120 | }); | |
| 121 | ||
| 122 | test("viewers who see different lists never share a tag", async () => { | |
| 123 | const [a, b, anon] = [await tagOf("alice"), await tagOf("bob"), await tagOf(undefined)]; | |
| 124 | expect(new Set([a, b, anon]).size).toBe(3); | |
| 125 | expect((await get("bob", a)).status).toBe(200); | |
| 126 | expect((await get(undefined, a)).status).toBe(200); | |
| 127 | }); | |
| 128 | ||
| 129 | test("a private repository moves no tag for a viewer who cannot see it", async () => { | |
| 130 | const tag = await tagOf("bob"); | |
| 131 | const made = await createRepo(app, alice, { name: "hidden", visibility: "private" }); | |
| 132 | if (!made.ok) throw new Error("repo"); | |
| 133 | expect((await get("bob", tag)).status).toBe(304); | |
| 134 | }); | |
| 135 | ||
| 136 | test("filing a sealed report moves no tag; an open issue does", async () => { | |
| 137 | const fresh = app.db.query<{ id: string }, []>("SELECT id FROM repo WHERE name = 'fresh'").get()!; | |
| 138 | const [anon, other] = [await tagOf(undefined), await tagOf("bob")]; | |
| 139 | const sealed = createIssue(app.db, { repoId: fresh.id, authorId: alice.id, title: "hole", sealed: true }); | |
| 140 | if (!sealed.ok) throw new Error("issue"); | |
| 141 | expect((await get(undefined, anon)).status).toBe(304); | |
| 142 | expect((await get("bob", other)).status).toBe(304); | |
| 143 | const plain = createIssue(app.db, { repoId: fresh.id, authorId: alice.id, title: "bug" }); | |
| 144 | if (!plain.ok) throw new Error("issue"); | |
| 145 | expect((await get(undefined, anon)).status).toBe(200); | |
| 146 | }); | |
| 147 | }); |
new above
offline
This computes the full-row marker on every request, including the 304 path. Make it a cheap change marker (see the review body, item 1).