diff --git a/src/tables.rs b/src/tables.rs index 1869cd25..f4b31f07 100644 --- a/src/tables.rs +++ b/src/tables.rs @@ -551,7 +551,72 @@ impl StateDb { } } +/// The opt-in list of record types whose [`Table`] may be scanned generically. +/// +/// A [`Table`] scan covers the whole column family, so it is safe only where +/// the column family holds nothing but records. Most tables are of that shape +/// and are listed below; `operation_attempt` is not, because it reserves the +/// top of its key space for secondary indexes, and is therefore deliberately +/// absent. +/// +/// The trait is not exported, so this list is the only way in: nothing outside +/// the crate can implement it, and a record type added here gets generic +/// iteration only once someone writes it down. That is the point — the +/// exclusion holds unless it is removed on purpose, rather than lapsing the +/// next time a bound elsewhere is broadened. +mod iteration { + use crate::{tables, types}; + + pub trait Eligible {} + + impl Eligible for tables::AccessToken {} + impl Eligible for types::Account {} + impl Eligible for tables::Agent {} + impl Eligible for tables::AllowNetwork {} + impl Eligible for crate::BatchInfo {} + impl Eligible for tables::BlockNetwork {} + impl Eligible for crate::Category {} + impl Eligible for tables::Cluster {} + impl Eligible for tables::ColumnStats {} + impl Eligible for tables::CoreComponent {} + impl Eligible for tables::CsvColumnExtra {} + impl Eligible for tables::Customer {} + impl Eligible for tables::CustomerDataDeletionJob {} + impl Eligible for tables::DataSource {} + impl Eligible for tables::ExternalService {} + impl Eligible for tables::Filter {} + impl Eligible for tables::Host {} + impl Eligible for tables::InnerNode {} + impl Eligible for tables::LabelDb {} + impl Eligible for tables::Model {} + impl Eligible for tables::ModelIndicator {} + impl Eligible for tables::Network {} + impl Eligible for tables::OutlierInfo {} + impl Eligible for types::Qualifier {} + impl Eligible for tables::SamplingPolicy {} + impl Eligible for types::Status {} + impl Eligible for tables::Template {} + impl Eligible for tables::TimeSeries {} + impl Eligible for tables::TorExitNode {} + impl Eligible for tables::TrafficFilter {} + impl Eligible for tables::TriageExclusionReason {} + impl Eligible for tables::TriagePolicy {} + impl Eligible for tables::TriageResponse {} + impl Eligible for tables::TrustedDomain {} + impl Eligible for tables::TrustedUserAgent {} +} + /// Represents a table that can be iterated over. +/// +/// A [`Table`] offers this only for a record type whose column family holds +/// records and nothing else, because both scans cover key ranges the caller +/// does not choose: [`iter`](Iterable::iter) covers the whole column family, +/// and [`prefix_iter`](Iterable::prefix_iter) covers whatever the given +/// prefix spans, which for an empty prefix is again the whole of it. A table +/// that reserves part of its key space for secondary index entries — at +/// present only `operation_attempt`, whose keys are described on +/// [`OperationAttempt`] — is therefore left out, and offers a bounded +/// iterator of its own that stops below the reserved range. pub trait Iterable<'i, I> where I: Iterator, @@ -718,11 +783,16 @@ impl Table<'_, R> { } } +// The `iteration::Eligible` bound is what keeps `Table` out +// of this implementation, whose scans reach the index key spaces that table +// reserves. `IndexedTable` below does not carry the bound: no table that +// reserves part of its key space is indexed, and `NodeTable` iterates through +// that implementation. impl<'i, 'j, 'k, R> Iterable<'i, TableIter<'k, R>> for Table<'j, R> where 'j: 'k, 'i: 'k, - R: FromKeyValue, + R: FromKeyValue + iteration::Eligible, { fn iter(&self, direction: Direction, from: Option<&[u8]>) -> TableIter<'k, R> { use rocksdb::IteratorMode; diff --git a/src/tables/agent.rs b/src/tables/agent.rs index 547f7796..e7f1cc79 100644 --- a/src/tables/agent.rs +++ b/src/tables/agent.rs @@ -342,6 +342,37 @@ mod test { assert!(result.is_none()); } + /// This column family holds records and nothing else, so the generic + /// `Iterable` API stays available for it: both of its scans cover a key + /// range the caller does not choose, and here every key in that range is a + /// record. + #[test] + fn generic_iteration_yields_every_stored_agent() { + let (_permit, store) = setup_store(); + let table = store.agents_map(); + + let first = create_agent(1, "001.piglet", AgentKind::Sensor, Some(VALID_TOML), None); + let second = create_agent(1, "002.piglet", AgentKind::Sensor, Some(VALID_TOML), None); + table.insert(&first).unwrap(); + table.insert(&second).unwrap(); + + let in_key_order = vec![first, second]; + assert_eq!( + table + .iter(rocksdb::Direction::Forward, None) + .collect::>>() + .unwrap(), + in_key_order + ); + assert_eq!( + table + .prefix_iter(rocksdb::Direction::Forward, None, b"") + .collect::>>() + .unwrap(), + in_key_order + ); + } + #[test] fn new_agent_has_no_install_state() { let agent = create_agent(1, "test_key", AgentKind::Sensor, Some(VALID_TOML), None); diff --git a/src/tables/core_component.rs b/src/tables/core_component.rs index 36a0f3d1..a75a5f38 100644 --- a/src/tables/core_component.rs +++ b/src/tables/core_component.rs @@ -440,6 +440,39 @@ mod tests { table.delete("roxyd", "host-99.example.com").unwrap(); } + /// This column family holds records and nothing else, so the generic + /// `Iterable` API stays available for it: both of its scans cover a key + /// range the caller does not choose, and here every key in that range is a + /// record. + #[test] + fn generic_iteration_yields_every_stored_record() { + let test_db = TestDb::new(); + let table = test_db.table(); + + let bootroot = installed("bootroot", "host-01.example.com"); + let roxyd = installed("roxyd", "host-01.example.com"); + table.insert(&bootroot).unwrap(); + table.insert(&roxyd).unwrap(); + + // The component name is length-prefixed, so the shorter one sorts + // first whatever the letters say. + let in_key_order = vec![roxyd, bootroot]; + assert_eq!( + table + .iter(Direction::Forward, None) + .collect::>>() + .unwrap(), + in_key_order + ); + assert_eq!( + table + .prefix_iter(Direction::Forward, None, b"") + .collect::>>() + .unwrap(), + in_key_order + ); + } + /// `get` and `delete` build their key from the pair they are given, so /// iteration is the one path that hands `from_key_value` raw bytes. #[test] diff --git a/src/tables/external_service.rs b/src/tables/external_service.rs index dbdad462..fbe81962 100644 --- a/src/tables/external_service.rs +++ b/src/tables/external_service.rs @@ -220,8 +220,8 @@ mod test { use std::sync::Arc; use super::*; - use crate::Store; use crate::test::{DbGuard, acquire_db_permit}; + use crate::{Iterable, Store}; const VALID_TOML: &str = r#"test = "true""#; @@ -345,6 +345,38 @@ mod test { assert!(external_service.bound_addrs.is_empty()); } + /// This column family holds records and nothing else, so the generic + /// `Iterable` API stays available for it: both of its scans cover a key + /// range the caller does not choose, and here every key in that range is a + /// record. + #[test] + fn generic_iteration_yields_every_stored_external_service() { + let (_permit, store) = setup_store(); + let table = store.external_service_map(); + + let first = create_external_service(1, "001.store", ExternalServiceKind::DataStore, None); + let second = + create_external_service(1, "002.store", ExternalServiceKind::TiContainer, None); + table.insert(&first).unwrap(); + table.insert(&second).unwrap(); + + let in_key_order = vec![first, second]; + assert_eq!( + table + .iter(rocksdb::Direction::Forward, None) + .collect::>>() + .unwrap(), + in_key_order + ); + assert_eq!( + table + .prefix_iter(rocksdb::Direction::Forward, None, b"") + .collect::>>() + .unwrap(), + in_key_order + ); + } + #[test] fn install_state_round_trips() { let empty = create_external_service( diff --git a/src/tables/node.rs b/src/tables/node.rs index ee81a942..22184d39 100644 --- a/src/tables/node.rs +++ b/src/tables/node.rs @@ -1302,6 +1302,67 @@ mod test { assert_eq!(returned, node); } + /// `NodeTable` writes its own `Iterable` implementation over an + /// `IndexedTable`, so it is the one iteration route that is neither + /// the blanket `Table` implementation nor a table-specific inherent + /// iterator, and it composes each `Node` from three column families. Its + /// inherent `iter` shadows the trait method under method syntax, so that + /// half is called through UFCS to keep the trait route the thing under + /// test. + #[test] + fn generic_iteration_yields_every_stored_node() { + let (_permit, store) = setup_store(); + + let agent_kinds = vec![AgentKind::Sensor]; + let agent_configs = create_agent_configs(&agent_kinds); + let external_service_kinds = vec![ExternalServiceKind::DataStore]; + let external_service_configs = create_external_service_configs(&external_service_kinds); + + let node_table = store.node_map(); + let mut names = vec![]; + for name in ["node-a", "node-b"] { + // Two nodes cannot share a hostname, so each carries its own. + let profile = Profile { + hostname: format!("{name}.example.com"), + ..Profile::default() + }; + let node = create_node( + 0, + name, + None, + Some(profile), + None, + create_agents(0, &agent_kinds, &agent_configs, &[None]), + create_external_services(0, &external_service_kinds, &external_service_configs), + ); + node_table.put(&node).unwrap(); + names.push(name.to_string()); + } + + for iterated in [ + Iterable::iter(&node_table, Direction::Forward, None) + .collect::>>() + .unwrap(), + node_table + .prefix_iter(Direction::Forward, None, b"") + .collect::>>() + .unwrap(), + ] { + let mut read: Vec<_> = iterated + .iter() + .map(|node| node.name.clone()) + .collect::>(); + read.sort_unstable(); + assert_eq!(read, names); + // The agent and the external service come from their own column + // families, so an empty one would mean the composition was skipped. + for node in &iterated { + assert_eq!(node.agents.len(), 1); + assert_eq!(node.external_services.len(), 1); + } + } + } + #[test] fn remove() { let (_permit, store) = setup_store(); diff --git a/src/tables/operation_attempt.rs b/src/tables/operation_attempt.rs index a306aa44..aa457518 100644 --- a/src/tables/operation_attempt.rs +++ b/src/tables/operation_attempt.rs @@ -40,6 +40,17 @@ //! learn about, or drop one and leave its entries behind. The record's key and //! serialized value are reached through [`OperationAttempt::record_key`] and //! [`OperationAttempt::record_value`], which are private to this module. +//! +//! The same boundary governs reading. [`Table::iter`] bounds its scan below +//! [`RESERVED`], so it yields records and nothing else, and the generic +//! [`Iterable`](crate::Iterable) API is not implemented for this record at +//! all. Its scans cover key ranges the caller does not choose — the whole +//! column family for `iter`, and for `prefix_iter` whatever the prefix spans, +//! which an empty or deliberately reserved prefix extends over the index +//! entries — and an index key is not a record and does not decode as one, so +//! such a scan would yield a decoding error rather than a row. That exclusion +//! is enforced as the write one is, by a bound this record does not satisfy +//! rather than by convention. use std::borrow::Cow; use std::collections::HashMap; @@ -200,6 +211,39 @@ pub struct RetryPolicy { /// fn generic_write_api() {} /// generic_write_api::(); /// ``` +/// +/// # Iterating +/// +/// The record space and the index spaces share one column family, so a scan +/// whose range the caller does not choose can run off the records and into the +/// index entries, which are not records and do not decode as one. The generic +/// [`Iterable`](crate::Iterable) API on [`Table`] is therefore not implemented +/// for this record: `iter` there covers the whole column family, and +/// `prefix_iter` covers whatever the prefix spans, which for an empty or +/// deliberately reserved prefix is the index entries too. `Table::iter` on +/// this table is the supported record iterator, and it bounds its scan below +/// the reserved range. +/// +/// A record whose column family holds records alone is admitted: +/// +/// ``` +/// use review_database::{Iterable, Table, TorExitNode}; +/// +/// fn generic_iteration(table: &Table) { +/// let _ = table.prefix_iter(todo!(), None, b""); +/// } +/// ``` +/// +/// An `OperationAttempt` is not, which is what stops a caller from reaching an +/// index entry through a scan that expects a row: +/// +/// ```compile_fail,E0599 +/// use review_database::{Iterable, OperationAttempt, Table}; +/// +/// fn generic_iteration(table: &Table) { +/// let _ = table.prefix_iter(todo!(), None, b""); +/// } +/// ``` #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] pub struct OperationAttempt { /// The globally unique key of the logical operation. Never empty. @@ -504,10 +548,13 @@ impl<'d> Table<'d, OperationAttempt> { /// Returns an iterator over the stored attempts, whose keys are the /// idempotency keys in lexicographic order. /// - /// This shadows the blanket [`Iterable::iter`](crate::Iterable) - /// implementation on [`Table`], whose scan covers the whole column family - /// and would therefore also yield the index key spaces, which are not - /// records and do not decode as one. + /// This is the table's only iterator. The generic + /// [`Iterable`](crate::Iterable) API on [`Table`] is not implemented for + /// this record, because its scans cover key ranges the caller does not + /// choose and would therefore also yield the index key spaces, which are + /// not records and do not decode as one. This one stops below the first + /// byte the index key spaces reserve, so it yields records and nothing + /// else. #[must_use] pub fn iter( &self, @@ -1365,6 +1412,30 @@ mod tests { // A key that was never written seeks to the next one in the direction // of travel, so a scan resuming past a deleted row does not stall. assert_eq!(keys(&table, Direction::Forward, Some(b"op-25")), ["op-3"]); + + // An attempt that is non-terminal and still owes a cleanup owns an + // entry in all three indexes, so it is the case that puts the most of + // the reserved space in the iterator's way. + let mut owes_cleanup = module_attempt("op-4"); + owes_cleanup.instance = Some(3); + owes_cleanup.cleanup_state = Some(CleanupState::PendingDeregister); + table.upsert(&owes_cleanup).unwrap(); + let index_entries = test_db + .raw_keys() + .into_iter() + .filter(|key| key.first().is_some_and(|first| *first >= RESERVED)) + .count(); + assert_eq!( + index_entries, 9, + "the three earlier attempts own two entries each, and this one owns three" + ); + + // `keys` unwraps the decode of every item, so this is also the + // assertion that the bounded scan yields no index key. + assert_eq!( + keys(&table, Direction::Forward, None), + ["op-1", "op-2", "op-3", "op-4"] + ); } #[test]