diff --git a/workspaces/api/migration/migration-helpers/src/datastore.rs b/workspaces/api/migration/migration-helpers/src/datastore.rs index 473d7b9620a..0b77b6ee960 100644 --- a/workspaces/api/migration/migration-helpers/src/datastore.rs +++ b/workspaces/api/migration/migration-helpers/src/datastore.rs @@ -16,11 +16,11 @@ use apiserver::datastore::{ /// Retrieves data from the specified data store in a consistent format for easy modification. pub(crate) fn get_input_data( datastore: &D, - committed: Committed, + committed: &Committed, ) -> Result { let raw_data = datastore .get_prefix("", committed) - .context(error::GetData { committed })?; + .with_context(|| error::GetData { committed: committed.clone() })?; let mut data = HashMap::new(); for (data_key, value_str) in raw_data.into_iter() { @@ -66,7 +66,7 @@ pub(crate) fn get_input_data( pub(crate) fn set_output_data( datastore: &mut D, input: &MigrationData, - committed: Committed, + committed: &Committed, ) -> Result<()> { // Prepare serialized data let mut data = HashMap::new(); diff --git a/workspaces/api/migration/migration-helpers/src/error.rs b/workspaces/api/migration/migration-helpers/src/error.rs index 5f743441136..0a336b3fc95 100644 --- a/workspaces/api/migration/migration-helpers/src/error.rs +++ b/workspaces/api/migration/migration-helpers/src/error.rs @@ -52,6 +52,9 @@ pub enum Error { key: String, source: datastore::Error, }, + + #[snafu(display("Unable to list transactions in data store: {}", source))] + ListTransactions { source: datastore::Error }, } /// Result alias containing our Error type. diff --git a/workspaces/api/migration/migration-helpers/src/lib.rs b/workspaces/api/migration/migration-helpers/src/lib.rs index d7dcfb90559..dfbb6dddf6e 100644 --- a/workspaces/api/migration/migration-helpers/src/lib.rs +++ b/workspaces/api/migration/migration-helpers/src/lib.rs @@ -17,6 +17,7 @@ mod datastore; pub mod error; mod workarounds; +use snafu::ResultExt; use std::collections::HashMap; use std::env; use std::fmt; @@ -40,9 +41,10 @@ type DataStoreImplementation = FilesystemDataStore; /// necessary. /// /// Migrations must not assume any key will exist because they're run on pending data as well as -/// live, and pending may be empty. For the same reason, migrations must not add a key in all -/// cases if it's missing, because you could add a key to the pending data when the user didn't -/// have any pending data. Instead, make sure you're adding a key to an existing structure. +/// live, and pending transactions usually do not impact all keys. For the same reason, migrations +/// must not add a key in all cases if it's missing, because you could be adding the key to an +/// unrelated pending transaction. Instead, make sure you're adding a key to an existing +/// structure. pub trait Migration { /// Migrates data forward from the prior version to the version specified in the migration /// name. @@ -90,8 +92,13 @@ pub fn run_migration(mut migration: impl Migration, args: &Args) -> Result<()> { let source = DataStoreImplementation::new(&args.source_datastore); let mut target = DataStoreImplementation::new(&args.target_datastore); - for committed in &[Committed::Live, Committed::Pending] { - let input = get_input_data(&source, *committed)?; + // Run for live data and for each pending transaction + let mut committeds = vec![Committed::Live]; + let transactions = source.list_transactions().context(error::ListTransactions)?; + committeds.extend(transactions.into_iter().map(|tx| Committed::Pending { tx })); + + for committed in committeds { + let input = get_input_data(&source, &committed)?; let mut migrated = input.clone(); migrated = match args.migration_type { @@ -104,13 +111,13 @@ pub fn run_migration(mut migration: impl Migration, args: &Args) -> Result<()> { &mut migrated, &source, &mut target, - *committed, + &committed, &args, )?; validate_migrated_data(&migrated)?; - set_output_data(&mut target, &migrated, *committed)?; + set_output_data(&mut target, &migrated, &committed)?; } Ok(()) } diff --git a/workspaces/api/migration/migration-helpers/src/workarounds.rs b/workspaces/api/migration/migration-helpers/src/workarounds.rs index 420668ae9ce..fb825bf3d59 100644 --- a/workspaces/api/migration/migration-helpers/src/workarounds.rs +++ b/workspaces/api/migration/migration-helpers/src/workarounds.rs @@ -11,7 +11,7 @@ pub(crate) fn fix_migrated_data( output: &MigrationData, _source_datastore: &D, target_datastore: &mut D, - committed: Committed, + committed: &Committed, args: &Args, ) -> Result<()> { // If the source and target data store path are the same, we're using the old migrator @@ -33,7 +33,7 @@ pub(crate) fn fix_migrated_data( key: *removed_key_str, })?; target_datastore - .unset_key(&removed_key, committed) + .unset_key(&removed_key, &committed) .context(error::DataStoreRemove { key: *removed_key_str, })?; @@ -122,19 +122,19 @@ mod test { // The point of the workaround is affecting the data store directly, so make test stores let mut source = MemoryDataStore::new(); - set_output_data(&mut source, &input, Committed::Live).unwrap(); + set_output_data(&mut source, &input, &Committed::Live).unwrap(); // To replicate old interface, the target data store starts with the input data, and // we're going to confirm that removed values are actually removed let mut target = MemoryDataStore::new(); - set_output_data(&mut target, &input, Committed::Live).unwrap(); + set_output_data(&mut target, &input, &Committed::Live).unwrap(); // Ensure values are there at the start let kept_data = Key::new(KeyType::Data, "keepdata").unwrap(); let removed_data = Key::new(KeyType::Data, "removedata").unwrap(); let kept_meta = Key::new(KeyType::Meta, "keepmeta").unwrap(); let removed_meta = Key::new(KeyType::Meta, "removemeta").unwrap(); - assert_eq!(target.get_key(&kept_data, Committed::Live).unwrap(), Some("\"hi\"".into())); - assert_eq!(target.get_key(&removed_data, Committed::Live).unwrap(), Some("\"sup\"".into())); + assert_eq!(target.get_key(&kept_data, &Committed::Live).unwrap(), Some("\"hi\"".into())); + assert_eq!(target.get_key(&removed_data, &Committed::Live).unwrap(), Some("\"sup\"".into())); assert_eq!(target.get_metadata(&kept_meta, &kept_data).unwrap(), Some("\"howdy\"".into())); assert_eq!(target.get_metadata(&kept_meta, &removed_data).unwrap(), Some("\"hello\"".into())); assert_eq!(target.get_metadata(&removed_meta, &kept_data).unwrap(), Some("\"yo\"".into())); @@ -151,17 +151,17 @@ mod test { &expected, &source, &mut target, - Committed::Live, + &Committed::Live, &args, ) .unwrap(); // Ensure unaffected values were kept - assert_eq!(target.get_key(&kept_data, Committed::Live).unwrap(), Some("\"hi\"".into())); + assert_eq!(target.get_key(&kept_data, &Committed::Live).unwrap(), Some("\"hi\"".into())); assert_eq!(target.get_metadata(&kept_meta, &kept_data).unwrap(), Some("\"howdy\"".into())); assert_eq!(target.get_metadata(&kept_meta, &removed_data).unwrap(), Some("\"hello\"".into())); // Ensure removed values were removed - assert_eq!(target.get_key(&removed_data, Committed::Live).unwrap(), None); + assert_eq!(target.get_key(&removed_data, &Committed::Live).unwrap(), None); assert_eq!(target.get_metadata(&removed_meta, &kept_data).unwrap(), None); assert_eq!(target.get_metadata(&removed_meta, &removed_data).unwrap(), None); }