Pagination

[davepacheco]

This PR is a very rough draft implementation of pagination for Dropshot. Briefly: pagination is the API design pattern by which API endpoints (e.g., HTTP resources) that list large collections of objects limit the number of items you can list at a time, forcing clients to make several requests to fetch the complete list. There are lots of good reasons to do this. These are documentation in dropshot/src/pagination.rs under the heading "Background: Why paginate HTTP APIs?".

This has taken a while and it's a lot more complex than I expected. There's definitely more work to be done, but before I go much deeper, I'd like some feedback on the Rust API. Whether it's a thumbs-up, thumbs-down, or concrete suggestions for improvement, I'd like to know what you think!

---

An important opening question is: why put any of this in Dropshot at all? As far as I can tell, other popular Rust frameworks don't have pagination as a first-class thing. The current Oxide control plane prototype implements paginated resources without any specific support from Dropshot. But this leads to a few issues:

• There's nothing to ensure (or even help people ensure) that they paginte at all. In an ideal world, any time somebody wanted to return a collection of things, they'd reach for a Dropshot abstraction that makes it paginated by default. This is necessary for building scalable, highly-available APIs, and getting this wrong early can require breaking API changes to fix, so it's helpful if we can guide people early.
• There's nothing to ensure (or even help people ensure) that the paginated endpoints look the same in terms of query parameters and returned structures. Again, getting this wrong either becomes cruft a client has to know about or cruft the server has to deal with for compatibility reasons (or both).
• There's nothing to guarantee that the various manually-paginated endpoints behave correctly with respect to pagination. For example, there'd be nothing to prevent one of them from forgetting to check "limit" and returning some huge number of results. For a consumer that wants to make sure this works correctly, they'd have to test this behavior for every single paginated API.

Now, at the REST level, there are many different ways of doing pagination. These too are discussed in the block comment in dropshot/src/pagination.rs, under the heading "Patterns for pagination." There's also a range of complexity needed by clients: the simplest case is a collection of items with a single field you can sort by (e.g., "id", a unique integer, or "name", a unique string). A more complicated case would be a collection of items with several combinations (e.g., "name", a unique string, in either ascending or descending order; or a combination of "mtime" and "name"). I think these more complicated cases are likely more common than they sound.

---

That background aside, here's how I'd probably go about looking at this. In this PR I've implemented two pagination APIs: a general one and a simpler "marker" one.

• Start with examples/pagination-marker.rs. This uses the "marker" interface to paginate a collection of Projects.
• Check out examples/pagination-multi.rs. This is an example of the more complicated case. The documentation at the top of this file also goes into detail about how everything looks to an HTTP client (e.g., the query params and responses).
• Check out examples/pagination-multi-resources.rs. This is a more realistic example of an API with multiple resources with similar pagination endpoints. This use-case exposed some ugliness in the previous revisions of this interface.
• Check out examples/pagination-basic.rs (again, sorry for the dubious naming here). This uses the general API to paginate in the same way as the marker example does. I included this to show what the simple one is doing under the hood.

If you want, check out dropshot/src/pagination.rs to see how most of it's implemented. There are also some bits in dropshot/src/handler.rs.

Here are my own thoughts about this right now:

• I think this works and does what I want, and I think it's close to a reasonable level of boilerplate / ergonomics. Can we do better?
• Does the pattern in examples/pagination-multi-resources.rs make sense? Should callers use a procedural macro where we currently define a macro rule?
• of particular note for @ahl : the openapi generation does not seem to include any of the information about these query parameters. I haven't dug into why not yet.

I'm interested in any and all feedback, especially on those questions. I really want this to be something easy for people to reach for whenever they build an API that returns a list of things so that pagination becomes the default. But I also want to ensure as much as possible that the resulting APIs are correct, robust, use scalable patterns, etc.

---

History and status:

2020-07-16 (commit 1791d7a): initial PR

2020-07-17 (commit 1e91d3c):

• renamed "simple" interface to "marker" and simplified the interface a bunch. Renamed the examples to match.
• removed page_selector_for from the PaginatedResource trait. Instead, callers pass a callback where this would go. This seems much more convenient for both simple and more complicated consumers.

2020-07-17 (commit f4d5e85): Remove PaginatedResource altogether. This removed some boilerplate from the more complicated examples.

2020-07-31 (commit fa8865a): Lots of changes, largely to simplify the interface while also making it more flexible. See my comment on the changes.

2020-08-05 (commit 9d6347b): Added automated testing and removed Incoming pattern from examples.

