pagination should not require ObjectIdentity

common/src/api/external/http_pagination.rs

@@ -92,12 +92,6 @@ pub trait ScanParams:
     /// Return the direction of the scan
     fn direction(&self) -> PaginationOrder;
 
-    /// Given an item, return the appropriate marker value
-    ///
-    /// For example, when scanning by name, this returns the "name" field of the
-    /// item.
-    fn marker_for_item<T: ObjectIdentity>(&self, t: &T) -> Self::MarkerValue;
-
     /// Given pagination parameters, return the current scan parameters
     ///
     /// This can fail if the pagination parameters are not self-consistent (e.g.,
@@ -112,28 +106,72 @@ pub trait ScanParams:
     ///
     /// `list` contains the items that should appear on the page.  It's not
     /// expected that consumers would override this implementation.
-    fn results_page<T>(
+    ///
+    /// `marker_for_item` is a function that returns the appropriate marker
+    /// value for a given item.  For example, when scanning by name, this
+    /// returns the "name" field of the item.
+    fn results_page<T, F>(
         query: &PaginationParams<Self, PageSelector<Self, Self::MarkerValue>>,
         list: Vec<T>,
+        marker_for_item: &F,
     ) -> Result<ResultsPage<T>, dropshot::HttpError>
     where
-        T: ObjectIdentity + Serialize,
+        F: Fn(&Self, &T) -> Self::MarkerValue,
+        T: Serialize,
     {
         let scan_params = Self::from_query(query)?;
-        ResultsPage::new(list, scan_params, page_selector_for)
+        let page_selector =
+            |item: &T, s: &Self| page_selector_for(item, s, marker_for_item);
+        ResultsPage::new(list, scan_params, page_selector)
+    }
+}
+
+/// Marker function that extracts the "name" from an object
+///
+/// This is intended for use with [`ScanByName::results_page`] with objects that
+/// impl [`ObjectIdentity`].
+pub fn marker_for_name<S, T: ObjectIdentity>(_: &S, t: &T) -> Name {
+    t.identity().name.clone()
+}
+
+/// Marker function that extracts the "id" from an object
+///
+/// This is intended for use with [`ScanById::results_page`] with objects that
+/// impl [`ObjectIdentity`].
+pub fn marker_for_id<S, T: ObjectIdentity>(_: &S, t: &T) -> Uuid {
+    t.identity().id
+}
+
+/// Marker function that extracts the "name" or "id" from an object, depending
+/// on the scan in use
+///
+/// This is intended for use with [`ScanByNameOrId::results_page`] with objects
+/// that impl [`ObjectIdentity`].
+pub fn marker_for_name_or_id<T: ObjectIdentity>(
+    scan: &ScanByNameOrId,
+    item: &T,
+) -> NameOrIdMarker {
+    let identity = item.identity();
+    match pagination_field_for_scan_params(scan) {
+        PagField::Name => NameOrIdMarker::Name(identity.name.clone()),
+        PagField::Id => NameOrIdMarker::Id(identity.id),
     }
 }
 
 /// See `dropshot::ResultsPage::new`
-fn page_selector_for<T, S, M>(item: &T, scan_params: &S) -> PageSelector<S, M>
+fn page_selector_for<F, T, S, M>(
+    item: &T,
+    scan_params: &S,
+    marker_for_item: &F,
+) -> PageSelector<S, M>
 where
-    T: ObjectIdentity,
+    F: Fn(&S, &T) -> M,
     S: ScanParams<MarkerValue = M>,
     M: Clone + Debug + DeserializeOwned + PartialEq + Serialize,
 {
     PageSelector {
         scan: scan_params.clone(),
-        last_seen: scan_params.marker_for_item(item),
+        last_seen: marker_for_item(scan_params, item),
     }
 }
 
