pagination: identify when consumers provide the wrong item count

Open
#21 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
5/5
Estimated time
Over a week
Newbie friendliness
35/100
Issue type
Feature
Clarity
Mostly clear
Activity status
Stale
Tech stack
rust
Domain
api

Research direction

Start with RequestContext::page_limit() and ResultsPage::new(), and review related issue #20 for the pagination context they share. Decide the policy for a mismatched item count, then make ResultsPage identify the mismatch and apply that policy consistently for paginated resources.

Written by the indexing model from the issue text.

Description

For paginated resources, there's always a per-page limit that's either the limit requested by the client, the default limit provided by the server, or the maximum limit supported by the server. We provide RequestContext::page_limit() to choose the right value, but of course consumers have to actually use that. It'd be nice if we Dropshot could identify when they've done the wrong thing. That way, consumers wouldn't have to test this behavior themselves for every one of their paginated resources. We could do this if, when calling ResultsPage::new(), the consumer provided the pagination parameters. Then we could call page_limit() directly and compare that to the number of items they provided.

There are a lot of things we could choose to do if the consumer gave us the wrong number of items:

  • panic: this is, after all, a programmer error, and a core file this will give people the best chance of debugging it.
  • return a 500 response: this presumably will have a smaller blast radius for the user than panicking, but still noticeable and potentially debuggable from the log (although probably less so than a core file would be). It's still not great for clients.
  • log a warning and proceed anyway, returning all the items we were given: less noticeable (which is potentially bad), but also doesn't impact the consumer's users
  • log a warning and truncate the results to the intended limit: also least noticeable, probably more correct in that it obeys the limit.

On the one hand, it seems silly to panic or fail a request that we can otherwise process correctly. But allowing these requests to complete defeats the whole purpose of pagination: among the reasons we use pagination are to bound the amount of work an operation does and the amount of time it takes, in turn to facilitate scalability and availability and mitigate DoS. If we get here because a consumer forgets to append "LIMIT N" to a SQL query, and Dropshot is basically silent about it, the user may get by for a long time, only to discover after months when they dig into some pathological performance problem that this has been acting like an unpaginated API (and they've got a DoS vector in their application).

We could make this a tunable policy but I think we may as well start by picking a policy we want for ourselves and doing that. I lean towards panicking or returning a 500.

This is related to #20, which also needs the limit available in ResultsPage::new().

Dominant language
Rust
Stars
1.2k
Forks
104
Avg merge
1d 19h
Merged PRs (30d)
22

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from oxidecomputer/dropshot

All issues in oxidecomputer/dropshot

Similar issues

More Rust issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.