Update pagination strategy to prevent duplication #2206
Merged
Conversation
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
davepacheco
approved these changes
Jan 24, 2023
Collaborator
davepacheco
left a comment
davepacheco
left a comment
There was a problem hiding this comment.
Nice! This seems like a lot less boilerplate overall.
| ) -> Result<HttpResponseOk<ResultsPage<Silo>>, HttpError> { | ||
| let apictx = rqctx.context(); | ||
| let nexus = &apictx.nexus; | ||
| let query = query_params.into_inner(); |
Collaborator
There was a problem hiding this comment.
nit: a downside of putting this stuff outside the handler async block is that the request timers won't account for the time required to do this. Hopefully it's not much but that's generally why I try to keep almost everything inside that block.
Contributor
Author
There was a problem hiding this comment.
Cool, I didn't realize that. I'm happy to move stuff back into the handler.
22 tasks
Contributor
|
FYI I took a look through and agree that this is a solid improvement. |
Merged
just-be-dev
added a commit
that referenced
this pull request
Feb 15, 2023
See #2028 This PR adds V1 versions of the developer facing networking APIs. Firewall rules aren't in this PR. I'll do that in a follow up. ## New Endpoints All endpoints here are scoped via query parameters by their parent selectors. So for `vpc-subnets` if you provide the vpc by name, you'll also need to provide the project. If the projected is specified by name you'll need the org. So `/v1/vpc-subnet?vpc={vpc_id}`, `/v1/vpc-subnet?vpc={vpc_name}&project={project_id}`, etc ```shell # Scoped via ?project={project} GET /v1/vpcs # List VPCs POST /v1/vpcs # Create a VPC GET /v1/vpcs/{vpc} # Get a VPC DELETE /v1/vpcs/{vpc} # Delete a VPC # Scoped via ?vpc={vpc} GET /v1/vpc-subnets # List VPC subnets POST /v1/vpc-subnets # Create a VPC subnet GET /v1/vpc-subnets/{subnet} # Get a VPC subnet DELETE /v1/vpc-subnets/{subnet} # Delete a VPC subnet # Scoped via ?vpc={vpc} GET /v1/vpc-routers # List VPCs POST /v1/vpc-routers # Create a VPC GET /v1/vpc-routers/{router} # Get a VPC DELETE /v1/vpc-routers/{router} # Delete a VPC # Scoped via ?router={router} GET /v1/vpc-router-routes # List VPC router routes POST /v1/vpc-router-routes # Create a VPC router route GET /v1/vpc-router-routes/{route} # Get a VPC router route DELETE /v1/vpc-router-routes/{route} # Delete a VPC router route # Scoped via ?instance={instance} GET /v1/network-interfaces # List network interfaces POST /v1/network-interfaces # Create network interface GET /v1/network-interfaces/{interface} # Get a network interface DELETE /v1/network-interfaces/{interface} # Delete network interface ``` For a quick overview of the entire API (and the changes introduced in this PR) check out the [nexus_tags.txt file](https://github.com/oxidecomputer/omicron/pull/2057/files#diff-482331ae638fae81e451868307fcd2f8836600a2ac552c965da987a402d94e54) ## Notable changes - I pulled app level network interface functions out into their [own file](https://github.com/oxidecomputer/omicron/pull/2057/files#diff-4d675c5f8ef51a7a12255d0a6cd593b4bfb7b77dbf5e791c97abf9e874a588bf). - I've slowly started to update the naming convention of app and datastore level functions to be less nested (and closer to the operation names) as per [this comment](#2008 (comment)). It's a little bit inconsistent now but the intent is to do a larger follow up to clean these all up. At present I'm just updating them as I come across them. - I've begun moving all possible contents of an http entrypoint into its handler for more timing information as per [this comment](#2206 (comment))
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.
This is an attempt to consolidate pagination functionality such that the datastore layer is the only layer that needs to be aware of
idornamedifferences. The handlers will treat all list endpoints as the same and only one implementation of listing an endpoint will be required in the app layer.There's a small about of verbosity in the handlers where multiple parameter types must be constructed to retrieve different sources of information, but it's largely duplicative and can likely just be refactored into a shared function. I've kept it flat for now. I'm generally unhappy with the names of the different parameter types and how we refer to them in parameters but again that feels like something that could be tidied up later after the base implementation.