@@ -206,9 +244,6 @@ impl ScanParams for ScanByName {
     fn direction(&self) -> PaginationOrder {
         PaginationOrder::Ascending
     }
-    fn marker_for_item<T: ObjectIdentity>(&self, item: &T) -> Name {
-        item.identity().name.clone()
-    }
     fn from_query(
         p: &PaginationParams<Self, PageSelector<Self, Self::MarkerValue>>,
     ) -> Result<&Self, HttpError> {
@@ -251,9 +286,6 @@ impl ScanParams for ScanById {
     fn direction(&self) -> PaginationOrder {
         PaginationOrder::Ascending
     }
-    fn marker_for_item<T: ObjectIdentity>(&self, item: &T) -> Uuid {
-        item.identity().id
-    }
     fn from_query(p: &PaginatedById) -> Result<&Self, HttpError> {
         Ok(match p.page {
             WhichPage::First(ref scan_params) => scan_params,
@@ -337,14 +369,6 @@ impl ScanParams for ScanByNameOrId {
         }
     }
 
-    fn marker_for_item<T: ObjectIdentity>(&self, item: &T) -> NameOrIdMarker {
-        let identity = item.identity();
-        match pagination_field_for_scan_params(self) {
-            PagField::Name => NameOrIdMarker::Name(identity.name.clone()),
-            PagField::Id => NameOrIdMarker::Id(identity.id),
-        }
-    }
-
     fn from_query(
         p: &PaginationParams<Self, PageSelector<Self, Self::MarkerValue>>,
     ) -> Result<&Self, HttpError> {
@@ -446,6 +470,9 @@ 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;
@@ -617,32 +644,34 @@ mod test {
     }
 
     /// Function for running a bunch of tests on a ScanParams type.
-    fn test_scan_param_common<S>(
+    fn test_scan_param_common<F, S>(
         list: &Vec<MyThing>,
         scan: &S,
         querystring: &str,
         item0_marker: &S::MarkerValue,
         itemlast_marker: &S::MarkerValue,
         scan_default: &S,
+        marker_for_item: &F,
     ) -> (
         PaginationParams<S, PageSelector<S, S::MarkerValue>>,
         PaginationParams<S, PageSelector<S, S::MarkerValue>>,
     )
     where
         S: ScanParams,
+        F: Fn(&S, &MyThing) -> S::MarkerValue,
     {
         let li = list.len() - 1;
 
         // Test basic parts of ScanParams interface.
-        assert_eq!(&scan.marker_for_item(&list[0]), item0_marker);
-        assert_eq!(&scan.marker_for_item(&list[li]), itemlast_marker);
+        assert_eq!(&marker_for_item(scan, &list[0]), item0_marker);
+        assert_eq!(&marker_for_item(scan, &list[li]), itemlast_marker);
 
         // Test page_selector_for().
-        let page_selector = page_selector_for(&list[0], scan);
+        let page_selector = page_selector_for(&list[0], scan, marker_for_item);
         assert_eq!(&page_selector.scan, scan);
         assert_eq!(&page_selector.last_seen, item0_marker);
 
-        let page_selector = page_selector_for(&list[li], scan);
+        let page_selector = page_selector_for(&list[li], scan, marker_for_item);
         assert_eq!(&page_selector.scan, scan);
         assert_eq!(&page_selector.last_seen, itemlast_marker);
 
@@ -659,7 +688,7 @@ mod test {
 
         // Generate a results page from that, verify it, pull the token out, and
         // use it to generate pagination parameters for a NextPage request.
-        let page = S::results_page(&p0, list.clone()).unwrap();
+        let page = S::results_page(&p0, list.clone(), marker_for_item).unwrap();
         assert_eq!(&page.items, list);
         assert!(page.next_page.is_some());
         let q = format!("page_token={}", page.next_page.unwrap());
@@ -694,6 +723,7 @@ mod test {
             &"thing0".parse().unwrap(),
             &"thing19".parse().unwrap(),
             &scan,
+            &marker_for_name,
         );
         assert_eq!(scan.direction(), PaginationOrder::Ascending);
 
@@ -733,6 +763,7 @@ mod test {
             &list[0].identity.id,
             &list[list.len() - 1].identity.id,
             &scan,
+            &marker_for_id,
         );
         assert_eq!(scan.direction(), PaginationOrder::Ascending);
 
@@ -798,6 +829,7 @@ mod test {
             &thing0_marker,
             &thinglast_marker,
             &ScanByNameOrId { sort_by: NameOrIdSortMode::NameAscending },
+            &marker_for_name_or_id,
         );
 
         // Verify data pages based on the query params.
@@ -836,6 +868,7 @@ mod test {
             &thing0_marker,
             &thinglast_marker,
             &ScanByNameOrId { sort_by: NameOrIdSortMode::NameAscending },
+            &marker_for_name_or_id,
         );
 
         // Verify data pages based on the query params.

common/src/api/external/mod.rs

@@ -867,44 +867,15 @@ impl DiskState {
 // These are currently only intended for observability by developers.  We will
 // eventually want to flesh this out into something more observable for end
 // users.
-#[derive(ObjectIdentity, Clone, Debug, Serialize, JsonSchema)]
+#[derive(Clone, Debug, Serialize, JsonSchema)]
 pub struct Saga {
     pub id: Uuid,
     pub state: SagaState,
-    // TODO-cleanup This object contains a fake `IdentityMetadata`.  Why?  We
-    // want to paginate these objects.  http_pagination.rs provides a bunch of
-    // useful facilities -- notably `PaginatedById`.  `PaginatedById`
-    // requires being able to take an arbitrary object in the result set and get
-    // its id.  To do that, it uses the `ObjectIdentity` trait, which expects
-    // to be able to return an `IdentityMetadata` reference from an object.
-    // Finally, the pagination facilities just pull the `id` out of that.
-    //
-    // In this case (as well as others, like sleds and racks), we have ids, and
-    // we want to be able to paginate by id, but we don't have full identity
-    // metadata.  (Or we do, but it's similarly faked up.)  What we should
-    // probably do is create a new trait, say `ObjectId`, that returns _just_
-    // an id.  We can provide a blanket impl for anything that impls
-    // IdentityMetadata.  We can define one-off impls for structs like this
-    // one.  Then the id-only pagination interfaces can require just
-    // `ObjectId`.
-    #[serde(skip)]
-    pub identity: IdentityMetadata,
 }
 
 impl From<steno::SagaView> for Saga {
     fn from(s: steno::SagaView) -> Self {
-        Saga {
-            id: Uuid::from(s.id),
-            state: SagaState::from(s.state),
-            identity: IdentityMetadata {
-                // TODO-cleanup See the note in Saga above.
-                id: Uuid::from(s.id),
-                name: format!("saga-{}", s.id).parse().unwrap(),
-                description: format!("saga {}", s.id),
-                time_created: Utc::now(),
-                time_modified: Utc::now(),
-            },
-        }
+        Saga { id: Uuid::from(s.id), state: SagaState::from(s.state) }
     }
 }
 

nexus/src/authn/mod.rs

@@ -186,7 +186,7 @@ impl Context {
         Context {
             kind: Kind::Authenticated(Details {
                 actor: Actor::SiloUser {
-                    silo_user_id: USER_TEST_PRIVILEGED.identity().id,
+                    silo_user_id: USER_TEST_PRIVILEGED.id(),
                     silo_id: USER_TEST_PRIVILEGED.silo_id,
                 },
             }),
@@ -201,7 +201,7 @@ impl Context {
         Context {
             kind: Kind::Authenticated(Details {
                 actor: Actor::SiloUser {
-                    silo_user_id: USER_TEST_UNPRIVILEGED.identity().id,
+                    silo_user_id: USER_TEST_UNPRIVILEGED.id(),
                     silo_id: USER_TEST_UNPRIVILEGED.silo_id,
                 },
             }),

nexus/src/db/identity.rs

@@ -49,14 +49,4 @@ pub trait Asset {
     fn id(&self) -> Uuid;
     fn time_created(&self) -> DateTime<Utc>;
     fn time_modified(&self) -> DateTime<Utc>;
-
-    fn identity(&self) -> external::IdentityMetadata {
-        external::IdentityMetadata {
-            id: self.id(),
-            name: "no-name".parse().unwrap(),
-            description: "no description".to_string(),
-            time_created: self.time_created(),
-            time_modified: self.time_modified(),
-        }
-    }
 }