From bdecd5917fa4ff25625ad64d052f26dab75ba029 Mon Sep 17 00:00:00 2001 From: Bryan Haberberger Date: Tue, 8 Sep 2026 13:49:47 -0500 Subject: [PATCH 1/9] Some bug fixing for limit and skip parameters --- __tests__/utils.test.js | 130 +++++++++++++++++++++++++++++--- controllers/crud.js | 2 +- controllers/gog.js | 10 ++- controllers/history.js | 2 +- controllers/search.js | 10 +-- controllers/utils.js | 109 +++++++++++++++++++++++--- public/API.html | 83 +++++++++++++++----- routes/__tests__/query.test.js | 90 ++++++++++++++++++++++ routes/__tests__/search.test.js | 33 ++++++++ 9 files changed, 418 insertions(+), 51 deletions(-) diff --git a/__tests__/utils.test.js b/__tests__/utils.test.js index 8ec29a8f..df63ca27 100644 --- a/__tests__/utils.test.js +++ b/__tests__/utils.test.js @@ -327,30 +327,138 @@ describe('controllers/utils.js idNegotiation edge cases', () => { }) describe('controllers/utils.js getPagination', () => { + /** Assert that a query is rejected as a 400 rather than guessed at. */ + const assertRejects = (query, matcher) => { + assert.throws( + () => getPagination(query), + (err) => { + assert.strictEqual(err.statusCode, 400) + assert.match(err.statusMessage, matcher) + return true + }, + `${JSON.stringify(query)} should be rejected` + ) + } + it('returns the default limit and skip 0 for an empty query', () => { - const result = getPagination({}, 100) + const result = getPagination({}, null, 100) assert.strictEqual(result.limit, 100) assert.strictEqual(result.skip, 0) }) - it('parses numeric string values from the query', () => { + it('parses decimal integer string values from the query', () => { const result = getPagination({ limit: '50', skip: '10' }) assert.strictEqual(result.limit, 50) assert.strictEqual(result.skip, 10) }) - it('falls back to defaults on non-numeric or non-positive input', () => { - const result = getPagination({ limit: 'bogus', skip: 'nope' }, 100) - assert.strictEqual(result.limit, 100) - assert.strictEqual(result.skip, 0) + it('rejects a limit that is not a whole number greater than 0', () => { + for (const limit of ['abc', '10abc', '1e3', '0x10', '250.7', '-5', '', '0']) { + assertRejects({ limit }, /'limit' URL parameter must be a whole number greater than 0/) + } + }) + + it('rejects a skip that is not a whole number of 0 or greater', () => { + for (const skip of ['abc', '10abc', '1e3', '0x10', '2.9', '-5', '']) { + assertRejects({ skip }, /'skip' URL parameter must be a whole number 0 or greater/) + } + }) + + it('accepts skip 0, which limit does not', () => { + assert.strictEqual(getPagination({ skip: '0' }).skip, 0) + assertRejects({ limit: '0' }, /whole number greater than 0/) + }) + + it('rejects a repeated parameter rather than taking a guess at which one was meant', () => { + // Express hands a repeated URL parameter over as an Array. '?limit=100&limit=200' has no + // single correct reading. + assertRejects({ limit: ['100', '200'] }, /'limit' URL parameter was provided more than once/) + assertRejects({ skip: ['1', '2'] }, /'skip' URL parameter was provided more than once/) + }) + + it('truncates a long rejected value rather than reflecting all of it back', () => { + assert.throws( + () => getPagination({ limit: 'x'.repeat(500) }), + (err) => err.statusMessage.length < 200 + ) + }) + + it('clamps a limit above the maximum instead of rejecting it', () => { + const { limit } = getPagination({ limit: String(Number.MAX_SAFE_INTEGER) }) + const { 'Pagination-Limit-Max': max } = capturedHeadersFor({}) + assert.strictEqual(limit, Number(max)) }) - it('clamps an unreasonably large limit below the max', () => { - const huge = Number.MAX_SAFE_INTEGER - const result = getPagination({ limit: String(huge) }) - assert.ok(result.limit > 0) - assert.ok(result.limit < huge, `limit should be clamped below ${huge}`) + it('rejects a skip above the maximum rather than serving the same page forever', () => { + // Clamping it would hand back the page at the maximum on every request past it. A client + // advancing skip and stopping on an empty page would never terminate. + const max = Number(capturedHeadersFor({})['Pagination-Skip-Max']) + assertRejects({ skip: String(max + 1) }, /beyond the maximum/) + assertRejects({ skip: String(max + 50000) }, /beyond the maximum/) }) + + it('names the configured maximum in the rejection, so a client can act on it', () => { + const original = process.env.MAX_QUERY_SKIP + try { + process.env.MAX_QUERY_SKIP = '2500' + assertRejects({ skip: '2501' }, /2501 is beyond the maximum of 2500/) + assert.strictEqual(getPagination({ skip: '2500' }).skip, 2500) + } finally { + if (original === undefined) delete process.env.MAX_QUERY_SKIP + else process.env.MAX_QUERY_SKIP = original + } + }) + + it('accepts a skip exactly at the maximum', () => { + const max = Number(capturedHeadersFor({})['Pagination-Skip-Max']) + assert.strictEqual(getPagination({ skip: String(max) }).skip, max) + }) + + it('reports the applied values and the maximums on every paged response', () => { + const headers = capturedHeadersFor({ limit: '25', skip: '10' }) + assert.strictEqual(headers['Pagination-Limit'], '25') + assert.strictEqual(headers['Pagination-Skip'], '10') + assert.ok(Number(headers['Pagination-Limit-Max']) > 0) + assert.ok(Number(headers['Pagination-Skip-Max']) > 0) + }) + + it('reports the clamped limit, not the one that was asked for', () => { + const headers = capturedHeadersFor({ limit: '999999' }) + assert.strictEqual(headers['Pagination-Limit'], headers['Pagination-Limit-Max']) + }) + + it('reads the caps from the environment at call time', () => { + // Captured at module load these would be unreadable, which is how the RERUM_MAX_QUERY_* / + // MAX_QUERY_* name mismatch went unnoticed. + const original = process.env.MAX_QUERY_LIMIT + try { + process.env.MAX_QUERY_LIMIT = '25' + const { limit } = getPagination({ limit: '100' }) + assert.strictEqual(limit, 25) + assert.strictEqual(capturedHeadersFor({})['Pagination-Limit-Max'], '25') + } finally { + if (original === undefined) delete process.env.MAX_QUERY_LIMIT + else process.env.MAX_QUERY_LIMIT = original + } + }) + + it('falls back to the code default when a configured cap is unusable', () => { + const original = process.env.MAX_QUERY_LIMIT + try { + process.env.MAX_QUERY_LIMIT = 'not-a-number' + assert.strictEqual(capturedHeadersFor({})['Pagination-Limit-Max'], '500') + } finally { + if (original === undefined) delete process.env.MAX_QUERY_LIMIT + else process.env.MAX_QUERY_LIMIT = original + } + }) + + /** Run getPagination against a minimal response double and hand back the headers it set. */ + function capturedHeadersFor(query) { + let captured + getPagination(query, { set: (headers) => { captured = headers } }) + return captured + } }) describe('controllers/utils.js findLeafAnnotationsFor', () => { diff --git a/controllers/crud.js b/controllers/crud.js index 7d1f90dc..4f9f9991 100644 --- a/controllers/crud.js +++ b/controllers/crud.js @@ -74,7 +74,7 @@ const create = async function (req, res, next) { const query = async function (req, res, next) { res.set("Content-Type", "application/json; charset=utf-8") let props = req.body - const { limit, skip } = getPagination(req.query, 100) + const { limit, skip } = getPagination(req.query, res, 100) if (!props || Object.keys(props).length === 0) { //Hey now, don't ask for everything...this can happen by accident. Don't allow it. let err = { diff --git a/controllers/gog.js b/controllers/gog.js index 778658a7..c9cbef79 100644 --- a/controllers/gog.js +++ b/controllers/gog.js @@ -23,7 +23,8 @@ const GOG_AGENTS = [GOG_PROD_AGENT, GOG_DEV_AGENT] * The Bearer Token in the header must be from TinyMatt. * The body must be formatted correctly - {"ManuscriptWitness":"witness_uri_here"} * - * TODO? Some sort of limit and skip for large responses? + * Paged with the 'limit' and 'skip' URL parameters, defaulting to 50 per page. The applied + * values and the configured maximums come back in the 'Pagination-*' response headers. * * @return The set of {'@id':'123', '@type':'WitnessFragment'} objects that match this criteria, as an Array * */ @@ -33,7 +34,7 @@ const _gog_fragments_from_manuscript = async function (req, res, next) { if (!agent) return const agentID = agent.split("/").pop() const manID = req.body["ManuscriptWitness"] - const { limit, skip } = getPagination(req.query, 50) + const { limit, skip } = getPagination(req.query, res, 50) let err = { message: `` } // This request can only be made my Gallery of Glosses production apps. if (agentID !== GOG_PROD_AGENT) { @@ -154,7 +155,8 @@ const _gog_fragments_from_manuscript = async function (req, res, next) { * The Bearer Token in the header must be from TinyMatt. * The body must be formatted correctly - {"ManuscriptWitness":"witness_uri_here"} * - * TODO? Some sort of limit and skip for large responses? + * Paged with the 'limit' and 'skip' URL parameters, defaulting to 50 per page. The applied + * values and the configured maximums come back in the 'Pagination-*' response headers. * * @return The set of {'@id':'123', '@type':'Gloss'} objects that match this criteria, as an Array * */ @@ -164,7 +166,7 @@ const _gog_glosses_from_manuscript = async function (req, res, next) { if (!agent) return const agentID = agent.split("/").pop() const manID = req.body["ManuscriptWitness"] - const { limit, skip } = getPagination(req.query, 50) + const { limit, skip } = getPagination(req.query, res, 50) let err = { message: `` } // This request can only be made my Gallery of Glosses production apps. if (agentID !== GOG_PROD_AGENT) { diff --git a/controllers/history.js b/controllers/history.js index b7b55a57..f43fe6f1 100644 --- a/controllers/history.js +++ b/controllers/history.js @@ -86,7 +86,7 @@ const history = async function (req, res, next) { const queryHeadRequest = async function (req, res, next) { res.set("Content-Type", "application/json; charset=utf-8") let props = req.body - const { limit, skip } = getPagination(req.query, 100) + const { limit, skip } = getPagination(req.query, res, 100) try { const matches = await db.find(props).limit(limit).skip(skip).toArray() if (matches.length) { diff --git a/controllers/search.js b/controllers/search.js index 816a1aa6..c712f15b 100644 --- a/controllers/search.js +++ b/controllers/search.js @@ -271,7 +271,7 @@ const searchAsWords = async function (req, res, next) { } return next(utils.createExpressError(err)) } - const { limit, skip } = getPagination(req.query, 100) + const { limit, skip } = getPagination(req.query, res, 100) const [queryPresi3, queryPresi2] = buildDualIndexQueries(searchText, { type: "text", options: searchOptions }, limit, skip) try { const [resultsPresi3, resultsPresi2] = await Promise.all([ @@ -357,7 +357,7 @@ const searchAsPhrase = async function (req, res, next) { } return next(utils.createExpressError(err)) } - const { limit, skip } = getPagination(req.query, 100) + const { limit, skip } = getPagination(req.query, res, 100) const [queryPresi3, queryPresi2] = buildDualIndexQueries(searchText, { type: "phrase", options: phraseOptions }, limit, skip) try { const [resultsPresi3, resultsPresi2] = await Promise.all([ @@ -435,7 +435,7 @@ const searchFuzzily = async function (req, res, next) { } return next(utils.createExpressError(err)) } - const { limit, skip } = getPagination(req.query, 100) + const { limit, skip } = getPagination(req.query, res, 100) const [queryPresi3, queryPresi2] = buildDualIndexQueries(searchText, { type: "text", options: fuzzyOptions }, limit, skip) try { const [resultsPresi3, resultsPresi2] = await Promise.all([ @@ -529,7 +529,7 @@ const searchWildly = async function (req, res, next) { } return next(utils.createExpressError(err)) } - const { limit, skip } = getPagination(req.query, 100) + const { limit, skip } = getPagination(req.query, res, 100) const [queryPresi3, queryPresi2] = buildDualIndexQueries(searchText, { type: "wildcard", options: wildcardOptions }, limit, skip) try { const [resultsPresi3, resultsPresi2] = await Promise.all([ @@ -622,7 +622,7 @@ const searchAlikes = async function (req, res, next) { } return next(utils.createExpressError(err)) } - const { limit, skip } = getPagination(req.query, 100) + const { limit, skip } = getPagination(req.query, res, 100) // Build moreLikeThis queries for both IIIF 3.0 and IIIF 2.1 indexes const searchQuery_presi3 = [ { diff --git a/controllers/utils.js b/controllers/utils.js index e7be35db..1511922d 100644 --- a/controllers/utils.js +++ b/controllers/utils.js @@ -9,21 +9,108 @@ import utils from '../utils.js' const ObjectID = newID -const MAX_QUERY_LIMIT = Number.parseInt(process.env.RERUM_MAX_QUERY_LIMIT ?? 500, 10) -const MAX_QUERY_SKIP = Number.parseInt(process.env.RERUM_MAX_QUERY_SKIP ?? 100000, 10) +const DEFAULT_MAX_QUERY_LIMIT = 500 +const DEFAULT_MAX_QUERY_SKIP = 100000 -function clampNonNegativeInt(value, fallback, max) { - const parsed = Number.parseInt(value, 10) - if (!Number.isFinite(parsed) || parsed <= 0) return fallback - return parsed > max ? max : parsed +/** + * The longest raw parameter value echoed back in a 400 message. A query string can be arbitrarily + * long and there is no reason to reflect all of it into the response. + */ +const PARAM_ECHO_MAX = 40 + +/** + * Resolve a configured pagination cap from the environment. + * + * Read per call rather than captured at module load, so a deployment's '.env' and the tests can + * both set it. A missing or unusable value falls back to the code default. + * + * @param key The 'process.env' key holding the cap. + * @param fallback The cap to use when the key is unset or unusable. + * @return A usable cap greater than 0. + */ +function resolveQueryCap(key, fallback) { + const configured = Number.parseInt(process.env[key] ?? "", 10) + return Number.isFinite(configured) && configured > 0 ? configured : fallback } -function getPagination(query = {}, defaultLimit = 100) { - const limitMax = Number.isFinite(MAX_QUERY_LIMIT) && MAX_QUERY_LIMIT > 0 ? MAX_QUERY_LIMIT : 500 - const skipMax = Number.isFinite(MAX_QUERY_SKIP) && MAX_QUERY_SKIP >= 0 ? MAX_QUERY_SKIP : 100000 +/** + * Read one pagination URL parameter as a whole number, rejecting anything it cannot read exactly. + * + * The raw value is validated as a decimal integer string before it is parsed, because + * 'Number.parseInt' guesses: it reads '10abc' as 10, '1e3' as 1, and '250.7' as 250. A client + * asking for 1000 records and silently receiving 1 is the worst of those. + * + * A parameter supplied more than once arrives from Express as an Array. There is no single correct + * reading of '?limit=100&limit=200', so it is a client mistake rather than something to guess at. + * + * '?limit[a]=5' cannot be caught here. Under Express's default 'simple' query parser the key never + * reaches the server, so it is indistinguishable from a request that omitted the parameter. + * + * @param raw The raw value from 'req.query', or undefined when the parameter was omitted. + * @param name The parameter name, used in the error message. + * @param fallback The value to use when the parameter was omitted. + * @param min The smallest acceptable value. + * @throws A 400 express error when the value is not a whole number of at least 'min'. + * @return The parsed value, not yet clamped to any maximum. + */ +function readWholeNumberParam(raw, name, fallback, min) { + if (raw === undefined) return fallback + if (Array.isArray(raw)) { + throw utils.createExpressError({ + message: `The '${name}' URL parameter was provided more than once. Provide it exactly once.`, + status: 400 + }) + } + const parsed = typeof raw === "string" && /^\d+$/.test(raw) ? Number.parseInt(raw, 10) : NaN + if (!Number.isFinite(parsed) || parsed < min) { + const bound = min > 0 ? `greater than 0` : `0 or greater` + throw utils.createExpressError({ + message: `The '${name}' URL parameter must be a whole number ${bound}. Received '${String(raw).slice(0, PARAM_ECHO_MAX)}'.`, + status: 400 + }) + } + return parsed +} + +/** + * Resolve the 'limit' and 'skip' URL parameters for a paged endpoint, and report what was applied. + * + * An unreadable value is rejected with a 400 instead of being guessed at. + * + * The two maximums are not treated alike, because being over them does not mean the same thing. + * A 'limit' above its maximum is clamped, which is conventional for a page size and leaves the + * response readable. A 'skip' above its maximum is rejected, because clamping it would serve the + * page at the maximum over and over: a client advancing 'skip' and stopping on an empty page would + * never terminate, and would accumulate the same records on every pass. + * + * The applied values and both maximums are reported in the response headers, so a client can tell + * a truncated page from a genuine final one and can configure itself from any single response. + * + * @param query The Express 'req.query' object. + * @param res The Express response, so the applied values can be reported. Optional. + * @param defaultLimit The limit to apply when the client does not ask for one. + * @throws A 400 express error when either parameter is not a whole number in range, or when 'skip' + * is beyond the configured maximum. + * @return An object carrying the applied 'limit' and 'skip'. + */ +function getPagination(query = {}, res = null, defaultLimit = 100) { + const limitMax = resolveQueryCap("MAX_QUERY_LIMIT", DEFAULT_MAX_QUERY_LIMIT) + const skipMax = resolveQueryCap("MAX_QUERY_SKIP", DEFAULT_MAX_QUERY_SKIP) const safeDefaultLimit = defaultLimit > 0 ? defaultLimit : 100 - const limit = clampNonNegativeInt(query.limit, safeDefaultLimit, limitMax) - const skip = clampNonNegativeInt(query.skip, 0, skipMax) + const limit = Math.min(readWholeNumberParam(query.limit, "limit", safeDefaultLimit, 1), limitMax) + const skip = readWholeNumberParam(query.skip, "skip", 0, 0) + if (skip > skipMax) { + throw utils.createExpressError({ + message: `The 'skip' URL parameter of ${skip} is beyond the maximum of ${skipMax}. Reading deeper than that is not supported, because every page past it would repeat the one at the maximum. Narrow the query so the records you want fall within the first ${skipMax} results.`, + status: 400 + }) + } + res?.set({ + "Pagination-Limit": String(limit), + "Pagination-Skip": String(skip), + "Pagination-Limit-Max": String(limitMax), + "Pagination-Skip-Max": String(skipMax) + }) return { limit, skip } } diff --git a/public/API.html b/public/API.html index 54804b09..27537e00 100644 --- a/public/API.html +++ b/public/API.html @@ -60,6 +60,7 @@

API (1.1.0)

  • Create
  • Bulk Create
  • Custom Query
    • +
  • Pagination parameters
  • Text Search
  • Phrase Search
  • Expanded record with filters
    • @@ -503,8 +504,8 @@

    Custom Query

    This simple format will be made more complex in the future, but should serve the basic needs as it is. - RERUM will test for property matches. By default, this is limited to 10 records in the response to guard against unreasonable queries. To allow for more records in the response one can add the URL parameter limit to the query requests. If you expect the query request will have a very large response with many objects, your application should use a paged query by also using the skip URL parameter. You will see an example of this below. -

    Note that your application may experience strange behavior with large limits, such as ?limit=1000. It is recommended to use a limit of 100 or less. If you expect there are more than 100 matching records, use a paged query to make consecutive requests until all records all gathered.

    + RERUM will test for property matches. By default, this is limited to 100 records in the response to guard against unreasonable queries. To allow for more records in the response one can add the URL parameter limit to the query requests. If you expect the query request will have a very large response with many objects, your application should use a paged query by also using the skip URL parameter. You will see an example of this below. +

    A limit above the maximum of 500 is not an error. It is reduced to 500, and the response says so — see Pagination parameters. Read the applied value from the Pagination-Limit response header rather than assuming you received everything you asked for.

    Non-Paged Query Javascript Example
    @@ -552,29 +553,75 @@

    Custom Query

     
                 const many_results = await pagedQuery(100, 0, {"type": "Thing"})
                 
    
-                function pagedQuery(lim, it = 0, queryObj, allResults = []) {
-                    return fetch(`https://devstore.rerum.io/v1/api/query?limit=${lim}&skip=${it}`, {
+                async function pagedQuery(lim, it = 0, queryObj, allResults = []) {
+                    const response = await fetch(`https://devstore.rerum.io/v1/api/query?limit=${lim}&skip=${it}`, {
                          method: "POST",
                         headers: {
                             "Content-Type": "application/json; charset=utf-8"
                         },
                         body: JSON.stringify(queryObj)
                     })
-                    .then(response => response.json())
-                    .then(results => {
-                        if (results.length) {
-                            allResults = allResults.concat(results)
-                            return pagedQuery(lim, it + results.length, queryObj, allResults)
-                        }
-                        return allResults
-                    })
-                    .catch(err => {
-                        console.warn("Could not process a result in paged query")
-                        throw err
-                    })
+                    // Also how a walk past the skip maximum ends. The message says so.
+                    if (!response.ok) throw new Error(await response.text())
+                    const results = await response.json()
+                    allResults = allResults.concat(results)
+                    
    
+                    // Advance by the page size the server applied, which may be smaller than the one you asked for.
+                    const appliedLimit = Number(response.headers.get("Pagination-Limit")) || lim
+                    
    
+                    // A short page is the last page. Do not wait for an empty one.
+                    if (results.length < appliedLimit) return allResults
+                    return pagedQuery(lim, it + appliedLimit, queryObj, allResults)
                 }

    +

    Pagination parameters

    +

    + limit and skip are URL parameters shared by Custom Query, Text Search, and Phrase Search. Both must be whole numbers written in plain decimal digits. Anything else is a 400 rather than a guess, because guessing is how a client asking for ?limit=1e3 used to receive a single record and report a completed walk. +

    + + + + + + + + + + + + + + + + + + + + + + + +
    ParameterDefaultMaximumRules
    limit100500A whole number of 1 or greater. Above the maximum it is reduced to the maximum, and Pagination-Limit reports what was applied.
    skip0100000A whole number of 0 or greater. Above the maximum it is a 400 naming the maximum, because clamping it would return the page at the maximum over and over.
    +

    + Rejected with a 400: ?limit=abc, ?limit=10abc, ?limit=1e3, ?limit=0x10, ?limit=250.7, ?limit=0, ?limit=-5, ?limit=, and the same forms of skip. Supplying either parameter more than once, as in ?limit=100&limit=200, is also a 400 — there is no correct reading of it. +

    +

    One form cannot be caught. ?limit[a]=5 never reaches the server as a limit at all, so it is indistinguishable from leaving the parameter out and you will silently get the default.

    +

    + The two maximums are not treated alike, because being over them does not mean the same thing. An over-maximum limit is a page size RERUM can honour in part, so it is reduced. An over-maximum skip has no honest reading at all — every page past it would repeat the page at the maximum — so it is refused. If you are reaching that depth, narrow the query rather than paging further. +

    +

    + Every paged response reports what was applied and what the maximums are, so a client can configure itself from any single response and tell a truncated page from a genuine final one. These headers are readable cross-origin. +

    +

    + 

     
+                Pagination-Limit: 500
+                Pagination-Skip: 0
+                Pagination-Limit-Max: 500
+                Pagination-Skip-Max: 100000
+
    +

    +

    Text Search

    @@ -609,7 +656,7 @@

    Text Search

    To allow for more records in the response one can add the URL parameter limit to the search requests. If you expect the search request will have a very large response with many objects, your application should use a paged search by also using the skip URL parameter. You will see an example of this below. -

    Note that your application may experience strange behavior with large limits, such as ?limit=1000. It is recommended to use a limit of 100 or less. If you expect there are more than 100 matching records, use a paged search to make consecutive requests until all records all gathered.

    +

    A limit above the maximum of 500 is reduced to 500, and the response says so — see Pagination parameters. Read the applied value from the Pagination-Limit response header rather than assuming you received everything you asked for.

    Search behavior: @@ -727,7 +774,7 @@

    To allow for more records in the response one can add the URL parameter limit to the search requests. If you expect the search request will have a very large response with many objects, your application should use a paged search by also using the skip URL parameter. You will see an example of this below. -

    Note that your application may experience strange behavior with large limits, such as ?limit=1000. It is recommended to use a limit of 100 or less. If you expect there are more than 100 matching records, use a paged search to make consecutive requests until all records are gathered.

    +

    A limit above the maximum of 500 is reduced to 500, and the response says so — see Pagination parameters. Read the applied value from the Pagination-Limit response header rather than assuming you received everything you asked for.

    Search behavior: diff --git a/routes/__tests__/query.test.js b/routes/__tests__/query.test.js index 8d47f8af..306b675a 100644 --- a/routes/__tests__/query.test.js +++ b/routes/__tests__/query.test.js @@ -5,6 +5,7 @@ import assert from 'node:assert/strict' import express from "express" import request from "supertest" import controller from '../../db-controller.js' +import rest from '../../rest.js' const routeTester = new express() routeTester.use(express.json({ type: ["application/json", "application/ld+json"] })) @@ -99,3 +100,92 @@ describe('HEAD /query', () => { assert.strictEqual(response.statusCode, 404) }) }) + +describe('pagination parameters on /query', () => { + // rest.messenger renders the 400s that getPagination throws. The routeTester above deliberately + // mounts no error handler, so these get their own app rather than changing how it behaves. + const pagedTester = express() + pagedTester.use(express.json({ type: ["application/json", "application/ld+json"] })) + pagedTester.head("/query", controller.queryHeadRequest) + pagedTester.use("/query", controller.query) + pagedTester.use(rest.messenger) + + /** A cursor that records the limit and skip the controller actually applied. */ + const recordingCursor = (docs, applied) => ({ + limit(n) { + applied.limit = n + return this + }, + skip(n) { + applied.skip = n + return this + }, + async toArray() { return docs } + }) + + const post = (queryString) => { + db.find.mockReturnValueOnce(recordingCursor([mockDoc], {})) + return request(pagedTester) + .post(`/query${queryString}`) + .set("Content-Type", "application/json") + .send({ test: "item" }) + } + + it("rejects a limit or skip it cannot read exactly", async () => { + for (const queryString of ["?limit=abc", "?limit=1e3", "?limit=0", "?limit=-5", "?limit=250.7", "?limit=", "?skip=abc", "?skip=2.9", "?skip=-5"]) { + const response = await post(queryString) + assert.strictEqual(response.statusCode, 400, `${queryString} should be a 400`) + } + }) + + it("rejects a repeated limit rather than taking one of the two values", async () => { + assert.strictEqual((await post("?limit=100&limit=200")).statusCode, 400) + assert.strictEqual((await post("?limit=200&limit=100")).statusCode, 400) + }) + + it("reports the applied limit and skip, and the maximums, on a paged response", async () => { + const response = await post("?limit=25&skip=10") + assert.strictEqual(response.statusCode, 200) + assert.strictEqual(response.headers['pagination-limit'], '25') + assert.strictEqual(response.headers['pagination-skip'], '10') + assert.ok(Number(response.headers['pagination-limit-max']) > 0) + assert.ok(Number(response.headers['pagination-skip-max']) > 0) + }) + + it("reports the clamp when the limit asked for is above the maximum", async () => { + const response = await post("?limit=999999") + assert.strictEqual(response.statusCode, 200) + assert.strictEqual(response.headers['pagination-limit'], response.headers['pagination-limit-max']) + }) + + it("applies the reported limit and skip to the database cursor", async () => { + const applied = {} + db.find.mockReturnValueOnce(recordingCursor([mockDoc], applied)) + const response = await request(pagedTester) + .post("/query?limit=7&skip=3") + .set("Content-Type", "application/json") + .send({ test: "item" }) + + assert.strictEqual(applied.limit, 7) + assert.strictEqual(applied.skip, 3) + assert.strictEqual(response.headers['pagination-limit'], '7') + assert.strictEqual(response.headers['pagination-skip'], '3') + }) + + it("rejects a skip beyond the maximum instead of repeating the last page", async () => { + const paged = await post("?limit=2&skip=0") + const skipMax = Number(paged.headers['pagination-skip-max']) + + assert.strictEqual((await post(`?skip=${skipMax}`)).statusCode, 200, 'the maximum itself is still readable') + + const response = await post(`?skip=${skipMax + 1}`) + assert.strictEqual(response.statusCode, 400) + assert.match(response.text, new RegExp(`beyond the maximum of ${skipMax}`)) + }) + + it("rejects the same values on HEAD /query", async () => { + db.find.mockReturnValueOnce(recordingCursor([mockDoc], {})) + const response = await request(pagedTester).head("/query?limit=abc") + assert.strictEqual(response.statusCode, 400) + }) +}) diff --git a/routes/__tests__/search.test.js b/routes/__tests__/search.test.js index 54f1dbbb..42f70dd5 100644 --- a/routes/__tests__/search.test.js +++ b/routes/__tests__/search.test.js @@ -102,3 +102,36 @@ describe('search controllers', () => { assert.strictEqual(response.body.length, 1, 'duplicate _id across indexes should be deduped') }) }) + +describe('search pagination parameters', () => { + // getPagination is shared with /query, so this proves the search endpoints are covered by the + // same contract rather than re-testing every rejected form here. + const searchFor = (path, queryString) => { + mockAggregateResults([]) + return request(routeTester) + .post(`${path}${queryString}`) + .set('Content-Type', 'text/plain') + .send('manuscript') + } + + it("searchAsWords rejects a limit or skip it cannot read exactly", async () => { + for (const queryString of ["?limit=abc", "?limit=1e3", "?limit=0", "?skip=-5", "?limit=100&limit=200"]) { + const response = await searchFor('/search', queryString) + assert.strictEqual(response.statusCode, 400, `${queryString} should be a 400`) + } + }) + + it("searchAsPhrase rejects them too", async () => { + const response = await searchFor('/search/phrase', '?skip=2.9') + assert.strictEqual(response.statusCode, 400) + }) + + it("reports the applied limit and skip on a search response", async () => { + const response = await searchFor('/search', '?limit=25&skip=10') + assert.strictEqual(response.statusCode, 200) + assert.strictEqual(response.headers['pagination-limit'], '25') + assert.strictEqual(response.headers['pagination-skip'], '10') + assert.ok(Number(response.headers['pagination-limit-max']) > 0) + assert.ok(Number(response.headers['pagination-skip-max']) > 0) + }) +}) From dcf3b32b634a99028f7094b4184cf04727d04787 Mon Sep 17 00:00:00 2001 From: Bryan Haberberger Date: Wed, 9 Sep 2026 11:39:22 -0500 Subject: [PATCH 2/9] changes during review and testing --- __tests__/utils.test.js | 19 +++++++++++++++++-- controllers/crud.js | 3 ++- controllers/gog.js | 16 ++++++++++------ controllers/utils.js | 29 ++++++++++++++++++++++++----- public/API.html | 3 ++- routes/__tests__/query.test.js | 13 +++++++++++++ 6 files changed, 68 insertions(+), 15 deletions(-) diff --git a/__tests__/utils.test.js b/__tests__/utils.test.js index df63ca27..b56dfcaf 100644 --- a/__tests__/utils.test.js +++ b/__tests__/utils.test.js @@ -443,16 +443,31 @@ describe('controllers/utils.js getPagination', () => { }) it('falls back to the code default when a configured cap is unusable', () => { + // '1e3' and '100_000' are what a hand-typed six figure cap looks like. Parsed rather than + // validated they become 1, which would serve one record per page across the deployment. const original = process.env.MAX_QUERY_LIMIT try { - process.env.MAX_QUERY_LIMIT = 'not-a-number' - assert.strictEqual(capturedHeadersFor({})['Pagination-Limit-Max'], '500') + for (const configured of ['not-a-number', '1e3', '100_000', '500abc', '250.7', '0', '-5', ' 500']) { + process.env.MAX_QUERY_LIMIT = configured + assert.strictEqual( + capturedHeadersFor({})['Pagination-Limit-Max'], + '500', + `MAX_QUERY_LIMIT='${configured}' should fall back to the code default` + ) + } } finally { if (original === undefined) delete process.env.MAX_QUERY_LIMIT else process.env.MAX_QUERY_LIMIT = original } }) + it('accepts an already-integral number, which req.query never holds but a caller might pass', () => { + const result = getPagination({ limit: 50, skip: 10 }) + assert.strictEqual(result.limit, 50) + assert.strictEqual(result.skip, 10) + assertRejects({ limit: 250.7 }, /whole number greater than 0/) + }) + /** Run getPagination against a minimal response double and hand back the headers it set. */ function capturedHeadersFor(query) { let captured diff --git a/controllers/crud.js b/controllers/crud.js index 4f9f9991..2a7b4a07 100644 --- a/controllers/crud.js +++ b/controllers/crud.js @@ -74,7 +74,6 @@ const create = async function (req, res, next) { const query = async function (req, res, next) { res.set("Content-Type", "application/json; charset=utf-8") let props = req.body - const { limit, skip } = getPagination(req.query, res, 100) if (!props || Object.keys(props).length === 0) { //Hey now, don't ask for everything...this can happen by accident. Don't allow it. let err = { @@ -83,6 +82,8 @@ const query = async function (req, res, next) { } return next(utils.createExpressError(err)) } + // Below the guard above, so a request that is never paged does not report a page in its headers. + const { limit, skip } = getPagination(req.query, res, 100) try { let matches = await db.find(props).limit(limit).skip(skip).toArray() matches = matches.map(o => idNegotiation(o)) diff --git a/controllers/gog.js b/controllers/gog.js index c9cbef79..1219feb2 100644 --- a/controllers/gog.js +++ b/controllers/gog.js @@ -23,8 +23,10 @@ const GOG_AGENTS = [GOG_PROD_AGENT, GOG_DEV_AGENT] * The Bearer Token in the header must be from TinyMatt. * The body must be formatted correctly - {"ManuscriptWitness":"witness_uri_here"} * - * Paged with the 'limit' and 'skip' URL parameters, defaulting to 50 per page. The applied - * values and the configured maximums come back in the 'Pagination-*' response headers. + * The 'limit' and 'skip' URL parameters bound the candidate Annotations this aggregation considers, + * defaulting to 50, not the entities it returns. Later stages filter and unwind that window, so the + * response length is not the limit in either direction. No 'Pagination-*' headers are reported, + * because there is nothing honest to report until the window moves to the end of the pipeline. * * @return The set of {'@id':'123', '@type':'WitnessFragment'} objects that match this criteria, as an Array * */ @@ -34,7 +36,7 @@ const _gog_fragments_from_manuscript = async function (req, res, next) { if (!agent) return const agentID = agent.split("/").pop() const manID = req.body["ManuscriptWitness"] - const { limit, skip } = getPagination(req.query, res, 50) + const { limit, skip } = getPagination(req.query, null, 50) let err = { message: `` } // This request can only be made my Gallery of Glosses production apps. if (agentID !== GOG_PROD_AGENT) { @@ -155,8 +157,10 @@ const _gog_fragments_from_manuscript = async function (req, res, next) { * The Bearer Token in the header must be from TinyMatt. * The body must be formatted correctly - {"ManuscriptWitness":"witness_uri_here"} * - * Paged with the 'limit' and 'skip' URL parameters, defaulting to 50 per page. The applied - * values and the configured maximums come back in the 'Pagination-*' response headers. + * The 'limit' and 'skip' URL parameters bound the candidate Annotations this aggregation considers, + * defaulting to 50, not the entities it returns. Later stages filter and unwind that window, so the + * response length is not the limit in either direction. No 'Pagination-*' headers are reported, + * because there is nothing honest to report until the window moves to the end of the pipeline. * * @return The set of {'@id':'123', '@type':'Gloss'} objects that match this criteria, as an Array * */ @@ -166,7 +170,7 @@ const _gog_glosses_from_manuscript = async function (req, res, next) { if (!agent) return const agentID = agent.split("/").pop() const manID = req.body["ManuscriptWitness"] - const { limit, skip } = getPagination(req.query, res, 50) + const { limit, skip } = getPagination(req.query, null, 50) let err = { message: `` } // This request can only be made my Gallery of Glosses production apps. if (agentID !== GOG_PROD_AGENT) { diff --git a/controllers/utils.js b/controllers/utils.js index 1511922d..20af96a5 100644 --- a/controllers/utils.js +++ b/controllers/utils.js @@ -22,15 +22,29 @@ const PARAM_ECHO_MAX = 40 * Resolve a configured pagination cap from the environment. * * Read per call rather than captured at module load, so a deployment's '.env' and the tests can - * both set it. A missing or unusable value falls back to the code default. + * both set it. + * + * The raw value is validated as a decimal integer string before it is parsed, for the same reason + * the client parameters are. 'Number.parseInt' would read a hand-typed 'MAX_QUERY_LIMIT=1e3' as a + * cap of 1 and serve one record per page across the whole deployment, with only + * 'Pagination-Limit-Max: 1' as evidence. + * + * An unset key falls back quietly. A value that is present but unusable falls back loudly, because + * a cap that is silently wrong is very hard to notice. 0 is not a usable cap for either maximum. * * @param key The 'process.env' key holding the cap. * @param fallback The cap to use when the key is unset or unusable. * @return A usable cap greater than 0. */ function resolveQueryCap(key, fallback) { - const configured = Number.parseInt(process.env[key] ?? "", 10) - return Number.isFinite(configured) && configured > 0 ? configured : fallback + const raw = process.env[key] + if (raw === undefined || raw === "") return fallback + const configured = /^\d+$/.test(raw) ? Number.parseInt(raw, 10) : NaN + if (!Number.isInteger(configured) || configured <= 0) { + console.warn(`\x1b[33m[pagination] ${key}='${String(raw).slice(0, PARAM_ECHO_MAX)}' is not a whole number greater than 0. Falling back to ${fallback}.\x1b[0m`) + return fallback + } + return configured } /** @@ -46,6 +60,9 @@ function resolveQueryCap(key, fallback) { * '?limit[a]=5' cannot be caught here. Under Express's default 'simple' query parser the key never * reaches the server, so it is indistinguishable from a request that omitted the parameter. * + * An already-integral Number is taken as-is. 'req.query' never holds one, but a non-Express caller + * should not get a 400 complaining that 50 is not a whole number. + * * @param raw The raw value from 'req.query', or undefined when the parameter was omitted. * @param name The parameter name, used in the error message. * @param fallback The value to use when the parameter was omitted. @@ -61,8 +78,10 @@ function readWholeNumberParam(raw, name, fallback, min) { status: 400 }) } - const parsed = typeof raw === "string" && /^\d+$/.test(raw) ? Number.parseInt(raw, 10) : NaN - if (!Number.isFinite(parsed) || parsed < min) { + const parsed = typeof raw === "number" ? raw + : typeof raw === "string" && /^\d+$/.test(raw) ? Number.parseInt(raw, 10) + : NaN + if (!Number.isInteger(parsed) || parsed < min) { const bound = min > 0 ? `greater than 0` : `0 or greater` throw utils.createExpressError({ message: `The '${name}' URL parameter must be a whole number ${bound}. Received '${String(raw).slice(0, PARAM_ECHO_MAX)}'.`, diff --git a/public/API.html b/public/API.html index 27537e00..d56f24b2 100644 --- a/public/API.html +++ b/public/API.html @@ -561,7 +561,8 @@

    Custom Query

    },body: JSON.stringify(queryObj)}) - // Also how a walk past the skip maximum ends. The message says so. + // A walk past the skip maximum ends here. Keep the pages already gathered. + if (response.status === 400 && it > 0) return allResultsif (!response.ok) throw new Error(await response.text())const results = await response.json()allResults = allResults.concat(results) diff --git a/routes/__tests__/query.test.js b/routes/__tests__/query.test.js index 306b675a..128e761f 100644 --- a/routes/__tests__/query.test.js +++ b/routes/__tests__/query.test.js @@ -183,6 +183,19 @@ describe('pagination parameters on /query', () => { assert.match(response.text, new RegExp(`beyond the maximum of ${skipMax}`)) }) + it("reports no page on the empty body 400, because none was served", async () => { + const response = await request(pagedTester) + .post("/query?limit=25&skip=5") + .set("Content-Type", "application/json") + .send({}) + + assert.strictEqual(response.statusCode, 400) + assert.match(response.text, /Detected empty JSON object/) + for (const header of ['pagination-limit', 'pagination-skip', 'pagination-limit-max', 'pagination-skip-max']) { + assert.strictEqual(response.headers[header], undefined, `${header} should not be set`) + } + }) + it("rejects the same values on HEAD /query", async () => { db.find.mockReturnValueOnce(recordingCursor([mockDoc], {})) const response = await request(pagedTester).head("/query?limit=abc") From 748f58779d15726091869296db399ccc2883fbe9 Mon Sep 17 00:00:00 2001 From: Bryan Haberberger Date: Thu, 10 Sep 2026 12:52:58 -0500 Subject: [PATCH 3/9] Changes during testing and review --- __tests__/utils.test.js | 32 +++++++++++++++++++++++++++++--- controllers/crud.js | 5 ++++- controllers/history.js | 3 ++- controllers/utils.js | 15 ++++++++++++--- database/__mocks__/index.js | 1 + public/API.html | 17 +++++++++++++++-- routes/__tests__/query.test.js | 31 +++++++++++++++++++++++++++++++ 7 files changed, 94 insertions(+), 10 deletions(-) diff --git a/__tests__/utils.test.js b/__tests__/utils.test.js index b56dfcaf..595b7c15 100644 --- a/__tests__/utils.test.js +++ b/__tests__/utils.test.js @@ -461,6 +461,22 @@ describe('controllers/utils.js getPagination', () => { } }) + it('reports the ceilings on a rejection, so a client can recover from the 400 it just got', () => { + // The 400 for an over-deep skip is the response a client most needs the ceiling from, and it + // is what lets a paged walk tell that boundary apart from every other 400 it could receive. + for (const query of [{ limit: 'abc' }, { skip: 'abc' }, { limit: ['100', '200'] }, { skip: '999999999' }]) { + const headers = capturedHeadersFor(query) + assert.ok(Number(headers['Pagination-Limit-Max']) > 0, `${JSON.stringify(query)} should still report the limit ceiling`) + assert.ok(Number(headers['Pagination-Skip-Max']) > 0, `${JSON.stringify(query)} should still report the skip ceiling`) + } + }) + + it('reports no applied page on a rejection, because none was served', () => { + const headers = capturedHeadersFor({ skip: '999999999' }) + assert.strictEqual(headers['Pagination-Limit'], undefined) + assert.strictEqual(headers['Pagination-Skip'], undefined) + }) + it('accepts an already-integral number, which req.query never holds but a caller might pass', () => { const result = getPagination({ limit: 50, skip: 10 }) assert.strictEqual(result.limit, 50) @@ -468,10 +484,20 @@ describe('controllers/utils.js getPagination', () => { assertRejects({ limit: 250.7 }, /whole number greater than 0/) }) - /** Run getPagination against a minimal response double and hand back the headers it set. */ + /** + * Run getPagination against a minimal response double and hand back the headers it set. + * + * The ceilings and the applied values arrive as two separate set() calls, so they are merged + * rather than overwritten. A rejected query still reports the ceilings, so a 400 is caught here + * instead of propagating. Anything else still throws, so a real fault is not swallowed. + */ function capturedHeadersFor(query) { - let captured - getPagination(query, { set: (headers) => { captured = headers } }) + const captured = {} + try { + getPagination(query, { set: (headers) => Object.assign(captured, headers) }) + } catch (err) { + if (err.statusCode !== 400) throw err + } return captured } }) diff --git a/controllers/crud.js b/controllers/crud.js index 2a7b4a07..0865a45e 100644 --- a/controllers/crud.js +++ b/controllers/crud.js @@ -85,7 +85,10 @@ const query = async function (req, res, next) { // Below the guard above, so a request that is never paged does not report a page in its headers. const { limit, skip } = getPagination(req.query, res, 100) try { - let matches = await db.find(props).limit(limit).skip(skip).toArray() + // A skip offset only means something over a deterministic order. '_id' is unique, always + // present, and always indexed, so it is the cheapest total order available, and it is the + // key a keyset cursor would resume from if paging ever moves off offsets. + let matches = await db.find(props).sort({ _id: 1 }).limit(limit).skip(skip).toArray() matches = matches.map(o => idNegotiation(o)) res.set(utils.configureLDHeadersFor(matches)) res.json(matches) diff --git a/controllers/history.js b/controllers/history.js index f43fe6f1..331158a8 100644 --- a/controllers/history.js +++ b/controllers/history.js @@ -88,7 +88,8 @@ const queryHeadRequest = async function (req, res, next) { let props = req.body const { limit, skip } = getPagination(req.query, res, 100) try { - const matches = await db.find(props).limit(limit).skip(skip).toArray() + // Sorted the same way POST /query is, so the two verbs page over one order. + const matches = await db.find(props).sort({ _id: 1 }).limit(limit).skip(skip).toArray() if (matches.length) { const negotiated = matches.map(o => idNegotiation(o)) const size = Buffer.byteLength(JSON.stringify(negotiated)) diff --git a/controllers/utils.js b/controllers/utils.js index 20af96a5..ced2250e 100644 --- a/controllers/utils.js +++ b/controllers/utils.js @@ -104,6 +104,9 @@ function readWholeNumberParam(raw, name, fallback, min) { * * The applied values and both maximums are reported in the response headers, so a client can tell * a truncated page from a genuine final one and can configure itself from any single response. + * The maximums are reported before either parameter is read, so a rejection carries them too: the + * 400 for an over-deep 'skip' is the one response a client most needs the ceiling from, and it is + * what lets a paged walk tell that boundary apart from every other 400 it could receive. * * @param query The Express 'req.query' object. * @param res The Express response, so the applied values can be reported. Optional. @@ -115,6 +118,13 @@ function readWholeNumberParam(raw, name, fallback, min) { function getPagination(query = {}, res = null, defaultLimit = 100) { const limitMax = resolveQueryCap("MAX_QUERY_LIMIT", DEFAULT_MAX_QUERY_LIMIT) const skipMax = resolveQueryCap("MAX_QUERY_SKIP", DEFAULT_MAX_QUERY_SKIP) + // Both ceilings are known before either parameter is read, so they are reported before anything + // can throw. A client that gets a 400 back can then read the boundary it hit off the same + // response rather than parsing it out of the message. + res?.set({ + "Pagination-Limit-Max": String(limitMax), + "Pagination-Skip-Max": String(skipMax) + }) const safeDefaultLimit = defaultLimit > 0 ? defaultLimit : 100 const limit = Math.min(readWholeNumberParam(query.limit, "limit", safeDefaultLimit, 1), limitMax) const skip = readWholeNumberParam(query.skip, "skip", 0, 0) @@ -124,11 +134,10 @@ function getPagination(query = {}, res = null, defaultLimit = 100) { status: 400 }) } + // The applied values are only knowable once both parameters have survived validation. res?.set({ "Pagination-Limit": String(limit), - "Pagination-Skip": String(skip), - "Pagination-Limit-Max": String(limitMax), - "Pagination-Skip-Max": String(skipMax) + "Pagination-Skip": String(skip) }) return { limit, skip } } diff --git a/database/__mocks__/index.js b/database/__mocks__/index.js index 886842ba..f51e2607 100644 --- a/database/__mocks__/index.js +++ b/database/__mocks__/index.js @@ -49,6 +49,7 @@ function createMockFunction(implementation = () => undefined) { */ export function createCursor(docs = []) { const cursor = { + sort: createMockFunction(function () { return this }), limit: createMockFunction(function () { return this }), skip: createMockFunction(function () { return this }), batchSize: createMockFunction(function () { return this }), diff --git a/public/API.html b/public/API.html index d56f24b2..b8a17e85 100644 --- a/public/API.html +++ b/public/API.html @@ -561,8 +561,10 @@

    Custom Query

    }, body: JSON.stringify(queryObj) }) - // A walk past the skip maximum ends here. Keep the pages already gathered. - if (response.status === 400 && it > 0) return allResults + // Only the skip ceiling ends a walk early. Every other 400 is a real error, and + // returning the pages gathered so far would report a walk that never finished. + const skipMax = Number(response.headers.get("Pagination-Skip-Max")) + if (response.status === 400 && skipMax && it > skipMax) return allResults if (!response.ok) throw new Error(await response.text()) const results = await response.json() allResults = allResults.concat(results) @@ -614,6 +616,9 @@

    Pagination parameters

    Every paged response reports what was applied and what the maximums are, so a client can configure itself from any single response and tell a truncated page from a genuine final one. These headers are readable cross-origin.

    +

    + Pagination-Limit-Max and Pagination-Skip-Max are reported on rejections too, including the 400 for a skip past the maximum. That is what lets a paged walk recognise the depth boundary and stop cleanly, instead of treating every 400 as the end of the results. Pagination-Limit and Pagination-Skip appear only where a page was actually served. + 

     
                 Pagination-Limit: 500
@@ -622,6 +627,14 @@ 
    Pagination parameters
    
                 Pagination-Skip-Max: 100000

    +

    Result ordering

    +

    + Custom Query pages in ascending _id order, on both POST and HEAD. A skip offset only means something over a fixed order, so consecutive pages tile that order without repeating or missing a record. +

    +

    This is not oldest-to-newest. RERUM _id values are a mix of BSON types across the repository's history, and BSON sorts each type into its own block, so the order is stable and complete but carries no chronological meaning. Sort client-side on __rerum.createdAt if you need records in the order they were made.

    +

    + The search endpoints do not currently make an ordering guarantee. Do not assume paged search results tile the same way. +

    Text Search

    diff --git a/routes/__tests__/query.test.js b/routes/__tests__/query.test.js index 128e761f..b54095fc 100644 --- a/routes/__tests__/query.test.js +++ b/routes/__tests__/query.test.js @@ -42,6 +42,9 @@ beforeEach(() => { it("'/query' route functions", async () => { const queryCursor = { + sort() { + return this + }, limit() { return this }, @@ -67,6 +70,7 @@ it("'/query' route functions", async () => { describe('HEAD /query', () => { const buildCursor = (docs) => ({ + sort() { return this }, limit() { return this }, skip() { return this }, async toArray() { return docs } @@ -112,6 +116,10 @@ describe('pagination parameters on /query', () => { /** A cursor that records the limit and skip the controller actually applied. */ const recordingCursor = (docs, applied) => ({ + sort(order) { + applied.sort = order + return this + }, limit(n) { applied.limit = n return this @@ -183,6 +191,29 @@ describe('pagination parameters on /query', () => { assert.match(response.text, new RegExp(`beyond the maximum of ${skipMax}`)) }) + it("reports the ceilings on the skip rejection, so a walk can tell that boundary from any other 400", async () => { + const paged = await post("?limit=2&skip=0") + const skipMax = Number(paged.headers['pagination-skip-max']) + + const response = await post(`?skip=${skipMax + 1}`) + assert.strictEqual(response.statusCode, 400) + assert.strictEqual(response.headers['pagination-skip-max'], String(skipMax)) + assert.ok(Number(response.headers['pagination-limit-max']) > 0) + assert.strictEqual(response.headers['pagination-limit'], undefined, 'no page was served') + assert.strictEqual(response.headers['pagination-skip'], undefined, 'no page was served') + }) + + it("pages over a deterministic order, because a skip offset means nothing without one", async () => { + const applied = {} + db.find.mockReturnValueOnce(recordingCursor([mockDoc], applied)) + await request(pagedTester) + .post("/query?limit=7&skip=3") + .set("Content-Type", "application/json") + .send({ test: "item" }) + + assert.deepStrictEqual(applied.sort, { _id: 1 }) + }) + it("reports no page on the empty body 400, because none was served", async () => { const response = await request(pagedTester) .post("/query?limit=25&skip=5") From f9030c08a8bd7db2d35f804653cd3b6a71a945ff Mon Sep 17 00:00:00 2001 From: Bryan Haberberger Date: Fri, 11 Sep 2026 08:37:43 -0500 Subject: [PATCH 4/9] Changes during testing and review --- public/API.html | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/public/API.html b/public/API.html index b8a17e85..e1c857cc 100644 --- a/public/API.html +++ b/public/API.html @@ -563,8 +563,12 @@

    Custom Query

    }) // Only the skip ceiling ends a walk early. Every other 400 is a real error, and // returning the pages gathered so far would report a walk that never finished. + // Stopping at the ceiling still truncates, so say so rather than returning quietly. const skipMax = Number(response.headers.get("Pagination-Skip-Max")) - if (response.status === 400 && skipMax && it > skipMax) return allResults + if (response.status === 400 && skipMax && it > skipMax) { + console.warn(`Stopped at the skip ceiling of ${skipMax}. These results are incomplete. Narrow the query.`) + return allResults + } if (!response.ok) throw new Error(await response.text()) const results = await response.json() allResults = allResults.concat(results) From 3ebbfbe07c7b623e264aabe50e510c5883f23527 Mon Sep 17 00:00:00 2001 From: Bryan Haberberger Date: Fri, 11 Sep 2026 09:07:07 -0500 Subject: [PATCH 5/9] Changes during testing and review --- __tests__/utils.test.js | 25 +++++++++++++++++++++++++ controllers/utils.js | 18 ++++++++++++++---- routes/__tests__/query.test.js | 20 +++++++++++++++++++- 3 files changed, 58 insertions(+), 5 deletions(-) diff --git a/__tests__/utils.test.js b/__tests__/utils.test.js index 595b7c15..bdfa835a 100644 --- a/__tests__/utils.test.js +++ b/__tests__/utils.test.js @@ -484,6 +484,31 @@ describe('controllers/utils.js getPagination', () => { assertRejects({ limit: 250.7 }, /whole number greater than 0/) }) + it('reports nothing rather than throwing when the second argument cannot set headers', () => { + // The pre-existing shape was getPagination(query, defaultLimit). A caller still using it would + // otherwise get a TypeError out of the header reporting, which surfaces as a 500 on an endpoint + // that meant to answer 200. + for (const notAResponse of [100, 'res', true, {}, { set: 'not a function' }]) { + const result = getPagination({ limit: '25', skip: '5' }, notAResponse) + assert.strictEqual(result.limit, 25, `${JSON.stringify(notAResponse)} should not change the limit`) + assert.strictEqual(result.skip, 5, `${JSON.stringify(notAResponse)} should not change the skip`) + } + }) + + it('echoes the raw skip in the ceiling rejection, not the value it parsed to', () => { + // A digit string long enough to lose precision parses to a different number than the client + // sent, and a message naming a value nobody asked for cannot be matched back to its request. + const raw = '99999999999999999999' + assert.throws( + () => getPagination({ skip: raw }), + (err) => { + assert.strictEqual(err.statusCode, 400) + assert.match(err.statusMessage, new RegExp(`of ${raw} is beyond the maximum`)) + return true + } + ) + }) + /** * Run getPagination against a minimal response double and hand back the headers it set. * diff --git a/controllers/utils.js b/controllers/utils.js index ced2250e..15cabfbe 100644 --- a/controllers/utils.js +++ b/controllers/utils.js @@ -109,7 +109,9 @@ function readWholeNumberParam(raw, name, fallback, min) { * what lets a paged walk tell that boundary apart from every other 400 it could receive. * * @param query The Express 'req.query' object. - * @param res The Express response, so the applied values can be reported. Optional. + * @param res The Express response, so the applied values can be reported. Optional. Anything + * without a 'set' method reports nothing rather than throwing, so a caller using the older + * two-argument shape still gets its values back instead of a 500. * @param defaultLimit The limit to apply when the client does not ask for one. * @throws A 400 express error when either parameter is not a whole number in range, or when 'skip' * is beyond the configured maximum. @@ -118,10 +120,15 @@ function readWholeNumberParam(raw, name, fallback, min) { function getPagination(query = {}, res = null, defaultLimit = 100) { const limitMax = resolveQueryCap("MAX_QUERY_LIMIT", DEFAULT_MAX_QUERY_LIMIT) const skipMax = resolveQueryCap("MAX_QUERY_SKIP", DEFAULT_MAX_QUERY_SKIP) + // 'res' is only ever used to report headers, so anything that cannot report is treated as an + // omitted response rather than an error. 'res?.set(...)' would throw a TypeError on the older + // getPagination(query, defaultLimit) shape, and a caller asking for a page would get a 500 + // naming optional chaining instead of the page it asked for. + const report = typeof res?.set === "function" ? (headers) => res.set(headers) : () => undefined // Both ceilings are known before either parameter is read, so they are reported before anything // can throw. A client that gets a 400 back can then read the boundary it hit off the same // response rather than parsing it out of the message. - res?.set({ + report({ "Pagination-Limit-Max": String(limitMax), "Pagination-Skip-Max": String(skipMax) }) @@ -129,13 +136,16 @@ function getPagination(query = {}, res = null, defaultLimit = 100) { const limit = Math.min(readWholeNumberParam(query.limit, "limit", safeDefaultLimit, 1), limitMax) const skip = readWholeNumberParam(query.skip, "skip", 0, 0) if (skip > skipMax) { + // Echo the raw value, not the parsed one. A digit string long enough to lose precision + // parses to a different number than the client sent, and a message quoting a value nobody + // asked for is hard to match back to the request that caused it. throw utils.createExpressError({ - message: `The 'skip' URL parameter of ${skip} is beyond the maximum of ${skipMax}. Reading deeper than that is not supported, because every page past it would repeat the one at the maximum. Narrow the query so the records you want fall within the first ${skipMax} results.`, + message: `The 'skip' URL parameter of ${String(query.skip).slice(0, PARAM_ECHO_MAX)} is beyond the maximum of ${skipMax}. Reading deeper than that is not supported, because every page past it would repeat the one at the maximum. Narrow the query so the records you want fall within the first ${skipMax} results.`, status: 400 }) } // The applied values are only knowable once both parameters have survived validation. - res?.set({ + report({ "Pagination-Limit": String(limit), "Pagination-Skip": String(skip) }) diff --git a/routes/__tests__/query.test.js b/routes/__tests__/query.test.js index b54095fc..03b7c048 100644 --- a/routes/__tests__/query.test.js +++ b/routes/__tests__/query.test.js @@ -108,10 +108,13 @@ describe('HEAD /query', () => { describe('pagination parameters on /query', () => { // rest.messenger renders the 400s that getPagination throws. The routeTester above deliberately // mounts no error handler, so these get their own app rather than changing how it behaves. + // Mounted the way routes/query.js mounts it, verifyJsonContentType included, so the order the + // real app answers in is what is under test: a Content-Type it cannot accept is a 415 before any + // pagination parameter is read. const pagedTester = express() pagedTester.use(express.json({ type: ["application/json", "application/ld+json"] })) pagedTester.head("/query", controller.queryHeadRequest) - pagedTester.use("/query", controller.query) + pagedTester.post("/query", rest.verifyJsonContentType, controller.query) pagedTester.use(rest.messenger) /** A cursor that records the limit and skip the controller actually applied. */ @@ -232,4 +235,19 @@ describe('pagination parameters on /query', () => { const response = await request(pagedTester).head("/query?limit=abc") assert.strictEqual(response.statusCode, 400) }) + + it("answers an unacceptable Content-Type before it reads a pagination parameter", async () => { + // Both faults are present. The Content-Type is the one the endpoint can answer without + // looking at the query string, so it is the one that should decide the status. + db.find.mockReturnValueOnce(recordingCursor([mockDoc], {})) + const response = await request(pagedTester) + .post("/query?limit=abc") + .set("Content-Type", "text/plain") + .send("not json") + + assert.strictEqual(response.statusCode, 415) + for (const header of ['pagination-limit-max', 'pagination-skip-max']) { + assert.strictEqual(response.headers[header], undefined, `${header} should not be set`) + } + }) }) From bd91fc71f384c32dc7afbc6bc83f434fdbc88dd7 Mon Sep 17 00:00:00 2001 From: Bryan Haberberger Date: Fri, 11 Sep 2026 10:07:45 -0500 Subject: [PATCH 6/9] Changes during testing and review --- controllers/search.js | 13 ++++-- public/API.html | 3 +- routes/__tests__/search.test.js | 70 +++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 4 deletions(-) diff --git a/controllers/search.js b/controllers/search.js index c712f15b..6596006a 100644 --- a/controllers/search.js +++ b/controllers/search.js @@ -23,7 +23,11 @@ import { idNegotiation, getPagination } from './utils.js' * 1. Combines both result arrays * 2. Removes duplicates based on MongoDB _id (keeps first occurrence) * 3. Sorts by search score in descending order (highest relevance first) - * + * + * The score is read from '__rerum.score', which is where buildDualIndexQueries() and the + * searchAlikes() pipelines put '$meta: "searchScore"'. There is no top-level 'score' on any + * document, so a comparator that reads one silently ranks nothing. + * * The function handles different _id formats: * - ObjectId objects with $oid property * - String-based _id values @@ -41,8 +45,11 @@ function mergeSearchResults(results1, results2) { } } - // Sort by score descending - return merged.sort((a, b) => (b.score || 0) - (a.score || 0)) + // Sort by score descending. The branch pipelines write each document's relevance to + // '__rerum.score', so it is read from there. No document carries a top-level 'score', so a + // comparator reading one returns 0 for every pair and leaves the merge in its construction + // order - every IIIF 3.0 hit, then every IIIF 2.1 hit, regardless of relevance. + return merged.sort((a, b) => (b.__rerum?.score ?? 0) - (a.__rerum?.score ?? 0)) } /** diff --git a/public/API.html b/public/API.html index e1c857cc..ca371284 100644 --- a/public/API.html +++ b/public/API.html @@ -637,8 +637,9 @@

    Result ordering

    This is not oldest-to-newest. RERUM _id values are a mix of BSON types across the repository's history, and BSON sorts each type into its own block, so the order is stable and complete but carries no chronological meaning. Sort client-side on __rerum.createdAt if you need records in the order they were made.

    - The search endpoints do not currently make an ordering guarantee. Do not assume paged search results tile the same way. + The search endpoints page in descending relevance score, which you can read back from __rerum.score on each result. That order is the same across both the IIIF 3.0 and IIIF 2.1 indexes, so the best match is the first record of the first page whichever shape it was written in.

    +

    Descending score is an ordering, not a tiling guarantee. Custom Query is the endpoint to walk when you need every matching record exactly once.

    Text Search

    diff --git a/routes/__tests__/search.test.js b/routes/__tests__/search.test.js index 42f70dd5..f939e966 100644 --- a/routes/__tests__/search.test.js +++ b/routes/__tests__/search.test.js @@ -27,6 +27,27 @@ function mockAggregateResults(docs) { }) } +/** + * Answer the two branches with different documents, so cross-index behavior can be observed. + * + * The controllers build the Promise.all array literal presi3 first, and array elements evaluate + * left to right, so the first queued result is the IIIF 3.0 branch. + */ +function mockBranchResults(presi3Docs, presi2Docs) { + db.aggregate.mockReturnValueOnce({ toArray: () => Promise.resolve(presi3Docs) }) + db.aggregate.mockReturnValueOnce({ toArray: () => Promise.resolve(presi2Docs) }) +} + +/** A search hit carrying its relevance where the branch pipelines actually put it. */ +const scoredDoc = (id, score) => ({ + _id: id, + '@id': `https://store.rerum.io/v1/id/${id}`, + __rerum: { score } +}) + +/** The document ids of a search response, in the order the endpoint returned them. */ +const idsOf = (response) => response.body.map(o => o['@id'].split('/').pop()) + describe('search controllers', () => { it("searchAsWords returns 400 when the body is empty", async () => { const response = await request(routeTester) @@ -101,6 +122,55 @@ describe('search controllers', () => { assert.strictEqual(response.statusCode, 200) assert.strictEqual(response.body.length, 1, 'duplicate _id across indexes should be deduped') }) + + // The branch pipelines write relevance to '__rerum.score'. A comparator reading a top-level + // 'score' finds nothing on any document, so the merge keeps its construction order and every + // IIIF 2.1 match is ranked behind every IIIF 3.0 match however well it scores. + it("searchAsWords ranks across both indexes by score, not by which index answered", async () => { + mockBranchResults( + [scoredDoc('presi3-weak', 1.69), scoredDoc('presi3-weaker', 1.24)], + [scoredDoc('presi2-best', 83.92)] + ) + + const response = await request(routeTester) + .post('/search') + .set('Content-Type', 'text/plain') + .send('line') + + assert.strictEqual(response.statusCode, 200) + assert.deepStrictEqual(idsOf(response), ['presi2-best', 'presi3-weak', 'presi3-weaker']) + }) + + it("searchAsPhrase ranks across both indexes too", async () => { + mockBranchResults([scoredDoc('presi3-weak', 2.45)], [scoredDoc('presi2-best', 6.95)]) + + const response = await request(routeTester) + .post('/search/phrase') + .set('Content-Type', 'text/plain') + .send('exact phrase') + + assert.strictEqual(response.statusCode, 200) + assert.deepStrictEqual(idsOf(response), ['presi2-best', 'presi3-weak']) + }) + + // The point of the ranking, for this endpoint: 'limit' and 'skip' slice the merged order, so a + // merge that does not rank hands back a window of the wrong records rather than a wrong order. + it("pages the score order, so skip walks best-first across both indexes", async () => { + const branches = () => mockBranchResults( + [scoredDoc('p3-c', 3), scoredDoc('p3-d', 2)], + [scoredDoc('p2-a', 9), scoredDoc('p2-b', 5)] + ) + const page = (queryString) => { + branches() + return request(routeTester) + .post(`/search${queryString}`) + .set('Content-Type', 'text/plain') + .send('line') + } + + assert.deepStrictEqual(idsOf(await page('?limit=2&skip=0')), ['p2-a', 'p2-b']) + assert.deepStrictEqual(idsOf(await page('?limit=2&skip=2')), ['p3-c', 'p3-d']) + }) }) describe('search pagination parameters', () => { From 10ef58b674ee14c148aa14d61b1f362b4a52a776 Mon Sep 17 00:00:00 2001 From: Bryan Haberberger Date: Fri, 11 Sep 2026 11:15:58 -0500 Subject: [PATCH 7/9] Last touch ups from review, let's do some manual review and cleanup. --- __tests__/utils.test.js | 12 ++++++++++++ controllers/utils.js | 10 ++++++++++ public/API.html | 4 +++- 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/__tests__/utils.test.js b/__tests__/utils.test.js index bdfa835a..8cb4506d 100644 --- a/__tests__/utils.test.js +++ b/__tests__/utils.test.js @@ -389,6 +389,18 @@ describe('controllers/utils.js getPagination', () => { assert.strictEqual(limit, Number(max)) }) + it('clamps a limit too large for a Number rather than calling it not a whole number', () => { + // Past about 309 digits Number.parseInt returns Infinity. Rejected there, a digit-only value + // would get a 400 saying it is not a whole number, and a limit the contract promises to clamp + // would be refused for being large - the one thing an over-maximum limit is never supposed to be. + const { limit } = getPagination({ limit: '9'.repeat(400) }) + assert.strictEqual(limit, Number(capturedHeadersFor({})['Pagination-Limit-Max'])) + }) + + it('sends a skip too large for a Number to the ceiling message, not the malformed one', () => { + assertRejects({ skip: '9'.repeat(400) }, /beyond the maximum/) + }) + it('rejects a skip above the maximum rather than serving the same page forever', () => { // Clamping it would hand back the page at the maximum on every request past it. A client // advancing skip and stopping on an empty page would never terminate. diff --git a/controllers/utils.js b/controllers/utils.js index 15cabfbe..13c5d4b8 100644 --- a/controllers/utils.js +++ b/controllers/utils.js @@ -63,6 +63,11 @@ function resolveQueryCap(key, fallback) { * An already-integral Number is taken as-is. 'req.query' never holds one, but a non-Express caller * should not get a 400 complaining that 50 is not a whole number. * + * A digit string too large for a Number is saturated to 'Number.MAX_SAFE_INTEGER' rather than + * rejected. It is a whole number, so the caller's maximum is what should decide it: a 'limit' + * clamps the way the contract promises, and a 'skip' gets the ceiling message naming the maximum + * instead of one claiming 400 digits are not a whole number. + * * @param raw The raw value from 'req.query', or undefined when the parameter was omitted. * @param name The parameter name, used in the error message. * @param fallback The value to use when the parameter was omitted. @@ -81,6 +86,11 @@ function readWholeNumberParam(raw, name, fallback, min) { const parsed = typeof raw === "number" ? raw : typeof raw === "string" && /^\d+$/.test(raw) ? Number.parseInt(raw, 10) : NaN + // A digit string past Number's range parses to Infinity. It is still a whole number, just a + // larger one than any maximum, so it belongs in the caller's clamp and ceiling handling rather + // than here. Rejecting it would tell a client that '9'.repeat(400) is not a whole number, and + // would reject a 'limit' the contract promises to clamp. + if (parsed === Infinity) return Number.MAX_SAFE_INTEGER if (!Number.isInteger(parsed) || parsed < min) { const bound = min > 0 ? `greater than 0` : `0 or greater` throw utils.createExpressError({ diff --git a/public/API.html b/public/API.html index ca371284..01bc08e2 100644 --- a/public/API.html +++ b/public/API.html @@ -571,7 +571,9 @@

    Custom Query

    } if (!response.ok) throw new Error(await response.text()) const results = await response.json() - allResults = allResults.concat(results) + // Append in place. Walking to the skip ceiling at this page size is a thousand pages, + // and concat() would copy every record gathered so far on each one of them. + allResults.push(...results)
    // Advance by the page size the server applied, which may be smaller than the one you asked for. const appliedLimit = Number(response.headers.get("Pagination-Limit")) || lim From 5639496b194509d7bc35d669eaa81c991b7847f9 Mon Sep 17 00:00:00 2001 From: Bryan Haberberger Date: Fri, 11 Sep 2026 11:29:00 -0500 Subject: [PATCH 8/9] cleanup --- controllers/crud.js | 3 --- controllers/gog.js | 10 ---------- controllers/search.js | 9 --------- controllers/utils.js | 46 ++----------------------------------------- public/API.html | 11 ++--------- 5 files changed, 4 insertions(+), 75 deletions(-) diff --git a/controllers/crud.js b/controllers/crud.js index 0865a45e..df8e0fe7 100644 --- a/controllers/crud.js +++ b/controllers/crud.js @@ -85,9 +85,6 @@ const query = async function (req, res, next) { // Below the guard above, so a request that is never paged does not report a page in its headers. const { limit, skip } = getPagination(req.query, res, 100) try { - // A skip offset only means something over a deterministic order. '_id' is unique, always - // present, and always indexed, so it is the cheapest total order available, and it is the - // key a keyset cursor would resume from if paging ever moves off offsets. let matches = await db.find(props).sort({ _id: 1 }).limit(limit).skip(skip).toArray() matches = matches.map(o => idNegotiation(o)) res.set(utils.configureLDHeadersFor(matches)) diff --git a/controllers/gog.js b/controllers/gog.js index 1219feb2..4d5117cb 100644 --- a/controllers/gog.js +++ b/controllers/gog.js @@ -23,11 +23,6 @@ const GOG_AGENTS = [GOG_PROD_AGENT, GOG_DEV_AGENT] * The Bearer Token in the header must be from TinyMatt. * The body must be formatted correctly - {"ManuscriptWitness":"witness_uri_here"} * - * The 'limit' and 'skip' URL parameters bound the candidate Annotations this aggregation considers, - * defaulting to 50, not the entities it returns. Later stages filter and unwind that window, so the - * response length is not the limit in either direction. No 'Pagination-*' headers are reported, - * because there is nothing honest to report until the window moves to the end of the pipeline. - * * @return The set of {'@id':'123', '@type':'WitnessFragment'} objects that match this criteria, as an Array * */ const _gog_fragments_from_manuscript = async function (req, res, next) { @@ -157,11 +152,6 @@ const _gog_fragments_from_manuscript = async function (req, res, next) { * The Bearer Token in the header must be from TinyMatt. * The body must be formatted correctly - {"ManuscriptWitness":"witness_uri_here"} * - * The 'limit' and 'skip' URL parameters bound the candidate Annotations this aggregation considers, - * defaulting to 50, not the entities it returns. Later stages filter and unwind that window, so the - * response length is not the limit in either direction. No 'Pagination-*' headers are reported, - * because there is nothing honest to report until the window moves to the end of the pipeline. - * * @return The set of {'@id':'123', '@type':'Gloss'} objects that match this criteria, as an Array * */ const _gog_glosses_from_manuscript = async function (req, res, next) { diff --git a/controllers/search.js b/controllers/search.js index 6596006a..c5163fe9 100644 --- a/controllers/search.js +++ b/controllers/search.js @@ -24,10 +24,6 @@ import { idNegotiation, getPagination } from './utils.js' * 2. Removes duplicates based on MongoDB _id (keeps first occurrence) * 3. Sorts by search score in descending order (highest relevance first) * - * The score is read from '__rerum.score', which is where buildDualIndexQueries() and the - * searchAlikes() pipelines put '$meta: "searchScore"'. There is no top-level 'score' on any - * document, so a comparator that reads one silently ranks nothing. - * * The function handles different _id formats: * - ObjectId objects with $oid property * - String-based _id values @@ -44,11 +40,6 @@ function mergeSearchResults(results1, results2) { merged.push(result) } } - - // Sort by score descending. The branch pipelines write each document's relevance to - // '__rerum.score', so it is read from there. No document carries a top-level 'score', so a - // comparator reading one returns 0 for every pair and leaves the merge in its construction - // order - every IIIF 3.0 hit, then every IIIF 2.1 hit, regardless of relevance. return merged.sort((a, b) => (b.__rerum?.score ?? 0) - (a.__rerum?.score ?? 0)) } diff --git a/controllers/utils.js b/controllers/utils.js index 13c5d4b8..f5316fcc 100644 --- a/controllers/utils.js +++ b/controllers/utils.js @@ -20,15 +20,6 @@ const PARAM_ECHO_MAX = 40 /** * Resolve a configured pagination cap from the environment. - * - * Read per call rather than captured at module load, so a deployment's '.env' and the tests can - * both set it. - * - * The raw value is validated as a decimal integer string before it is parsed, for the same reason - * the client parameters are. 'Number.parseInt' would read a hand-typed 'MAX_QUERY_LIMIT=1e3' as a - * cap of 1 and serve one record per page across the whole deployment, with only - * 'Pagination-Limit-Max: 1' as evidence. - * * An unset key falls back quietly. A value that is present but unusable falls back loudly, because * a cap that is silently wrong is very hard to notice. 0 is not a usable cap for either maximum. * @@ -49,25 +40,9 @@ function resolveQueryCap(key, fallback) { /** * Read one pagination URL parameter as a whole number, rejecting anything it cannot read exactly. - * - * The raw value is validated as a decimal integer string before it is parsed, because - * 'Number.parseInt' guesses: it reads '10abc' as 10, '1e3' as 1, and '250.7' as 250. A client - * asking for 1000 records and silently receiving 1 is the worst of those. - * * A parameter supplied more than once arrives from Express as an Array. There is no single correct * reading of '?limit=100&limit=200', so it is a client mistake rather than something to guess at. * - * '?limit[a]=5' cannot be caught here. Under Express's default 'simple' query parser the key never - * reaches the server, so it is indistinguishable from a request that omitted the parameter. - * - * An already-integral Number is taken as-is. 'req.query' never holds one, but a non-Express caller - * should not get a 400 complaining that 50 is not a whole number. - * - * A digit string too large for a Number is saturated to 'Number.MAX_SAFE_INTEGER' rather than - * rejected. It is a whole number, so the caller's maximum is what should decide it: a 'limit' - * clamps the way the contract promises, and a 'skip' gets the ceiling message naming the maximum - * instead of one claiming 400 digits are not a whole number. - * * @param raw The raw value from 'req.query', or undefined when the parameter was omitted. * @param name The parameter name, used in the error message. * @param fallback The value to use when the parameter was omitted. @@ -86,10 +61,6 @@ function readWholeNumberParam(raw, name, fallback, min) { const parsed = typeof raw === "number" ? raw : typeof raw === "string" && /^\d+$/.test(raw) ? Number.parseInt(raw, 10) : NaN - // A digit string past Number's range parses to Infinity. It is still a whole number, just a - // larger one than any maximum, so it belongs in the caller's clamp and ceiling handling rather - // than here. Rejecting it would tell a client that '9'.repeat(400) is not a whole number, and - // would reject a 'limit' the contract promises to clamp. if (parsed === Infinity) return Number.MAX_SAFE_INTEGER if (!Number.isInteger(parsed) || parsed < min) { const bound = min > 0 ? `greater than 0` : `0 or greater` @@ -103,20 +74,15 @@ function readWholeNumberParam(raw, name, fallback, min) { /** * Resolve the 'limit' and 'skip' URL parameters for a paged endpoint, and report what was applied. - * * An unreadable value is rejected with a 400 instead of being guessed at. * * The two maximums are not treated alike, because being over them does not mean the same thing. * A 'limit' above its maximum is clamped, which is conventional for a page size and leaves the * response readable. A 'skip' above its maximum is rejected, because clamping it would serve the - * page at the maximum over and over: a client advancing 'skip' and stopping on an empty page would - * never terminate, and would accumulate the same records on every pass. + * page at the maximum over and over. * * The applied values and both maximums are reported in the response headers, so a client can tell * a truncated page from a genuine final one and can configure itself from any single response. - * The maximums are reported before either parameter is read, so a rejection carries them too: the - * 400 for an over-deep 'skip' is the one response a client most needs the ceiling from, and it is - * what lets a paged walk tell that boundary apart from every other 400 it could receive. * * @param query The Express 'req.query' object. * @param res The Express response, so the applied values can be reported. Optional. Anything @@ -131,13 +97,8 @@ function getPagination(query = {}, res = null, defaultLimit = 100) { const limitMax = resolveQueryCap("MAX_QUERY_LIMIT", DEFAULT_MAX_QUERY_LIMIT) const skipMax = resolveQueryCap("MAX_QUERY_SKIP", DEFAULT_MAX_QUERY_SKIP) // 'res' is only ever used to report headers, so anything that cannot report is treated as an - // omitted response rather than an error. 'res?.set(...)' would throw a TypeError on the older - // getPagination(query, defaultLimit) shape, and a caller asking for a page would get a 500 - // naming optional chaining instead of the page it asked for. + // omitted response rather than an error. const report = typeof res?.set === "function" ? (headers) => res.set(headers) : () => undefined - // Both ceilings are known before either parameter is read, so they are reported before anything - // can throw. A client that gets a 400 back can then read the boundary it hit off the same - // response rather than parsing it out of the message. report({ "Pagination-Limit-Max": String(limitMax), "Pagination-Skip-Max": String(skipMax) @@ -146,9 +107,6 @@ function getPagination(query = {}, res = null, defaultLimit = 100) { const limit = Math.min(readWholeNumberParam(query.limit, "limit", safeDefaultLimit, 1), limitMax) const skip = readWholeNumberParam(query.skip, "skip", 0, 0) if (skip > skipMax) { - // Echo the raw value, not the parsed one. A digit string long enough to lose precision - // parses to a different number than the client sent, and a message quoting a value nobody - // asked for is hard to match back to the request that caused it. throw utils.createExpressError({ message: `The 'skip' URL parameter of ${String(query.skip).slice(0, PARAM_ECHO_MAX)} is beyond the maximum of ${skipMax}. Reading deeper than that is not supported, because every page past it would repeat the one at the maximum. Narrow the query so the records you want fall within the first ${skipMax} results.`, status: 400 diff --git a/public/API.html b/public/API.html index 01bc08e2..9db1f369 100644 --- a/public/API.html +++ b/public/API.html @@ -586,7 +586,7 @@

    Custom Query

    Pagination parameters

    - limit and skip are URL parameters shared by Custom Query, Text Search, and Phrase Search. Both must be whole numbers written in plain decimal digits. Anything else is a 400 rather than a guess, because guessing is how a client asking for ?limit=1e3 used to receive a single record and report a completed walk. + limit and skip are URL parameters shared by Custom Query, Text Search, and Phrase Search. Both must be whole numbers written in plain decimal digits. Anything else is a 400.

    @@ -612,18 +612,11 @@

    Pagination parameters

    -

    - Rejected with a 400: ?limit=abc, ?limit=10abc, ?limit=1e3, ?limit=0x10, ?limit=250.7, ?limit=0, ?limit=-5, ?limit=, and the same forms of skip. Supplying either parameter more than once, as in ?limit=100&limit=200, is also a 400 — there is no correct reading of it. -

    -

    One form cannot be caught. ?limit[a]=5 never reaches the server as a limit at all, so it is indistinguishable from leaving the parameter out and you will silently get the default.

    -

    - The two maximums are not treated alike, because being over them does not mean the same thing. An over-maximum limit is a page size RERUM can honour in part, so it is reduced. An over-maximum skip has no honest reading at all — every page past it would repeat the page at the maximum — so it is refused. If you are reaching that depth, narrow the query rather than paging further. -

    Every paged response reports what was applied and what the maximums are, so a client can configure itself from any single response and tell a truncated page from a genuine final one. These headers are readable cross-origin.

    - Pagination-Limit-Max and Pagination-Skip-Max are reported on rejections too, including the 400 for a skip past the maximum. That is what lets a paged walk recognise the depth boundary and stop cleanly, instead of treating every 400 as the end of the results. Pagination-Limit and Pagination-Skip appear only where a page was actually served. + Pagination-Limit-Max and Pagination-Skip-Max are reported on rejections too, including the 400 for a skip past the maximum. That is what lets a paged walk recognise the depth boundary and stop cleanly. 

     

From a469f8930d756ae8d747581f4117b067da6fb5d8 Mon Sep 17 00:00:00 2001
From: Bryan Haberberger 
Date: Fri, 11 Sep 2026 12:15:45 -0500
Subject: [PATCH 9/9] less tests

---
 __tests__/utils.test.js         | 25 +++++-----------------
 routes/__tests__/query.test.js  | 38 +++++++++------------------------
 routes/__tests__/search.test.js | 14 +-----------
 3 files changed, 16 insertions(+), 61 deletions(-)

diff --git a/__tests__/utils.test.js b/__tests__/utils.test.js
index 8cb4506d..6decbc09 100644
--- a/__tests__/utils.test.js
+++ b/__tests__/utils.test.js
@@ -353,7 +353,7 @@ describe('controllers/utils.js getPagination', () => {
   })
 
   it('rejects a limit that is not a whole number greater than 0', () => {
-    for (const limit of ['abc', '10abc', '1e3', '0x10', '250.7', '-5', '', '0']) {
+    for (const limit of ['abc', '10abc', '1e3', '0x10', '250.7', '-5', '']) {
       assertRejects({ limit }, /'limit' URL parameter must be a whole number greater than 0/)
     }
   })
@@ -383,12 +383,6 @@ describe('controllers/utils.js getPagination', () => {
     )
   })
 
-  it('clamps a limit above the maximum instead of rejecting it', () => {
-    const { limit } = getPagination({ limit: String(Number.MAX_SAFE_INTEGER) })
-    const { 'Pagination-Limit-Max': max } = capturedHeadersFor({})
-    assert.strictEqual(limit, Number(max))
-  })
-
   it('clamps a limit too large for a Number rather than calling it not a whole number', () => {
     // Past about 309 digits Number.parseInt returns Infinity.  Rejected there, a digit-only value
     // would get a 400 saying it is not a whole number, and a limit the contract promises to clamp
@@ -403,13 +397,8 @@ describe('controllers/utils.js getPagination', () => {
 
   it('rejects a skip above the maximum rather than serving the same page forever', () => {
     // Clamping it would hand back the page at the maximum on every request past it.  A client
-    // advancing skip and stopping on an empty page would never terminate.
-    const max = Number(capturedHeadersFor({})['Pagination-Skip-Max'])
-    assertRejects({ skip: String(max + 1) }, /beyond the maximum/)
-    assertRejects({ skip: String(max + 50000) }, /beyond the maximum/)
-  })
-
-  it('names the configured maximum in the rejection, so a client can act on it', () => {
+    // advancing skip and stopping on an empty page would never terminate.  The rejection names the
+    // configured maximum, so a client can act on it, and the maximum itself is still readable.
     const original = process.env.MAX_QUERY_SKIP
     try {
       process.env.MAX_QUERY_SKIP = '2500'
@@ -421,11 +410,6 @@ describe('controllers/utils.js getPagination', () => {
     }
   })
 
-  it('accepts a skip exactly at the maximum', () => {
-    const max = Number(capturedHeadersFor({})['Pagination-Skip-Max'])
-    assert.strictEqual(getPagination({ skip: String(max) }).skip, max)
-  })
-
   it('reports the applied values and the maximums on every paged response', () => {
     const headers = capturedHeadersFor({ limit: '25', skip: '10' })
     assert.strictEqual(headers['Pagination-Limit'], '25')
@@ -476,7 +460,8 @@ describe('controllers/utils.js getPagination', () => {
   it('reports the ceilings on a rejection, so a client can recover from the 400 it just got', () => {
     // The 400 for an over-deep skip is the response a client most needs the ceiling from, and it
     // is what lets a paged walk tell that boundary apart from every other 400 it could receive.
-    for (const query of [{ limit: 'abc' }, { skip: 'abc' }, { limit: ['100', '200'] }, { skip: '999999999' }]) {
+    // One query per throw site: the value it cannot read, and the skip it can read but will not serve.
+    for (const query of [{ limit: 'abc' }, { skip: '999999999' }]) {
       const headers = capturedHeadersFor(query)
       assert.ok(Number(headers['Pagination-Limit-Max']) > 0, `${JSON.stringify(query)} should still report the limit ceiling`)
       assert.ok(Number(headers['Pagination-Skip-Max']) > 0, `${JSON.stringify(query)} should still report the skip ceiling`)
diff --git a/routes/__tests__/query.test.js b/routes/__tests__/query.test.js
index 03b7c048..b62b6a1c 100644
--- a/routes/__tests__/query.test.js
+++ b/routes/__tests__/query.test.js
@@ -142,16 +142,20 @@ describe('pagination parameters on /query', () => {
       .send({ test: "item" })
   }
 
+  // The rejected forms themselves are covered against getPagination in __tests__/utils.test.js.
+  // What is left to prove here is the wiring: the controller reads req.query, and the 400 it
+  // throws reaches the client as a 400 rather than as an unhandled error.
   it("rejects a limit or skip it cannot read exactly", async () => {
-    for (const queryString of ["?limit=abc", "?limit=1e3", "?limit=0", "?limit=-5", "?limit=250.7", "?limit=", "?skip=abc", "?skip=2.9", "?skip=-5"]) {
+    for (const queryString of ["?limit=abc", "?skip=2.9"]) {
       const response = await post(queryString)
       assert.strictEqual(response.statusCode, 400, `${queryString} should be a 400`)
     }
   })
 
+  // Asserted over HTTP rather than only against getPagination, because this is what proves
+  // Express really does hand a repeated parameter over as the Array that getPagination rejects.
   it("rejects a repeated limit rather than taking one of the two values", async () => {
     assert.strictEqual((await post("?limit=100&limit=200")).statusCode, 400)
-    assert.strictEqual((await post("?limit=200&limit=100")).statusCode, 400)
   })
 
   it("reports the applied limit and skip, and the maximums, on a paged response", async () => {
@@ -163,13 +167,7 @@ describe('pagination parameters on /query', () => {
     assert.ok(Number(response.headers['pagination-skip-max']) > 0)
   })
 
-  it("reports the clamp when the limit asked for is above the maximum", async () => {
-    const response = await post("?limit=999999")
-    assert.strictEqual(response.statusCode, 200)
-    assert.strictEqual(response.headers['pagination-limit'], response.headers['pagination-limit-max'])
-  })
-
-  it("applies the reported limit and skip to the database cursor", async () => {
+  it("applies the reported limit and skip to the database cursor, over a deterministic order", async () => {
     const applied = {}
     db.find.mockReturnValueOnce(recordingCursor([mockDoc], applied))
     const response = await request(pagedTester)
@@ -179,6 +177,8 @@ describe('pagination parameters on /query', () => {
 
     assert.strictEqual(applied.limit, 7)
     assert.strictEqual(applied.skip, 3)
+    // A skip offset means nothing without a stable order to count into.
+    assert.deepStrictEqual(applied.sort, { _id: 1 })
     assert.strictEqual(response.headers['pagination-limit'], '7')
     assert.strictEqual(response.headers['pagination-skip'], '3')
   })
@@ -192,31 +192,13 @@ describe('pagination parameters on /query', () => {
     const response = await post(`?skip=${skipMax + 1}`)
     assert.strictEqual(response.statusCode, 400)
     assert.match(response.text, new RegExp(`beyond the maximum of ${skipMax}`))
-  })
-
-  it("reports the ceilings on the skip rejection, so a walk can tell that boundary from any other 400", async () => {
-    const paged = await post("?limit=2&skip=0")
-    const skipMax = Number(paged.headers['pagination-skip-max'])
-
-    const response = await post(`?skip=${skipMax + 1}`)
-    assert.strictEqual(response.statusCode, 400)
+    // The ceilings still come back, so a paged walk can tell this boundary from any other 400.
     assert.strictEqual(response.headers['pagination-skip-max'], String(skipMax))
     assert.ok(Number(response.headers['pagination-limit-max']) > 0)
     assert.strictEqual(response.headers['pagination-limit'], undefined, 'no page was served')
     assert.strictEqual(response.headers['pagination-skip'], undefined, 'no page was served')
   })
 
-  it("pages over a deterministic order, because a skip offset means nothing without one", async () => {
-    const applied = {}
-    db.find.mockReturnValueOnce(recordingCursor([mockDoc], applied))
-    await request(pagedTester)
-      .post("/query?limit=7&skip=3")
-      .set("Content-Type", "application/json")
-      .send({ test: "item" })
-
-    assert.deepStrictEqual(applied.sort, { _id: 1 })
-  })
-
   it("reports no page on the empty body 400, because none was served", async () => {
     const response = await request(pagedTester)
       .post("/query?limit=25&skip=5")
diff --git a/routes/__tests__/search.test.js b/routes/__tests__/search.test.js
index f939e966..b853189d 100644
--- a/routes/__tests__/search.test.js
+++ b/routes/__tests__/search.test.js
@@ -141,18 +141,6 @@ describe('search controllers', () => {
     assert.deepStrictEqual(idsOf(response), ['presi2-best', 'presi3-weak', 'presi3-weaker'])
   })
 
-  it("searchAsPhrase ranks across both indexes too", async () => {
-    mockBranchResults([scoredDoc('presi3-weak', 2.45)], [scoredDoc('presi2-best', 6.95)])
-
-    const response = await request(routeTester)
-      .post('/search/phrase')
-      .set('Content-Type', 'text/plain')
-      .send('exact phrase')
-
-    assert.strictEqual(response.statusCode, 200)
-    assert.deepStrictEqual(idsOf(response), ['presi2-best', 'presi3-weak'])
-  })
-
   // The point of the ranking, for this endpoint: 'limit' and 'skip' slice the merged order, so a
   // merge that does not rank hands back a window of the wrong records rather than a wrong order.
   it("pages the score order, so skip walks best-first across both indexes", async () => {
@@ -185,7 +173,7 @@ describe('search pagination parameters', () => {
   }
 
   it("searchAsWords rejects a limit or skip it cannot read exactly", async () => {
-    for (const queryString of ["?limit=abc", "?limit=1e3", "?limit=0", "?skip=-5", "?limit=100&limit=200"]) {
+    for (const queryString of ["?limit=abc", "?skip=-5"]) {
       const response = await searchFor('/search', queryString)
       assert.strictEqual(response.statusCode, 400, `${queryString} should be a 400`)
     }