2020-08-05 (commit 7ba2ee2): Sync'ed up with "master" branch (force push, unfortunately), added an openapi test, and fixed style.

[davepacheco]

Regarding the openapi issue that I mentioned: take the example of "pagination-multi.rs":

• the first page in any scan has an optional "list_mode" querystring parameter which may have values "by-name-ascending", "by-name-descending", "by-mtime-ascending", and "by-mtime-descending". In Rust, this corresponds to the ProjectScanMode type, which is an enum.
• subsequent pages in the scan have a required "page_token" querystring parameter. This is a string. In Rust, this corresponds to PageToken, though this struct is defined with #[serde(try_from = "String")], which is how we deserialize a struct from a String.
• all pages in the scan have an optional "limit" parameter of type NonZeroU64

I modified the example to dump the openapi spec:

diff --git a/dropshot/examples/pagination-multi.rs b/dropshot/examples/pagination-multi.rs
index 1feaf21..5bef672 100644
--- a/dropshot/examples/pagination-multi.rs
+++ b/dropshot/examples/pagination-multi.rs
@@ -346,6 +346,8 @@ async fn main() -> Result<(), String> {
         .map_err(|error| format!("failed to create logger: {}", error))?;
     let mut api = ApiDescription::new();
     api.register(example_list_projects).unwrap();
+    api.print_openapi(&String::from("example"), None, None, None, None, None,
+        None, None, &String::from("1.0.0"));
     let mut server = HttpServer::new(&config_dropshot, api, ctx, &log)
         .map_err(|error| format!("failed to create server: {}", error))?;
     let server_task = server.run();

and here's what it looks like:

{
  "openapi": "3.0.3",
  "info": {
    "title": "example",
    "contact": {},
    "version": "1.0.0"
  },
  "paths": {
    "/projects": {
      "get": {
        "description": "API endpoint for listing projects\n\n This implementation stores all the projects in a BTreeMap, which makes it\n very easy to fetch a particular range of items based on the key.",
        "operationId": "example_list_projects",
        "parameters": [
          {
            "in": "query",
            "name": "page_params",
            "description": "Specifies either how the client wants to begin a new scan or how to\n resume a previous scan.  This field is flattened by serde, so see the\n variants of [`WhichPage`] to see which fields actually show up in the\n serialized form.",
            "required": true,
            "schema": {
              "type": "string"
            },
            "allowReserved": true,
            "style": "form"
          },
          {
            "in": "query",
            "name": "limit",
            "description": "Optional client-requested limit on page size.  Consumers should use\n [`RequestContext::page_limit()`] to access this value.",
            "required": true,
            "schema": {
              "type": "string"
            },
            "allowReserved": true,
            "style": "form"
          }
        ],
        "responses": {
          "200": {
            "description": "TODO: placeholder",
            "content": {
              "application/json": {
                "schema": {
                  "description": "Client's view of a page of results from a paginated API",
                  "type": "object",
                  "properties": {
                    "items": {
                      "description": "list of items on this page of results",
                      "type": "array",
                      "items": {
                        "$ref": "#/components/schemas/Project"
                      }
                    },
                    "next_page": {
                      "description": "token to be used in pagination params for the next page of results",
                      "type": "string"
                    }
                  },
                  "required": [
                    "items"
                  ]
                }
              }
            }
          }
        }
      }
    }
  },
  "components": {
    "schemas": {
      "Project": {
        "description": "Item returned by our paginated endpoint\n\nLike anything returned by Dropshot, we must implement `JsonSchema` and `Serialize`.  We also implement `Clone` to simplify the example.",
        "type": "object",
        "properties": {
          "mtime": {
            "type": "string",
            "format": "date-time"
          },
          "name": {
            "type": "string"
          }
        },
        "required": [
          "mtime",
          "name"
        ]
      }
    }
  }
}

There are a couple of problems here:

• "limit" should probably be some numeric type instead of "string". Ideally, it would say it's a positive integer. I'm not sure if that's because I used NonZeroU64 instead of u64?
• both "limit" and the other query parameters should be optional
• instead of either "list_mode" or "page_token", we have "page_params". This is probably because of the way I've implemented PaginationParams: page_params is the right field in the Rust struct, but it's got #[serde(flatten)], so it's not present on serialization/deserialization.

[ahl]

I'm pretty sure the openapi oddness is related to some shortcuts I took in the parameter processing code. I think we're likely falling into this case: https://github.com/oxidecomputer/dropshot/blob/master/dropshot_endpoint/src/lib.rs#L186

[ahl]

This is beautiful.

Thinking about ways we could potentially make the interface simpler:

1. Maybe have first_page and next_page closures passed to some central interface rather than the endpoint handling it itself?
2. push the limit() call on the iterator into the new_with_paginator() method?

[davepacheco]

Thanks for taking a look!

[davepacheco]

I just pushed a pretty big set of changes based on the feedback and more ideas I had to simplify the interface. I don't think there's anything controversial here.

Below is a rough summary of changes between commits f4d5e85..fa8865a

On the input side:

• Previously, the only parameter consumers could accept for the first request in a scan was the "list_mode", which was expected to be an enum of things like "name-ascending". Adam pointed out that you probably need to be able to apply filters here as well. So ScanMode became ScanParams, and it's expected that consumers would use a structure type for this rather than an enum.
• I streamlined the PaginationParams / WhichPage interface a bit. WhichPage::{FirstPage,NextPage} are now {First, Next}, and they're both tuple variants. The consumer doesn't need to know about "page_token" any more. (Internally, these structures are now decoupled from exactly what comes in from the querystring, so we have some flexibility to structure them however we want.)
• I got rid of the "Marker" interface altogether. See examples/pagination-basic.rs for the simplest case now. This eliminated the awkwardness around HttpResponseOkPage::new_with_paginator/new_with_marker.
• I added a test case where the consumer accepts non-pagination-related query parameters (e.g., "debug=true"). I expected this to "just work" by embedding PaginationParams inside a bigger structure and using #[serde(flatten)], but this failed due to using #[serde(flatten)] breaks deserializing nox/serde_urlencoded#33, which is really Internal buffering disrupts format-specific deserialization features serde-rs/serde#1183. After an hour or so of panic thinking the whole design was sunk, I found that a consumer can simply have their handler function accept two different Query arguments. (This will parse the query string twice, but that's arguably the most correct thing to do anyway.)

On the output side:

• I realized that consumers should be able to return their own structure that might include the list of returned items and a page token. (For example, they might want to include other metadata like the total number of results.) That's when I realized that we shouldn't conflate the HttpResponseTyped type with the actual data returned. So I got rid of HttpResponseOkPage altogether and moved that functionality to a new type called ResultsPage. Now, you return an HttpResponseOkObject<ResultsPage<T>>. If you want, you can embed ResultsPage in your own type that has more stuff around it (and I've tested that). This also eliminated the awkwardness that HttpResponseOkPage was the only one of the HttpResponseTyped structs that had a constructor.

Other stuff:

• I've added rustdoc comments for all the new stuff.
• I prototyped applying this to oxide-api-prototype. It's a lot of boilerplate to support three different kinds of scan, but I'm otherwise happy with it (and critically the boilerplate is O(number of types of scan), not O(number of endpoints that use each type of scan)). You can see this here.

What's still remaining:

• I want to write a complete battery of automated tests for all this.
• I need to sync up with other changes in master.
• There's a bunch more to wrap up on the oxide-api-prototype change, but none of it feels like a blocker to me.

There's other stuff I'm not planning to do now, but I will file issues about:

• We may want to provide a version of ResultsPage that includes the total result set size. This should be a separate type so that the OpenAPI spec can reflect whether it's present or not.
• We still need to fix the openapi spec output for the query parameters.
• The "next_page" token currently shows up in `ResultsPage" as long as there are any results on the page. That's wrong -- it should only show up if there's a complete list of results. Unfortunately, we don't currently have access to the limit that was used here. This would be good to flesh out, but it doesn't seem urgent and I'm getting itchy to get this integrated.
• Similarly, it would be good to have some policy around what to do when the consumer provides more results than we expected and then enforce that policy. Example policies could include: panic, return a 500 error, log a warning, silently truncate the result set, allow it anyway. This is blocked on the same thing: having the limit in the result set.
• We should provide some way for consumers to test the format of the page token. This is an interface that they might not realize because it doesn't appear in the OpenAPI spec.
• The max and default page sizes should be really configurable, instead of hardcoded values.

@ahl do you want to take another look?

[ahl]

I'm pretty sure the openapi oddness is related to some shortcuts I took in the parameter processing code. I think we're likely falling into this case: https://github.com/oxidecomputer/dropshot/blob/master/dropshot_endpoint/src/lib.rs#L186

I'll be interested to know if post-merging with #16 this changes.

[ahl]

I realized that consumers should be able to return their own structure that might include the list of returned items and a page token. (For example, they might want to include other metadata like the total number of results.) That's when I realized that we shouldn't conflate the HttpResponseTyped type with the actual data returned. So I got rid of HttpResponseOkPage altogether and moved that functionality to a new type called ResultsPage. Now, you return an HttpResponseOkObject<ResultsPage>. If you want, you can embed ResultsPage in your own type that has more stuff around it (and I've tested that). This also eliminated the awkwardness that HttpResponseOkPage was the only one of the HttpResponseTyped structs that had a constructor.

This makes sense, but I had been hoping to use that special return type as an easy way to statically check if we were dealing with a paginated endpoint. I was hoping to add an OpenAPI extension to the spec output and then modify a language binding generator (e.g. for JS and Rust) to automatically generate iterators.

Statically, I think we've moved to these endpoints being a little harder to identify. We could still look at the type parameter for HttpResponseOkObject but ResultsPage could be embedded. I guess we can look for Query<PaginationParams<_,_>>.

I think it might be nice to be able to provide some sort of compile-time check for these paginated endpoints to confirm that inputs and outputs jived.

To that end: This is off-the-cuff, so may be full of holes. I'm not 100% where this would get us, but I want to write it down so I don't lose it:

1. Make ResultsPage parameterized on ResultsPage<ItemType, PageSelector>
2. Have a custom serializer for the token field that would do the stringificating
3. Change ResultsPage::new to look like this

impl<ItemType, PageSelector> ResultsPage<ItemType, PageSelector> {
    pub fn new<ScanParams>(
        items: Vec<ItemType>,
        scan_params: &ScanParams,
    ) -> Result<ResultsPage<ItemType, PageSelector>, HttpError>
    where
        PageSelector: From<(ItemType, ScanParams>
    {

There seems to be a common pattern of the ExScanParamsIncoming turning into ExScanParams (with the difference being Optional<..>). Is there something clever we can do to obviate the need for that?

[ahl]

I see why this works, but this strikes me as odd. I imagined that each of Query, Path and Json (body) would appear at most once. I get why we're doing it, but 1. I wonder if we want to support this and 2. I think we'd have issues with more than 3 Extractors at present.

[davepacheco]

I can definitely see how it's a little strange. On the other hand, it would seem a little strange to me if it didn't work -- that would imply that extractors somehow mutate the request or are otherwise intertwined instead of being self-contained things that construct a higher-level view of something in the request.

With the serde bug, I don't know another way to support taking non-pagination query parameters, which seems pretty useful.

I thought about whether we can avoid parsing the querystring twice. Unfortunately, we don't know until we've parsed the querystring whether it's the WhichPage::First case, in which case all of the query parameters should be deserialized into the user's struct. But once we've parsed it to determine we're in the WhichPage::First case, we don't have a way to take the representation we've got and parse it into the user's type. The only way we have to get to the user's struct is to parse the querystring again. Alternatively, we could define an intermediate representation (e.g., BTreeMap<String, String>) and define a serde Deserializer for that representation that can reconstitute the user's type, but this doesn't feel worth the work right now just to avoid a second parse.

[ahl]

fingers crossed!

[davepacheco]

There seems to be a common pattern of the ExScanParamsIncoming turning into ExScanParams (with the difference being Optional<..>). Is there something clever we can do to obviate the need for that?

Consumers can do a few things:

1. Just use the Incoming version (they don't have to call it Incoming in that case) and deal with the fact that every where it appears, they have this Option that they need to either deal with or make assumptions about. This is kind of annoying because there are a bunch of places you use these ScanParams and it's a lot cleaner and more robust if you've filled in the default once and you know you have a valid value everywhere else.

2. Do as I did in this example and define a separate type for Incoming, convert it to the non-optional type once (filling in defaults at that point), and use that everywhere else. This is kind of annoying for the extra type and the conversion.

3. Implement Default for their ScanParams type or whatever members of it are optional. This works fine but I'm a little skeptical of having a general Default implementation if we only want to apply default values when deserializing. I think that's the only case where it comes up today, but if it came up in another case, I'd rather force us to think about whether we want Default values or whether the values should be coming from somewhere else explicitly.

4. Use #[serde(default)] to define a function that produces a default value when deseriailizing. I did this in the oxide-api-prototype integration here:
   https://github.com/davepacheco/oxide-api-prototype/blob/58ddf0a243555d169c66cf5e42d356e9e86f547f/src/http_pagination.rs#L69-L70

   This is kind of annoying for the extra attribute and function (but no moreso than an implementation of Default or the extra Incoming type).

They basically all seem fine and also imperfect. I think (1) opens the door to various runtime bugs. (2), (3), and (4) all get the job done, at the cost of some extra boilerplate, though that code has to exist somewhere.

I like (4) best, and I can do that in the example if that seems better.

[davepacheco]

Statically, I think we've moved to these endpoints being a little harder to identify.

I think that's right. The only thing we'd know about them now is that they take a Query<PaginationParams<...>> (or, if that serde bug gets fixed, a Query<X> where X somehow includes a PaginationParams) and they return a Y that somehow includes a ResultsPage. It would be nice if this were easier to identify. From talking offline, maybe we can defer this for now.

I think it might be nice to be able to provide some sort of compile-time check for these paginated endpoints to confirm that inputs and outputs jived.

Yeah, I'm interested in this as well. I tried a bunch of things (described below for future context) and went in a bunch of circles as I tried to validate them against my design goals. There may well be a way to do this with less boilerplate and/or better compile-time checking, and we should definitely iterate as we find better ways to do it. From talking offline, we'll defer this for now.

---

The rest of this describes my goals and some of the things I tried for reference if we revisit this. Some goals:

• support for multiple consumer-defined ways to paginate a resource (e.g., different sort orders, possibly on different combinations of fields)
• arbitrary consumer-defined first-page parameters (e.g., for filtering)
• arbitrary consumer-defined next-page parameters to support whatever approach the consumer wants for the marker
• as little boilerplate as possible for the simple case (no scan options, paginated by one marker field)
• as little boilerplate as possible for each new resource that shares pagination behavior with an existing one (e.g., if you already have N resources paginated by "name" in ascending order with no filtering params, you shouldn't need much boilerplate -- if any -- for another one that works that way too)
• an opaque token for the consumer that encapsulates whatever the server needs for the next page. (This was modeled loosely after the Google pagination guidelines linked in support for paginated endpoints #9, after surveying several cloud infrastructure APIs)
• support for consuming other querystring parameters not related to pagination (e.g., "debug=true" or something)
• support for returning other output in addition to the results (e.g., to include other metadata with the results, like maybe the total count of items or a request id)

My early intuition was that the page of results could be parametrized by the ScanMode and PageSelector types so that you could tell statically if, say, an endpoint told Dropshot to generate a page selector that was different than the one it accepted on input. Similarly, it seems like there's some kind of checking you could do to make sure the page selector makes sense for the scan mode.

The very first version I had looked similar to what you describe:
https://github.com/davepacheco/dropshot/blob/1b7813e1df4cc23592ecde6bcf46016d4aaa3d27/dropshot/src/handler.rs#L862-L882

This was before the interface supported multiple different scan modes, so for<'a> &'a ItemType: Into<MarkerFields>, is analogous to your PageSelector: From<(ItemType, ScanParams>. One of the next commits removed the MarkerFields (the analog at that time for PageSelector) as a type parameter, and I remember that being important, but I don't remember why :(

I don't think this necessarily helps match up the input and output unless there's something to make sure that the PageSelector incoming is the same one you use in the ResultsPage. At one point, I had a trait PaginatedResource:
https://github.com/davepacheco/dropshot/blob/1e91d3c7bbca159385d32d6746086946656b8a14/dropshot/src/pagination.rs#L194

that was just the combination of ScanMode, PageSelector, and Item types. When you called new_with_paginator to construct the results page, it's parametrized by PaginatedResource. At least this way, you can't easily wind up using the wrong PageSelector because you'd have to pass an impl of PaginatedResource, which means you'd either have to implement a new one for no reason or you were passing one from a different handler function (e.g., copy/paste). But if you passed a valid one from a different handler, its ScanMode and Item types would likely be different, and since you had to pass a scan_mode and list of items, and the item type also appears in handler function's signature, it just seemed unlikely you'd get all those things wrong and not notice.

Something I really didn't like about this approach was that you need a new PaginatedResource impl for every single paginated resource in the API. That seems like a non-starter, particularly for an API with lots of resources that all paginate the same way (e.g., by "id" or "name").

[davepacheco]

I've pushed a few more changes: one adds automated tests for most of the surface area here, and the other replaces the Incoming pattern in the examples with defaults provided by one-off functions (option 4 in this comment.)

My plan next is to sync up, retest, potentially add some more openapi tests, and wrap up.

[davepacheco]

I filed #19 #20 #21 #22 and #23. I did not end up filing a ticket for an alternate version of ResultsPage with a field for the total number of results. I figured consumers can build this very easily, and we can incorporate it into Dropshot if/when we find it useful.