fix: reject page[limit] above max_page_size instead of truncating silently - #1002
Merged
rainhead merged 2 commits intoAug 4, 2026
Merged
Conversation
…ently
Ash clamps the SQL LIMIT to the action's max_page_size but keeps the
*requested* limit for its "is there more?" bookkeeping: to_page/7 splits
the rows on page_opts[:limit], so an over-large limit yields
{all_rows, []} and more?: false. AshJsonApi then hits
`Enum.count(results) < limit` and emits links.next: null.
The result is a page that is both short and claims to be complete.
On /api/json/detections, page[limit]=500 returns 251 rows
(max_page_size 250, plus Ash's probe row) with no next link, so clients
stop paginating and silently lose data. Passing page[count]=true does
not help — the results-count branch is reached before the count is
consulted. Present in ash 3.30.1 and ash_json_api 1.7.1 as well, so an
upgrade does not fix it.
Add OrcasiteWeb.Plugs.EnforceMaxPageSize, which resolves the JSON:API
route ahead of dispatch and returns a 400 invalid_pagination error when
page[limit] exceeds the matched action's max_page_size. This covers
every index route, not just detections. It runs as a wrapper around the
generated Ash router because AshJsonApi's before_dispatch hook calls the
controller regardless of conn.halted.
Also raise max_page_size to 1000 on Detection's :index and :by_category,
matching Candidate and AudioImage.
Fixes orcasound#992
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughJSON:API routing now enforces action-specific maximum page sizes before dispatch. Detection actions allow limits up to 1000, and tests cover rejected, valid, omitted, and unmatched-route requests. ChangesPagination enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant JsonApiRouter
participant EnforceMaxPageSize
participant AshRouter
Client->>JsonApiRouter: Send request with page[limit]
JsonApiRouter->>EnforceMaxPageSize: Validate requested limit
EnforceMaxPageSize->>AshRouter: Dispatch valid request
AshRouter-->>Client: Return JSON:API response
EnforceMaxPageSize-->>Client: Return 400 invalid_pagination when limit is too large
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
CodeQL and Scorecard workflows were disabled due to repo inactivity. I've reenabled them and will close and reopen this PR to trigger them. |
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.
Fixes #992.
What's happening
page[limit]=500on/api/json/detectionsreturns 251 rows andlinks.next: null— a page that is both short and claims to be complete. 251 is not data-dependent; it'smax_page_size (250) + 1, and it's still exactly 251 today against live (30,457 detections total).page[limit]links.nextThree pieces line up:
Detection's:indexaction doesn't setmax_page_size, so Ash defaults it to 250.Ash.Actions.Read.limit_offset_pagination/3issuesLIMIT min(requested, max_page_size) + 1(the+1is the "is there more?" probe row) → 251 for any request above 250. Butto_page/7then doesEnum.split(data, page_opts[:limit])using the raw requested limit. Splitting 251 rows at 500 gives{251, []}→more?: false, and the probe row is never trimmed.AshJsonApi.Serializer.add_next_link/4hasEnum.count(results) < limit -> next: nil, again against the raw requested limit.Requesting a limit above the cap is silently ignored — no error, no signal in the response.
page[count]=truedoesn't rescue it either: the count branches are checked first, but0 + 500 >= 30457is false, so it falls through to the results-count branch.Requesting exactly 251 "works" only by coincidence — the clamped SQL limit happens to equal the requested limit, so
251 < 251is false.Not limited to detections:
/api/json/feed_segmentsreproduces the identical 251/252 boundary. And the same code is present inash 3.30.1/ash_json_api 1.7.1(current latest; we pin 3.5.34 / 1.4.41), so upgrading does not fix it. Worth filing upstream separately — Ash should either error, or write the clamped limit back intoquery.page[:limit]so both the split and the next-link check use the effective limit.This PR
1.
OrcasiteWeb.Plugs.EnforceMaxPageSize— resolves the JSON:API route ahead of dispatch and returns a400 invalid_paginationwhenpage[limit]exceeds the matched action'smax_page_size. Under-serving a request while claiming completeness is the dangerous failure; erroring is the honest one. This covers every index route, not just detections.It runs as a
Plug.Builderwrapper (OrcasiteWeb.JsonApiRouter) around the generated Ash router (nowOrcasiteWeb.JsonApiRouter.AshRouter), becauseAshJsonApi.Router'sbefore_dispatchhook can't reject a request — it calls the route's controller regardless ofconn.halted. The plug does no work at all unlesspage[limit]is present.2.
max_page_size 1000onDetection's:indexand:by_category, matching whatCandidateandAudioImagealready do.Notes for review
200with"next":nulland an empty page, i.e. the bug — so they're real regression tests rather than vacuous ones.page[limit]above the cap will start getting a 400 instead of a silently truncated page. That's the point, but it is a breaking change for anyone who had adapted to the old behavior. The 1000 ceiling on detections gives most of them headroom.AshJsonApi.Controllers.Router's route resolution. If that ever drifts, the plug degrades to a no-op (falls through, request proceeds) rather than misbehaving.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
API Improvements