migration-helpers: update to understand transactions

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<D: DataStore>(
     datastore: &D,
-    committed: Committed,
+    committed: &Committed,
 ) -> Result<MigrationData> {
     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<D: DataStore>(
 pub(crate) fn set_output_data<D: DataStore>(
     datastore: &mut D,
     input: &MigrationData,
-    committed: Committed,
+    committed: &Committed,
 ) -> Result<()> {
     // Prepare serialized data
     let mut data = HashMap::new();

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.

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(())
 }

workspaces/api/migration/migration-helpers/src/workarounds.rs

@@ -11,7 +11,7 @@ pub(crate) fn fix_migrated_data<D: DataStore>(
     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<D: DataStore>(
                     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);
     }