perf(api): bound the listing fetch and order in SQL

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`,

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)

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

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 = {}

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