better message, but at what cost

oxidecomputer · Jan 24, 2024 · e7b60ee · e7b60ee
1 parent 624fbba
commit e7b60ee
Show file tree

Hide file tree

Showing 4 changed files with 18 additions and 27 deletions.
diff --git a/common/src/api/external/error.rs b/common/src/api/external/error.rs
@@ -144,11 +144,11 @@ pub enum LookupType {
     ByName(String),
     /// a specific id was requested
     ById(Uuid),
-    /// a session token was requested
-    BySessionToken(String),
     /// a specific id was requested with some composite type
     /// (caller summarizes it)
     ByCompositeId(String),
+    /// object selected by criteria that would be confusing to call an ID
+    ByOther(String),
 }
 
 impl LookupType {
@@ -359,23 +359,22 @@ impl From<Error> for HttpError {
     fn from(error: Error) -> HttpError {
         match error {
             Error::ObjectNotFound { type_name: t, lookup_type: lt } => {
-                // TODO-cleanup is there a better way to express this?
-                let (lookup_field, lookup_value) = match lt {
-                    LookupType::ByName(name) => ("name", name),
-                    LookupType::ById(id) => ("id", id.to_string()),
-                    LookupType::ByCompositeId(label) => ("id", label),
-                    LookupType::BySessionToken(token) => {
-                        ("session token", token)
+                let message = match lt {
+                    LookupType::ByName(name) => {
+                        format!("{} with name \"{}\"", t, name)
                     }
+                    LookupType::ById(id) => {
+                        format!("{} with id \"{}\"", t, id)
+                    }
+                    LookupType::ByCompositeId(label) => {
+                        format!("{} with id \"{}\"", t, label)
+                    }
+                    LookupType::ByOther(msg) => msg,
                 };
-                let message = format!(
-                    "not found: {} with {} \"{}\"",
-                    t, lookup_field, lookup_value
-                );
                 HttpError::for_client_error(
                     Some(String::from("ObjectNotFound")),
                     http::StatusCode::NOT_FOUND,
-                    message,
+                    format!("not found: {}", message),
                 )
             }
 

diff --git a/nexus/db-queries/src/authz/policy_test/resource_builder.rs b/nexus/db-queries/src/authz/policy_test/resource_builder.rs
@@ -92,7 +92,7 @@ impl<'a> ResourceBuilder<'a> {
                 // (e.g., "fleet").
                 resource.resource_type().to_string().to_lowercase()
             }
-            LookupType::BySessionToken(_) | LookupType::ByCompositeId(_) => {
+            LookupType::ByCompositeId(_) | LookupType::ByOther(_) => {
                 panic!("test resources must be given names");
             }
         };
@@ -212,7 +212,7 @@ where
             LookupType::ByName(name) => format!("{:?}", name),
             LookupType::ById(id) => format!("id {:?}", id.to_string()),
             LookupType::ByCompositeId(id) => format!("id {:?}", id),
-            LookupType::BySessionToken(_) => {
+            LookupType::ByOther(_) => {
                 unimplemented!()
             }
         };

diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs
@@ -134,12 +134,8 @@ impl DataStore {
         //     .authorize(authz::Action::ListChildren, &authz::IP_POOL_LIST)
         //     .await?;
 
-        // join ip_pool to ip_pool_resource and filter
-
-        // used in both success and error outcomes
-        let lookup_type = LookupType::ByCompositeId(
-            "Default pool for current silo".to_string(),
-        );
+        let lookup_type =
+            LookupType::ByOther("default IP pool for current silo".to_string());
 
         ip_pool::table
             .inner_join(ip_pool_resource::table)
@@ -161,9 +157,6 @@ impl DataStore {
             )
             .await
             .map_err(|e| {
-                // janky to do this manually, but this is an unusual kind of
-                // lookup in that it is by (silo_id, is_default=true), which is
-                // arguably a composite ID.
                 public_error_from_diesel_lookup(
                     e,
                     ResourceType::IpPool,

diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs
@@ -3838,8 +3838,7 @@ async fn test_instance_ephemeral_ip_no_default_pool_error(
     let url = format!("/v1/instances?project={}", PROJECT_NAME);
     let error =
         object_create_error(client, &url, &body, StatusCode::NOT_FOUND).await;
-    let msg = "not found: ip-pool with id \"Default pool for current silo\""
-        .to_string();
+    let msg = "not found: default IP pool for current silo".to_string();
     assert_eq!(error.message, msg);
 
     // same deal if you specify a pool that doesn't exist