Eng 488 fix app migrations system (#719)

* improve app level migration system

* basic unit tests for app migrator
We kinda don't want this going wrong so extra barrier is good.

* fix migrator tests

.gitignore

@@ -70,3 +70,4 @@ playwright-report
 dev.db-journal
 .build/
 .swiftpm
+/core/migration_test

core/src/lib.rs

@@ -19,6 +19,7 @@ pub mod custom_uri;
 pub(crate) mod job;
 pub(crate) mod library;
 pub(crate) mod location;
+pub(crate) mod migrations;
 pub(crate) mod node;
 pub(crate) mod object;
 pub(crate) mod p2p;
@@ -216,7 +217,7 @@ pub enum NodeError {
 	#[error("Failed to create data directory: {0}")]
 	FailedToCreateDataDirectory(#[from] std::io::Error),
 	#[error("Failed to initialize config: {0}")]
-	FailedToInitializeConfig(#[from] node::NodeConfigError),
+	FailedToInitializeConfig(#[from] util::migrator::MigratorError),
 	#[error("Failed to initialize library manager: {0}")]
 	FailedToInitializeLibraryManager(#[from] library::LibraryManagerError),
 	#[error("Location manager error: {0}")]

core/src/library/config.rs

@@ -1,23 +1,22 @@
-use std::{
-	fs::File,
-	io::{BufReader, Seek},
-	path::PathBuf,
-};
+use std::{marker::PhantomData, path::PathBuf};
 
 use rspc::Type;
 use serde::{Deserialize, Serialize};
-use std::io::Write;
 use uuid::Uuid;
 
-use crate::node::ConfigMetadata;
+use crate::{migrations, util::migrator::FileMigrator};
 
 use super::LibraryManagerError;
 
+const MIGRATOR: FileMigrator<LibraryConfig> = FileMigrator {
+	current_version: migrations::LIBRARY_VERSION,
+	migration_fn: migrations::migration_library,
+	phantom: PhantomData,
+};
+
 /// LibraryConfig holds the configuration for a specific library. This is stored as a '{uuid}.sdlibrary' file.
 #[derive(Debug, Serialize, Deserialize, Clone, Type, Default)]
 pub struct LibraryConfig {
-	#[serde(flatten)]
-	pub metadata: ConfigMetadata,
 	/// name is the display name of the library. This is used in the UI and is set by the user.
 	pub name: String,
 	/// description is a user set description of the library. This is used in the UI and is set by the user.
@@ -29,38 +28,18 @@ pub struct LibraryConfig {
 
 impl LibraryConfig {
 	/// read will read the configuration from disk and return it.
-	pub(super) async fn read(file_dir: PathBuf) -> Result<LibraryConfig, LibraryManagerError> {
-		let mut file = File::open(&file_dir)?;
-		let base_config: ConfigMetadata = serde_json::from_reader(BufReader::new(&mut file))?;
-
-		Self::migrate_config(base_config.version, file_dir)?;
-
-		file.rewind()?;
-		Ok(serde_json::from_reader(BufReader::new(&mut file))?)
+	pub(super) fn read(file_dir: PathBuf) -> Result<LibraryConfig, LibraryManagerError> {
+		MIGRATOR.load(&file_dir).map_err(Into::into)
 	}
 
 	/// save will write the configuration back to disk
-	pub(super) async fn save(
+	pub(super) fn save(
 		file_dir: PathBuf,
 		config: &LibraryConfig,
 	) -> Result<(), LibraryManagerError> {
-		File::create(file_dir)?.write_all(serde_json::to_string(config)?.as_bytes())?;
+		MIGRATOR.save(&file_dir, config.clone())?;
 		Ok(())
 	}
-
-	/// migrate_config is a function used to apply breaking changes to the library config file.
-	fn migrate_config(
-		current_version: Option<String>,
-		config_path: PathBuf,
-	) -> Result<(), LibraryManagerError> {
-		match current_version {
-			None => Err(LibraryManagerError::Migration(format!(
-				"Your Spacedrive library at '{}' is missing the `version` field",
-				config_path.display()
-			))),
-			_ => Ok(()),
-		}
-	}
 }
 
 // used to return to the frontend with uuid context

core/src/library/manager.rs

@@ -58,6 +58,8 @@ pub enum LibraryManagerError {
 	Seeder(#[from] SeederError),
 	#[error("failed to initialise the key manager")]
 	KeyManager(#[from] sd_crypto::Error),
+	#[error("failed to run library migrations: {0}")]
+	MigratorError(#[from] crate::util::migrator::MigratorError),
 }
 
 impl From<LibraryManagerError> for rspc::Error {
@@ -162,7 +164,7 @@ impl LibraryManager {
 				continue;
 			}
 
-			let config = LibraryConfig::read(config_path).await?;
+			let config = LibraryConfig::read(config_path)?;
 			libraries.push(Self::load(library_id, &db_path, config, node_context.clone()).await?);
 		}
 
@@ -187,8 +189,7 @@ impl LibraryManager {
 		LibraryConfig::save(
 			Path::new(&self.libraries_dir).join(format!("{id}.sdlibrary")),
 			&config,
-		)
-		.await?;
+		)?;
 
 		let library = Self::load(
 			id,
@@ -255,8 +256,7 @@ impl LibraryManager {
 		LibraryConfig::save(
 			Path::new(&self.libraries_dir).join(format!("{id}.sdlibrary")),
 			&library.config,
-		)
-		.await?;
+		)?;
 
 		invalidate_query!(library, "library.list");
 

core/src/migrations.rs

@@ -0,0 +1,25 @@
+use serde_json::{Map, Value};
+
+use crate::util::migrator::MigratorError;
+
+pub(crate) const NODE_VERSION: u32 = 0;
+pub(crate) const LIBRARY_VERSION: u32 = 0;
+
+/// Used to run migrations at a node level. This is useful for breaking changes to the `NodeConfig` file.
+pub fn migration_node(version: u32, _config: &mut Map<String, Value>) -> Result<(), MigratorError> {
+	match version {
+		0 => Ok(()),
+		v => unreachable!("Missing migration for library version {}", v),
+	}
+}
+
+/// Used to run migrations at a library level. This will be run for every library as necessary.
+pub fn migration_library(
+	version: u32,
+	_config: &mut Map<String, Value>,
+) -> Result<(), MigratorError> {
+	match version {
+		0 => Ok(()),
+		v => unreachable!("Missing migration for library version {}", v),
+	}
+}

core/src/node/config.rs

@@ -2,39 +2,30 @@ use rspc::Type;
 use sd_p2p::Keypair;
 use serde::{Deserialize, Serialize};
 use std::{
-	fs::File,
-	io::{self, BufReader, Seek, Write},
+	marker::PhantomData,
 	path::{Path, PathBuf},
 	sync::Arc,
 };
-use thiserror::Error;
 use tokio::sync::{RwLock, RwLockWriteGuard};
 use uuid::Uuid;
 
+use crate::{
+	migrations,
+	util::migrator::{FileMigrator, MigratorError},
+};
+
 /// NODE_STATE_CONFIG_NAME is the name of the file which stores the NodeState
 pub const NODE_STATE_CONFIG_NAME: &str = "node_state.sdconfig";
 
-/// ConfigMetadata is a part of node configuration that is loaded before the main configuration and contains information about the schema of the config.
-/// This allows us to migrate breaking changes to the config format between Spacedrive releases.
-#[derive(Debug, Serialize, Deserialize, Clone, Type)]
-pub struct ConfigMetadata {
-	/// version of Spacedrive. Determined from `CARGO_PKG_VERSION` environment variable.
-	pub version: Option<String>,
-}
-
-impl Default for ConfigMetadata {
-	fn default() -> Self {
-		Self {
-			version: Some(env!("CARGO_PKG_VERSION").into()),
-		}
-	}
-}
+const MIGRATOR: FileMigrator<NodeConfig> = FileMigrator {
+	current_version: migrations::NODE_VERSION,
+	migration_fn: migrations::migration_node,
+	phantom: PhantomData,
+};
 
 /// NodeConfig is the configuration for a node. This is shared between all libraries and is stored in a JSON file on disk.
 #[derive(Debug, Serialize, Deserialize, Clone, Type)]
 pub struct NodeConfig {
-	#[serde(flatten)]
-	pub metadata: ConfigMetadata,
 	/// id is a unique identifier for the current node. Each node has a public identifier (this one) and is given a local id for each library (done within the library code).
 	pub id: Uuid,
 	/// name is the display name of the current node. This is set by the user and is shown in the UI. // TODO: Length validation so it can fit in DNS record
@@ -42,7 +33,6 @@ pub struct NodeConfig {
 	// the port this node uses for peer to peer communication. By default a random free port will be chosen each time the application is started.
 	pub p2p_port: Option<u32>,
 	/// The p2p identity keypair for this node. This is used to identify the node on the network.
-	#[serde(default = "default_keypair")]
 	#[specta(skip)]
 	pub keypair: Keypair,
 	// TODO: These will probs be replaced by your Spacedrive account in the near future.
@@ -50,22 +40,7 @@ pub struct NodeConfig {
 	pub p2p_img_url: Option<String>,
 }
 
-// TODO: Probs remove this in future. It's just to prevent breaking changes.
-fn default_keypair() -> Keypair {
-	Keypair::generate()
-}
-
-#[derive(Error, Debug)]
-pub enum NodeConfigError {
-	#[error("error saving or loading the config from the filesystem")]
-	IO(#[from] io::Error),
-	#[error("error serializing or deserializing the JSON in the config file")]
-	Json(#[from] serde_json::Error),
-	#[error("error migrating the config file")]
-	Migration(String),
-}
-
-impl NodeConfig {
+impl Default for NodeConfig {
 	fn default() -> Self {
 		NodeConfig {
 			id: Uuid::new_v4(),
@@ -78,9 +53,6 @@ impl NodeConfig {
 				}
 			},
 			p2p_port: None,
-			metadata: ConfigMetadata {
-				version: Some(env!("CARGO_PKG_VERSION").into()),
-			},
 			keypair: Keypair::generate(),
 			p2p_email: None,
 			p2p_img_url: None,
@@ -92,13 +64,17 @@ pub struct NodeConfigManager(RwLock<NodeConfig>, PathBuf);
 
 impl NodeConfigManager {
 	/// new will create a new NodeConfigManager with the given path to the config file.
-	pub(crate) async fn new(data_path: PathBuf) -> Result<Arc<Self>, NodeConfigError> {
+	pub(crate) async fn new(data_path: PathBuf) -> Result<Arc<Self>, MigratorError> {
 		Ok(Arc::new(Self(
-			RwLock::new(Self::read(&data_path).await?),
+			RwLock::new(MIGRATOR.load(&Self::path(&data_path))?),
 			data_path,
 		)))
 	}
 
+	fn path(base_path: &Path) -> PathBuf {
+		base_path.join(NODE_STATE_CONFIG_NAME)
+	}
+
 	/// get will return the current NodeConfig in a read only state.
 	pub(crate) async fn get(&self) -> NodeConfig {
 		self.0.read().await.clone()
@@ -114,53 +90,16 @@ impl NodeConfigManager {
 	pub(crate) async fn write<F: FnOnce(RwLockWriteGuard<NodeConfig>)>(
 		&self,
 		mutation_fn: F,
-	) -> Result<NodeConfig, NodeConfigError> {
+	) -> Result<NodeConfig, MigratorError> {
 		mutation_fn(self.0.write().await);
 		let config = self.0.read().await;
-		Self::save(&self.1, &config).await?;
+		Self::save(&self.1, &config)?;
 		Ok(config.clone())
 	}
 
-	/// read will read the configuration from disk and return it.
-	async fn read(base_path: &PathBuf) -> Result<NodeConfig, NodeConfigError> {
-		let path = Path::new(base_path).join(NODE_STATE_CONFIG_NAME);
-
-		match path.try_exists().unwrap() {
-			true => {
-				let mut file = File::open(&path)?;
-				let base_config: ConfigMetadata =
-					serde_json::from_reader(BufReader::new(&mut file))?;
-
-				Self::migrate_config(base_config.version, path)?;
-
-				file.rewind()?;
-				Ok(serde_json::from_reader(BufReader::new(&mut file))?)
-			}
-			false => {
-				let config = NodeConfig::default();
-				Self::save(base_path, &config).await?;
-				Ok(config)
-			}
-		}
-	}
-
 	/// save will write the configuration back to disk
-	async fn save(base_path: &PathBuf, config: &NodeConfig) -> Result<(), NodeConfigError> {
-		let path = Path::new(base_path).join(NODE_STATE_CONFIG_NAME);
-		File::create(path)?.write_all(serde_json::to_string(config)?.as_bytes())?;
+	fn save(base_path: &Path, config: &NodeConfig) -> Result<(), MigratorError> {
+		MIGRATOR.save(&Self::path(base_path), config.clone())?;
 		Ok(())
 	}
-
-	/// migrate_config is a function used to apply breaking changes to the config file.
-	fn migrate_config(
-		current_version: Option<String>,
-		config_path: PathBuf,
-	) -> Result<(), NodeConfigError> {
-		match current_version {
-			None => {
-				Err(NodeConfigError::Migration(format!("Your Spacedrive config file stored at '{}' is missing the `version` field. If you just upgraded please delete the file and restart Spacedrive! Please note this upgrade will stop using your old 'library.db' as the folder structure has changed.", config_path.display())))
-			}
-			_ => Ok(()),
-		}
-	}
 }

core/src/util/migrator.rs

@@ -0,0 +1,263 @@
+use std::{
+	any::type_name,
+	fs::File,
+	io::{self, BufReader, Seek, Write},
+	marker::PhantomData,
+	path::Path,
+};
+
+use serde::{de::DeserializeOwned, Deserialize, Serialize};
+use serde_json::{Map, Value};
+use specta::Type;
+use thiserror::Error;
+
+/// is used to decode the configuration and work out what migrations need to be applied before the config can be properly loaded.
+/// This allows us to migrate breaking changes to the config format between Spacedrive releases.
+#[derive(Debug, Serialize, Deserialize, Clone, Type)]
+pub struct BaseConfig {
+	/// version of Spacedrive. Determined from `CARGO_PKG_VERSION` environment variable.
+	pub version: u32,
+	// Collect all extra fields
+	#[serde(flatten)]
+	other: Map<String, Value>,
+}
+
+/// System for managing app level migrations on a config file so we can introduce breaking changes to the app without the user needing to reset their whole system.
+pub struct FileMigrator<T>
+where
+	T: Serialize + DeserializeOwned + Default,
+{
+	pub current_version: u32,
+	pub migration_fn: fn(u32, &mut Map<String, Value>) -> Result<(), MigratorError>,
+	pub phantom: PhantomData<T>,
+}
+
+impl<T> FileMigrator<T>
+where
+	T: Serialize + DeserializeOwned + Default,
+{
+	// TODO: This is blocked on Rust. Make sure to make all fields private when this is introduced! Tracking issue: https://github.com/rust-lang/rust/issues/57349
+	// pub const fn new(
+	// 	current_version: u32,
+	// 	migration_fn: fn(u32, &mut Map<String, Value>) -> Result<(), MigratorError>,
+	// ) -> Self {
+	// 	Self {
+	// 		current_version,
+	// 		migration_fn,
+	// 		phantom: PhantomData,
+	// 	}
+	// }
+
+	pub fn load(&self, path: &Path) -> Result<T, MigratorError> {
+		match path.try_exists().unwrap() {
+			true => {
+				let mut file = File::options().read(true).write(true).open(path)?;
+				let mut cfg: BaseConfig = serde_json::from_reader(BufReader::new(&mut file))?;
+				file.rewind()?; // Fail early so we don't end up invalid state
+
+				if cfg.version > self.current_version {
+					return Err(MigratorError::YourAppIsOutdated);
+				}
+
+				let is_latest = cfg.version == self.current_version;
+				for v in (cfg.version + 1)..=self.current_version {
+					cfg.version = v;
+					match (self.migration_fn)(v, &mut cfg.other) {
+						Ok(()) => (),
+						Err(err) => {
+							file.write_all(serde_json::to_string(&cfg)?.as_bytes())?; // Writes updated version
+							return Err(err);
+						}
+					}
+				}
+
+				if !is_latest {
+					file.write_all(serde_json::to_string(&cfg)?.as_bytes())?; // Writes updated version
+				}
+
+				Ok(serde_json::from_value(Value::Object(cfg.other))?)
+			}
+			false => Ok(serde_json::from_value(Value::Object(
+				self.save(path, T::default())?.other,
+			))?),
+		}
+	}
+
+	pub fn save(&self, path: &Path, content: T) -> Result<BaseConfig, MigratorError> {
+		let config = BaseConfig {
+			version: self.current_version,
+			other: match serde_json::to_value(content)? {
+				Value::Object(map) => map,
+				_ => {
+					panic!(
+						"Type '{}' as generic `Migrator::T` must be serialiable to a Serde object!",
+						type_name::<T>()
+					);
+				}
+			},
+		};
+
+		let mut file = File::create(path)?;
+		file.write_all(serde_json::to_string(&config)?.as_bytes())?;
+		Ok(config)
+	}
+}
+
+#[derive(Error, Debug)]
+pub enum MigratorError {
+	#[error("error saving or loading the config from the filesystem: {0}")]
+	Io(#[from] io::Error),
+	#[error("error serializing or deserializing the JSON in the config file: {0}")]
+	Json(#[from] serde_json::Error),
+	#[error(
+		"the config file is for a newer version of the app. Please update to the latest version to load it!"
+	)]
+	YourAppIsOutdated,
+	#[error("custom migration error: {0}")]
+	Custom(String),
+}
+
+#[cfg(test)]
+mod test {
+	use std::{fs, io::Read, path::PathBuf};
+
+	use futures::executor::block_on;
+
+	use super::*;
+
+	#[derive(Debug, Clone, Serialize, Deserialize, Default, PartialEq, Eq)]
+	pub struct MyConfigType {
+		a: u8,
+		// For testing add new fields without breaking the passing.
+		#[serde(flatten)]
+		other: Map<String, Value>,
+	}
+
+	pub fn migration_node(
+		version: u32,
+		config: &mut Map<String, Value>,
+	) -> Result<(), MigratorError> {
+		match version {
+			0 => Ok(()),
+			// Add field to config
+			1 => {
+				config.insert("b".into(), 2.into());
+				Ok(())
+			}
+			// Async migration
+			2 => {
+				let mut a = false;
+				block_on(async {
+					a = true;
+					config.insert("c".into(), 3.into());
+				});
+				assert!(a, "Async block was not blocked on correctly!");
+				Ok(())
+			}
+			v => unreachable!("Missing migration for library version {}", v),
+		}
+	}
+
+	fn path(file_name: &'static str) -> PathBuf {
+		let dir = PathBuf::from("./migration_test");
+		std::fs::create_dir(&dir).ok();
+		dir.join(file_name)
+	}
+
+	fn file_as_str(path: &Path) -> String {
+		let mut file = File::open(&path).unwrap();
+		let mut contents = String::new();
+		file.read_to_string(&mut contents).unwrap();
+		contents
+	}
+
+	fn write_to_file(path: &Path, contents: &str) {
+		let mut file = File::create(&path).unwrap();
+		file.write_all(contents.as_bytes()).unwrap();
+	}
+
+	#[test]
+	fn test_migrator_happy_path() {
+		let migrator = FileMigrator::<MyConfigType> {
+			current_version: 0,
+			migration_fn: migration_node,
+			phantom: PhantomData,
+		};
+
+		let p = path("test_migrator_happy_path.config");
+
+		// Check config is created when it's missing
+		assert_eq!(p.exists(), false, "config file should start out deleted");
+		let default_cfg = migrator.load(&p).unwrap();
+		assert_eq!(p.exists(), true, "config file was not initialised");
+		assert_eq!(file_as_str(&p), r#"{"version":0,"a":0}"#);
+
+		// Check config can be loaded back into the system correctly
+		let config = migrator.load(&p).unwrap();
+		assert_eq!(default_cfg, config, "Config has got mangled somewhere");
+
+		// Update the config and check it saved correctly
+		let mut new_config = config.clone();
+		new_config.a = 1;
+		migrator.save(&p, new_config.clone()).unwrap();
+		assert_eq!(file_as_str(&p), r#"{"version":0,"a":1}"#);
+
+		// Try loading in the new config and check it's correct
+		let config = migrator.load(&p).unwrap();
+		assert_eq!(
+			new_config, config,
+			"Config has got mangled during the saving process"
+		);
+
+		// Test upgrading to a new version which adds a field
+		let migrator = FileMigrator::<MyConfigType> {
+			current_version: 1,
+			migration_fn: migration_node,
+			phantom: PhantomData,
+		};
+
+		// Try loading in the new config and check it was updated
+		let config = migrator.load(&p).unwrap();
+		assert_eq!(file_as_str(&p), r#"{"version":1,"a":1,"b":2}"#);
+
+		// Check editing works
+		let mut new_config = config.clone();
+		new_config.a = 2;
+		migrator.save(&p, new_config.clone()).unwrap();
+		assert_eq!(file_as_str(&p), r#"{"version":1,"a":2,"b":2}"#);
+
+		// Test upgrading to a new version which adds a field asynchronously
+		let migrator = FileMigrator::<MyConfigType> {
+			current_version: 2,
+			migration_fn: migration_node,
+			phantom: PhantomData,
+		};
+
+		// Try loading in the new config and check it was updated
+		migrator.load(&p).unwrap();
+		assert_eq!(file_as_str(&p), r#"{"version":2,"a":2,"b":2,"c":3}"#);
+
+		// Cleanup
+		fs::remove_file(&p).unwrap();
+	}
+
+	#[test]
+	pub fn test_time_traveling_backwards() {
+		let p = path("test_time_traveling_backwards.config");
+
+		// You opened a new database in an older version of the app
+		write_to_file(&p, r#"{"version":5,"a":1}"#);
+		let migrator = FileMigrator::<MyConfigType> {
+			current_version: 2,
+			migration_fn: migration_node,
+			phantom: PhantomData,
+		};
+		match migrator.load(&p) {
+			Err(MigratorError::YourAppIsOutdated) => (),
+			_ => panic!("Should have failed to load config from a super newer version of the app"),
+		}
+
+		// Cleanup
+		fs::remove_file(&p).unwrap();
+	}
+}

core/src/util/mod.rs

@@ -1,3 +1,4 @@
 pub mod db;
+pub mod migrator;
 pub mod secure_temp_keystore;
 pub mod seeder;

packages/client/src/core.ts

@@ -99,12 +99,6 @@ export type CRDTOperation = { node: string, timestamp: number, id: string, typ:
 
 export type CRDTOperationType = SharedOperation | RelationOperation | OwnedOperation
 
-/**
- *  ConfigMetadata is a part of node configuration that is loaded before the main configuration and contains information about the schema of the config.
- *  This allows us to migrate breaking changes to the config format between Spacedrive releases.
- */
-export type ConfigMetadata = { version: string | null }
-
 export type CreateLibraryArgs = { name: string, auth: AuthOption, algorithm: Algorithm, hashing_algorithm: HashingAlgorithm }
 
 export type EditLibraryArgs = { id: string, name: string | null, description: string | null }
@@ -177,7 +171,7 @@ export type LibraryArgs<T> = { library_id: string, arg: T }
 /**
  *  LibraryConfig holds the configuration for a specific library. This is stored as a '{uuid}.sdlibrary' file.
  */
-export type LibraryConfig = ({ version: string | null }) & { name: string, description: string }
+export type LibraryConfig = { name: string, description: string }
 
 export type LibraryConfigWrapped = { uuid: string, config: LibraryConfig }
 
@@ -213,9 +207,9 @@ export type Node = { id: number, pub_id: number[], name: string, platform: numbe
 /**
  *  NodeConfig is the configuration for a node. This is shared between all libraries and is stored in a JSON file on disk.
  */
-export type NodeConfig = ({ version: string | null }) & { id: string, name: string, p2p_port: number | null, p2p_email: string | null, p2p_img_url: string | null }
+export type NodeConfig = { id: string, name: string, p2p_port: number | null, p2p_email: string | null, p2p_img_url: string | null }
 
-export type NodeState = (({ version: string | null }) & { id: string, name: string, p2p_port: number | null, p2p_email: string | null, p2p_img_url: string | null }) & { data_path: string }
+export type NodeState = ({ id: string, name: string, p2p_port: number | null, p2p_email: string | null, p2p_img_url: string | null }) & { data_path: string }
 
 /**
  *  This should be used for providing a nonce to encrypt/decrypt functions.