From 9e6f41aca3c7f17b3594aa660e348e88c04dd7e9 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Wed, 4 Jan 2023 15:45:07 -0500 Subject: [PATCH 1/8] Update organization tests to use v1 api to verify pagination works --- nexus/test-utils/src/resource_helpers.rs | 2 +- nexus/tests/integration_tests/organizations.rs | 13 +++++++------ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 5efc14c2ec7..4a1b34023d0 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -184,7 +184,7 @@ pub async fn create_project( organization_name: &str, project_name: &str, ) -> Project { - let url = format!("/organizations/{}/projects", &organization_name); + let url = format!("/v1/projects?organization={}", &organization_name); object_create( client, &url, diff --git a/nexus/tests/integration_tests/organizations.rs b/nexus/tests/integration_tests/organizations.rs index b6fff263ea8..c45cc3732f4 100644 --- a/nexus/tests/integration_tests/organizations.rs +++ b/nexus/tests/integration_tests/organizations.rs @@ -26,7 +26,7 @@ async fn test_organizations(cptestctx: &ControlPlaneTestContext) { create_organization(&client, &o2_name).await; // Verify GET /organizations/{org} works - let o1_url = format!("/organizations/{}", o1_name); + let o1_url = format!("/v1/organizations/{}", o1_name); let organization: Organization = NexusRequest::object_get(&client, &o1_url) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -36,7 +36,7 @@ async fn test_organizations(cptestctx: &ControlPlaneTestContext) { .unwrap(); assert_eq!(organization.identity.name, o1_name); - let o2_url = format!("/organizations/{}", o2_name); + let o2_url = format!("/v1/organizations/{}", o2_name); let organization: Organization = NexusRequest::object_get(&client, &o2_url) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -51,7 +51,7 @@ async fn test_organizations(cptestctx: &ControlPlaneTestContext) { &client, StatusCode::NOT_FOUND, Method::GET, - &"/organizations/fake-org", + &"/v1/organizations/fake-org", ) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -60,7 +60,7 @@ async fn test_organizations(cptestctx: &ControlPlaneTestContext) { // Verify GET /organizations works let organizations = - objects_list_page_authz::(client, "/organizations") + objects_list_page_authz::(client, "/v1/organizations") .await .items; assert_eq!(organizations.len(), 2); @@ -90,7 +90,7 @@ async fn test_organizations(cptestctx: &ControlPlaneTestContext) { // Verify the org is gone from the organizations list let organizations = - objects_list_page_authz::(client, "/organizations") + objects_list_page_authz::(client, "/v1/organizations") .await .items; assert_eq!(organizations.len(), 1); @@ -111,7 +111,8 @@ async fn test_organizations(cptestctx: &ControlPlaneTestContext) { // Attempt to delete a non-empty organization let project_name = "p1"; - let project_url = format!("{}/projects/{}", o2_url, project_name); + let project_url = + format!("/organizations/{}/projects/{}", o2_name, project_name); create_project(&client, &o2_name, &project_name).await; NexusRequest::expect_failure( &client, From 797525e67adaa2eb00e959788445294206b47a54 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Thu, 5 Jan 2023 00:44:37 -0500 Subject: [PATCH 2/8] Simplify name or id pagination api --- common/src/api/external/http_pagination.rs | 209 +++++++------------- common/src/api/external/mod.rs | 53 ++++- common/tests/output/pagination-examples.txt | 8 +- common/tests/output/pagination-schema.txt | 30 ++- nexus/src/external_api/http_entrypoints.rs | 163 +++++++-------- 5 files changed, 208 insertions(+), 255 deletions(-) diff --git a/common/src/api/external/http_pagination.rs b/common/src/api/external/http_pagination.rs index 1ae6706e101..06c4076a20c 100644 --- a/common/src/api/external/http_pagination.rs +++ b/common/src/api/external/http_pagination.rs @@ -42,6 +42,7 @@ use crate::api::external::DataPageParams; use crate::api::external::Name; +use crate::api::external::NameOrId; use crate::api::external::ObjectIdentity; use crate::api::external::PaginationOrder; use dropshot::HttpError; @@ -150,11 +151,12 @@ pub fn marker_for_id(_: &S, t: &T) -> Uuid { pub fn marker_for_name_or_id( scan: &ScanByNameOrId, item: &T, -) -> NameOrIdMarker { +) -> NameOrId { let identity = item.identity(); - match pagination_field_for_scan_params(scan) { - PagField::Name => NameOrIdMarker::Name(identity.name.clone()), - PagField::Id => NameOrIdMarker::Id(identity.id), + match scan.sort_by { + NameOrIdSortMode::NameAscending => identity.name.clone().into(), + NameOrIdSortMode::NameDescending => identity.name.clone().into(), + NameOrIdSortMode::IdAscending => identity.id.into(), } } @@ -177,10 +179,6 @@ where /// Given a request and pagination parameters, return a [`DataPageParams`] /// describing the current page of results to return -/// -/// This implementation is used for `ScanByName` and `ScanById`. See -/// [`data_page_params_nameid_name`] and [`data_page_params_nameid_id`] for -/// variants that can be used for `ScanByNameOrId`. pub fn data_page_params_for<'a, S, C>( rqctx: &'a Arc>, pag_params: &'a PaginationParams>, @@ -301,7 +299,26 @@ impl ScanParams for ScanById { pub type PaginatedByNameOrId = PaginationParams; /// Page selector for pagination by name or id -pub type PageSelectorByNameOrId = PageSelector; +pub type PageSelectorByNameOrId = PageSelector; + +pub fn name_or_id_pagination<'a>( + params: &'a PaginatedByNameOrId, + page_params: &'a DataPageParams, +) -> Result, HttpError> { + let params = ScanByNameOrId::from_query(params)?; + match params.sort_by { + NameOrIdSortMode::NameAscending => { + Ok(PaginatedBy::Name(page_params.try_into()?)) + } + NameOrIdSortMode::NameDescending => { + Ok(PaginatedBy::Name(page_params.try_into()?)) + } + NameOrIdSortMode::IdAscending => { + Ok(PaginatedBy::Id(page_params.try_into()?)) + } + } +} + /// Scan parameters for resources that support scanning by name or id #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] pub struct ScanByNameOrId { @@ -324,42 +341,18 @@ fn default_nameid_sort_mode() -> NameOrIdSortMode { NameOrIdSortMode::NameAscending } -// TODO-correctness It's tempting to make this a serde(untagged) enum, which -// would clean up the format of the page selector parameter. However, it would -// have the side effect that if the name happened to be a valid uuid, then we'd -// parse it as a uuid here, even if the corresponding scan parameters indicated -// that we were doing a scan by name. Then we'd fail later on an invalid -// combination. We could infer the correct variant here from the "sort_by" -// field of the adjacent scan params, but we'd have to write our own -// `Deserialize` to do this. This might be worth revisiting before we commit to -// any particular version of the API. -#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] -#[serde(rename_all = "camelCase")] -pub enum NameOrIdMarker { - Id(Uuid), - Name(Name), -} - fn bad_token_error() -> HttpError { HttpError::for_bad_request(None, String::from("invalid page token")) } -#[derive(Debug, PartialEq)] -pub enum PagField { - Id, - Name, -} - -pub fn pagination_field_for_scan_params(p: &ScanByNameOrId) -> PagField { - match p.sort_by { - NameOrIdSortMode::NameAscending => PagField::Name, - NameOrIdSortMode::NameDescending => PagField::Name, - NameOrIdSortMode::IdAscending => PagField::Id, - } +#[derive(Debug)] +pub enum PaginatedBy<'a> { + Id(DataPageParams<'a, Uuid>), + Name(DataPageParams<'a, Name>), } impl ScanParams for ScanByNameOrId { - type MarkerValue = NameOrIdMarker; + type MarkerValue = NameOrId; fn direction(&self) -> PaginationOrder { match self.sort_by { @@ -377,7 +370,7 @@ impl ScanParams for ScanByNameOrId { WhichPage::Next(PageSelectorByNameOrId { scan, - last_seen: NameOrIdMarker::Name(_), + last_seen: NameOrId::Name(_), }) => match scan.sort_by { NameOrIdSortMode::NameAscending => Ok(scan), NameOrIdSortMode::NameDescending => Ok(scan), @@ -386,7 +379,7 @@ impl ScanParams for ScanByNameOrId { WhichPage::Next(PageSelectorByNameOrId { scan, - last_seen: NameOrIdMarker::Id(_), + last_seen: NameOrId::Id(_), }) => match scan.sort_by { NameOrIdSortMode::NameAscending => Err(()), NameOrIdSortMode::NameDescending => Err(()), @@ -397,94 +390,23 @@ impl ScanParams for ScanByNameOrId { } } -/// Serves the same purpose as [`data_page_params_for`] for the specific case of -/// `ScanByNameOrId` when scanning by `name` -/// -/// Why do we need a separate function here? Because `data_page_params_for` only -/// knows how to return the (statically-defined) marker value from the page -/// selector. For `ScanByNameOrId`, this would return the enum -/// `NameOrIdMarker`. But at some point our caller needs the specific type -/// (e.g., `Name` for a scan by name or `Uuid` for a scan by Uuid). They get -/// that from this function and its partner, [`data_page_params_nameid_id`]. -/// These functions are where we look at the enum variant and extract the -/// specific marker value out. -pub fn data_page_params_nameid_name<'a, C>( - rqctx: &'a Arc>, - pag_params: &'a PaginatedByNameOrId, -) -> Result, HttpError> -where - C: dropshot::ServerContext, -{ - let limit = rqctx.page_limit(pag_params)?; - data_page_params_nameid_name_limit(limit, pag_params) -} - -fn data_page_params_nameid_name_limit( - limit: NonZeroU32, - pag_params: &PaginatedByNameOrId, -) -> Result, HttpError> { - let data_page = data_page_params_with_limit(limit, pag_params)?; - let direction = data_page.direction; - let marker = match data_page.marker { - None => None, - Some(NameOrIdMarker::Name(name)) => Some(name), - // This should arguably be a panic or a 500 error, since the caller - // should not have invoked this version of the function if they didn't - // know they were looking at a name-based marker. - Some(NameOrIdMarker::Id(_)) => return Err(bad_token_error()), - }; - Ok(DataPageParams { limit, direction, marker }) -} - -/// See [`data_page_params_nameid_name`]. -pub fn data_page_params_nameid_id<'a, C>( - rqctx: &'a Arc>, - pag_params: &'a PaginatedByNameOrId, -) -> Result, HttpError> -where - C: dropshot::ServerContext, -{ - let limit = rqctx.page_limit(pag_params)?; - data_page_params_nameid_id_limit(limit, pag_params) -} - -fn data_page_params_nameid_id_limit( - limit: NonZeroU32, - pag_params: &PaginatedByNameOrId, -) -> Result, HttpError> { - let data_page = data_page_params_with_limit(limit, pag_params)?; - let direction = data_page.direction; - let marker = match data_page.marker { - None => None, - Some(NameOrIdMarker::Id(id)) => Some(id), - // This should arguably be a panic or a 500 error, since the caller - // should not have invoked this version of the function if they didn't - // know they were looking at an id-based marker. - Some(NameOrIdMarker::Name(_)) => return Err(bad_token_error()), - }; - Ok(DataPageParams { limit, direction, marker }) -} - #[cfg(test)] mod test { - use super::data_page_params_nameid_id_limit; - use super::data_page_params_nameid_name_limit; use super::data_page_params_with_limit; use super::marker_for_id; use super::marker_for_name; use super::marker_for_name_or_id; use super::page_selector_for; - use super::pagination_field_for_scan_params; use super::IdSortMode; use super::Name; - use super::NameOrIdMarker; + use super::NameOrId; use super::NameOrIdSortMode; use super::NameSortMode; - use super::PagField; use super::PageSelector; use super::PageSelectorById; use super::PageSelectorByName; use super::PageSelectorByNameOrId; + use super::PaginatedBy; use super::PaginatedById; use super::PaginatedByName; use super::PaginatedByNameOrId; @@ -492,6 +414,7 @@ mod test { use super::ScanByName; use super::ScanByNameOrId; use super::ScanParams; + use crate::api::external::http_pagination::name_or_id_pagination; use crate::api::external::IdentityMetadata; use crate::api::external::ObjectIdentity; use chrono::Utc; @@ -499,7 +422,6 @@ mod test { use dropshot::PaginationParams; use dropshot::WhichPage; use expectorate::assert_contents; - use http::StatusCode; use schemars::schema_for; use serde::Serialize; use serde_json::to_string_pretty; @@ -595,7 +517,7 @@ mod test { "page selector: by name or id, using id ascending", to_string_pretty(&PageSelectorByNameOrId { scan: scan_by_nameid_id, - last_seen: NameOrIdMarker::Id(id), + last_seen: NameOrId::Id(id), }) .unwrap(), ), @@ -603,7 +525,7 @@ mod test { "page selector: by name or id, using id ascending", to_string_pretty(&PageSelectorByNameOrId { scan: scan_by_nameid_name, - last_seen: NameOrIdMarker::Name(name), + last_seen: NameOrId::Name(name), }) .unwrap(), ), @@ -815,13 +737,12 @@ mod test { fn test_scan_by_nameid_name() { // Start with the common battery of tests. let scan = ScanByNameOrId { sort_by: NameOrIdSortMode::NameDescending }; - assert_eq!(pagination_field_for_scan_params(&scan), PagField::Name); assert_eq!(scan.direction(), PaginationOrder::Descending); let list = list_of_things(); - let thing0_marker = NameOrIdMarker::Name("thing0".parse().unwrap()); + let thing0_marker = NameOrId::Name("thing0".parse().unwrap()); let thinglast_name: Name = "thing19".parse().unwrap(); - let thinglast_marker = NameOrIdMarker::Name(thinglast_name.clone()); + let thinglast_marker = NameOrId::Name(thinglast_name.clone()); let (p0, p1) = test_scan_param_common( &list, &scan, @@ -834,33 +755,39 @@ mod test { // Verify data pages based on the query params. let limit = NonZeroU32::new(123).unwrap(); - let data_page = data_page_params_nameid_name_limit(limit, &p0).unwrap(); + let data_page = data_page_params_with_limit(limit, &p0).unwrap(); + let data_page = match name_or_id_pagination(&p0, &data_page) { + Ok(PaginatedBy::Name(params)) => params, + _ => { + panic!("Expected Name pagination, got Id pagination") + } + }; assert_eq!(data_page.marker, None); assert_eq!(data_page.direction, PaginationOrder::Descending); assert_eq!(data_page.limit, limit); - let data_page = data_page_params_nameid_name_limit(limit, &p1).unwrap(); - assert_eq!(data_page.marker, Some(&thinglast_name)); + let data_page = data_page_params_with_limit(limit, &p1).unwrap(); + let data_page = match name_or_id_pagination(&p1, &data_page) { + Ok(PaginatedBy::Name(params)) => params, + _ => { + panic!("Expected Name pagination, got Id pagination") + } + }; + assert_eq!(data_page.marker, Some(&thinglast_name.into())); assert_eq!(data_page.direction, PaginationOrder::Descending); assert_eq!(data_page.limit, limit); - - let error = data_page_params_nameid_id_limit(limit, &p1).unwrap_err(); - assert_eq!(error.status_code, StatusCode::BAD_REQUEST); - assert_eq!(error.external_message, "invalid page token"); } #[test] fn test_scan_by_nameid_id() { // Start with the common battery of tests. let scan = ScanByNameOrId { sort_by: NameOrIdSortMode::IdAscending }; - assert_eq!(pagination_field_for_scan_params(&scan), PagField::Id); assert_eq!(scan.direction(), PaginationOrder::Ascending); let list = list_of_things(); - let thing0_marker = NameOrIdMarker::Id(list[0].identity.id); + let thing0_marker = NameOrId::Id(list[0].identity.id); let thinglast_id = list[list.len() - 1].identity.id; - let thinglast_marker = - NameOrIdMarker::Id(list[list.len() - 1].identity.id); + let thinglast_marker = NameOrId::Id(list[list.len() - 1].identity.id); let (p0, p1) = test_scan_param_common( &list, &scan, @@ -873,18 +800,26 @@ mod test { // Verify data pages based on the query params. let limit = NonZeroU32::new(123).unwrap(); - let data_page = data_page_params_nameid_id_limit(limit, &p0).unwrap(); + let data_page = data_page_params_with_limit(limit, &p0).unwrap(); + let data_page = match name_or_id_pagination(&p0, &data_page) { + Ok(PaginatedBy::Id(params)) => params, + _ => { + panic!("Expected id pagination, got name pagination") + } + }; assert_eq!(data_page.marker, None); assert_eq!(data_page.direction, PaginationOrder::Ascending); assert_eq!(data_page.limit, limit); - let data_page = data_page_params_nameid_id_limit(limit, &p1).unwrap(); - assert_eq!(data_page.marker, Some(&thinglast_id)); + let data_page = data_page_params_with_limit(limit, &p1).unwrap(); + let data_page = match name_or_id_pagination(&p1, &data_page) { + Ok(PaginatedBy::Id(params)) => params, + _ => { + panic!("Expected id pagination, got name pagination") + } + }; + assert_eq!(data_page.marker, Some(&thinglast_id.into())); assert_eq!(data_page.direction, PaginationOrder::Ascending); assert_eq!(data_page.limit, limit); - - let error = data_page_params_nameid_name_limit(limit, &p1).unwrap_err(); - assert_eq!(error.status_code, StatusCode::BAD_REQUEST); - assert_eq!(error.external_message, "invalid page token"); } } diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 6d988c61397..0ea4ffe6023 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -9,6 +9,7 @@ mod error; pub mod http_pagination; +use dropshot::HttpError; pub use error::*; use anyhow::anyhow; @@ -111,6 +112,56 @@ impl<'a, NameType> DataPageParams<'a, NameType> { } } +impl<'a> TryFrom<&DataPageParams<'a, NameOrId>> for DataPageParams<'a, Name> { + type Error = HttpError; + + fn try_from( + value: &DataPageParams<'a, NameOrId>, + ) -> Result { + match value.marker { + Some(NameOrId::Name(name)) => Ok(DataPageParams { + marker: Some(name), + direction: value.direction, + limit: value.limit, + }), + None => Ok(DataPageParams { + marker: None, + direction: value.direction, + limit: value.limit, + }), + _ => Err(HttpError::for_bad_request( + None, + String::from("invalid pagination marker"), + )), + } + } +} + +impl<'a> TryFrom<&DataPageParams<'a, NameOrId>> for DataPageParams<'a, Uuid> { + type Error = HttpError; + + fn try_from( + value: &DataPageParams<'a, NameOrId>, + ) -> Result { + match value.marker { + Some(NameOrId::Id(id)) => Ok(DataPageParams { + marker: Some(id), + direction: value.direction, + limit: value.limit, + }), + None => Ok(DataPageParams { + marker: None, + direction: value.direction, + limit: value.limit, + }), + _ => Err(HttpError::for_bad_request( + None, + String::from("invalid pagination marker"), + )), + } + } +} + /// A name used in the API /// /// Names are generally user-provided unique identifiers, highly constrained as @@ -268,7 +319,7 @@ impl Name { } } -#[derive(Serialize, Deserialize, Display, Clone)] +#[derive(Debug, Serialize, Deserialize, Display, Clone, PartialEq)] #[display("{0}")] #[serde(untagged)] pub enum NameOrId { diff --git a/common/tests/output/pagination-examples.txt b/common/tests/output/pagination-examples.txt index f9bfefbe21d..304509448a5 100644 --- a/common/tests/output/pagination-examples.txt +++ b/common/tests/output/pagination-examples.txt @@ -27,14 +27,10 @@ example pagination parameters: page selector: by name ascending example pagination parameters: page selector: by name or id, using id ascending { "sort_by": "id_ascending", - "last_seen": { - "id": "61a78113-d3c6-4b35-a410-23e9eae64328" - } + "last_seen": "61a78113-d3c6-4b35-a410-23e9eae64328" } example pagination parameters: page selector: by name or id, using id ascending { "sort_by": "name_ascending", - "last_seen": { - "name": "bort" - } + "last_seen": "bort" } diff --git a/common/tests/output/pagination-schema.txt b/common/tests/output/pagination-schema.txt index 2fdc227cdaa..721bb8f7d2b 100644 --- a/common/tests/output/pagination-schema.txt +++ b/common/tests/output/pagination-schema.txt @@ -196,7 +196,7 @@ schema for pagination parameters: page selector, scan by id only schema for pagination parameters: page selector, scan by name or id { "$schema": "http://json-schema.org/draft-07/schema#", - "title": "PageSelector_for_ScanByNameOrId_and_NameOrIdMarker", + "title": "PageSelector_for_ScanByNameOrId_and_NameOrId", "description": "Specifies which page of results we're on\n\nThis type is generic over the different scan modes that we support.", "type": "object", "required": [ @@ -207,7 +207,7 @@ schema for pagination parameters: page selector, scan by name or id "description": "value of the marker field last seen by the client", "allOf": [ { - "$ref": "#/definitions/NameOrIdMarker" + "$ref": "#/definitions/NameOrId" } ] }, @@ -228,32 +228,24 @@ schema for pagination parameters: page selector, scan by name or id "maxLength": 63, "pattern": "^(?![0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$)^[a-z][a-z0-9-]*[a-zA-Z0-9]$" }, - "NameOrIdMarker": { + "NameOrId": { "oneOf": [ { - "type": "object", - "required": [ - "id" - ], - "properties": { - "id": { + "title": "id", + "allOf": [ + { "type": "string", "format": "uuid" } - }, - "additionalProperties": false + ] }, { - "type": "object", - "required": [ - "name" - ], - "properties": { - "name": { + "title": "name", + "allOf": [ + { "$ref": "#/definitions/Name" } - }, - "additionalProperties": false + ] } ] }, diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index b66f980d36b..d55a1c023f5 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -41,12 +41,10 @@ use dropshot::{ }; use ipnetwork::IpNetwork; use omicron_common::api::external::http_pagination::data_page_params_for; -use omicron_common::api::external::http_pagination::data_page_params_nameid_id; -use omicron_common::api::external::http_pagination::data_page_params_nameid_name; use omicron_common::api::external::http_pagination::marker_for_name; use omicron_common::api::external::http_pagination::marker_for_name_or_id; -use omicron_common::api::external::http_pagination::pagination_field_for_scan_params; -use omicron_common::api::external::http_pagination::PagField; +use omicron_common::api::external::http_pagination::name_or_id_pagination; +use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::api::external::http_pagination::PaginatedById; use omicron_common::api::external::http_pagination::PaginatedByName; use omicron_common::api::external::http_pagination::PaginatedByNameOrId; @@ -481,20 +479,18 @@ async fn silo_list( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let query = query_params.into_inner(); - let params = ScanByNameOrId::from_query(&query)?; - let field = pagination_field_for_scan_params(params); - - let silos = match field { - PagField::Id => { - let page_selector = data_page_params_nameid_id(&rqctx, &query)?; + let pag_params = data_page_params_for(&rqctx, &query)?; + let silos = match name_or_id_pagination(&query, &pag_params)? { + PaginatedBy::Id(page_selector) => { nexus.silos_list_by_id(&opctx, &page_selector).await? } - - PagField::Name => { - let page_selector = - data_page_params_nameid_name(&rqctx, &query)? - .map_name(|n| Name::ref_cast(n)); - nexus.silos_list_by_name(&opctx, &page_selector).await? + PaginatedBy::Name(page_selector) => { + nexus + .silos_list_by_name( + &opctx, + &page_selector.map_name(|n| Name::ref_cast(n)), + ) + .await? } } .into_iter() @@ -946,20 +942,18 @@ async fn organization_list_v1( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let query = query_params.into_inner(); - let params = ScanByNameOrId::from_query(&query)?; - let field = pagination_field_for_scan_params(params); - - let organizations = match field { - PagField::Id => { - let page_selector = data_page_params_nameid_id(&rqctx, &query)?; + let pag_params = data_page_params_for(&rqctx, &query)?; + let organizations = match name_or_id_pagination(&query, &pag_params)? { + PaginatedBy::Id(page_selector) => { nexus.organizations_list_by_id(&opctx, &page_selector).await? } - - PagField::Name => { - let page_selector = - data_page_params_nameid_name(&rqctx, &query)? - .map_name(|n| Name::ref_cast(n)); - nexus.organizations_list_by_name(&opctx, &page_selector).await? + PaginatedBy::Name(page_selector) => { + nexus + .organizations_list_by_name( + &opctx, + &page_selector.map_name(|n| Name::ref_cast(n)), + ) + .await? } } .into_iter() @@ -991,20 +985,18 @@ async fn organization_list( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let query = query_params.into_inner(); - let params = ScanByNameOrId::from_query(&query)?; - let field = pagination_field_for_scan_params(params); - - let organizations = match field { - PagField::Id => { - let page_selector = data_page_params_nameid_id(&rqctx, &query)?; + let pag_params = data_page_params_for(&rqctx, &query)?; + let organizations = match name_or_id_pagination(&query, &pag_params)? { + PaginatedBy::Id(page_selector) => { nexus.organizations_list_by_id(&opctx, &page_selector).await? } - - PagField::Name => { - let page_selector = - data_page_params_nameid_name(&rqctx, &query)? - .map_name(|n| Name::ref_cast(n)); - nexus.organizations_list_by_name(&opctx, &page_selector).await? + PaginatedBy::Name(page_selector) => { + nexus + .organizations_list_by_name( + &opctx, + &page_selector.map_name(|n| Name::ref_cast(n)), + ) + .await? } } .into_iter() @@ -1431,41 +1423,35 @@ async fn project_list_v1( let apictx = rqctx.context(); let nexus = &apictx.nexus; let query = query_params.into_inner(); + let pag_params = data_page_params_for(&rqctx, &query.pagination)?; let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let organization_lookup = nexus.organization_lookup(&opctx, &query.organization)?; - let params = ScanByNameOrId::from_query(&query.pagination)?; - let field = pagination_field_for_scan_params(params); - let projects = match field { - PagField::Id => { - let page_selector = - data_page_params_nameid_id(&rqctx, &query.pagination)?; - nexus - .projects_list_by_id( - &opctx, - &organization_lookup, - &page_selector, - ) - .await? - } - - PagField::Name => { - let page_selector = - data_page_params_nameid_name(&rqctx, &query.pagination)? - .map_name(|n| Name::ref_cast(n)); - nexus - .projects_list_by_name( - &opctx, - &organization_lookup, - &page_selector, - ) - .await? + let projects = + match name_or_id_pagination(&query.pagination, &pag_params)? { + PaginatedBy::Id(page_selector) => { + nexus + .projects_list_by_id( + &opctx, + &organization_lookup, + &page_selector, + ) + .await? + } + PaginatedBy::Name(page_selector) => { + nexus + .projects_list_by_name( + &opctx, + &organization_lookup, + &page_selector.map_name(|n| Name::ref_cast(n)), + ) + .await? + } } - } - .into_iter() - .map(|p| p.into()) - .collect(); + .into_iter() + .map(|p| p.into()) + .collect(); Ok(HttpResponseOk(ScanByNameOrId::results_page( &query.pagination, projects, @@ -1492,7 +1478,7 @@ async fn project_list( let nexus = &apictx.nexus; let query = query_params.into_inner(); let path = path_params.into_inner(); - + let pag_params = data_page_params_for(&rqctx, &query)?; let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let organization_selector = ¶ms::OrganizationSelector { @@ -1500,11 +1486,8 @@ async fn project_list( }; let organization_lookup = nexus.organization_lookup(&opctx, &organization_selector)?; - let params = ScanByNameOrId::from_query(&query)?; - let field = pagination_field_for_scan_params(params); - let projects = match field { - PagField::Id => { - let page_selector = data_page_params_nameid_id(&rqctx, &query)?; + let projects = match name_or_id_pagination(&query, &pag_params)? { + PaginatedBy::Id(page_selector) => { nexus .projects_list_by_id( &opctx, @@ -1513,16 +1496,12 @@ async fn project_list( ) .await? } - - PagField::Name => { - let page_selector = - data_page_params_nameid_name(&rqctx, &query)? - .map_name(|n| Name::ref_cast(n)); + PaginatedBy::Name(page_selector) => { nexus .projects_list_by_name( &opctx, &organization_lookup, - &page_selector, + &page_selector.map_name(|n| Name::ref_cast(n)), ) .await? } @@ -1966,20 +1945,20 @@ async fn ip_pool_list( let apictx = rqctx.context(); let nexus = &apictx.nexus; let query = query_params.into_inner(); + let pag_params = data_page_params_for(&rqctx, &query)?; let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; - let params = ScanByNameOrId::from_query(&query)?; - let field = pagination_field_for_scan_params(params); - let pools = match field { - PagField::Id => { - let page_selector = data_page_params_nameid_id(&rqctx, &query)?; + let pools = match name_or_id_pagination(&query, &pag_params)? { + PaginatedBy::Id(page_selector) => { nexus.ip_pools_list_by_id(&opctx, &page_selector).await? } - PagField::Name => { - let page_selector = - data_page_params_nameid_name(&rqctx, &query)? - .map_name(|n| Name::ref_cast(n)); - nexus.ip_pools_list_by_name(&opctx, &page_selector).await? + PaginatedBy::Name(page_selector) => { + nexus + .ip_pools_list_by_name( + &opctx, + &page_selector.map_name(|n| Name::ref_cast(n)), + ) + .await? } } .into_iter() From be9279025bf514dc9cd1444b8cb31a77887e9415 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Fri, 6 Jan 2023 00:58:24 -0500 Subject: [PATCH 3/8] Update pagination for name or id to accept selectors --- common/src/api/external/http_pagination.rs | 104 ++++++++++------ nexus/src/app/instance.rs | 17 ++- nexus/src/db/datastore/instance.rs | 22 +++- nexus/src/external_api/http_entrypoints.rs | 136 ++++++++++++--------- nexus/test-utils/src/http_testing.rs | 8 +- nexus/tests/integration_tests/projects.rs | 6 +- nexus/types/src/external_api/params.rs | 21 +--- 7 files changed, 188 insertions(+), 126 deletions(-) diff --git a/common/src/api/external/http_pagination.rs b/common/src/api/external/http_pagination.rs index 06c4076a20c..e373c6b69af 100644 --- a/common/src/api/external/http_pagination.rs +++ b/common/src/api/external/http_pagination.rs @@ -148,8 +148,8 @@ pub fn marker_for_id(_: &S, t: &T) -> Uuid { /// /// This is intended for use with [`ScanByNameOrId::results_page`] with objects /// that impl [`ObjectIdentity`]. -pub fn marker_for_name_or_id( - scan: &ScanByNameOrId, +pub fn marker_for_name_or_id( + scan: &ScanByNameOrId, item: &T, ) -> NameOrId { let identity = item.identity(); @@ -296,34 +296,45 @@ impl ScanParams for ScanById { // We include this now primarily to exercise the interface for doing so. /// Query parameters for pagination by name or id -pub type PaginatedByNameOrId = - PaginationParams; +pub type PaginatedByNameOrId = PaginationParams< + ScanByNameOrId, + PageSelectorByNameOrId, +>; /// Page selector for pagination by name or id -pub type PageSelectorByNameOrId = PageSelector; - -pub fn name_or_id_pagination<'a>( - params: &'a PaginatedByNameOrId, - page_params: &'a DataPageParams, -) -> Result, HttpError> { - let params = ScanByNameOrId::from_query(params)?; +pub type PageSelectorByNameOrId = + PageSelector, NameOrId>; + +pub fn name_or_id_pagination< + 'a, + Selector: Clone + Debug + DeserializeOwned + JsonSchema + PartialEq + Serialize, +>( + params: &PaginatedByNameOrId, + pagparams: &'a DataPageParams, +) -> Result, HttpError> { + let params = ScanByNameOrId::::from_query(params)?; match params.sort_by { - NameOrIdSortMode::NameAscending => { - Ok(PaginatedBy::Name(page_params.try_into()?)) - } - NameOrIdSortMode::NameDescending => { - Ok(PaginatedBy::Name(page_params.try_into()?)) - } + NameOrIdSortMode::NameAscending => Ok(PaginatedBy::Name( + pagparams.try_into()?, + params.selector.clone(), + )), + NameOrIdSortMode::NameDescending => Ok(PaginatedBy::Name( + pagparams.try_into()?, + params.selector.clone(), + )), NameOrIdSortMode::IdAscending => { - Ok(PaginatedBy::Id(page_params.try_into()?)) + Ok(PaginatedBy::Id(pagparams.try_into()?, params.selector.clone())) } } } /// Scan parameters for resources that support scanning by name or id #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] -pub struct ScanByNameOrId { +pub struct ScanByNameOrId { #[serde(default = "default_nameid_sort_mode")] sort_by: NameOrIdSortMode, + + #[serde(flatten)] + selector: Selector, } /// Supported set of sort modes for scanning by name or id #[derive(Copy, Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] @@ -346,12 +357,15 @@ fn bad_token_error() -> HttpError { } #[derive(Debug)] -pub enum PaginatedBy<'a> { - Id(DataPageParams<'a, Uuid>), - Name(DataPageParams<'a, Name>), +pub enum PaginatedBy<'a, Selector> { + Id(DataPageParams<'a, Uuid>, Selector), + Name(DataPageParams<'a, Name>, Selector), } -impl ScanParams for ScanByNameOrId { +impl< + T: Clone + Debug + DeserializeOwned + JsonSchema + PartialEq + Serialize, + > ScanParams for ScanByNameOrId +{ type MarkerValue = NameOrId; fn direction(&self) -> PaginationOrder { @@ -444,7 +458,7 @@ mod test { ("scan parameters, scan by id only", schema_for!(ScanById)), ( "scan parameters, scan by name or id", - schema_for!(ScanByNameOrId), + schema_for!(ScanByNameOrId<()>), ), ( "page selector, scan by name only", @@ -475,10 +489,14 @@ mod test { fn test_pagination_examples() { let scan_by_id = ScanById { sort_by: IdSortMode::IdAscending }; let scan_by_name = ScanByName { sort_by: NameSortMode::NameAscending }; - let scan_by_nameid_name = - ScanByNameOrId { sort_by: NameOrIdSortMode::NameAscending }; - let scan_by_nameid_id = - ScanByNameOrId { sort_by: NameOrIdSortMode::IdAscending }; + let scan_by_nameid_name = ScanByNameOrId::<()> { + sort_by: NameOrIdSortMode::NameAscending, + selector: (), + }; + let scan_by_nameid_id = ScanByNameOrId::<()> { + sort_by: NameOrIdSortMode::IdAscending, + selector: (), + }; let id: Uuid = "61a78113-d3c6-4b35-a410-23e9eae64328".parse().unwrap(); let name: Name = "bort".parse().unwrap(); let examples = vec![ @@ -715,7 +733,7 @@ mod test { #[test] fn test_scan_by_nameid_generic() { // Test from_query(): error case. - let error = serde_urlencoded::from_str::( + let error = serde_urlencoded::from_str::>( "sort_by=id_descending", ) .unwrap_err(); @@ -736,7 +754,10 @@ mod test { #[test] fn test_scan_by_nameid_name() { // Start with the common battery of tests. - let scan = ScanByNameOrId { sort_by: NameOrIdSortMode::NameDescending }; + let scan = ScanByNameOrId { + sort_by: NameOrIdSortMode::NameDescending, + selector: (), + }; assert_eq!(scan.direction(), PaginationOrder::Descending); let list = list_of_things(); @@ -749,7 +770,10 @@ mod test { "sort_by=name_descending", &thing0_marker, &thinglast_marker, - &ScanByNameOrId { sort_by: NameOrIdSortMode::NameAscending }, + &ScanByNameOrId { + sort_by: NameOrIdSortMode::NameAscending, + selector: (), + }, &marker_for_name_or_id, ); @@ -757,7 +781,7 @@ mod test { let limit = NonZeroU32::new(123).unwrap(); let data_page = data_page_params_with_limit(limit, &p0).unwrap(); let data_page = match name_or_id_pagination(&p0, &data_page) { - Ok(PaginatedBy::Name(params)) => params, + Ok(PaginatedBy::Name(params, ..)) => params, _ => { panic!("Expected Name pagination, got Id pagination") } @@ -768,7 +792,7 @@ mod test { let data_page = data_page_params_with_limit(limit, &p1).unwrap(); let data_page = match name_or_id_pagination(&p1, &data_page) { - Ok(PaginatedBy::Name(params)) => params, + Ok(PaginatedBy::Name(params, ..)) => params, _ => { panic!("Expected Name pagination, got Id pagination") } @@ -781,7 +805,10 @@ mod test { #[test] fn test_scan_by_nameid_id() { // Start with the common battery of tests. - let scan = ScanByNameOrId { sort_by: NameOrIdSortMode::IdAscending }; + let scan = ScanByNameOrId { + sort_by: NameOrIdSortMode::IdAscending, + selector: (), + }; assert_eq!(scan.direction(), PaginationOrder::Ascending); let list = list_of_things(); @@ -794,7 +821,10 @@ mod test { "sort_by=id_ascending", &thing0_marker, &thinglast_marker, - &ScanByNameOrId { sort_by: NameOrIdSortMode::NameAscending }, + &ScanByNameOrId { + sort_by: NameOrIdSortMode::NameAscending, + selector: (), + }, &marker_for_name_or_id, ); @@ -802,7 +832,7 @@ mod test { let limit = NonZeroU32::new(123).unwrap(); let data_page = data_page_params_with_limit(limit, &p0).unwrap(); let data_page = match name_or_id_pagination(&p0, &data_page) { - Ok(PaginatedBy::Id(params)) => params, + Ok(PaginatedBy::Id(params, ..)) => params, _ => { panic!("Expected id pagination, got name pagination") } @@ -813,7 +843,7 @@ mod test { let data_page = data_page_params_with_limit(limit, &p1).unwrap(); let data_page = match name_or_id_pagination(&p1, &data_page) { - Ok(PaginatedBy::Id(params)) => params, + Ok(PaginatedBy::Id(params, ..)) => params, _ => { panic!("Expected id pagination, got name pagination") } diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index 0794f19c33c..44bb4b9f9be 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -228,7 +228,20 @@ impl super::Nexus { Ok(db_instance) } - pub async fn project_list_instances( + pub async fn project_list_instances_by_id( + &self, + opctx: &OpContext, + project_lookup: &lookup::Project<'_>, + pagparams: &DataPageParams<'_, Uuid>, + ) -> ListResultVec { + let (.., authz_project) = + project_lookup.lookup_for(authz::Action::ListChildren).await?; + self.db_datastore + .project_list_instances_by_id(opctx, &authz_project, pagparams) + .await + } + + pub async fn project_list_instances_by_name( &self, opctx: &OpContext, project_lookup: &lookup::Project<'_>, @@ -237,7 +250,7 @@ impl super::Nexus { let (.., authz_project) = project_lookup.lookup_for(authz::Action::ListChildren).await?; self.db_datastore - .project_list_instances(opctx, &authz_project, pagparams) + .project_list_instances_by_name(opctx, &authz_project, pagparams) .await } diff --git a/nexus/src/db/datastore/instance.rs b/nexus/src/db/datastore/instance.rs index 2b9cef3c299..917e2902294 100644 --- a/nexus/src/db/datastore/instance.rs +++ b/nexus/src/db/datastore/instance.rs @@ -112,7 +112,25 @@ impl DataStore { Ok(instance) } - pub async fn project_list_instances( + pub async fn project_list_instances_by_id( + &self, + opctx: &OpContext, + authz_project: &authz::Project, + pagparams: &DataPageParams<'_, Uuid>, + ) -> ListResultVec { + opctx.authorize(authz::Action::ListChildren, authz_project).await?; + + use db::schema::instance::dsl; + paginated(dsl::instance, dsl::id, &pagparams) + .filter(dsl::project_id.eq(authz_project.id())) + .filter(dsl::time_deleted.is_null()) + .select(Instance::as_select()) + .load_async::(self.pool_authorized(opctx).await?) + .await + .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) + } + + pub async fn project_list_instances_by_name( &self, opctx: &OpContext, authz_project: &authz::Project, @@ -122,8 +140,8 @@ impl DataStore { use db::schema::instance::dsl; paginated(dsl::instance, dsl::name, &pagparams) - .filter(dsl::time_deleted.is_null()) .filter(dsl::project_id.eq(authz_project.id())) + .filter(dsl::time_deleted.is_null()) .select(Instance::as_select()) .load_async::(self.pool_authorized(opctx).await?) .await diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index d55a1c023f5..9f3e7633856 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -472,7 +472,7 @@ async fn policy_update( }] async fn silo_list( rqctx: Arc>>, - query_params: Query, + query_params: Query>, ) -> Result>, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; @@ -481,10 +481,10 @@ async fn silo_list( let query = query_params.into_inner(); let pag_params = data_page_params_for(&rqctx, &query)?; let silos = match name_or_id_pagination(&query, &pag_params)? { - PaginatedBy::Id(page_selector) => { + PaginatedBy::Id(page_selector, ..) => { nexus.silos_list_by_id(&opctx, &page_selector).await? } - PaginatedBy::Name(page_selector) => { + PaginatedBy::Name(page_selector, ..) => { nexus .silos_list_by_name( &opctx, @@ -935,7 +935,7 @@ async fn local_idp_user_set_password( }] async fn organization_list_v1( rqctx: Arc>>, - query_params: Query, + query_params: Query>, ) -> Result>, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; @@ -944,10 +944,10 @@ async fn organization_list_v1( let query = query_params.into_inner(); let pag_params = data_page_params_for(&rqctx, &query)?; let organizations = match name_or_id_pagination(&query, &pag_params)? { - PaginatedBy::Id(page_selector) => { + PaginatedBy::Id(page_selector, ..) => { nexus.organizations_list_by_id(&opctx, &page_selector).await? } - PaginatedBy::Name(page_selector) => { + PaginatedBy::Name(page_selector, ..) => { nexus .organizations_list_by_name( &opctx, @@ -978,7 +978,7 @@ async fn organization_list_v1( }] async fn organization_list( rqctx: Arc>>, - query_params: Query, + query_params: Query>, ) -> Result>, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; @@ -987,10 +987,10 @@ async fn organization_list( let query = query_params.into_inner(); let pag_params = data_page_params_for(&rqctx, &query)?; let organizations = match name_or_id_pagination(&query, &pag_params)? { - PaginatedBy::Id(page_selector) => { + PaginatedBy::Id(page_selector, ..) => { nexus.organizations_list_by_id(&opctx, &page_selector).await? } - PaginatedBy::Name(page_selector) => { + PaginatedBy::Name(page_selector, ..) => { nexus .organizations_list_by_name( &opctx, @@ -1418,42 +1418,43 @@ async fn organization_policy_update( }] async fn project_list_v1( rqctx: Arc>>, - query_params: Query, + query_params: Query>, ) -> Result>, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; let query = query_params.into_inner(); - let pag_params = data_page_params_for(&rqctx, &query.pagination)?; + let pag_params = data_page_params_for(&rqctx, &query)?; let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; - let organization_lookup = - nexus.organization_lookup(&opctx, &query.organization)?; - let projects = - match name_or_id_pagination(&query.pagination, &pag_params)? { - PaginatedBy::Id(page_selector) => { - nexus - .projects_list_by_id( - &opctx, - &organization_lookup, - &page_selector, - ) - .await? - } - PaginatedBy::Name(page_selector) => { - nexus - .projects_list_by_name( - &opctx, - &organization_lookup, - &page_selector.map_name(|n| Name::ref_cast(n)), - ) - .await? - } + let projects = match name_or_id_pagination(&query, &pag_params)? { + PaginatedBy::Id(pagparams, selector) => { + let organization_lookup = + nexus.organization_lookup(&opctx, &selector)?; + nexus + .projects_list_by_id( + &opctx, + &organization_lookup, + &pagparams, + ) + .await? } - .into_iter() - .map(|p| p.into()) - .collect(); + PaginatedBy::Name(pagparams, selector) => { + let organization_lookup = + nexus.organization_lookup(&opctx, &selector)?; + nexus + .projects_list_by_name( + &opctx, + &organization_lookup, + &pagparams.map_name(|n| Name::ref_cast(n)), + ) + .await? + } + } + .into_iter() + .map(|p| p.into()) + .collect(); Ok(HttpResponseOk(ScanByNameOrId::results_page( - &query.pagination, + &query, projects, &marker_for_name_or_id, )?)) @@ -1471,7 +1472,7 @@ async fn project_list_v1( }] async fn project_list( rqctx: Arc>>, - query_params: Query, + query_params: Query>, path_params: Path, ) -> Result>, HttpError> { let apictx = rqctx.context(); @@ -1487,7 +1488,7 @@ async fn project_list( let organization_lookup = nexus.organization_lookup(&opctx, &organization_selector)?; let projects = match name_or_id_pagination(&query, &pag_params)? { - PaginatedBy::Id(page_selector) => { + PaginatedBy::Id(page_selector, ..) => { nexus .projects_list_by_id( &opctx, @@ -1496,7 +1497,7 @@ async fn project_list( ) .await? } - PaginatedBy::Name(page_selector) => { + PaginatedBy::Name(page_selector, ..) => { nexus .projects_list_by_name( &opctx, @@ -1940,7 +1941,7 @@ pub struct IpPoolPathParam { }] async fn ip_pool_list( rqctx: Arc>>, - query_params: Query, + query_params: Query>, ) -> Result>, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; @@ -1949,10 +1950,10 @@ async fn ip_pool_list( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let pools = match name_or_id_pagination(&query, &pag_params)? { - PaginatedBy::Id(page_selector) => { + PaginatedBy::Id(page_selector, ..) => { nexus.ip_pools_list_by_id(&opctx, &page_selector).await? } - PaginatedBy::Name(page_selector) => { + PaginatedBy::Name(page_selector, ..) => { nexus .ip_pools_list_by_name( &opctx, @@ -2514,30 +2515,43 @@ async fn disk_metrics_list( }] async fn instance_list_v1( rqctx: Arc>>, - query_params: Query, + query_params: Query>, ) -> Result>, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; let query = query_params.into_inner(); + let pag_params = data_page_params_for(&rqctx, &query)?; let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; - let project_lookup = - nexus.project_lookup(&opctx, &query.project_selector)?; - let instances = nexus - .project_list_instances( - &opctx, - &project_lookup, - &data_page_params_for(&rqctx, &query.pagination)? - .map_name(|n| Name::ref_cast(n)), - ) - .await? - .into_iter() - .map(|i| i.into()) - .collect(); - Ok(HttpResponseOk(ScanByName::results_page( - &query.pagination, + let instances = match name_or_id_pagination(&query, &pag_params)? { + PaginatedBy::Id(pag_params, selector) => { + let project_lookup = nexus.project_lookup(&opctx, &selector)?; + nexus + .project_list_instances_by_id( + &opctx, + &project_lookup, + &pag_params, + ) + .await? + } + PaginatedBy::Name(pag_params, selector) => { + let project_lookup = nexus.project_lookup(&opctx, &selector)?; + nexus + .project_list_instances_by_name( + &opctx, + &project_lookup, + &pag_params.map_name(|n| Name::ref_cast(n)), + ) + .await? + } + } + .into_iter() + .map(|i| i.into()) + .collect(); + Ok(HttpResponseOk(ScanByNameOrId::results_page( + &query, instances, - &marker_for_name, + &marker_for_name_or_id, )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await @@ -2566,7 +2580,7 @@ async fn instance_list( let opctx = OpContext::for_external_api(&rqctx).await?; let project_lookup = nexus.project_lookup(&opctx, &project_selector)?; let instances = nexus - .project_list_instances( + .project_list_instances_by_name( &opctx, &project_lookup, &data_page_params_for(&rqctx, &query)? diff --git a/nexus/test-utils/src/http_testing.rs b/nexus/test-utils/src/http_testing.rs index 73e66846042..2030a89b702 100644 --- a/nexus/test-utils/src/http_testing.rs +++ b/nexus/test-utils/src/http_testing.rs @@ -653,11 +653,15 @@ impl<'a> NexusRequest<'a> { let mut next_token: Option = None; const DEFAULT_PAGE_SIZE: usize = 10; let limit = limit.unwrap_or(DEFAULT_PAGE_SIZE); - let url_base = format!("{}?limit={}&", collection_url, limit); + let url_base = if collection_url.contains("?") { + format!("{}&limit={}", collection_url, limit) + } else { + format!("{}?limit={}", collection_url, limit) + }; loop { let url = if let Some(next_token) = &next_token { - format!("{}page_token={}", url_base, next_token) + format!("{}&page_token={}", url_base, next_token) } else { format!("{}{}", url_base, initial_params) }; diff --git a/nexus/tests/integration_tests/projects.rs b/nexus/tests/integration_tests/projects.rs index 2ebe24b9ed3..627187ba329 100644 --- a/nexus/tests/integration_tests/projects.rs +++ b/nexus/tests/integration_tests/projects.rs @@ -40,11 +40,11 @@ async fn test_projects(cptestctx: &ControlPlaneTestContext) { create_project(&client, &org_name, &p2_name).await; - let p1_url = format!("/organizations/{}/projects/{}", org_name, p1_name); + let p1_url = format!("/v1/projects/{}?organization={}", p1_name, org_name); let project: Project = project_get(&client, &p1_url).await; assert_eq!(project.identity.name, p1_name); - let p2_url = format!("/organizations/{}/projects/{}", org_name, p2_name); + let p2_url = format!("/v1/projects/{}?organization={}", p2_name, org_name); let project: Project = project_get(&client, &p2_url).await; assert_eq!(project.identity.name, p2_name); @@ -75,7 +75,7 @@ async fn test_projects(cptestctx: &ControlPlaneTestContext) { // Make sure the list projects results for the new org make sense let projects = NexusRequest::iter_collection_authn::( &client, - &format!("/organizations/{}/projects", org2_name), + &format!("/v1/projects?organization={}", org2_name), "", None, ) diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 913154d748e..84c7c497c95 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -7,7 +7,6 @@ use crate::external_api::shared; use chrono::{DateTime, Utc}; use omicron_common::api::external::{ - http_pagination::{PaginatedByName, PaginatedByNameOrId}, ByteCount, IdentityMetadataCreateParams, IdentityMetadataUpdateParams, InstanceCpuCount, Ipv4Net, Ipv6Net, Name, NameOrId, }; @@ -34,7 +33,7 @@ pub struct InstancePath { pub instance: NameOrId, } -#[derive(Deserialize, JsonSchema)] +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq)] pub struct OrganizationSelector { pub organization: NameOrId, } @@ -51,7 +50,7 @@ pub struct OptionalOrganizationSelector { pub organization_selector: Option, } -#[derive(Deserialize, JsonSchema)] +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq)] pub struct ProjectSelector { #[serde(flatten)] pub organization_selector: Option, @@ -69,14 +68,6 @@ impl ProjectSelector { } } -#[derive(Deserialize, JsonSchema)] -pub struct ProjectList { - #[serde(flatten)] - pub pagination: PaginatedByNameOrId, - #[serde(flatten)] - pub organization: OrganizationSelector, -} - #[derive(Deserialize, JsonSchema)] pub struct OptionalProjectSelector { #[serde(flatten)] @@ -105,14 +96,6 @@ impl InstanceSelector { } } -#[derive(Deserialize, JsonSchema)] -pub struct InstanceList { - #[serde(flatten)] - pub pagination: PaginatedByName, - #[serde(flatten)] - pub project_selector: ProjectSelector, -} - #[derive(Deserialize, JsonSchema)] pub struct InstanceSerialConsole { #[serde(flatten)] From 087bd21791856d14513368cb2613e6d8854c676e Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Fri, 6 Jan 2023 12:29:23 -0500 Subject: [PATCH 4/8] Update instances tests to hit new endpoints; fix some bad responses --- nexus/src/external_api/http_entrypoints.rs | 12 +- nexus/test-utils/src/resource_helpers.rs | 4 +- nexus/tests/integration_tests/instances.rs | 190 ++++++++++----------- openapi/nexus.json | 4 +- 4 files changed, 100 insertions(+), 110 deletions(-) diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 9f3e7633856..21d520152cc 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -2899,7 +2899,7 @@ async fn instance_reboot_v1( rqctx: Arc>>, query_params: Query, path_params: Path, -) -> Result, HttpError> { +) -> Result, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; let path = path_params.into_inner(); @@ -2913,7 +2913,7 @@ async fn instance_reboot_v1( let instance_lookup = nexus.instance_lookup(&opctx, &instance_selector)?; let instance = nexus.instance_reboot(&opctx, &instance_lookup).await?; - Ok(HttpResponseOk(instance.into())) + Ok(HttpResponseAccepted(instance.into())) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -2956,7 +2956,7 @@ async fn instance_start_v1( rqctx: Arc>>, query_params: Query, path_params: Path, -) -> Result, HttpError> { +) -> Result, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; let path = path_params.into_inner(); @@ -2970,7 +2970,7 @@ async fn instance_start_v1( let instance_lookup = nexus.instance_lookup(&opctx, &instance_selector)?; let instance = nexus.instance_start(&opctx, &instance_lookup).await?; - Ok(HttpResponseOk(instance.into())) + Ok(HttpResponseAccepted(instance.into())) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -3012,7 +3012,7 @@ async fn instance_stop_v1( rqctx: Arc>>, query_params: Query, path_params: Path, -) -> Result, HttpError> { +) -> Result, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; let path = path_params.into_inner(); @@ -3026,7 +3026,7 @@ async fn instance_stop_v1( let instance_lookup = nexus.instance_lookup(&opctx, &instance_selector)?; let instance = nexus.instance_stop(&opctx, &instance_lookup).await?; - Ok(HttpResponseOk(instance.into())) + Ok(HttpResponseAccepted(instance.into())) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 4a1b34023d0..b429a744574 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -168,7 +168,7 @@ pub async fn create_organization( ) -> Organization { object_create( client, - "/organizations", + "/v1/organizations", ¶ms::OrganizationCreate { identity: IdentityMetadataCreateParams { name: organization_name.parse().unwrap(), @@ -269,7 +269,7 @@ pub async fn create_instance_with( disks: Vec, ) -> Instance { let url = format!( - "/organizations/{}/projects/{}/instances", + "/v1/instances?organization={}&project={}", organization_name, project_name ); object_create( diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index 4e9079b39dc..2d4527e7f71 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -52,12 +52,18 @@ type ControlPlaneTestContext = static ORGANIZATION_NAME: &str = "test-org"; static PROJECT_NAME: &str = "springfield-squidport"; -fn get_project_url() -> String { - format!("/organizations/{}/projects/{}", ORGANIZATION_NAME, PROJECT_NAME) +fn get_instances_url() -> String { + format!( + "/v1/instances?organization={}&project={}", + ORGANIZATION_NAME, PROJECT_NAME + ) } -fn get_instances_url() -> String { - format!("{}/instances", get_project_url()) +fn get_instance_url(instance_name: &str) -> String { + format!( + "/v1/instances/{}?organization={}&project={}", + instance_name, ORGANIZATION_NAME, PROJECT_NAME + ) } async fn create_org_and_project(client: &ClientTestContext) -> Uuid { @@ -75,10 +81,7 @@ async fn test_instances_access_before_create_returns_not_found( // Create a project that we'll use for testing. create_organization(&client, ORGANIZATION_NAME).await; - let url_instances = format!( - "/organizations/{}/projects/{}/instances", - ORGANIZATION_NAME, PROJECT_NAME - ); + let url_instances = get_instances_url(); let _ = create_project(&client, ORGANIZATION_NAME, PROJECT_NAME).await; // List instances. There aren't any yet. @@ -86,7 +89,7 @@ async fn test_instances_access_before_create_returns_not_found( assert_eq!(instances.len(), 0); // Make sure we get a 404 if we fetch one. - let instance_url = format!("{}/just-rainsticks", url_instances); + let instance_url = get_instance_url("just-rainsticks"); let error: HttpErrorResponseBody = NexusRequest::expect_failure( client, StatusCode::NOT_FOUND, @@ -189,24 +192,21 @@ async fn test_instances_create_reboot_halt( let client = &cptestctx.external_client; let apictx = &cptestctx.server.apictx; let nexus = &apictx.nexus; + let instance_name = "just-rainsticks"; create_org_and_project(&client).await; - let url_instances = format!( - "/organizations/{}/projects/{}/instances", - ORGANIZATION_NAME, PROJECT_NAME - ); + let url_instances = get_instances_url(); // Create an instance. - let instance_url = format!("{}/just-rainsticks", url_instances); - let instance = create_instance( - client, - ORGANIZATION_NAME, - PROJECT_NAME, - "just-rainsticks", - ) - .await; - assert_eq!(instance.identity.name, "just-rainsticks"); - assert_eq!(instance.identity.description, "instance \"just-rainsticks\""); + let instance_url = get_instance_url(instance_name); + let instance = + create_instance(client, ORGANIZATION_NAME, PROJECT_NAME, instance_name) + .await; + assert_eq!(instance.identity.name, instance_name); + assert_eq!( + instance.identity.description, + format!("instance \"{}\"", instance_name).as_str() + ); let InstanceCpuCount(nfoundcpus) = instance.ncpus; // These particulars are hardcoded in create_instance(). assert_eq!(nfoundcpus, 4); @@ -243,7 +243,10 @@ async fn test_instances_create_reboot_halt( .unwrap() .parsed_body() .unwrap(); - assert_eq!(error.message, "already exists: instance \"just-rainsticks\""); + assert_eq!( + error.message, + format!("already exists: instance \"{}\"", instance_name).as_str() + ); // List instances again and expect to find the one we just created. let instances = instances_list(&client, &url_instances).await; @@ -285,7 +288,7 @@ async fn test_instances_create_reboot_halt( // not even the state timestamp. let instance = instance_next; let instance_next = - instance_post(&client, &instance_url, InstanceOp::Start).await; + instance_post(&client, instance_name, InstanceOp::Start).await; instances_eq(&instance, &instance_next); let instance_next = instance_get(&client, &instance_url).await; instances_eq(&instance, &instance_next); @@ -293,7 +296,7 @@ async fn test_instances_create_reboot_halt( // Reboot the instance. let instance = instance_next; let instance_next = - instance_post(&client, &instance_url, InstanceOp::Reboot).await; + instance_post(&client, instance_name, InstanceOp::Reboot).await; assert_eq!(instance_next.runtime.run_state, InstanceState::Rebooting); assert!( instance_next.runtime.time_run_state_updated @@ -321,7 +324,7 @@ async fn test_instances_create_reboot_halt( // Request a halt and verify both the immediate state and the finished state. let instance = instance_next; let instance_next = - instance_post(&client, &instance_url, InstanceOp::Stop).await; + instance_post(&client, instance_name, InstanceOp::Stop).await; assert_eq!(instance_next.runtime.run_state, InstanceState::Stopping); assert!( instance_next.runtime.time_run_state_updated @@ -341,7 +344,7 @@ async fn test_instances_create_reboot_halt( // not even the state timestamp. let instance = instance_next; let instance_next = - instance_post(&client, &instance_url, InstanceOp::Stop).await; + instance_post(&client, instance_name, InstanceOp::Stop).await; instances_eq(&instance, &instance_next); let instance_next = instance_get(&client, &instance_url).await; instances_eq(&instance, &instance_next); @@ -352,7 +355,7 @@ async fn test_instances_create_reboot_halt( client, StatusCode::BAD_REQUEST, Method::POST, - &format!("{}/reboot", &instance_url), + get_instance_url(format!("{}/reboot", instance_name).as_str()).as_str(), ) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -373,7 +376,7 @@ async fn test_instances_create_reboot_halt( // succeed, having stopped in between. let instance = instance_next; let instance_next = - instance_post(&client, &instance_url, InstanceOp::Start).await; + instance_post(&client, instance_name, InstanceOp::Start).await; assert_eq!(instance_next.runtime.run_state, InstanceState::Starting); assert!( instance_next.runtime.time_run_state_updated @@ -382,7 +385,7 @@ async fn test_instances_create_reboot_halt( let instance = instance_next; let instance_next = - instance_post(&client, &instance_url, InstanceOp::Reboot).await; + instance_post(&client, instance_name, InstanceOp::Reboot).await; assert_eq!(instance_next.runtime.run_state, InstanceState::Rebooting); assert!( instance_next.runtime.time_run_state_updated @@ -412,7 +415,7 @@ async fn test_instances_create_reboot_halt( // state. let instance = instance_next; let instance_next = - instance_post(&client, &instance_url, InstanceOp::Stop).await; + instance_post(&client, instance_name, InstanceOp::Stop).await; assert_eq!(instance_next.runtime.run_state, InstanceState::Stopping); assert!( instance_next.runtime.time_run_state_updated @@ -423,7 +426,7 @@ async fn test_instances_create_reboot_halt( client, StatusCode::BAD_REQUEST, Method::POST, - &format!("{}/reboot", instance_url), + get_instance_url(format!("{}/reboot", instance_name).as_str()).as_str(), ) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -475,7 +478,7 @@ async fn test_instances_create_reboot_halt( client, StatusCode::NOT_FOUND, Method::POST, - &format!("{}/reboot", instance_url), + get_instance_url(format!("{}/reboot", instance_name).as_str()).as_str(), ) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -487,7 +490,7 @@ async fn test_instances_create_reboot_halt( client, StatusCode::NOT_FOUND, Method::POST, - &format!("{}/start", instance_url), + get_instance_url(format!("{}/start", instance_name).as_str()).as_str(), ) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -497,7 +500,7 @@ async fn test_instances_create_reboot_halt( client, StatusCode::NOT_FOUND, Method::POST, - &format!("{}/stop", instance_url), + get_instance_url(format!("{}/stop", instance_name).as_str()).as_str(), ) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -512,6 +515,7 @@ async fn test_instances_create_stopped_start( let client = &cptestctx.external_client; let apictx = &cptestctx.server.apictx; let nexus = &apictx.nexus; + let instance_name = "just-rainsticks"; create_org_and_project(&client).await; let url_instances = format!( @@ -525,8 +529,8 @@ async fn test_instances_create_stopped_start( &url_instances, ¶ms::InstanceCreate { identity: IdentityMetadataCreateParams { - name: "just-rainsticks".parse().unwrap(), - description: "instance just-rainsticks".to_string(), + name: instance_name.parse().unwrap(), + description: format!("instance {}", instance_name), }, ncpus: InstanceCpuCount(4), memory: ByteCount::from_gibibytes_u32(1), @@ -543,9 +547,9 @@ async fn test_instances_create_stopped_start( assert_eq!(instance.runtime.run_state, InstanceState::Stopped); // Start the instance. - let instance_url = format!("{}/just-rainsticks", url_instances); + let instance_url = get_instance_url(instance_name); let instance = - instance_post(&client, &instance_url, InstanceOp::Start).await; + instance_post(&client, instance_name, InstanceOp::Start).await; // Now, simulate completion of instance boot and check the state reported. instance_simulate(nexus, &instance.identity.id).await; @@ -565,15 +569,12 @@ async fn test_instances_delete_fails_when_running_succeeds_when_stopped( let client = &cptestctx.external_client; let apictx = &cptestctx.server.apictx; let nexus = &apictx.nexus; + let instance_name = "just-rainsticks"; create_org_and_project(&client).await; - let url_instances = format!( - "/organizations/{}/projects/{}/instances", - ORGANIZATION_NAME, PROJECT_NAME - ); // Create an instance. - let instance_url = format!("{}/just-rainsticks", url_instances); + let instance_url = get_instance_url(instance_name); let instance = create_instance( client, ORGANIZATION_NAME, @@ -608,7 +609,7 @@ async fn test_instances_delete_fails_when_running_succeeds_when_stopped( // Stop the instance let instance = - instance_post(&client, &instance_url, InstanceOp::Stop).await; + instance_post(&client, instance_name, InstanceOp::Stop).await; instance_simulate(nexus, &instance.identity.id).await; let instance = instance_get(&client, &instance_url).await; assert_eq!(instance.runtime.run_state, InstanceState::Stopped); @@ -1026,6 +1027,7 @@ async fn test_instance_create_delete_network_interface( ) { let client = &cptestctx.external_client; let nexus = &cptestctx.server.apictx.nexus; + let instance_name = "nic-attach-test-inst"; create_org_and_project(&client).await; let url_instances = format!( @@ -1056,7 +1058,7 @@ async fn test_instance_create_delete_network_interface( // Create an instance with no network interfaces let instance_params = params::InstanceCreate { identity: IdentityMetadataCreateParams { - name: Name::try_from(String::from("nic-attach-test-inst")).unwrap(), + name: instance_name.parse().unwrap(), description: String::from("instance to test attaching new nic"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), @@ -1075,8 +1077,6 @@ async fn test_instance_create_delete_network_interface( .await .expect("Failed to create instance with two network interfaces"); let instance = response.parsed_body::().unwrap(); - let url_instance = - format!("{}/{}", url_instances, instance.identity.name.as_str()); // Verify there are no interfaces let url_interfaces = format!( @@ -1143,8 +1143,7 @@ async fn test_instance_create_delete_network_interface( ); // Stop the instance - let instance = - instance_post(client, url_instance.as_str(), InstanceOp::Stop).await; + let instance = instance_post(client, instance_name, InstanceOp::Stop).await; instance_simulate(nexus, &instance.identity.id).await; // Verify we can now make the requests again @@ -1172,7 +1171,7 @@ async fn test_instance_create_delete_network_interface( // Restart the instance, verify the interfaces are still correct. let instance = - instance_post(client, url_instance.as_str(), InstanceOp::Start).await; + instance_post(client, instance_name, InstanceOp::Start).await; instance_simulate(nexus, &instance.identity.id).await; // Get all interfaces in one request. @@ -1214,8 +1213,7 @@ async fn test_instance_create_delete_network_interface( } // Stop the instance and verify we can delete the interface - let instance = - instance_post(client, url_instance.as_str(), InstanceOp::Stop).await; + let instance = instance_post(client, instance_name, InstanceOp::Stop).await; instance_simulate(nexus, &instance.identity.id).await; // We should not be able to delete the primary interface, while the @@ -1274,12 +1272,10 @@ async fn test_instance_update_network_interfaces( ) { let client = &cptestctx.external_client; let nexus = &cptestctx.server.apictx.nexus; + let instance_name = "nic-update-test-inst"; create_org_and_project(&client).await; - let url_instances = format!( - "/organizations/{}/projects/{}/instances", - ORGANIZATION_NAME, PROJECT_NAME - ); + let url_instances = get_instances_url(); // Create the VPC Subnet for the secondary interface let secondary_subnet = params::VpcSubnetCreate { @@ -1304,7 +1300,7 @@ async fn test_instance_update_network_interfaces( // Create an instance with no network interfaces let instance_params = params::InstanceCreate { identity: IdentityMetadataCreateParams { - name: Name::try_from(String::from("nic-update-test-inst")).unwrap(), + name: instance_name.parse().unwrap(), description: String::from("instance to test updatin nics"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), @@ -1323,8 +1319,6 @@ async fn test_instance_update_network_interfaces( .await .expect("Failed to create instance with two network interfaces"); let instance = response.parsed_body::().unwrap(); - let url_instance = - format!("{}/{}", url_instances, instance.identity.name.as_str()); let url_interfaces = format!( "/organizations/{}/projects/{}/instances/{}/network-interfaces", ORGANIZATION_NAME, PROJECT_NAME, instance.identity.name, @@ -1353,8 +1347,7 @@ async fn test_instance_update_network_interfaces( ]; // Stop the instance - let instance = - instance_post(client, url_instance.as_str(), InstanceOp::Stop).await; + let instance = instance_post(client, instance_name, InstanceOp::Stop).await; instance_simulate(nexus, &instance.identity.id).await; // Create the first interface on the instance. @@ -1376,7 +1369,7 @@ async fn test_instance_update_network_interfaces( // Restart the instance, to ensure we can only modify things when it's // stopped. let instance = - instance_post(client, url_instance.as_str(), InstanceOp::Start).await; + instance_post(client, instance_name, InstanceOp::Start).await; instance_simulate(nexus, &instance.identity.id).await; // We'll change the interface's name and description @@ -1414,8 +1407,7 @@ async fn test_instance_update_network_interfaces( ); // Stop the instance again, and now verify that the update works. - let instance = - instance_post(client, url_instance.as_str(), InstanceOp::Stop).await; + let instance = instance_post(client, instance_name, InstanceOp::Stop).await; instance_simulate(nexus, &instance.identity.id).await; let updated_primary_iface = NexusRequest::object_put( client, @@ -1521,7 +1513,7 @@ async fn test_instance_update_network_interfaces( // Restart the instance, and verify that we can't update either interface. let instance = - instance_post(client, url_instance.as_str(), InstanceOp::Start).await; + instance_post(client, instance_name, InstanceOp::Start).await; instance_simulate(nexus, &instance.identity.id).await; for if_name in @@ -1550,8 +1542,7 @@ async fn test_instance_update_network_interfaces( } // Stop the instance again. - let instance = - instance_post(client, url_instance.as_str(), InstanceOp::Stop).await; + let instance = instance_post(client, instance_name, InstanceOp::Stop).await; instance_simulate(nexus, &instance.identity.id).await; // Verify that we can set the secondary as the new primary, and that nothing @@ -1754,6 +1745,7 @@ async fn test_instance_with_multiple_nics_unwinds_completely( #[nexus_test] async fn test_attach_one_disk_to_instance(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; + let instance_name = "nifs"; // Test pre-reqs DiskTest::new(&cptestctx).await; @@ -1782,7 +1774,7 @@ async fn test_attach_one_disk_to_instance(cptestctx: &ControlPlaneTestContext) { // Create the instance let instance_params = params::InstanceCreate { identity: IdentityMetadataCreateParams { - name: Name::try_from(String::from("nfs")).unwrap(), + name: instance_name.parse().unwrap(), description: String::from("probably serving data"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), @@ -1799,10 +1791,7 @@ async fn test_attach_one_disk_to_instance(cptestctx: &ControlPlaneTestContext) { start: true, }; - let url_instances = format!( - "/organizations/{}/projects/{}/instances", - ORGANIZATION_NAME, PROJECT_NAME - ); + let url_instances = get_instances_url(); let builder = RequestBuilder::new(client, http::Method::POST, &url_instances) .body(Some(&instance_params)) @@ -2477,6 +2466,7 @@ async fn test_disks_detached_when_instance_destroyed( cptestctx: &ControlPlaneTestContext, ) { let client = &cptestctx.external_client; + let instance_name = "nfs"; // Test pre-reqs DiskTest::new(&cptestctx).await; @@ -2516,7 +2506,7 @@ async fn test_disks_detached_when_instance_destroyed( // Boot the instance let instance_params = params::InstanceCreate { identity: IdentityMetadataCreateParams { - name: Name::try_from(String::from("nfs")).unwrap(), + name: instance_name.parse().unwrap(), description: String::from("probably serving data"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), @@ -2540,10 +2530,7 @@ async fn test_disks_detached_when_instance_destroyed( start: true, }; - let url_instances = format!( - "/organizations/{}/projects/{}/instances", - ORGANIZATION_NAME, PROJECT_NAME - ); + let url_instances = get_instances_url(); let builder = RequestBuilder::new(client, http::Method::POST, &url_instances) @@ -2583,7 +2570,7 @@ async fn test_disks_detached_when_instance_destroyed( ); let instance = - instance_post(&client, &instance_url, InstanceOp::Stop).await; + instance_post(&client, instance_name, InstanceOp::Stop).await; let apictx = &cptestctx.server.apictx; let nexus = &apictx.nexus; instance_simulate(nexus, &instance.identity.id).await; @@ -2722,16 +2709,16 @@ async fn test_instance_serial(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; let apictx = &cptestctx.server.apictx; let nexus = &apictx.nexus; + let instance_name = "kris-picks"; create_org_and_project(&client).await; - let url_instances = format!( - "/organizations/{}/projects/{}/instances", - ORGANIZATION_NAME, PROJECT_NAME - ); + let instance_url = get_instance_url(instance_name); // Make sure we get a 404 if we try to access the serial console before creation. - let instance_serial_url = - format!("{}/kris-picks/serial-console?from_start=0", url_instances); + let instance_serial_url = get_instance_url( + format!("{}/kris-picks/serial-console?from_start=0", instance_name) + .as_str(), + ); let error: HttpErrorResponseBody = NexusRequest::expect_failure( client, StatusCode::NOT_FOUND, @@ -2744,12 +2731,14 @@ async fn test_instance_serial(cptestctx: &ControlPlaneTestContext) { .unwrap() .parsed_body() .unwrap(); - assert_eq!(error.message, "not found: instance with name \"kris-picks\""); + assert_eq!( + error.message, + format!("not found: instance with name \"{}\"", instance_name).as_str() + ); // Create an instance. - let instance_url = format!("{}/kris-picks", url_instances); let instance = - create_instance(client, ORGANIZATION_NAME, PROJECT_NAME, "kris-picks") + create_instance(client, ORGANIZATION_NAME, PROJECT_NAME, instance_name) .await; // Now, simulate completion of instance boot and check the state reported. @@ -2780,7 +2769,7 @@ async fn test_instance_serial(cptestctx: &ControlPlaneTestContext) { // Request a halt and verify both the immediate state and the finished state. let instance = instance_next; let instance_next = - instance_post(&client, &instance_url, InstanceOp::Stop).await; + instance_post(&client, instance_name, InstanceOp::Stop).await; assert_eq!(instance_next.runtime.run_state, InstanceState::Stopping); assert!( instance_next.runtime.time_run_state_updated @@ -2921,17 +2910,20 @@ pub enum InstanceOp { pub async fn instance_post( client: &ClientTestContext, - instance_url: &str, + instance_name: &str, which: InstanceOp, ) -> Instance { - let url = format!( - "{}/{}", - instance_url, - match which { - InstanceOp::Start => "start", - InstanceOp::Stop => "stop", - InstanceOp::Reboot => "reboot", - } + let url = get_instance_url( + format!( + "{}/{}", + instance_name, + match which { + InstanceOp::Start => "start", + InstanceOp::Stop => "stop", + InstanceOp::Reboot => "reboot", + } + ) + .as_str(), ); NexusRequest::new( RequestBuilder::new(client, Method::POST, &url) diff --git a/openapi/nexus.json b/openapi/nexus.json index f7654d75e38..1a16e4df14d 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -7341,7 +7341,6 @@ { "in": "query", "name": "project", - "required": true, "schema": { "$ref": "#/components/schemas/NameOrId" } @@ -7350,7 +7349,7 @@ "in": "query", "name": "sort_by", "schema": { - "$ref": "#/components/schemas/NameSortMode" + "$ref": "#/components/schemas/NameOrIdSortMode" } } ], @@ -8155,7 +8154,6 @@ { "in": "query", "name": "organization", - "required": true, "schema": { "$ref": "#/components/schemas/NameOrId" } From 124e20a7b2e909a93344888430e46d82ee2756dc Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Fri, 6 Jan 2023 17:22:16 -0500 Subject: [PATCH 5/8] Fix some failing tests --- nexus/src/external_api/http_entrypoints.rs | 11 +++++------ nexus/test-utils/src/http_testing.rs | 4 +++- nexus/tests/integration_tests/instances.rs | 7 ++++--- nexus/tests/integration_tests/vpc_subnets.rs | 7 ++++--- nexus/types/src/external_api/params.rs | 9 --------- 5 files changed, 16 insertions(+), 22 deletions(-) diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 21d520152cc..bf1e0bab369 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -3067,14 +3067,16 @@ async fn instance_stop( async fn instance_serial_console_v1( rqctx: Arc>>, path_params: Path, - query_params: Query, + query_params: Query, + selector_params: Query, ) -> Result, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; let path = path_params.into_inner(); let query = query_params.into_inner(); + let selector = selector_params.into_inner(); let instance_selector = params::InstanceSelector { - project_selector: query.project_selector, + project_selector: selector.project_selector, instance: path.instance, }; let handler = async { @@ -3082,10 +3084,7 @@ async fn instance_serial_console_v1( let instance_lookup = nexus.instance_lookup(&opctx, &instance_selector)?; let data = nexus - .instance_serial_console_data( - &instance_lookup, - &query.console_params, - ) + .instance_serial_console_data(&instance_lookup, &query) .await?; Ok(HttpResponseOk(data)) }; diff --git a/nexus/test-utils/src/http_testing.rs b/nexus/test-utils/src/http_testing.rs index 2030a89b702..02232345509 100644 --- a/nexus/test-utils/src/http_testing.rs +++ b/nexus/test-utils/src/http_testing.rs @@ -662,8 +662,10 @@ impl<'a> NexusRequest<'a> { loop { let url = if let Some(next_token) = &next_token { format!("{}&page_token={}", url_base, next_token) + } else if !initial_params.is_empty() { + format!("{}&{}", url_base, initial_params) } else { - format!("{}{}", url_base, initial_params) + url_base.clone() }; let page = NexusRequest::object_get(testctx, &url) diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index 2d4527e7f71..b4134961907 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -2715,9 +2715,10 @@ async fn test_instance_serial(cptestctx: &ControlPlaneTestContext) { let instance_url = get_instance_url(instance_name); // Make sure we get a 404 if we try to access the serial console before creation. - let instance_serial_url = get_instance_url( - format!("{}/kris-picks/serial-console?from_start=0", instance_name) - .as_str(), + let instance_serial_url = format!( + "{}&{}", + get_instance_url(format!("{}/serial-console", instance_name).as_str()), + "from_start=0" ); let error: HttpErrorResponseBody = NexusRequest::expect_failure( client, diff --git a/nexus/tests/integration_tests/vpc_subnets.rs b/nexus/tests/integration_tests/vpc_subnets.rs index 67f6ed77d0b..17a968df5b8 100644 --- a/nexus/tests/integration_tests/vpc_subnets.rs +++ b/nexus/tests/integration_tests/vpc_subnets.rs @@ -38,6 +38,7 @@ async fn test_delete_vpc_subnet_with_interfaces_fails( // Create a project that we'll use for testing. let org_name = "test-org"; let project_name = "springfield-squidport"; + let instance_name = "inst"; create_organization(&client, &org_name).await; let _ = create_project(&client, org_name, project_name).await; populate_ip_pool(client, "default", None).await; @@ -56,10 +57,10 @@ async fn test_delete_vpc_subnet_with_interfaces_fails( // Create an instance in the default VPC and VPC Subnet. Verify that we // cannot delete the subnet until the instance is gone. let instance_url = format!( - "/organizations/{org_name}/projects/{project_name}/instances/inst" + "/organizations/{org_name}/projects/{project_name}/instances/{instance_name}" ); let instance = - create_instance(client, &org_name, project_name, "inst").await; + create_instance(client, &org_name, project_name, instance_name).await; instance_simulate(nexus, &instance.identity.id).await; let err: HttpErrorResponseBody = NexusRequest::expect_failure( &client, @@ -80,7 +81,7 @@ async fn test_delete_vpc_subnet_with_interfaces_fails( ); // Stop and then delete the instance - instance_post(client, &instance_url, InstanceOp::Stop).await; + instance_post(client, instance_name, InstanceOp::Stop).await; instance_simulate(&nexus, &instance.identity.id).await; NexusRequest::object_delete(&client, &instance_url) .authn_as(AuthnMode::PrivilegedUser) diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 84c7c497c95..efcd31f2473 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -96,15 +96,6 @@ impl InstanceSelector { } } -#[derive(Deserialize, JsonSchema)] -pub struct InstanceSerialConsole { - #[serde(flatten)] - pub project_selector: Option, - - #[serde(flatten)] - pub console_params: InstanceSerialConsoleRequest, -} - // Silos /// Create-time parameters for a [`Silo`](crate::external_api::views::Silo) From eca4e2998a0910281979616e24e27df2bcee59bd Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Sat, 7 Jan 2023 00:35:00 -0500 Subject: [PATCH 6/8] Update pagination schema output --- common/tests/output/pagination-schema.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/tests/output/pagination-schema.txt b/common/tests/output/pagination-schema.txt index 721bb8f7d2b..732f853fd88 100644 --- a/common/tests/output/pagination-schema.txt +++ b/common/tests/output/pagination-schema.txt @@ -63,7 +63,7 @@ schema for pagination parameters: scan parameters, scan by id only schema for pagination parameters: scan parameters, scan by name or id { "$schema": "http://json-schema.org/draft-07/schema#", - "title": "ScanByNameOrId", + "title": "ScanByNameOrId_for_Null", "description": "Scan parameters for resources that support scanning by name or id", "type": "object", "properties": { @@ -196,7 +196,7 @@ schema for pagination parameters: page selector, scan by id only schema for pagination parameters: page selector, scan by name or id { "$schema": "http://json-schema.org/draft-07/schema#", - "title": "PageSelector_for_ScanByNameOrId_and_NameOrId", + "title": "PageSelector_for_ScanByNameOrId_for_Null_and_NameOrId", "description": "Specifies which page of results we're on\n\nThis type is generic over the different scan modes that we support.", "type": "object", "required": [ From 397dd8d7c832eb9b2d7eb7cd32d05b7866d8691a Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Sat, 7 Jan 2023 01:49:32 -0500 Subject: [PATCH 7/8] Update nexus.json --- openapi/nexus.json | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/openapi/nexus.json b/openapi/nexus.json index 6d4932473cc..a1c546824e2 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -7632,8 +7632,8 @@ } ], "responses": { - "200": { - "description": "successful operation", + "202": { + "description": "successfully enqueued operation", "content": { "application/json": { "schema": { @@ -7811,8 +7811,8 @@ } ], "responses": { - "200": { - "description": "successful operation", + "202": { + "description": "successfully enqueued operation", "content": { "application/json": { "schema": { @@ -7862,8 +7862,8 @@ } ], "responses": { - "200": { - "description": "successful operation", + "202": { + "description": "successfully enqueued operation", "content": { "application/json": { "schema": { From 797008e41128b417009156dfcf465b49a7361d15 Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 10 Jan 2023 16:08:58 -0500 Subject: [PATCH 8/8] Use default parameter for pagination type --- common/src/api/external/http_pagination.rs | 4 ++-- nexus/src/external_api/http_entrypoints.rs | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/common/src/api/external/http_pagination.rs b/common/src/api/external/http_pagination.rs index e373c6b69af..601534db431 100644 --- a/common/src/api/external/http_pagination.rs +++ b/common/src/api/external/http_pagination.rs @@ -296,7 +296,7 @@ impl ScanParams for ScanById { // We include this now primarily to exercise the interface for doing so. /// Query parameters for pagination by name or id -pub type PaginatedByNameOrId = PaginationParams< +pub type PaginatedByNameOrId = PaginationParams< ScanByNameOrId, PageSelectorByNameOrId, >; @@ -733,7 +733,7 @@ mod test { #[test] fn test_scan_by_nameid_generic() { // Test from_query(): error case. - let error = serde_urlencoded::from_str::>( + let error = serde_urlencoded::from_str::( "sort_by=id_descending", ) .unwrap_err(); diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index bf3bd607418..0337d6c8ac8 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -472,7 +472,7 @@ async fn policy_update( }] async fn silo_list( rqctx: Arc>>, - query_params: Query>, + query_params: Query, ) -> Result>, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; @@ -937,7 +937,7 @@ async fn local_idp_user_set_password( }] async fn organization_list_v1( rqctx: Arc>>, - query_params: Query>, + query_params: Query, ) -> Result>, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; @@ -980,7 +980,7 @@ async fn organization_list_v1( }] async fn organization_list( rqctx: Arc>>, - query_params: Query>, + query_params: Query, ) -> Result>, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; @@ -1479,7 +1479,7 @@ async fn project_list_v1( }] async fn project_list( rqctx: Arc>>, - query_params: Query>, + query_params: Query, path_params: Path, ) -> Result>, HttpError> { let apictx = rqctx.context(); @@ -1950,7 +1950,7 @@ pub struct IpPoolPathParam { }] async fn ip_pool_list( rqctx: Arc>>, - query_params: Query>, + query_params: Query, ) -> Result>, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus;