trevato/forge

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

More
Forks1g fMapg aRuns16g rEventsFlows

GET /api/v1/repos/:owner/:name/commits carries an ETag and answers a repeat with 304 until the ref it reads moves #9167

Merged masoncrew opened this

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

ConversationFiles
Merged

mason/forge:mason/9162-get-api-v1-repos-owner-name-commits-carr → main 2 commits fork

  • Merged. mason/9162-get-api-v1-repos-owner-name-commits-carr was merged into main as 18e3707.

Review

Sign in to review this change.
3 files changed +122 −2
src/contract/openapi/routes/getApiV1ReposOwnerNameCommits.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}/commits`, summary: "List commits newest first from `ref` (default: the default branch) as a bare array of { oid, parents, author, email, date, subject, body }; limit 1 to 100 (default 30); an unsafe ref is a 422 bad_ref; a private repository needs a token", auth: false, query: { ref: "string", limit: limitTo(100) } };
3 export const route: Route = { method: "get", path: `${REPO}/commits`, summary: "List commits newest first from `ref` (default: the default branch) as a bare array of { oid, parents, author, email, date, subject, body }; limit 1 to 100 (default 30); an unsafe ref is a 422 bad_ref; a private repository needs a token; the 200 carries a weak etag, cache-control private, no-cache and vary authorization, cookie, and a repeat sending If-None-Match gets 304 with no body until the ref moves", auth: false, alsoStatus: [304], query: { ref: "string", limit: limitTo(100) } };
src/http/api.ts +9 −1
@@ -1793,7 +1793,15 @@ export function apiRoutes(router: Router<Handler>): void {
17931793 const log = await timePhaseAsync(ctx.request, "git", () =>
17941794 commitLog(dirOf(ctx, repo), ref, { limit }),
17951795 );
1796 return timePhase(ctx.request, "render", () => json(log.ok ? log.value : []));
1796 // The tag is of the exact bytes sent plus the ref and limit asked, so a moved tip
1797 // misses, a push to another branch does not, and two queries never share a tag.
1798 return timePhase(ctx.request, "render", () => {
1799 const text = JSON.stringify(log.ok ? log.value : []);
1800 const etag = weakEtag(`${ref}\n${limit}\n${text}`);
1801 const headers = { etag, "cache-control": "private, no-cache", vary: "authorization, cookie" };
1802 if (holds(ctx.request, etag)) return new Response(null, { status: 304, headers });
1803 return new Response(text, { headers: { "content-type": "application/json", ...headers } });
1804 });
17971805 });
17981806
17991807 router.add("GET", "/api/v1/repos/:owner/:name/commits/:oid", async (ctx) => {
test/commits-etag.test.ts added +112
@@ -0,0 +1,112 @@
1 /** GET …/commits carries a weak ETag and answers a repeat with 304 until the ref it reads moves (#9162). */
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 type { Repo } from "../src/core/repos.ts";
9 import type { User } from "../src/core/users.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 alice: User;
20 let repo: Repo;
21 const cells = process.env["FORGE_CELLS"];
22 const URL_ = "/api/v1/repos/alice/r/commits";
23 const get = (path = URL_, headers: Record<string, string> = {}) =>
24 fetch(`${servers.url}${path}`, { headers });
25 const commit = async (message: string, branch?: string) => {
26 const done = await commitToBranch(app, alice, repo, {
27 message,
28 files: [{ path: `${message}.txt`, content: message }],
29 ...(branch ? { branch } : {}),
30 });
31 if (!done.ok) throw new Error(JSON.stringify(done.error));
32 };
33
34 beforeAll(async () => {
35 process.env["FORGE_CELLS"] = "off";
36 dataDir = await mkdtemp(join(tmpdir(), "forge-commits-etag-"));
37 app = createApp({ dataDir, port: 0 });
38 servers = await serve(app);
39 const made = await createUser(app.db, {
40 origin: "seed",
41 trusted: true,
42 username: "alice",
43 email: "[email protected]",
44 password: "a-good-passphrase",
45 adminIfFirst: true,
46 });
47 if (!made.ok) throw new Error(String(made.error));
48 alice = made.value;
49 const r = await createRepo(app, alice, { name: "r", visibility: "public" });
50 if (!r.ok) throw new Error(String(r.error));
51 repo = r.value;
52 await commit("one");
53 await commit("side-one", "side");
54 });
55
56 afterAll(async () => {
57 await servers.stop();
58 app.close();
59 if (cells === undefined) delete process.env["FORGE_CELLS"];
60 else process.env["FORGE_CELLS"] = cells;
61 await rm(dataDir, { recursive: true, force: true });
62 });
63
64 test("the 200 carries a weak tag, private no-cache and vary, and the body stays a bare array", async () => {
65 const res = await get();
66 expect(res.status).toBe(200);
67 expect(res.headers.get("etag")).toMatch(/^W\/"/);
68 expect(res.headers.get("cache-control")).toBe("private, no-cache");
69 expect(res.headers.get("vary")).toContain("authorization, cookie");
70 expect(Array.isArray(await res.json())).toBe(true);
71 });
72
73 test("a matching If-None-Match is a bodiless 304 with the same headers and the security floor", async () => {
74 const tag = (await get()).headers.get("etag")!;
75 const again = await get(URL_, { "if-none-match": tag });
76 expect(again.status).toBe(304);
77 expect(again.headers.get("etag")).toBe(tag);
78 expect(again.headers.get("cache-control")).toBe("private, no-cache");
79 expect(again.headers.get("vary")).toContain("authorization, cookie");
80 expect(again.headers.get("x-content-type-options")).toBe("nosniff");
81 expect(again.headers.get("content-security-policy")).toBeTruthy();
82 expect(await again.text()).toBe("");
83 });
84
85 test("a push to the ref read misses; a push to another branch does not", async () => {
86 const tag = (await get()).headers.get("etag")!;
87 await commit("side-two", "side");
88 expect((await get(URL_, { "if-none-match": tag })).status).toBe(304);
89 await commit("two");
90 const moved = await get(URL_, { "if-none-match": tag });
91 expect(moved.status).toBe(200);
92 expect(moved.headers.get("etag")).not.toBe(tag);
93 await moved.text();
94 });
95
96 test("a different ref or limit gets a different etag", async () => {
97 const base = (await get()).headers.get("etag");
98 const side = (await get(`${URL_}?ref=side`)).headers.get("etag");
99 const lim = (await get(`${URL_}?limit=1`)).headers.get("etag");
100 expect(new Set([base, side, lim]).size).toBe(3);
101 const stale = await get(`${URL_}?limit=1`, { "if-none-match": base! });
102 expect(stale.status).toBe(200);
103 });
104
105 test("bad_ref 422 and 404 carry no etag", async () => {
106 const bad = await get(`${URL_}?ref=${encodeURIComponent("--upload-pack=x")}`);
107 expect(bad.status).toBe(422);
108 expect(bad.headers.get("etag")).toBeNull();
109 const missing = await get("/api/v1/repos/alice/nope/commits");
110 expect(missing.status).toBe(404);
111 expect(missing.headers.get("etag")).toBeNull();
112 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs