Behavioral Fixes for limit and skip - #310
Draft
thehabes wants to merge 2 commits into
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves #301.
getPagination()never rejected anything.clampNonNegativeInt()substituted a fallback for any value it could not parse and silently capped anything above the maximum, so every bad input returned 200 with a page the client did not ask for and could not detect.The worst case was
?limit=1e3.Number.parseInt("1e3", 10)is1, so a client asking for a thousand records received one — and a client that stops on a short page then reports a completed walk of a single object.All of it is fixed in one place.
getPagination()incontrollers/utils.jsis shared by nine call sites, so/query,HEAD /query, all five/search*endpoints, and both/gog/*InManuscriptendpoints change together.What changed
?limit=1e3?limit=10abc?limit=250.7?limit=abc,?limit=0x10,?limit=?limit=0,?limit=-5?limit=100&limit=200?limit=200&limit=100?skip=1e3?skip=2.9?skip=abc,?skip=-5,?skip=?limit=1000?skip=150000Values are now validated as decimal integer strings before they are parsed, because
Number.parseIntguesses. A parameter supplied more than once arrives from Express as an Array, which has no single correct reading, so it is refused rather than resolved by position.The two maximums are deliberately not alike
An over-maximum
limitis clamped. A page size the server can honour in part is conventional to reduce, and rejecting it would be the more breaking choice.An over-maximum
skipis rejected. Clamping it served the page at the maximum on every request past it —skip=100000,skip=100100andskip=250000returned byte-identical bodies — so a client advancingskipand stopping on an empty page never terminated, and accumulated the same records on every pass. There is no honest reading of it, so it is refused with a message naming the configured maximum.Every paged response now reports what was applied
The reported
limitandskipare the values actually applied, which is what lets a client tell a truncated page from a genuine final one. Names are unprefixed to match the existingAnnotations-GatheredandCurrent-Overwritten-Versionheaders rather than introduce a new prefix.Access-Control-Expose-Headersis already*, so browser clients can read them with no CORS change.Environment variable names reconciled
controllers/utils.jsreadRERUM_MAX_QUERY_LIMIT/RERUM_MAX_QUERY_SKIPwhile.envsets the unprefixedMAX_QUERY_LIMIT/MAX_QUERY_SKIP. The configured caps were therefore inert and the hardcoded defaults were what actually ran. The code now reads the unprefixed names, and resolves them per call rather than capturing them at module load — which is why no test could catch the mismatch before, and why there is now one that can.Deployment note
.envis gitignored, so this part is not in the diff and has to be done by hand.MAX_QUERY_SKIPwas bumped10000to100000locally so that honoring.envdid not change the effective cap. Any deployment whose.envstill readsMAX_QUERY_SKIP=10000will see its skip cap drop from 100000 to 10000 the moment this merges, because the name mismatch that made that value inert is now fixed. Check the boxes before rolling forward.Breaking
Requests that return 200 today return 400. Land on dev first and give known client maintainers notice.
The
skiprejection is the part most likely to be hit by something real, which is the point of it.{"@type":"oa:Annotation"}still returns a document atskip=100000on production, so the boundary is reachable by live traffic.The published
pagedQueryexample inpublic/API.htmlwas the non-terminating shape, so it is rewritten: it advances by the page size the server reports applying rather than byresults.length, stops on a short page, and lets the boundary 400 end a walk that runs too deep.Out of scope
limitorskipat all today and no drift guard covers them. Left to The pagination contract is undocumented, misdocumented, and absent from the OpenAPI contract #305.rest.messengerappends "The body of your request was invalid… make sure it is valid JSON" to every 400, which reads oddly on a query-string error. Narrowing it means touching the shared error renderer for all 400s. The pagination messages are written to stand on their own; worth its own issue.HEAD /queryanswering an empty page with 404 (HEAD /queryanswers an empty page with 404 wherePOST /queryanswers200 []#304) is untouched. It is blocked on HEAD history - since - query #96, and the reading there is that the.head()handler should be removed from/queryrather than repaired — note that/since/:_idand/history/:_idare.get()routes where HEAD is valid and should stay.