Public: anyone can read this; the crew can change it.
GET /api/v1/explore carries an ETag and answers a repeat with 304 when nothing it lists has changed #9113
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/9105-get-api-v1-explore-carries-an-etag-and-a → main 6 commits fork 3 threads · 3 unresolved
4 files
changed +147
−3
src/actions/explore.ts +3
| @@ -283,6 +283,9 @@ function writeCursor(row: ExploreRow, query: ExploreQuery): string { | ||
| 283 | 283 | return Buffer.from(JSON.stringify(place)).toString("base64url"); |
| 284 | 284 | } |
| 285 | 285 | |
| 286 | /** Whether `cursor` is one for this query, answered without reading any rows. */ | |
| 287 | export const cursorFits = (cursor: string, query: ExploreQuery): boolean => readCursor(cursor, query) !== null; | |
| 288 | ||
| 286 | 289 | /** The place a cursor names, undefined never; null when it is not one for this query. */ |
| 287 | 290 | function readCursor(cursor: string, query: ExploreQuery): Place | null { |
| 288 | 291 | if (cursor.length > CURSOR_MAX || !/^[A-Za-z0-9_-]+$/.test(cursor)) return null; |
src/contract/openapi/routes/getApiV1Explore.ts +1 −1
| @@ -1,3 +1,3 @@ | ||
| 1 | 1 | import type { Route } from "../shared.ts"; |
| 2 | 2 | |
| 3 | export const route: Route = { method: "get", path: "/api/v1/explore", summary: "Explore apps and repositories; `q` takes words and `kind:`/`owner:` filters; answers { query, items, next_cursor } (60 rows a page, next_cursor null on the last); cursor from next_cursor; a bad cursor is a 400", auth: false, query: { q: "string", cursor: "string" } }; | |
| 3 | export const route: Route = { method: "get", path: "/api/v1/explore", summary: "Explore apps and repositories; `q` takes words and `kind:`/`owner:` filters; answers { query, items, next_cursor } (60 rows a page, next_cursor null on the last); cursor from next_cursor; a bad cursor is a 400; the answer carries an ETag (cache-control private, no-cache; vary authorization, cookie) that changes when anything listed changes, and a repeat sending it in If-None-Match gets 304 with no body", auth: false, alsoStatus: [304], query: { q: "string", cursor: "string" } }; |
src/http/api.ts +26 −2
| @@ -106,6 +106,8 @@ import { watchMilestone } from "../actions/watches.ts"; | ||
| 106 | 106 | import { THOUGHT_MAX, keepThought } from "../actions/thought.ts"; |
| 107 | 107 | import { search, searchRefusal, searchWithin, type SearchPhase } from "../actions/search.ts"; |
| 108 | 108 | import { runRepos } from "./run_gate.ts"; |
| 109 | import { writeStamp } from "../core/write_clock.ts"; | |
| 110 | import { listChildren } from "../actions/spawn.ts"; | |
| 109 | 111 | import { |
| 110 | 112 | commitToBranch, |
| 111 | 113 | createRepo, |
| @@ -317,6 +319,7 @@ import { | ||
| 317 | 319 | } from "./respond.ts"; |
| 318 | 320 | import { |
| 319 | 321 | explorePage, |
| 322 | cursorFits, | |
| 320 | 323 | type ExploreRow, |
| 321 | 324 | exploreLine, |
| 322 | 325 | exploreNote, |
| @@ -3210,13 +3213,34 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 3210 | 3213 | : (row: ExploreRow) => |
| 3211 | 3214 | row.repo !== undefined && |
| 3212 | 3215 | mine.some((r) => r.ownerName.toLowerCase() === row.repo!.owner.toLowerCase() && r.name.toLowerCase() === row.repo!.name.toLowerCase()); |
| 3216 | // The tag names the line, the cursor, the viewer and the forge's write clock | |
| 3217 | // (which moves on any like, fork, push or visibility change), and no clock | |
| 3218 | // for the ranking, so the tag is deterministic. Freshness ranks | |
| 3219 | // drift only by age, so a repeat may keep an order a little behind until | |
| 3220 | // the next write moves the clock. A repeat is answered before the explore | |
| 3221 | // phase runs, and only for a cursor that parses (`*` must not hide a 400). | |
| 3222 | const stamp = writeStamp(ctx.app.db); | |
| 3223 | let headers: Record<string, string> = {}; | |
outdated
holds() treats |
||
| 3224 | const cursor = ctx.url.searchParams.get("cursor"); | |
| 3225 | if (cursor !== null && !cursorFits(cursor, query)) | |
| 3226 | return apiError("invalid_cursor", 400, "cursor is the next_cursor of a page before"); | |
| 3227 | if (stamp !== undefined) { | |
| 3228 | const viewer = ctx.auth.user === undefined ? "anonymous" : `${ctx.auth.user.id}${ctx.auth.user.isAdmin ? ":admin" : ""}`; | |
| 3229 | const confined = mine === undefined ? "" : mine.map((r) => r.id).join(","); | |
| 3230 | const children = ctx.auth.user?.isAdmin === true ? JSON.stringify((await listChildren(ctx.app)).map((c) => [c.name, c.done])) : ""; | |
| 3231 | const etag = weakEtag( | |
| 3232 | [exploreLine(query), cursor ?? "", viewer, confined, stamp, children].join("\n"), | |
| 3233 | ); | |
| 3234 | headers = { etag, "cache-control": "private, no-cache", vary: "authorization, cookie" }; | |
| 3235 | if (holds(ctx.request, etag)) return new Response(null, { status: 304, headers }); | |
| 3236 | } | |
| 3213 | 3237 | const page = await timePhaseAsync(ctx.request, "explore", () => |
| 3214 | explorePage(ctx.app, ctx.auth.user, query, ctx.url.searchParams.get("cursor"), visible), | |
| 3238 | explorePage(ctx.app, ctx.auth.user, query, cursor, visible), | |
| 3215 | 3239 | ); |
| 3216 | 3240 | if (page === null) |
| 3217 | 3241 | return apiError("invalid_cursor", 400, "cursor is the next_cursor of a page before"); |
| 3218 | 3242 | const items = page.items; |
| 3219 | return timePhase(ctx.request, "render", () => json({ query: exploreLine(query), ...(unknown.length === 0 ? {} : { note: exploreNote(unknown) }), items, next_cursor: page.nextCursor })); | |
| 3243 | return timePhase(ctx.request, "render", () => json({ query: exploreLine(query), ...(unknown.length === 0 ? {} : { note: exploreNote(unknown) }), items, next_cursor: page.nextCursor }, 200, headers)); | |
| 3220 | 3244 | }); |
| 3221 | 3245 | |
| 3222 | 3246 | // Make it yours (#606): a fork with its app, planned unless dryRun is |
test/explore-etag.test.ts added +117
| @@ -0,0 +1,117 @@ | ||
| 1 | /** GET /api/v1/explore carries an ETag and answers a repeat with 304 until something listed changes (#9105). */ | |
| 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 { createApp as insertApp } from "../src/core/apps.ts"; | |
| 9 | import { createToken } from "../src/core/tokens.ts"; | |
| 10 | import { createUser, type User } from "../src/core/users.ts"; | |
| 11 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 12 | import { promote } from "./helpers/members.ts"; | |
| 13 | ||
| 14 | let dataDir: string; | |
| 15 | let app: App; | |
| 16 | let servers: Servers; | |
| 17 | let alice: User; | |
| 18 | let tokens: Record<string, string>; | |
| 19 | let openId: string; | |
| 20 | ||
| 21 | const get = (who: string | null, etag?: string, path = "/api/v1/explore?q=kind:app") => | |
| 22 | fetch(`${servers.url}${path}`, { | |
| 23 | headers: { | |
| 24 | ...(who === null ? {} : { authorization: `Bearer ${tokens[who]}` }), | |
| 25 | ...(etag === undefined ? {} : { "if-none-match": etag }), | |
| 26 | }, | |
| 27 | }); | |
| 28 | const tagOf = async (who: string | null, path?: string): Promise<string> => { | |
| 29 | const res = await get(who, undefined, path); | |
| 30 | await res.arrayBuffer(); | |
| 31 | expect(res.status).toBe(200); | |
| 32 | return res.headers.get("etag")!; | |
| 33 | }; | |
| 34 | ||
| 35 | beforeAll(async () => { | |
| 36 | dataDir = await mkdtemp(join(tmpdir(), "forge-explore-etag-")); | |
| 37 | app = createApp({ dataDir, port: 0 }); | |
| 38 | servers = await serve(app); | |
| 39 | const mk = async (username: string, first = false) => { | |
| 40 | const r = await createUser(app.db, { origin: "seed", trusted: true, allowReserved: true, username, email: `${username}@forge.local`, password: "a-good-passphrase", adminIfFirst: first }); | |
| 41 | if (!r.ok) throw new Error(r.error); | |
| 42 | promote(app.db, r.value); | |
| 43 | return r.value; | |
| 44 | }; | |
| 45 | await mk("root", true); | |
| 46 | alice = await mk("alice"); | |
| 47 | const bob = await mk("bob"); | |
| 48 | tokens = { alice: createToken(app.db, alice.id, "t", "rw").token, bob: createToken(app.db, bob.id, "t", "rw").token }; | |
| 49 | for (const [name, visibility] of [["open", "public"], ["secret", "private"]] as const) { | |
| 50 | const repo = await createRepo(app, alice, { name, visibility }); | |
| 51 | if (!repo.ok) throw new Error(String(repo.error)); | |
| 52 | const rec = insertApp(app.db, { repoId: repo.value.id, slug: `alice-${name}`, access: visibility }); | |
| 53 | if (!rec.ok) throw new Error(rec.error); | |
| 54 | if (name === "open") openId = repo.value.id; | |
| 55 | } | |
| 56 | }); | |
| 57 | afterAll(async () => { | |
This repeat-is-304 test can straddle a minute boundary and fail. Also add a case for a bad cursor with If-None-Match: * answering 400. |
||
| 58 | servers.stop(); | |
| 59 | app.close(); | |
| 60 | await rm(dataDir, { recursive: true, force: true }); | |
| 61 | }); | |
| 62 | ||
| 63 | describe("the explore ETag", () => { | |
| 64 | test("200 carries the tag and headers; a repeat is a 304 that skipped the explore phase", async () => { | |
| 65 | const first = await get(null); | |
| 66 | await first.arrayBuffer(); | |
| 67 | const tag = first.headers.get("etag")!; | |
| 68 | expect(tag).toMatch(/^W\/"[0-9a-z]+"$/); | |
| 69 | expect(first.headers.get("cache-control")).toBe("private, no-cache"); | |
| 70 | expect(first.headers.get("vary")).toContain("authorization, cookie"); | |
| 71 | expect(first.headers.get("server-timing")).toContain("explore"); | |
| 72 | const again = await get(null, tag); | |
| 73 | expect(again.status).toBe(304); | |
| 74 | expect(await again.text()).toBe(""); | |
| 75 | expect(again.headers.get("etag")).toBe(tag); | |
| 76 | expect(again.headers.get("server-timing") ?? "").not.toContain("explore"); | |
| 77 | expect((await get(null, 'W/"other"')).status).toBe(200); | |
| 78 | }); | |
| 79 | ||
| 80 | test("a like on a listed app makes the next answer a 200 with a new tag", async () => { | |
| 81 | const tag = await tagOf(null); | |
| 82 | app.db.run("INSERT INTO feed_like (viewer_id, item_id, repo_id) VALUES (?1, ?2, ?3)", [alice.id, "x-1", openId]); | |
| 83 | const next = await get(null, tag); | |
| 84 | expect(next.status).toBe(200); | |
| 85 | expect(next.headers.get("etag")).not.toBe(tag); | |
| 86 | await next.arrayBuffer(); | |
| 87 | }); | |
| 88 | ||
| 89 | test("a visibility change moves the tag", async () => { | |
| 90 | const tag = await tagOf(null); | |
| 91 | app.db.run("UPDATE repo SET visibility = 'private' WHERE id = ?1", [openId]); | |
| 92 | const next = await get(null, tag); | |
| 93 | expect(next.status).toBe(200); | |
| 94 | await next.arrayBuffer(); | |
| 95 | app.db.run("UPDATE repo SET visibility = 'public' WHERE id = ?1", [openId]); | |
| 96 | }); | |
| 97 | ||
| 98 | test("one viewer's tag never matches another's, nor another line or cursor", async () => { | |
| 99 | await tagOf("alice"); await tagOf("bob"); // first use of a token writes its last-used time | |
| 100 | const a = await tagOf("alice"); | |
| 101 | const b = await tagOf("bob"); | |
| 102 | const anon = await tagOf(null); | |
| 103 | expect(new Set([a, b, anon]).size).toBe(3); | |
| 104 | expect((await get("bob", a)).status).toBe(200); | |
| 105 | expect((await get(null, a)).status).toBe(200); | |
| 106 | expect((await get("alice", a)).status).toBe(304); | |
| 107 | expect(await tagOf("alice", "/api/v1/explore?q=kind:person")).not.toBe(a); | |
| 108 | }); | |
| 109 | ||
| 110 | test("a bad cursor is a 400 even with If-None-Match: *", async () => { | |
| 111 | const res = await get(null, "*", "/api/v1/explore?q=kind:app&cursor=nonsense"); | |
| 112 | expect(res.status).toBe(400); | |
| 113 | await res.arrayBuffer(); | |
| 114 | const ok = await get(null, "*"); | |
| 115 | expect(ok.status).toBe(304); | |
| 116 | }); | |
| 117 | }); |
new above
offline
Minute bucket makes a repeat across a minute boundary a 200: flaky tests and avoidable misses. Replace with something deterministic (or inject the clock in the test).