Skip to content

fix: reject page[limit] above max_page_size instead of truncating silently - #1002

Merged
rainhead merged 2 commits into
orcasound:mainfrom
rainhead:fix/992-page-limit-silent-truncation
Aug 4, 2026
Merged

fix: reject page[limit] above max_page_size instead of truncating silently#1002
rainhead merged 2 commits into
orcasound:mainfrom
rainhead:fix/992-page-limit-silent-truncation

Conversation

@rainhead

@rainhead rainhead commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Fixes #992.

What's happening

page[limit]=500 on /api/json/detections returns 251 rows and links.next: null — a page that is both short and claims to be complete. 251 is not data-dependent; it's max_page_size (250) + 1, and it's still exactly 251 today against live (30,457 detections total).

page[limit] rows links.next
250 250 present
251 251 present
252 251 null
500 251 null

Three pieces line up:

  1. Detection's :index action doesn't set max_page_size, so Ash defaults it to 250.
  2. Ash.Actions.Read.limit_offset_pagination/3 issues LIMIT min(requested, max_page_size) + 1 (the +1 is the "is there more?" probe row) → 251 for any request above 250. But to_page/7 then does Enum.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.
  3. AshJsonApi.Serializer.add_next_link/4 has Enum.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]=true doesn't rescue it either: the count branches are checked first, but 0 + 500 >= 30457 is 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 < 251 is false.

Not limited to detections: /api/json/feed_segments reproduces the identical 251/252 boundary. And the same code is present in ash 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 into query.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 a 400 invalid_pagination when page[limit] exceeds the matched action's max_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.Builder wrapper (OrcasiteWeb.JsonApiRouter) around the generated Ash router (now OrcasiteWeb.JsonApiRouter.AshRouter), because AshJsonApi.Router's before_dispatch hook can't reject a request — it calls the route's controller regardless of conn.halted. The plug does no work at all unless page[limit] is present.

2. max_page_size 1000 on Detection's :index and :by_category, matching what Candidate and AudioImage already do.

Notes for review

  • Verified locally against the repo's dev container (Elixir 1.17.3 / OTP 27, same toolchain as CI): full suite is 24 tests, 0 failures, 1 skipped. The two rejection tests were also confirmed to fail with the plug removed — they return 200 with "next":null and an empty page, i.e. the bug — so they're real regression tests rather than vacuous ones.
  • Behavior change: clients currently sending 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.
  • The plug duplicates 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

    • JSON:API requests with pagination limits above the permitted maximum are now rejected with a clear validation error.
    • Requests within the allowed limit, without a limit, or to unknown routes continue to behave as expected.
    • Radio list endpoints now support page sizes up to 1,000.
  • API Improvements

    • JSON schema and OpenAPI documentation remain available through the API.

…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>
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

JSON: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.

Changes

Pagination enforcement

Layer / File(s) Summary
Configure action pagination limits
server/lib/orcasite/radio/detection.ex
Detection index and category actions now set max_page_size to 1000.
Validate limits before Ash routing
server/lib/orcasite_web/plugs/enforce_max_page_size.ex, server/lib/orcasite_web/json_api_router.ex, server/lib/orcasite_web/json_api_router/ash_router.ex
A plug resolves matching actions, compares page[limit] with configured maxima, returns JSON:API 400 errors when exceeded, and delegates valid requests to the Ash router.
Cover pagination response behavior
server/test/orcasite_web/json_api/max_page_size_test.exs
Tests cover excessive, valid, omitted, and unmatched-route requests.

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
Loading

Possibly related PRs

Suggested reviewers: skanderm

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes address #992 by raising Detection max_page_size and rejecting oversize JSON:API page limits with 400 invalid_pagination.
Out of Scope Changes check ✅ Passed The router and plug refactor stays aligned with enforcing max page size for JSON:API pagination and the Detection actions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main behavior change: rejecting oversized page[limit] values instead of silently truncating results.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@rainhead
rainhead marked this pull request as ready for review July 26, 2026 00:08
@rainhead
rainhead requested a review from skanderm as a code owner July 26, 2026 00:08
@rainhead
rainhead requested a review from dthaler July 29, 2026 18:00
@dthaler
dthaler requested a review from paulcretu July 30, 2026 15:29
@rainhead

Copy link
Copy Markdown
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.

@rainhead rainhead closed this Jul 30, 2026
@rainhead rainhead reopened this Jul 30, 2026
@rainhead
rainhead enabled auto-merge (squash) August 4, 2026 16:08
@rainhead
rainhead merged commit 7dd5773 into orcasound:main Aug 4, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

/api/json/detections pagination doesn't work correctly

2 participants