Update pagination to support v1 API patterns #2111
Conversation
| async fn instance_serial_console_v1( | ||
| rqctx: Arc<RequestContext<Arc<ServerContext>>>, | ||
| path_params: Path<params::InstancePath>, | ||
| query_params: Query<params::InstanceSerialConsole>, | ||
| query_params: Query<params::InstanceSerialConsoleRequest>, | ||
| selector_params: Query<params::OptionalProjectSelector>, |
There was a problem hiding this comment.
So, this is a bit weird. It's implemented on the same principle as described in the docs here: https://docs.rs/dropshot/latest/dropshot/#advanced-usage-notes. Even though this endpoint doesn't do pagination, we can't flatten the InstanceSerialConsoleRequest and ProjectSelector structs into a single struct due to nox/serde_urlencoded#33.
| ScanByNameOrId<Selector>, | ||
| PageSelectorByNameOrId<Selector>, |
There was a problem hiding this comment.
So in general, it seems like the first parameter to PaginationParams is going to want to be the tuple of NameOrIds needed to find the item in question. The second parameter is going to want to be just the ID of the top level container. That is to say, I think we should only be doing name lookups for the first query; subsequent pages should just have the ID of the container that we're iterating through.
There was a problem hiding this comment.
I tried this, but it's rather difficult and made the handlers a lot messier.
Right now we take the query parameters and resolve them to a lookup and pass that to nexus. My understanding of this approach is that we'll need to resolve the lookup to an ID inside the handler as well as passing it along to be further resolved by Nexus (on the first page). That also means we have to have logic to check if it's a selector or id (or if the selector is an id) so we're not doing wasteful lookups. My attempts at this made both the handler much longer and harder to understand as well as increased the complexity of the types around pagination.
I agree that this would be an ideal approach. I also think it's an optimization that we can defer. From a user documentation perspective the same caveats would apply when looking up a resource by name to paginate as would when just looking up a name in general.
There was a problem hiding this comment.
I tried this, but it's rather difficult and made the handlers a lot messier.
That's disappointing; I could imagine it being a way to extract common patterns rather than to complicate each handler.
Right now we take the query parameters and resolve them to a lookup and pass that to nexus. My understanding of this approach is that we'll need to resolve the lookup to an ID inside the handler as well as passing it along to be further resolved by Nexus (on the first page).
Not necessarily. It could be as simple as having the nexus function that performs the resolution return the resolved ID.
That also means we have to have logic to check if it's a selector or id (or if the selector is an id) so we're not doing wasteful lookups. My attempts at this made both the handler much longer and harder to understand as well as increased the complexity of the types around pagination.
It could literally be the same types if, for some reason, that's simpler: it could just be that for the page selector only the simplest ID form is encoded in the next_token.
I agree that this would be an ideal approach. I also think it's an optimization that we can defer.
I think of this not as a performance optimization, but a simplification of the handlers. If it's not, agreed, we can defer or just not pursue it.
From a user documentation perspective the same caveats would apply when looking up a resource by name to paginate as would when just looking up a name in general.
Further caveats apply: with this approach a rename of any intermediate collection will cause the iteration to fail whereas using a resolved ID for the container means that renames (e.g. of the parent) won't interfere.
There was a problem hiding this comment.
Not necessarily. It could be as simple as having the nexus function that performs the resolution return the resolved ID.
Yeah, I was thinking the same. We turn the selector into a lookup and pass that along to the function that lists the resources. I tried returning a tuple of the resource list and ID but I had some issues with the return type no longer being a result list. Maybe it just needed to be a result of a tuple where one member was a result list? Uncertain.
I think of this not as a performance optimization, but a simplification of the handlers. If it's not, agreed, we can defer or just not pursue it.
I don't believe that it simplifies the handlers any. My assumption is that it'll make it more complicated regardless. Because it's somewhat of a correctness issue though, I think that's probably a worthwhile tradeoff assuming we find a way to do it without too much complexity.
Further caveats apply: with this approach a rename of any intermediate collection will cause the iteration to fail whereas using a resolved ID for the container means that renames (e.g. of the parent) won't interfere.
Yep! The current case is no worse than what we had previously though. It's worth improving still, just perhaps as a follow up.
@ahl pointed out in #2008 that the pagination parameters for the
/v1/endpoints wouldn't work as written. I hadn't converted the tests yet so I'd missed that. This PR converts some aspects of the existing tests to test pagination of the new endpoints and updates the pagination implementation to support the new API approach.Changes
NameOrIdPaginateByNameOrIdand its associated types to support aSelectorgeneric such that resource selector query parameters are properly included in pagination.organization,project, andinstancee2e tests to exercise new endpoints and pagination. This isn't a complete update because at leastprojectandinstanceinclude sub routes which aren't implemented yet. It should be enough to exercise pagination for each though./v1/endpoints to usePaginateByNameOrId. Going forward all endpoints with a name field will use that pagination strategy. This PR does not update thePaginateByNameorPaginateByIdstrategies. The former will likely be removed post the migration. The latter will be updated once the first ID only endpoint with pagination is implemented for the/v1/API.