diff --git a/CHANGELOG.md b/CHANGELOG.md index 4999d9d..68c6fb8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,20 @@ 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: +### Exact cursors for the `new` sort +- **`new` no longer pages by window.** Its key, `(created_at, id)`, never moves + once a post is written, so the database seeks straight to the cursor row with a + row-value comparison and returns only the page — no depth cap, nothing scanned. + Walking backwards runs the comparison the other way and flips the rows, so + callers always see one order. +- A cursor id that no longer exists makes the subquery NULL, so the comparison is + NULL and no rows return — a stale cursor reads as "nothing after this". +- The **ranked** sorts keep the windowed cap, which is a property of the data: + `hot`/`controversial`/`rising` rank on live vote counts, so a cursor into them + is approximate however it is implemented. `Posts.KEYSET_SORTS` lists what + qualifies, and `get_listing` asserts if a cursor is passed with a sort that + does not — that failure would be silent and subtly wrong otherwise. + ### 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 diff --git a/app/spec/api_pagination_spec.lua b/app/spec/api_pagination_spec.lua index 66e2e38..9f265d8 100644 --- a/app/spec/api_pagination_spec.lua +++ b/app/spec/api_pagination_spec.lua @@ -132,6 +132,91 @@ describe("api listing pagination", function() end) end) + describe("keyset cursors on the `new` sort", function() + -- `new` orders by (created_at, id), which never moves once a post is + -- written, so the database can seek straight to the cursor row instead of + -- scanning a windowed fetch. The ranked sorts cannot -- their key is live + -- vote counts. + local function ids_of(json) + local out = {} + for _, child in ipairs(json.data.children) do + out[#out + 1] = child.data.id + end + return out + end + + it("walks forward exactly, with no repeats or gaps", function() + local _, all = get("/api/listing/new?limit=" .. POSTS) + local expected = ids_of(all) + assert.same(POSTS, #expected) + + local walked, cursor = {}, nil + repeat + local path = "/api/listing/new?limit=4" .. (cursor and ("&after=" .. cursor) or "") + local _, json = get(path) + for _, id in ipairs(ids_of(json)) do + walked[#walked + 1] = id + end + cursor = json.data.after + until not cursor + assert.same(expected, walked) + end) + + it("walks backward to the page before a cursor", function() + local _, first = get("/api/listing/new?limit=4") + local _, second = get("/api/listing/new?limit=4&after=" .. first.data.after) + assert.is_truthy(second.data.before) + + local _, back = get("/api/listing/new?limit=4&before=" .. second.data.before) + assert.same(ids_of(first), ids_of(back)) + end) + + -- Capture the filters the endpoint hands the model, which is the only way + -- to tell the keyset path from the windowed one -- both are a single + -- query, so a query count cannot distinguish them. + local function filters_for(path) + local captured + local real = Posts.get_listing + Posts.get_listing = function(self, filters) -- luacheck: ignore 122 + captured = filters + return real(self, filters) + end + local ok, err = pcall(get, path) + Posts.get_listing = real -- luacheck: ignore 122 + assert.is_true(ok, tostring(err)) + return captured + end + + it("asks the database to seek, rather than fetching a window", function() + local _, first = get("/api/listing/new?limit=2") + + local cursored = filters_for("/api/listing/new?limit=2&after=" .. first.data.after) + assert.is_truthy(cursored.after_id, "should have passed a keyset cursor") + -- Just the page plus one lookahead row, not S.MAX_DEPTH. + assert.same(3, cursored.limit) + + -- A ranked sort has no stable key, so it still pages by window. + local ranked = filters_for("/api/listing/hot?limit=2&after=" .. first.data.after) + assert.is_nil(ranked.after_id) + assert.same(S.MAX_DEPTH, ranked.limit) + end) + + it("treats a vanished cursor row as the end of the listing", function() + local _, json = get("/api/listing/new?limit=4&after=" .. S.fullname("link", 999999)) + assert.same(0, #json.data.children) + assert.is_nil(json.data.after) + end) + + it("refuses a keyset cursor on a sort with no stable key", function() + -- Guard against a caller wiring after_id into a ranked sort, where the + -- key moves between requests and the result would silently be wrong. + local ok = pcall(function() + return Posts:get_listing({ sort = "hot", after_id = 1 }) + end) + assert.is_false(ok) + end) + end) + describe("GET /api/subreddits", function() it("bounds its window and still paginates", function() local status, json = get("/api/subreddits?limit=1") diff --git a/app/src/api.lua b/app/src/api.lua index 8e5fa1b..0e026c3 100644 --- a/app/src/api.lua +++ b/app/src/api.lua @@ -77,8 +77,36 @@ local function link_listing(self, filters) filters.exclude_hidden_for = user.id end filters.sort = sort - filters.limit = S.window(self.params) + -- `new` has a stable ordering key, so the database can seek straight to the + -- cursor row and return only the page -- no depth cap, nothing scanned. The + -- ranked sorts cannot: their key is live vote counts, so they page by + -- window (S.window) and `paginate` finds the cursor among those rows. + local cursor = self.params.after or self.params.before + local cursor_id + if cursor then + -- `x and f()` would truncate to one value, dropping the id. + local _ + _, cursor_id = S.parse_fullname(cursor) + end + if Posts.KEYSET_SORTS[sort] and cursor_id then + local limit = S.clamp_limit(self.params.limit) + filters.limit = limit + 1 + if self.params.after then + filters.after_id = cursor_id + else + filters.before_id = cursor_id + end + local rows = Posts:get_listing(filters) + local page, after, before = S.paginate_keyset(rows, limit, "link", true) + local children = {} + for _, p in ipairs(page) do + children[#children + 1] = S.link(p) + end + return { json = S.listing(children, { after = after, before = before }) } + end + + filters.limit = S.window(self.params) local rows = Posts:get_listing(filters) local page, after, before = S.paginate(rows, self.params, "link") local children = {} diff --git a/app/src/models/posts.lua b/app/src/models/posts.lua index 4c5a609..055f173 100644 --- a/app/src/models/posts.lua +++ b/app/src/models/posts.lua @@ -124,6 +124,15 @@ local ORDER_BY = { ELSE 0 END) DESC]], } +--- Sorts whose ordering key is stable enough to address with a cursor. +-- +-- Only `new`: its key is `(created_at, id)`, which never changes once a post is +-- written. Every other sort ranks on live vote counts, so a row's position moves +-- between requests and a cursor into it could not be exact however it was +-- implemented -- those page by window instead (see utils/api_serialize). +Posts.KEYSET_SORTS = { new = true } +local KEYSET_SORTS = Posts.KEYSET_SORTS + --- Listing rows for a frontpage / subreddit, with the vote and comment -- aggregates the templates expect. -- @@ -199,14 +208,45 @@ function Posts:get_listing(filters) ) end + -- Keyset cursors, for the sorts whose key is stable (see KEYSET_SORTS). + -- Comparing the whole key as a row value -- `(created_at, id) < (...)` -- + -- expresses "strictly past that row in this order" in one shot, including + -- the tiebreaker. A cursor id that no longer exists makes the subquery NULL, + -- so the comparison is NULL, so no rows come back: a stale cursor reads as + -- "nothing after this", which is what the API wants. + local reversed = false + if filters.after_id or filters.before_id then + assert( + KEYSET_SORTS[filters.sort or "new"], + "keyset cursors need a stable sort key; '" .. tostring(filters.sort) .. "' has none" + ) + if filters.after_id then + restrict( + "(a.created_at, a.id) < (SELECT created_at, id FROM posts WHERE id = ?)", + tonumber(filters.after_id) + ) + else + -- Walking backwards: take the rows *above* the cursor in ascending + -- order (so LIMIT keeps the ones nearest it) and flip them back. + restrict( + "(a.created_at, a.id) > (SELECT created_at, id FROM posts WHERE id = ?)", + tonumber(filters.before_id) + ) + reversed = true + end + end + -- `a.id DESC` breaks ties deterministically. Without it, equal-ranked rows -- could swap between requests and LIMIT/OFFSET paging would repeat or skip -- them -- the in-memory `table.sort` this replaces was likewise unstable. local order = ORDER_BY[filters.sort] or ORDER_BY.new + if reversed then + order = "a.created_at ASC" + end if filters.sticky_first then order = "a.stickied DESC, " .. order end - query = query .. " ORDER BY " .. order .. ", a.id DESC" + query = query .. " ORDER BY " .. order .. (reversed and ", a.id ASC" or ", a.id DESC") if filters.limit then query = query .. " LIMIT ? OFFSET ?" @@ -216,6 +256,13 @@ function Posts:get_listing(filters) local rows = db.select(query, unpack(params)) + if reversed then + -- Hand the caller the same descending order every other path returns. + for i = 1, math.floor(#rows / 2) do + rows[i], rows[#rows - i + 1] = rows[#rows - i + 1], rows[i] + end + end + for _, post in ipairs(rows) do post.permalink = "/r/" .. post.subreddit .. "/comments/" .. post.id -- Prefer the stored host; fall back to parsing for any pre-backfill row. diff --git a/app/src/utils/api_serialize.lua b/app/src/utils/api_serialize.lua index 4879e62..9d9cf2e 100644 --- a/app/src/utils/api_serialize.lua +++ b/app/src/utils/api_serialize.lua @@ -337,6 +337,29 @@ local clamp_limit = M.clamp_limit -- and a cursor into them is inherently approximate. M.MAX_DEPTH = 1000 +--- Build a page and its cursors from an already-windowed keyset result. +-- +-- The counterpart to `paginate` for callers that let the database do the +-- seeking: the rows are already exactly the page (plus one lookahead row), so +-- nothing is scanned to find a cursor. +-- @tparam table rows up to `limit + 1` rows, in display order +-- @tparam number limit the page size +-- @tparam string kind the rows' kind, for fullnames +-- @tparam boolean has_prev whether a cursor got us here (so a `before` exists) +-- @treturn table page rows +-- @treturn string|nil after fullname +-- @treturn string|nil before fullname +function M.paginate_keyset(rows, limit, kind, has_prev) + local more = #rows > limit + local page = {} + for i = 1, math.min(#rows, limit) do + page[i] = rows[i] + end + local after = (more and page[#page]) and M.fullname(kind, page[#page].id) or nil + local before = (has_prev and page[1]) and M.fullname(kind, page[1].id) or nil + return page, after, before +end + --- How many rows a listing endpoint should fetch for this request. -- -- Without a cursor -- the overwhelmingly common case, and every first page -- diff --git a/docs/sqlite-features.md b/docs/sqlite-features.md index 8343a9b..5b0dd45 100644 --- a/docs/sqlite-features.md +++ b/docs/sqlite-features.md @@ -158,12 +158,31 @@ endpoints size their window to the request (`S.window`): - **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. +**`new` skips the cap entirely.** Its key, `(created_at, id)`, never moves once a +post is written, so the database can seek straight to the cursor row with a +row-value comparison and return only the page: + +```sql +WHERE (a.created_at, a.id) < (SELECT created_at, id FROM posts WHERE id = ?) +``` + +Comparing the whole key as a row value expresses "strictly past that row in this +order" in one shot, tiebreaker included. A cursor id that no longer exists makes +the subquery NULL, so the comparison is NULL and no rows come back — a stale +cursor reads as "nothing after this", which is exactly the wanted answer. +Walking *backwards* runs the comparison the other way in ascending order and +flips the rows, so the caller always sees one order. + +The **ranked** sorts keep the window-and-cap treatment, and that is a property +of the data rather than a shortcut: `hot`, `controversial` and `rising` compute +rank from live vote counts, so a row's position moves between requests and a +cursor into one is approximate however it is implemented. Keyset would relocate +the inaccuracy, not remove it. Search engines and Reddit cap deep paging for the +same reason. + +`Posts.KEYSET_SORTS` is the list, and `get_listing` **asserts** when a cursor is +passed with a sort that is not on it — an unstable-key cursor would fail +silently and subtly otherwise. 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