Behavioral Fixes for limit and skip

__tests__/utils.test.js

@@ -327,30 +327,201 @@ 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', '']) {
+      assertRejects({ limit }, /'limit' URL parameter must be a whole number greater than 0/)
+    }
   })
 
-  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 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 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.  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'
+      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('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', () => {
+    // '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 {
+      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('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.
+    // 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`)
+    }
+  })
+
+  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)
+    assert.strictEqual(result.skip, 10)
+    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.
+   *
+   * 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) {
+    const captured = {}
+    try {
+      getPagination(query, { set: (headers) => Object.assign(captured, headers) })
+    } catch (err) {
+      if (err.statusCode !== 400) throw err
+    }
+    return captured
+  }
 })
 
 describe('controllers/utils.js findLeafAnnotationsFor', () => {

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, 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,8 +82,10 @@ 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()
+        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)

controllers/gog.js

@@ -23,8 +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"}
  *
- * TODO? Some sort of limit and skip for large responses?
- *
  * @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) {
@@ -33,7 +31,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, null, 50)
     let err = { message: `` }
     // This request can only be made my Gallery of Glosses production apps.
     if (agentID !== GOG_PROD_AGENT) {
@@ -154,8 +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"}
  *
- * TODO? Some sort of limit and skip for large responses?
- *
  * @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) {
@@ -164,7 +160,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, null, 50)
     let err = { message: `` }
     // This request can only be made my Gallery of Glosses production apps.
     if (agentID !== GOG_PROD_AGENT) {

controllers/history.js

@@ -86,9 +86,10 @@ 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()
+        // 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))

controllers/search.js

@@ -23,7 +23,7 @@ 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 function handles different _id formats:
  * - ObjectId objects with $oid property
  * - String-based _id values
@@ -40,9 +40,7 @@ function mergeSearchResults(results1, results2) {
             merged.push(result)
         }
     }
-
-    // Sort by score descending
-    return merged.sort((a, b) => (b.score || 0) - (a.score || 0))
+    return merged.sort((a, b) => (b.__rerum?.score ?? 0) - (a.__rerum?.score ?? 0))
 }
 
 /**
@@ -271,7 +269,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 +355,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 +433,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 +527,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 +620,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 = [
         {