diff --git a/CHANGELOG.md b/CHANGELOG.md index 139fc61..4999d9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,25 @@ Format loosely follows [Keep a Changelog](https://keepachangelog.com/). This run took the PoC from a rough, non-booting prototype to a running, test-covered Reddit clone. Highlights, newest first: +### The JSON API stops reading the whole table for a page +- **Listing endpoints fetched every matching row** and then ranked them in Lua, + even after the web listings moved to SQL. They now pass `sort` to + `Posts:get_listing` (so the database orders them) and size their fetch to the + request: the page plus one lookahead row when there is no cursor, opening to + `S.MAX_DEPTH` (1000) when there is, since `api_serialize.paginate` locates a + cursor by scanning the rows it was handed. +- `/api/subreddits` gained a `LIMIT` and a total order (`s.id DESC`) so its + window is stable between requests. +- **An unknown cursor now returns an empty page.** It used to silently restart at + the first page, which left a client paging in a loop with no way to notice it + had reached the end. +- Capping the depth is deliberate: for `hot`/`controversial`/`rising` the rank is + computed from live vote counts, so it moves between requests and a cursor into + a ranked listing is approximate however it is implemented. Recorded in + `docs/sqlite-features.md`. +- `utils/sort` is no longer used by the API — only by the specs that check the + SQL ordering agrees with it. + ### Referential actions, and one soft-delete convention - **Personal rows now cascade off `users`** (migration `[115]`): `subscriptions`, `saved_posts`, `hidden_posts`, `notifications`, diff --git a/app/spec/api_pagination_spec.lua b/app/spec/api_pagination_spec.lua new file mode 100644 index 0000000..66e2e38 --- /dev/null +++ b/app/spec/api_pagination_spec.lua @@ -0,0 +1,142 @@ +--- API listing pagination spec. +-- +-- The listing endpoints used to read every matching row and rank them in Lua. +-- They now order in SQL and fetch a bounded window: the page plus a lookahead +-- row when there is no cursor, opening to `S.MAX_DEPTH` when there is one, since +-- `S.paginate` locates a cursor by scanning the rows it was given. + +local use_test_env = require("lapis.spec").use_test_env +local simulate_request = require("lapis.spec.request").simulate_request +local db = require("lapis.db") + +describe("api listing pagination", function() + use_test_env() + + local Users = require("models.users") + local Forum = require("src.models.forum") + local Posts = require("src.models.posts") + local S = require("src.utils.api_serialize") + local cjson = require("cjson") + + local POSTS = 12 + + setup(function() + require("spec.schema_helper")() + local author = Users:create({ + user_name = "pager", + user_pass = "password", + user_email = "p@e.com", + }) + local sub = Forum:create({ name = "pagersub", creator_id = author.id }) + for i = 1, POSTS do + Posts:create({ + user_id = author.id, + sub_id = sub.id, + title = "post " .. i, + url = "https://example.com/" .. i, + }) + end + end) + + local app = require("app") + + local function get(path) + local status, body = simulate_request(app, path, { method = "GET" }) + return status, cjson.decode(body) + end + + -- Count queries for the duration of `fn`, to prove the fetch is bounded. + local function count_queries(fn) + local real = db.query + local n = 0 + db.query = function(...) -- luacheck: ignore 122 + n = n + 1 + return real(...) + end + local ok, err = pcall(fn) + db.query = real -- luacheck: ignore 122 + assert.is_true(ok, tostring(err)) + return n + end + + describe("S.window", function() + it("asks for just the page plus a lookahead row when there is no cursor", function() + assert.same(6, S.window({ limit = 5 })) + assert.same(26, S.window({})) + end) + + it("opens to MAX_DEPTH once a cursor is in play", function() + assert.same(S.MAX_DEPTH, S.window({ after = "t3_1" })) + assert.same(S.MAX_DEPTH, S.window({ before = "t3_1" })) + end) + + it("clamps a silly limit", function() + assert.same(2, S.window({ limit = 1 })) + assert.same(101, S.window({ limit = 9999 })) + assert.same(2, S.window({ limit = -3 })) + end) + end) + + describe("GET /api/listing", function() + it("returns a page and an after cursor", function() + local status, json = get("/api/listing?limit=5") + assert.same(200, status) + assert.same(5, #json.data.children) + assert.is_truthy(json.data.after) + end) + + it("walks the whole listing through its cursors, without repeats", function() + local seen, order, cursor, pages = {}, {}, nil, 0 + repeat + local path = "/api/listing?limit=5" .. (cursor and ("&after=" .. cursor) or "") + local _, json = get(path) + for _, child in ipairs(json.data.children) do + local id = child.data.id + assert.is_nil(seen[id], "id " .. tostring(id) .. " appeared twice") + seen[id] = true + order[#order + 1] = id + end + cursor = json.data.after + pages = pages + 1 + assert.is_true(pages < 10, "cursor walk did not terminate") + until not cursor + assert.same(POSTS, #order) + end) + + it("does not read the whole table for a page", function() + -- One listing query (plus whatever the request itself needs); the + -- point is that it does not scale with the row count. + local first = count_queries(function() + get("/api/listing?limit=1") + end) + local bigger = count_queries(function() + get("/api/listing?limit=10") + end) + assert.same(first, bigger) + end) + + it("answers an unknown cursor with an empty page, not the first page", function() + -- A cursor past the addressable window, or pointing at a row that has + -- gone. Restarting at the top would leave a client looping forever. + local _, json = get("/api/listing?limit=5&after=" .. S.fullname("link", 999999)) + assert.same(0, #json.data.children) + assert.is_nil(json.data.after) + end) + + it("orders by the requested sort", function() + for _, sort in ipairs({ "hot", "top", "new", "best", "controversial", "rising" }) do + local status, json = get("/api/listing/" .. sort .. "?limit=3") + assert.same(200, status, sort .. " should be a valid sort") + assert.same(3, #json.data.children) + end + end) + end) + + describe("GET /api/subreddits", function() + it("bounds its window and still paginates", function() + local status, json = get("/api/subreddits?limit=1") + assert.same(200, status) + assert.same(1, #json.data.children) + end) + end) +end) diff --git a/app/src/api.lua b/app/src/api.lua index 70d4c9b..8e5fa1b 100644 --- a/app/src/api.lua +++ b/app/src/api.lua @@ -19,7 +19,6 @@ local db = require("lapis.db") local S = require("src.utils.api_serialize") -local Sort = require("src.utils.sort") local timewindow = require("src.utils.timewindow") local Users = require("models.users") @@ -35,8 +34,8 @@ local Subscriptions = require("models.subscriptions") local POST_RATE, POST_WINDOW = 10, 600 local COMMENT_RATE, COMMENT_WINDOW = 30, 600 --- Sorts that map to a Sort comparator. "new" is intentionally absent: the --- listing query already returns rows newest-first, so we leave that order be. +-- Sorts the listing query knows how to order by (Posts.get_listing's ORDER_BY); +-- anything else falls back to "hot". local SORTS = { hot = true, top = true, @@ -60,17 +59,13 @@ end -- ---- listing assembly -------------------------------------------------------- --- Order a fresh listing by the requested sort (default "hot"); "new" keeps the --- query's created-desc order. -local function sorted_listing(rows, sort) - if sort == "new" then - return rows - end - return Sort:sort(rows, sort) -end - -- Build a paginated link Listing from get_listing `filters`, reading -- sort/t/limit/after/before from the request. +-- +-- Ordering happens in SQL (Posts.get_listing's ORDER_BY), and the fetch is +-- bounded: without a cursor only the page plus a lookahead row is read, and with +-- one the window opens to S.MAX_DEPTH so `paginate` can find the cursor row. +-- This used to read every matching row and sort them in Lua. local function link_listing(self, filters) local sort = self.params.sort if not SORTS[sort] then @@ -81,8 +76,10 @@ local function link_listing(self, filters) if user then filters.exclude_hidden_for = user.id end + filters.sort = sort + filters.limit = S.window(self.params) - local rows = sorted_listing(Posts:get_listing(filters), sort) + local rows = Posts:get_listing(filters) local page, after, before = S.paginate(rows, self.params, "link") local children = {} for _, p in ipairs(page) do @@ -278,11 +275,13 @@ local function api(app) app:get("/api/subreddits(/:where)", function(self) local order = self.params.where == "new" and "s.created_at DESC" or "subscribers DESC, s.name" + -- `s.id DESC` makes the order total so the window is stable between + -- requests; the LIMIT keeps the directory from reading every subreddit. local rows = db.select([[ - s.id, s.name, s.description, s.nsfw, s.created_at, + s.id, s.name, s.description, s.nsfw, s.created_at, s.public_id, (SELECT COUNT(*) FROM subscriptions x WHERE x.subreddit_id = s.id) AS subscribers FROM forum s WHERE s.deleted_at IS NULL - ORDER BY ]] .. order) + ORDER BY ]] .. order .. [[, s.id DESC LIMIT ?]], S.window(self.params)) local page, after, before = S.paginate(rows, self.params, "subreddit") local children = {} for _, f in ipairs(page) do diff --git a/app/src/utils/api_serialize.lua b/app/src/utils/api_serialize.lua index 9dba22d..4879e62 100644 --- a/app/src/utils/api_serialize.lua +++ b/app/src/utils/api_serialize.lua @@ -315,11 +315,41 @@ function M.listing(children, opts) } end --- Clamp a requested limit to [1, 100] (Reddit's ceiling), default 25. -local function clamp_limit(raw) +--- Clamp a requested limit to [1, 100] (Reddit's ceiling), default 25. +-- @tparam[opt] string|number raw the request's `limit` param +-- @treturn number +function M.clamp_limit(raw) local n = tonumber(raw) or 25 return math.max(1, math.min(100, math.floor(n))) end +local clamp_limit = M.clamp_limit + +--- How deep a cursor can address into a listing. +-- +-- `paginate` locates an `after`/`before` cursor by scanning the ordered rows, so +-- it can only reach a row the caller actually fetched. Callers therefore fetch a +-- bounded window rather than the whole table (see `M.window`), and this is that +-- bound: paging stops after 1000 ranked items, the way search engines and +-- Reddit itself cap deep pagination. +-- +-- Ranked listings could not offer exact deep cursors anyway -- `hot` and +-- `controversial` depend on vote counts, so a row's rank moves between requests +-- and a cursor into them is inherently approximate. +M.MAX_DEPTH = 1000 + +--- How many rows a listing endpoint should fetch for this request. +-- +-- Without a cursor -- the overwhelmingly common case, and every first page -- +-- only the page plus one lookahead row is needed. With a cursor, `paginate` has +-- to find that row among the ordered rows, so the window opens to `MAX_DEPTH`. +-- @tparam table params request params ({ after, before, limit }) +-- @treturn number rows to fetch +function M.window(params) + if params.after or params.before then + return M.MAX_DEPTH + end + return clamp_limit(params.limit) + 1 +end --- Cursor-paginate an array of listing rows (each with a numeric `.id`) by -- Reddit `after`/`before` fullnames and `limit`. Returns the page of rows plus @@ -347,15 +377,23 @@ function M.paginate(rows, params, kind) return nil end + -- A cursor that is not among these rows points past the addressable window + -- (or at a row that has since gone). Answer with an empty page rather than + -- silently restarting at the top, which would leave a client paging in a + -- loop without ever learning it had reached the end. local start = 1 - local after_idx = params.after and index_of(params.after) - if after_idx then - start = after_idx + 1 + if params.after then + local idx = index_of(params.after) + if not idx then + return {}, nil, nil + end + start = idx + 1 elseif params.before then - local before_idx = index_of(params.before) - if before_idx then - start = math.max(1, before_idx - limit) + local idx = index_of(params.before) + if not idx then + return {}, nil, nil end + start = math.max(1, idx - limit) end local page = {} diff --git a/docs/sqlite-features.md b/docs/sqlite-features.md index d3f12f3..8343a9b 100644 --- a/docs/sqlite-features.md +++ b/docs/sqlite-features.md @@ -143,10 +143,31 @@ counter-triggers above), which is a different trade. `utils/paginate_db` asks for `per_page + 1` rows and treats the extra one as "there is a next page", so a listing stays a single query with no companion -`COUNT(*)`. `utils/paginate` (array slicing) remains for callers that already -hold a full list — the profile's comment list, and the JSON API, whose -`after`/`before` cursors are resolved by scanning the ordered rows and so still -need them all. +`COUNT(*)`. `utils/paginate` (array slicing) remains for the profile's comment +list, which legitimately holds a full array. + +### The JSON API's cursors + +`/api` speaks Reddit's `after`/`before` **fullnames**, and `api_serialize.paginate` +finds the cursor row by scanning the ordered rows it was handed — so it can only +reach a row the caller actually fetched. Rather than fetch everything, the +endpoints size their window to the request (`S.window`): + +- **no cursor** (every first page, and the common case): the page plus one + lookahead row. +- **with a cursor**: `S.MAX_DEPTH` (1000) rows, which caps how deep a cursor can + address. + +Capping is honest for this data rather than a shortcut. True keyset pagination +would need `WHERE (rank, id) < (cursor_rank, cursor_id)`, and for `hot` / +`controversial` / `rising` the rank is computed from live vote counts — it moves +between requests, so a cursor into a ranked listing is inherently approximate no +matter how it is implemented. Search engines and Reddit itself cap deep paging +for the same reason. + +A cursor that is not in the window (past the cap, or a row since deleted) now +returns an **empty page**. It used to silently restart at the top, which left a +client paging in a loop without ever learning it had reached the end. ## Partial indexes — adopted, including for uniqueness