From ff9ad8185b3f2fb6c53f8a64fca529ade86be568 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sun, 12 Dec 2021 10:31:27 -0500 Subject: [PATCH 01/14] undo docs --- docs/src/storage.md | 52 ++++++++++++++++++++++++++++++++++++++---- docs/src/sync-model.md | 10 ++++++++ 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/docs/src/storage.md b/docs/src/storage.md index c1c34f7e1..f733f4f98 100644 --- a/docs/src/storage.md +++ b/docs/src/storage.md @@ -21,21 +21,63 @@ See [Tasks](./tasks.md) for details on the content of that map. Every change to the task database is captured as an operation. In other words, operations act as deltas between database states. -Operations are crucial to synchronization of replicas, using a technique known as Operational Transforms. +Operations are crucial to synchronization of replicas, described in [Synchronization Model](./sync-model.md). + +Operations are entirely managed by the replica, and some combinations of operations are described as "invalid" here. +A replica must not create invalid operations, but should be resilient to receiving invalid operations during a synchronization operation. Each operation has one of the forms * `Create(uuid)` - * `Delete(uuid)` - * `Update(uuid, property, value, timestamp)` + * `Delete(uuid, oldTask)` + * `Update(uuid, property, oldValue, newValue, timestamp)` + * `UndoPoint()` The Create form creates a new task. It is invalid to create a task that already exists. Similarly, the Delete form deletes an existing task. It is invalid to delete a task that does not exist. +The `oldTask` property contains the task data from before it was deleted. -The Update form updates the given property of the given task, where property and value are both strings. -Value can also be `None` to indicate deletion of a property. +The Update form updates the given property of the given task, where the property and values are strings. +The `oldValue` gives the old value of the property (or None to create a new property), while `newValue` gives the new value (or None to delete a property). It is invalid to update a task that does not exist. The timestamp on updates serves as additional metadata and is used to resolve conflicts. + +### Application + +Each operation can be "applied" to a task database in a natural way: + + * Applying `Create` creates a new, empty task in the task database. + * Applying `Delete` deletes a task, including all of its properties, from the task database. + * Applying `Update` modifies the properties of a task. + * Applying `UndoPoint` does nothing. + +### Undo + +Each operation also contains enough information to reverse its application: + + * Undoing `Create` deletes a task. + * Undoing `Delete` creates a task, including all of the properties in `oldTask`. + * Undoing `Update` modifies the properties of a task, reverting to `oldValue`. + * Undoing `UndoPoint` does nothing. + +The `UndoPoint` operation serves as a marker of points in the operation sequence to which the user might wish to undo. +For example, creation of a new task with several properities involves several operations, but is a single step from the user's perspective. +An "undo" command reverses operations, removing them from the operations sequence, until it reaches an `UndoPoint` operation. + +### Synchronizing Operations + +After operations are synchronized to the server, they can no longer be undone. +As such, the [synchronization model](./sync-model.md) uses simpler operations. +Replica operations are converted to sync operations as follows: + + * `Create(uuid)` -> `Create(uuid)` (no change) + * `Delete(uuid, oldTask)` -> `Delete(uuid)` + * `Update(uuid, property, oldValue, newValue, timestamp)` -> `Update(uuid, property, newValue, timestamp)` + * `UndoPoint()` -> Ø (dropped from operation sequence) + +Once a sequence of operations has been synchronized, there is no need to store those operations on the replica. +The current implementation deletes operations at that time. +An alternative approach is to keep operations for existing tasks, and provide access to those operations as a "history" of modifications to the task. diff --git a/docs/src/sync-model.md b/docs/src/sync-model.md index e75bab670..11d6e0855 100644 --- a/docs/src/sync-model.md +++ b/docs/src/sync-model.md @@ -34,6 +34,16 @@ Since the replicas are not connected, each may have additional operations that h The synchronization process uses operational transformation to "linearize" those operations. This process is analogous (vaguely) to rebasing a sequence of Git commits. +### Sync Operations + +The [Replica Storage](./storage.md) model contains additional information in its operations that is not included in operations synchronized to other replicas. +In this document, we will be discussing "sync operations" of the form + + * `Create(uuid)` + * `Delete(uuid)` + * `Update(uuid, property, value, timestamp)` + + ### Versions Occasionally, database states are given a name (that takes the form of a UUID). From 6f7794c7de9dd20c49248676243e1545d2c3389f Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sun, 19 Dec 2021 19:50:26 +0000 Subject: [PATCH 02/14] introduce a new taskchampion::server::SyncOp type --- taskchampion/src/server/mod.rs | 3 + taskchampion/src/server/op.rs | 420 +++++++++++++++++++++++++++++++++ taskchampion/src/taskdb/mod.rs | 28 ++- 3 files changed, 449 insertions(+), 2 deletions(-) create mode 100644 taskchampion/src/server/op.rs diff --git a/taskchampion/src/server/mod.rs b/taskchampion/src/server/mod.rs index eb77b9bd3..f97b9181b 100644 --- a/taskchampion/src/server/mod.rs +++ b/taskchampion/src/server/mod.rs @@ -14,6 +14,7 @@ pub(crate) mod test; mod config; mod crypto; mod local; +mod op; mod remote; mod types; @@ -21,3 +22,5 @@ pub use config::ServerConfig; pub use local::LocalServer; pub use remote::RemoteServer; pub use types::*; + +pub(crate) use op::SyncOp; diff --git a/taskchampion/src/server/op.rs b/taskchampion/src/server/op.rs new file mode 100644 index 000000000..6f6561ceb --- /dev/null +++ b/taskchampion/src/server/op.rs @@ -0,0 +1,420 @@ +use chrono::{DateTime, Utc}; +use serde::{Deserialize, Serialize}; +use uuid::Uuid; + +/// A SyncOp defines a single change to the task database, that can be synchronized +/// via a server. +#[derive(PartialEq, Clone, Debug, Serialize, Deserialize)] +pub enum SyncOp { + /// Create a new task. + /// + /// On application, if the task already exists, the operation does nothing. + Create { uuid: Uuid }, + + /// Delete an existing task. + /// + /// On application, if the task does not exist, the operation does nothing. + Delete { uuid: Uuid }, + + /// Update an existing task, setting the given property to the given value. If the value is + /// None, then the corresponding property is deleted. + /// + /// If the given task does not exist, the operation does nothing. + Update { + uuid: Uuid, + property: String, + value: Option, + timestamp: DateTime, + }, +} + +use SyncOp::*; + +impl SyncOp { + // Transform takes two operations A and B that happened concurrently and produces two + // operations A' and B' such that `apply(apply(S, A), B') = apply(apply(S, B), A')`. This + // function is used to serialize operations in a process similar to a Git "rebase". + // + // * + // / \ + // op1 / \ op2 + // / \ + // * * + // + // this function "completes the diamond: + // + // * * + // \ / + // op2' \ / op1' + // \ / + // * + // + // such that applying op2' after op1 has the same effect as applying op1' after op2. This + // allows two different systems which have already applied op1 and op2, respectively, and thus + // reached different states, to return to the same state by applying op2' and op1', + // respectively. + pub fn transform(operation1: SyncOp, operation2: SyncOp) -> (Option, Option) { + match (&operation1, &operation2) { + // Two creations or deletions of the same uuid reach the same state, so there's no need + // for any further operations to bring the state together. + (&Create { uuid: uuid1 }, &Create { uuid: uuid2 }) if uuid1 == uuid2 => (None, None), + (&Delete { uuid: uuid1 }, &Delete { uuid: uuid2 }) if uuid1 == uuid2 => (None, None), + + // Given a create and a delete of the same task, one of the operations is invalid: the + // create implies the task does not exist, but the delete implies it exists. Somewhat + // arbitrarily, we prefer the Create + (&Create { uuid: uuid1 }, &Delete { uuid: uuid2 }) if uuid1 == uuid2 => { + (Some(operation1), None) + } + (&Delete { uuid: uuid1 }, &Create { uuid: uuid2 }) if uuid1 == uuid2 => { + (None, Some(operation2)) + } + + // And again from an Update and a Create, prefer the Update + (&Update { uuid: uuid1, .. }, &Create { uuid: uuid2 }) if uuid1 == uuid2 => { + (Some(operation1), None) + } + (&Create { uuid: uuid1 }, &Update { uuid: uuid2, .. }) if uuid1 == uuid2 => { + (None, Some(operation2)) + } + + // Given a delete and an update, prefer the delete + (&Update { uuid: uuid1, .. }, &Delete { uuid: uuid2 }) if uuid1 == uuid2 => { + (None, Some(operation2)) + } + (&Delete { uuid: uuid1 }, &Update { uuid: uuid2, .. }) if uuid1 == uuid2 => { + (Some(operation1), None) + } + + // Two updates to the same property of the same task might conflict. + ( + &Update { + uuid: ref uuid1, + property: ref property1, + value: ref value1, + timestamp: ref timestamp1, + }, + &Update { + uuid: ref uuid2, + property: ref property2, + value: ref value2, + timestamp: ref timestamp2, + }, + ) if uuid1 == uuid2 && property1 == property2 => { + // if the value is the same, there's no conflict + if value1 == value2 { + (None, None) + } else if timestamp1 < timestamp2 { + // prefer the later modification + (None, Some(operation2)) + } else { + // prefer the later modification or, if the modifications are the same, + // just choose one of them + (Some(operation1), None) + } + } + + // anything else is not a conflict of any sort, so return the operations unchanged + (_, _) => (Some(operation1), Some(operation2)), + } + } +} + +#[cfg(test)] +mod test { + use super::*; + use crate::storage::InMemoryStorage; + use crate::taskdb::TaskDb; + use chrono::{Duration, Utc}; + use pretty_assertions::assert_eq; + use proptest::prelude::*; + + #[test] + fn test_json_create() -> anyhow::Result<()> { + let uuid = Uuid::new_v4(); + let op = Create { uuid }; + let json = serde_json::to_string(&op)?; + assert_eq!(json, format!(r#"{{"Create":{{"uuid":"{}"}}}}"#, uuid)); + let deser: SyncOp = serde_json::from_str(&json)?; + assert_eq!(deser, op); + Ok(()) + } + + #[test] + fn test_json_delete() -> anyhow::Result<()> { + let uuid = Uuid::new_v4(); + let op = Delete { uuid }; + let json = serde_json::to_string(&op)?; + assert_eq!(json, format!(r#"{{"Delete":{{"uuid":"{}"}}}}"#, uuid)); + let deser: SyncOp = serde_json::from_str(&json)?; + assert_eq!(deser, op); + Ok(()) + } + + #[test] + fn test_json_update() -> anyhow::Result<()> { + let uuid = Uuid::new_v4(); + let timestamp = Utc::now(); + + let op = Update { + uuid, + property: "abc".into(), + value: Some("false".into()), + timestamp, + }; + + let json = serde_json::to_string(&op)?; + assert_eq!( + json, + format!( + r#"{{"Update":{{"uuid":"{}","property":"abc","value":"false","timestamp":"{:?}"}}}}"#, + uuid, timestamp, + ) + ); + let deser: SyncOp = serde_json::from_str(&json)?; + assert_eq!(deser, op); + Ok(()) + } + + #[test] + fn test_json_update_none() -> anyhow::Result<()> { + let uuid = Uuid::new_v4(); + let timestamp = Utc::now(); + + let op = Update { + uuid, + property: "abc".into(), + value: None, + timestamp, + }; + + let json = serde_json::to_string(&op)?; + assert_eq!( + json, + format!( + r#"{{"Update":{{"uuid":"{}","property":"abc","value":null,"timestamp":"{:?}"}}}}"#, + uuid, timestamp, + ) + ); + let deser: SyncOp = serde_json::from_str(&json)?; + assert_eq!(deser, op); + Ok(()) + } + + fn test_transform( + setup: Option, + o1: SyncOp, + o2: SyncOp, + exp1p: Option, + exp2p: Option, + ) { + let (o1p, o2p) = SyncOp::transform(o1.clone(), o2.clone()); + assert_eq!((&o1p, &o2p), (&exp1p, &exp2p)); + + // check that the two operation sequences have the same effect, enforcing the invariant of + // the transform function. + let mut db1 = TaskDb::new_inmemory(); + if let Some(ref o) = setup { + db1.apply_sync_tmp(o.clone()).unwrap(); + } + db1.apply_sync_tmp(o1).unwrap(); + if let Some(o) = o2p { + db1.apply_sync_tmp(o).unwrap(); + } + + let mut db2 = TaskDb::new_inmemory(); + if let Some(ref o) = setup { + db2.apply_sync_tmp(o.clone()).unwrap(); + } + db2.apply_sync_tmp(o2).unwrap(); + if let Some(o) = o1p { + db2.apply_sync_tmp(o).unwrap(); + } + + assert_eq!(db1.sorted_tasks(), db2.sorted_tasks()); + } + + #[test] + fn test_unrelated_create() { + let uuid1 = Uuid::new_v4(); + let uuid2 = Uuid::new_v4(); + + test_transform( + None, + Create { uuid: uuid1 }, + Create { uuid: uuid2 }, + Some(Create { uuid: uuid1 }), + Some(Create { uuid: uuid2 }), + ); + } + + #[test] + fn test_related_updates_different_props() { + let uuid = Uuid::new_v4(); + let timestamp = Utc::now(); + + test_transform( + Some(Create { uuid }), + Update { + uuid, + property: "abc".into(), + value: Some("true".into()), + timestamp, + }, + Update { + uuid, + property: "def".into(), + value: Some("false".into()), + timestamp, + }, + Some(Update { + uuid, + property: "abc".into(), + value: Some("true".into()), + timestamp, + }), + Some(Update { + uuid, + property: "def".into(), + value: Some("false".into()), + timestamp, + }), + ); + } + + #[test] + fn test_related_updates_same_prop() { + let uuid = Uuid::new_v4(); + let timestamp1 = Utc::now(); + let timestamp2 = timestamp1 + Duration::seconds(10); + + test_transform( + Some(Create { uuid }), + Update { + uuid, + property: "abc".into(), + value: Some("true".into()), + timestamp: timestamp1, + }, + Update { + uuid, + property: "abc".into(), + value: Some("false".into()), + timestamp: timestamp2, + }, + None, + Some(Update { + uuid, + property: "abc".into(), + value: Some("false".into()), + timestamp: timestamp2, + }), + ); + } + + #[test] + fn test_related_updates_same_prop_same_time() { + let uuid = Uuid::new_v4(); + let timestamp = Utc::now(); + + test_transform( + Some(Create { uuid }), + Update { + uuid, + property: "abc".into(), + value: Some("true".into()), + timestamp, + }, + Update { + uuid, + property: "abc".into(), + value: Some("false".into()), + timestamp, + }, + Some(Update { + uuid, + property: "abc".into(), + value: Some("true".into()), + timestamp, + }), + None, + ); + } + + fn uuid_strategy() -> impl Strategy { + prop_oneof![ + Just(Uuid::parse_str("83a2f9ef-f455-4195-b92e-a54c161eebfc").unwrap()), + Just(Uuid::parse_str("56e0be07-c61f-494c-a54c-bdcfdd52d2a7").unwrap()), + Just(Uuid::parse_str("4b7ed904-f7b0-4293-8a10-ad452422c7b3").unwrap()), + Just(Uuid::parse_str("9bdd0546-07c8-4e1f-a9bc-9d6299f4773b").unwrap()), + ] + } + + fn operation_strategy() -> impl Strategy { + prop_oneof![ + uuid_strategy().prop_map(|uuid| Create { uuid }), + uuid_strategy().prop_map(|uuid| Delete { uuid }), + (uuid_strategy(), "(title|project|status)").prop_map(|(uuid, property)| { + Update { + uuid, + property, + value: Some("true".into()), + timestamp: Utc::now(), + } + }), + ] + } + + proptest! { + #![proptest_config(ProptestConfig { + cases: 1024, .. ProptestConfig::default() + })] + #[test] + // check that the two operation sequences have the same effect, enforcing the invariant of + // the transform function. + fn transform_invariant_holds(o1 in operation_strategy(), o2 in operation_strategy()) { + let (o1p, o2p) = SyncOp::transform(o1.clone(), o2.clone()); + + let mut db1 = TaskDb::new(Box::new(InMemoryStorage::new())); + let mut db2 = TaskDb::new(Box::new(InMemoryStorage::new())); + + // Ensure that any expected tasks already exist + if let Update{ uuid, .. } = o1 { + let _ = db1.apply_sync_tmp(Create{uuid}); + let _ = db2.apply_sync_tmp(Create{uuid}); + } + + if let Update{ uuid, .. } = o2 { + let _ = db1.apply_sync_tmp(Create{uuid}); + let _ = db2.apply_sync_tmp(Create{uuid}); + } + + if let Delete{ uuid } = o1 { + let _ = db1.apply_sync_tmp(Create{uuid}); + let _ = db2.apply_sync_tmp(Create{uuid}); + } + + if let Delete{ uuid } = o2 { + let _ = db1.apply_sync_tmp(Create{uuid}); + let _ = db2.apply_sync_tmp(Create{uuid}); + } + + // if applying the initial operations fail, that indicates the operation was invalid + // in the base state, so consider the case successful. + if db1.apply_sync_tmp(o1).is_err() { + return Ok(()); + } + if db2.apply_sync_tmp(o2).is_err() { + return Ok(()); + } + + if let Some(o) = o2p { + db1.apply_sync_tmp(o).map_err(|e| TestCaseError::Fail(format!("Applying to db1: {}", e).into()))?; + } + if let Some(o) = o1p { + db2.apply_sync_tmp(o).map_err(|e| TestCaseError::Fail(format!("Applying to db2: {}", e).into()))?; + } + assert_eq!(db1.sorted_tasks(), db2.sorted_tasks()); + } + } +} diff --git a/taskchampion/src/taskdb/mod.rs b/taskchampion/src/taskdb/mod.rs index 65546c462..026e82698 100644 --- a/taskchampion/src/taskdb/mod.rs +++ b/taskchampion/src/taskdb/mod.rs @@ -1,4 +1,4 @@ -use crate::server::Server; +use crate::server::{Server, SyncOp}; use crate::storage::{Operation, Storage, TaskMap}; use uuid::Uuid; @@ -22,7 +22,10 @@ impl TaskDb { #[cfg(test)] pub fn new_inmemory() -> TaskDb { - TaskDb::new(Box::new(crate::storage::InMemoryStorage::new())) + #[cfg(test)] + use crate::storage::InMemoryStorage; + + TaskDb::new(Box::new(InMemoryStorage::new())) } /// Apply an operation to the TaskDb. Aside from synchronization operations, this is the only way @@ -39,6 +42,27 @@ impl TaskDb { Ok(()) } + // temporary + pub fn apply_sync_tmp(&mut self, op: SyncOp) -> anyhow::Result<()> { + // create an op from SyncOp + let op = match op { + SyncOp::Create { uuid } => Operation::Create { uuid }, + SyncOp::Delete { uuid } => Operation::Delete { uuid }, + SyncOp::Update { + uuid, + property, + value, + timestamp, + } => Operation::Update { + uuid, + property, + value, + timestamp, + }, + }; + self.apply(op) + } + /// Get all tasks. pub fn all_tasks(&mut self) -> anyhow::Result> { let mut txn = self.storage.txn()?; From 0b29efab31950f7b93794a15620e828faa7b3e26 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sun, 19 Dec 2021 20:03:01 +0000 Subject: [PATCH 03/14] rename Operation to ReplicaOp for clarity --- taskchampion/src/replica.rs | 8 +-- taskchampion/src/storage/inmemory.rs | 10 +-- taskchampion/src/storage/mod.rs | 10 +-- .../src/storage/{operation.rs => op.rs} | 61 ++++++++++--------- taskchampion/src/storage/sqlite.rs | 44 ++++++------- taskchampion/src/taskdb/mod.rs | 22 +++---- taskchampion/src/taskdb/ops.rs | 34 +++++------ taskchampion/src/taskdb/sync.rs | 44 ++++++------- taskchampion/src/taskdb/working_set.rs | 6 +- 9 files changed, 120 insertions(+), 119 deletions(-) rename taskchampion/src/storage/{operation.rs => op.rs} (88%) diff --git a/taskchampion/src/replica.rs b/taskchampion/src/replica.rs index 7afbc90a3..6bdb756e8 100644 --- a/taskchampion/src/replica.rs +++ b/taskchampion/src/replica.rs @@ -1,6 +1,6 @@ use crate::errors::Error; use crate::server::Server; -use crate::storage::{Operation, Storage, TaskMap}; +use crate::storage::{ReplicaOp, Storage, TaskMap}; use crate::task::{Status, Task}; use crate::taskdb::TaskDb; use crate::workingset::WorkingSet; @@ -56,7 +56,7 @@ impl Replica { S1: Into, S2: Into, { - self.taskdb.apply(Operation::Update { + self.taskdb.apply(ReplicaOp::Update { uuid, property: property.into(), value: value.map(|v| v.into()), @@ -100,7 +100,7 @@ impl Replica { /// Create a new task. The task must not already exist. pub fn new_task(&mut self, status: Status, description: String) -> anyhow::Result { let uuid = Uuid::new_v4(); - self.taskdb.apply(Operation::Create { uuid })?; + self.taskdb.apply(ReplicaOp::Create { uuid })?; trace!("task {} created", uuid); let mut task = Task::new(uuid, TaskMap::new()).into_mut(self); task.set_description(description)?; @@ -118,7 +118,7 @@ impl Replica { if self.taskdb.get_task(uuid)?.is_none() { return Err(Error::Database(format!("Task {} does not exist", uuid)).into()); } - self.taskdb.apply(Operation::Delete { uuid })?; + self.taskdb.apply(ReplicaOp::Delete { uuid })?; trace!("task {} deleted", uuid); Ok(()) } diff --git a/taskchampion/src/storage/inmemory.rs b/taskchampion/src/storage/inmemory.rs index 448d9ae7f..4a4463c19 100644 --- a/taskchampion/src/storage/inmemory.rs +++ b/taskchampion/src/storage/inmemory.rs @@ -1,6 +1,6 @@ #![allow(clippy::new_without_default)] -use crate::storage::{Operation, Storage, StorageTxn, TaskMap, VersionId, DEFAULT_BASE_VERSION}; +use crate::storage::{ReplicaOp, Storage, StorageTxn, TaskMap, VersionId, DEFAULT_BASE_VERSION}; use std::collections::hash_map::Entry; use std::collections::HashMap; use uuid::Uuid; @@ -9,7 +9,7 @@ use uuid::Uuid; struct Data { tasks: HashMap, base_version: VersionId, - operations: Vec, + operations: Vec, working_set: Vec>, } @@ -87,16 +87,16 @@ impl<'t> StorageTxn for Txn<'t> { Ok(()) } - fn operations(&mut self) -> anyhow::Result> { + fn operations(&mut self) -> anyhow::Result> { Ok(self.data_ref().operations.clone()) } - fn add_operation(&mut self, op: Operation) -> anyhow::Result<()> { + fn add_operation(&mut self, op: ReplicaOp) -> anyhow::Result<()> { self.mut_data_ref().operations.push(op); Ok(()) } - fn set_operations(&mut self, ops: Vec) -> anyhow::Result<()> { + fn set_operations(&mut self, ops: Vec) -> anyhow::Result<()> { self.mut_data_ref().operations = ops; Ok(()) } diff --git a/taskchampion/src/storage/mod.rs b/taskchampion/src/storage/mod.rs index a16bb5a7e..d3d494a1b 100644 --- a/taskchampion/src/storage/mod.rs +++ b/taskchampion/src/storage/mod.rs @@ -11,14 +11,14 @@ use uuid::Uuid; mod config; mod inmemory; -mod operation; +mod op; pub(crate) mod sqlite; pub use config::StorageConfig; pub use inmemory::InMemoryStorage; pub use sqlite::SqliteStorage; -pub use operation::Operation; +pub use op::ReplicaOp; /// An in-memory representation of a task as a simple hashmap pub type TaskMap = HashMap; @@ -80,14 +80,14 @@ pub trait StorageTxn { /// Get the current set of outstanding operations (operations that have not been sync'd to the /// server yet) - fn operations(&mut self) -> Result>; + fn operations(&mut self) -> Result>; /// Add an operation to the end of the list of operations in the storage. Note that this /// merely *stores* the operation; it is up to the TaskDb to apply it. - fn add_operation(&mut self, op: Operation) -> Result<()>; + fn add_operation(&mut self, op: ReplicaOp) -> Result<()>; /// Replace the current list of operations with a new list. - fn set_operations(&mut self, ops: Vec) -> Result<()>; + fn set_operations(&mut self, ops: Vec) -> Result<()>; /// Get the entire working set, with each task UUID at its appropriate (1-based) index. /// Element 0 is always None. diff --git a/taskchampion/src/storage/operation.rs b/taskchampion/src/storage/op.rs similarity index 88% rename from taskchampion/src/storage/operation.rs rename to taskchampion/src/storage/op.rs index ed9a5182d..0ab9e1104 100644 --- a/taskchampion/src/storage/operation.rs +++ b/taskchampion/src/storage/op.rs @@ -2,9 +2,10 @@ use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use uuid::Uuid; -/// An Operation defines a single change to the task database +/// A ReplicaOp defines a single change to the task database, as stored locally in the replica. +/// This contains additional information not included in SyncOp. #[derive(PartialEq, Clone, Debug, Serialize, Deserialize)] -pub enum Operation { +pub enum ReplicaOp { /// Create a new task. /// /// On application, if the task already exists, the operation does nothing. @@ -27,9 +28,9 @@ pub enum Operation { }, } -use Operation::*; +use ReplicaOp::*; -impl Operation { +impl ReplicaOp { // Transform takes two operations A and B that happened concurrently and produces two // operations A' and B' such that `apply(apply(S, A), B') = apply(apply(S, B), A')`. This // function is used to serialize operations in a process similar to a Git "rebase". @@ -53,9 +54,9 @@ impl Operation { // reached different states, to return to the same state by applying op2' and op1', // respectively. pub fn transform( - operation1: Operation, - operation2: Operation, - ) -> (Option, Option) { + operation1: ReplicaOp, + operation2: ReplicaOp, + ) -> (Option, Option) { match (&operation1, &operation2) { // Two creations or deletions of the same uuid reach the same state, so there's no need // for any further operations to bring the state together. @@ -135,13 +136,13 @@ mod test { // thoroughly, so this testing is light. fn test_transform( - setup: Option, - o1: Operation, - o2: Operation, - exp1p: Option, - exp2p: Option, + setup: Option, + o1: ReplicaOp, + o2: ReplicaOp, + exp1p: Option, + exp2p: Option, ) { - let (o1p, o2p) = Operation::transform(o1.clone(), o2.clone()); + let (o1p, o2p) = ReplicaOp::transform(o1.clone(), o2.clone()); assert_eq!((&o1p, &o2p), (&exp1p, &exp2p)); // check that the two operation sequences have the same effect, enforcing the invariant of @@ -349,12 +350,12 @@ mod test { ] } - fn operation_strategy() -> impl Strategy { + fn operation_strategy() -> impl Strategy { prop_oneof![ - uuid_strategy().prop_map(|uuid| Operation::Create { uuid }), - uuid_strategy().prop_map(|uuid| Operation::Delete { uuid }), + uuid_strategy().prop_map(|uuid| ReplicaOp::Create { uuid }), + uuid_strategy().prop_map(|uuid| ReplicaOp::Delete { uuid }), (uuid_strategy(), "(title|project|status)").prop_map(|(uuid, property)| { - Operation::Update { + ReplicaOp::Update { uuid, property, value: Some("true".into()), @@ -372,30 +373,30 @@ mod test { // check that the two operation sequences have the same effect, enforcing the invariant of // the transform function. fn transform_invariant_holds(o1 in operation_strategy(), o2 in operation_strategy()) { - let (o1p, o2p) = Operation::transform(o1.clone(), o2.clone()); + let (o1p, o2p) = ReplicaOp::transform(o1.clone(), o2.clone()); let mut db1 = TaskDb::new(Box::new(InMemoryStorage::new())); let mut db2 = TaskDb::new(Box::new(InMemoryStorage::new())); // Ensure that any expected tasks already exist - if let Operation::Update{ ref uuid, .. } = o1 { - let _ = db1.apply(Operation::Create{uuid: uuid.clone()}); - let _ = db2.apply(Operation::Create{uuid: uuid.clone()}); + if let ReplicaOp::Update{ ref uuid, .. } = o1 { + let _ = db1.apply(ReplicaOp::Create{uuid: uuid.clone()}); + let _ = db2.apply(ReplicaOp::Create{uuid: uuid.clone()}); } - if let Operation::Update{ ref uuid, .. } = o2 { - let _ = db1.apply(Operation::Create{uuid: uuid.clone()}); - let _ = db2.apply(Operation::Create{uuid: uuid.clone()}); + if let ReplicaOp::Update{ ref uuid, .. } = o2 { + let _ = db1.apply(ReplicaOp::Create{uuid: uuid.clone()}); + let _ = db2.apply(ReplicaOp::Create{uuid: uuid.clone()}); } - if let Operation::Delete{ ref uuid } = o1 { - let _ = db1.apply(Operation::Create{uuid: uuid.clone()}); - let _ = db2.apply(Operation::Create{uuid: uuid.clone()}); + if let ReplicaOp::Delete{ ref uuid } = o1 { + let _ = db1.apply(ReplicaOp::Create{uuid: uuid.clone()}); + let _ = db2.apply(ReplicaOp::Create{uuid: uuid.clone()}); } - if let Operation::Delete{ ref uuid } = o2 { - let _ = db1.apply(Operation::Create{uuid: uuid.clone()}); - let _ = db2.apply(Operation::Create{uuid: uuid.clone()}); + if let ReplicaOp::Delete{ ref uuid } = o2 { + let _ = db1.apply(ReplicaOp::Create{uuid: uuid.clone()}); + let _ = db2.apply(ReplicaOp::Create{uuid: uuid.clone()}); } // if applying the initial operations fail, that indicates the operation was invalid diff --git a/taskchampion/src/storage/sqlite.rs b/taskchampion/src/storage/sqlite.rs index 86525d0ca..bf5620af5 100644 --- a/taskchampion/src/storage/sqlite.rs +++ b/taskchampion/src/storage/sqlite.rs @@ -1,4 +1,4 @@ -use crate::storage::{Operation, Storage, StorageTxn, TaskMap, VersionId, DEFAULT_BASE_VERSION}; +use crate::storage::{ReplicaOp, Storage, StorageTxn, TaskMap, VersionId, DEFAULT_BASE_VERSION}; use anyhow::Context; use rusqlite::types::{FromSql, ToSql}; use rusqlite::{params, Connection, OptionalExtension}; @@ -52,17 +52,17 @@ impl ToSql for StoredTaskMap { } } -/// Stores [`Operation`] in SQLite -impl FromSql for Operation { +/// Stores [`ReplicaOp`] in SQLite +impl FromSql for ReplicaOp { fn column_result(value: rusqlite::types::ValueRef<'_>) -> rusqlite::types::FromSqlResult { - let o: Operation = serde_json::from_str(value.as_str()?) + let o: ReplicaOp = serde_json::from_str(value.as_str()?) .map_err(|_| rusqlite::types::FromSqlError::InvalidType)?; Ok(o) } } -/// Parsers Operation stored as JSON in string column -impl ToSql for Operation { +/// Parses ReplicaOp stored as JSON in string column +impl ToSql for ReplicaOp { fn to_sql(&self) -> rusqlite::Result> { let s = serde_json::to_string(&self) .map_err(|e| rusqlite::Error::ToSqlConversionFailure(Box::new(e)))?; @@ -241,12 +241,12 @@ impl<'t> StorageTxn for Txn<'t> { Ok(()) } - fn operations(&mut self) -> anyhow::Result> { + fn operations(&mut self) -> anyhow::Result> { let t = self.get_txn()?; let mut q = t.prepare("SELECT data FROM operations ORDER BY id ASC")?; let rows = q.query_map([], |r| { - let data: Operation = r.get("data")?; + let data: ReplicaOp = r.get("data")?; Ok(data) })?; @@ -257,7 +257,7 @@ impl<'t> StorageTxn for Txn<'t> { Ok(ret) } - fn add_operation(&mut self, op: Operation) -> anyhow::Result<()> { + fn add_operation(&mut self, op: ReplicaOp) -> anyhow::Result<()> { let t = self.get_txn()?; t.execute("INSERT INTO operations (data) VALUES (?)", params![&op]) @@ -265,7 +265,7 @@ impl<'t> StorageTxn for Txn<'t> { Ok(()) } - fn set_operations(&mut self, ops: Vec) -> anyhow::Result<()> { + fn set_operations(&mut self, ops: Vec) -> anyhow::Result<()> { let t = self.get_txn()?; t.execute("DELETE FROM operations", []) .context("Clear all existing operations")?; @@ -611,8 +611,8 @@ mod test { // create some operations { let mut txn = storage.txn()?; - txn.add_operation(Operation::Create { uuid: uuid1 })?; - txn.add_operation(Operation::Create { uuid: uuid2 })?; + txn.add_operation(ReplicaOp::Create { uuid: uuid1 })?; + txn.add_operation(ReplicaOp::Create { uuid: uuid2 })?; txn.commit()?; } @@ -623,8 +623,8 @@ mod test { assert_eq!( ops, vec![ - Operation::Create { uuid: uuid1 }, - Operation::Create { uuid: uuid2 }, + ReplicaOp::Create { uuid: uuid1 }, + ReplicaOp::Create { uuid: uuid2 }, ] ); } @@ -633,8 +633,8 @@ mod test { { let mut txn = storage.txn()?; txn.set_operations(vec![ - Operation::Delete { uuid: uuid2 }, - Operation::Delete { uuid: uuid1 }, + ReplicaOp::Delete { uuid: uuid2 }, + ReplicaOp::Delete { uuid: uuid1 }, ])?; txn.commit()?; } @@ -642,8 +642,8 @@ mod test { // create some more operations (to test adding operations after clearing) { let mut txn = storage.txn()?; - txn.add_operation(Operation::Create { uuid: uuid3 })?; - txn.add_operation(Operation::Delete { uuid: uuid3 })?; + txn.add_operation(ReplicaOp::Create { uuid: uuid3 })?; + txn.add_operation(ReplicaOp::Delete { uuid: uuid3 })?; txn.commit()?; } @@ -654,10 +654,10 @@ mod test { assert_eq!( ops, vec![ - Operation::Delete { uuid: uuid2 }, - Operation::Delete { uuid: uuid1 }, - Operation::Create { uuid: uuid3 }, - Operation::Delete { uuid: uuid3 }, + ReplicaOp::Delete { uuid: uuid2 }, + ReplicaOp::Delete { uuid: uuid1 }, + ReplicaOp::Create { uuid: uuid3 }, + ReplicaOp::Delete { uuid: uuid3 }, ] ); } diff --git a/taskchampion/src/taskdb/mod.rs b/taskchampion/src/taskdb/mod.rs index 026e82698..bd886c29b 100644 --- a/taskchampion/src/taskdb/mod.rs +++ b/taskchampion/src/taskdb/mod.rs @@ -1,5 +1,5 @@ use crate::server::{Server, SyncOp}; -use crate::storage::{Operation, Storage, TaskMap}; +use crate::storage::{ReplicaOp, Storage, TaskMap}; use uuid::Uuid; mod ops; @@ -31,7 +31,7 @@ impl TaskDb { /// Apply an operation to the TaskDb. Aside from synchronization operations, this is the only way /// to modify the TaskDb. In cases where an operation does not make sense, this function will do /// nothing and return an error (but leave the TaskDb in a consistent state). - pub fn apply(&mut self, op: Operation) -> anyhow::Result<()> { + pub fn apply(&mut self, op: ReplicaOp) -> anyhow::Result<()> { // TODO: differentiate error types here? let mut txn = self.storage.txn()?; if let err @ Err(_) = ops::apply_op(txn.as_mut(), &op) { @@ -46,14 +46,14 @@ impl TaskDb { pub fn apply_sync_tmp(&mut self, op: SyncOp) -> anyhow::Result<()> { // create an op from SyncOp let op = match op { - SyncOp::Create { uuid } => Operation::Create { uuid }, - SyncOp::Delete { uuid } => Operation::Delete { uuid }, + SyncOp::Create { uuid } => ReplicaOp::Create { uuid }, + SyncOp::Delete { uuid } => ReplicaOp::Delete { uuid }, SyncOp::Update { uuid, property, value, timestamp, - } => Operation::Update { + } => ReplicaOp::Update { uuid, property, value, @@ -158,7 +158,7 @@ impl TaskDb { } #[cfg(test)] - pub(crate) fn operations(&mut self) -> Vec { + pub(crate) fn operations(&mut self) -> Vec { let mut txn = self.storage.txn().unwrap(); txn.operations() .unwrap() @@ -184,7 +184,7 @@ mod tests { // operations; more detailed tests are in the `ops` module. let mut db = TaskDb::new_inmemory(); let uuid = Uuid::new_v4(); - let op = Operation::Create { uuid }; + let op = ReplicaOp::Create { uuid }; db.apply(op.clone()).unwrap(); assert_eq!(db.sorted_tasks(), vec![(uuid, vec![]),]); @@ -197,7 +197,7 @@ mod tests { #[derive(Debug)] enum Action { - Op(Operation), + Op(ReplicaOp), Sync, } @@ -209,14 +209,14 @@ mod tests { .chunks(2) .map(|action_on| { let action = match action_on[0] { - b'C' => Action::Op(Operation::Create { uuid }), - b'U' => Action::Op(Operation::Update { + b'C' => Action::Op(ReplicaOp::Create { uuid }), + b'U' => Action::Op(ReplicaOp::Update { uuid, property: "title".into(), value: Some("foo".into()), timestamp: Utc::now(), }), - b'D' => Action::Op(Operation::Delete { uuid }), + b'D' => Action::Op(ReplicaOp::Delete { uuid }), b'S' => Action::Sync, _ => unreachable!(), }; diff --git a/taskchampion/src/taskdb/ops.rs b/taskchampion/src/taskdb/ops.rs index 7e23d04ce..45c66cb0c 100644 --- a/taskchampion/src/taskdb/ops.rs +++ b/taskchampion/src/taskdb/ops.rs @@ -1,20 +1,20 @@ use crate::errors::Error; -use crate::storage::{Operation, StorageTxn}; +use crate::storage::{ReplicaOp, StorageTxn}; -pub(super) fn apply_op(txn: &mut dyn StorageTxn, op: &Operation) -> anyhow::Result<()> { +pub(super) fn apply_op(txn: &mut dyn StorageTxn, op: &ReplicaOp) -> anyhow::Result<()> { match op { - Operation::Create { uuid } => { + ReplicaOp::Create { uuid } => { // insert if the task does not already exist if !txn.create_task(*uuid)? { return Err(Error::Database(format!("Task {} already exists", uuid)).into()); } } - Operation::Delete { ref uuid } => { + ReplicaOp::Delete { ref uuid } => { if !txn.delete_task(*uuid)? { return Err(Error::Database(format!("Task {} does not exist", uuid)).into()); } } - Operation::Update { + ReplicaOp::Update { ref uuid, ref property, ref value, @@ -49,7 +49,7 @@ mod tests { fn test_apply_create() -> anyhow::Result<()> { let mut db = TaskDb::new_inmemory(); let uuid = Uuid::new_v4(); - let op = Operation::Create { uuid }; + let op = ReplicaOp::Create { uuid }; { let mut txn = db.storage.txn()?; @@ -65,7 +65,7 @@ mod tests { fn test_apply_create_exists() -> anyhow::Result<()> { let mut db = TaskDb::new_inmemory(); let uuid = Uuid::new_v4(); - let op = Operation::Create { uuid }; + let op = ReplicaOp::Create { uuid }; { let mut txn = db.storage.txn()?; apply_op(txn.as_mut(), &op)?; @@ -86,7 +86,7 @@ mod tests { fn test_apply_create_update() -> anyhow::Result<()> { let mut db = TaskDb::new_inmemory(); let uuid = Uuid::new_v4(); - let op1 = Operation::Create { uuid }; + let op1 = ReplicaOp::Create { uuid }; { let mut txn = db.storage.txn()?; @@ -94,7 +94,7 @@ mod tests { txn.commit()?; } - let op2 = Operation::Update { + let op2 = ReplicaOp::Update { uuid, property: String::from("title"), value: Some("my task".into()), @@ -118,14 +118,14 @@ mod tests { fn test_apply_create_update_delete_prop() -> anyhow::Result<()> { let mut db = TaskDb::new_inmemory(); let uuid = Uuid::new_v4(); - let op1 = Operation::Create { uuid }; + let op1 = ReplicaOp::Create { uuid }; { let mut txn = db.storage.txn()?; apply_op(txn.as_mut(), &op1)?; txn.commit()?; } - let op2 = Operation::Update { + let op2 = ReplicaOp::Update { uuid, property: String::from("title"), value: Some("my task".into()), @@ -137,7 +137,7 @@ mod tests { txn.commit()?; } - let op3 = Operation::Update { + let op3 = ReplicaOp::Update { uuid, property: String::from("priority"), value: Some("H".into()), @@ -149,7 +149,7 @@ mod tests { txn.commit()?; } - let op4 = Operation::Update { + let op4 = ReplicaOp::Update { uuid, property: String::from("title"), value: None, @@ -177,7 +177,7 @@ mod tests { fn test_apply_update_does_not_exist() -> anyhow::Result<()> { let mut db = TaskDb::new_inmemory(); let uuid = Uuid::new_v4(); - let op = Operation::Update { + let op = ReplicaOp::Update { uuid, property: String::from("title"), value: Some("my task".into()), @@ -199,8 +199,8 @@ mod tests { fn test_apply_create_delete() -> anyhow::Result<()> { let mut db = TaskDb::new_inmemory(); let uuid = Uuid::new_v4(); - let op1 = Operation::Create { uuid }; - let op2 = Operation::Delete { uuid }; + let op1 = ReplicaOp::Create { uuid }; + let op2 = ReplicaOp::Delete { uuid }; { let mut txn = db.storage.txn()?; @@ -218,7 +218,7 @@ mod tests { fn test_apply_delete_not_present() -> anyhow::Result<()> { let mut db = TaskDb::new_inmemory(); let uuid = Uuid::new_v4(); - let op = Operation::Delete { uuid }; + let op = ReplicaOp::Delete { uuid }; { let mut txn = db.storage.txn()?; assert_eq!( diff --git a/taskchampion/src/taskdb/sync.rs b/taskchampion/src/taskdb/sync.rs index 7ce1d76ce..f5806656a 100644 --- a/taskchampion/src/taskdb/sync.rs +++ b/taskchampion/src/taskdb/sync.rs @@ -1,6 +1,6 @@ use super::{ops, snapshot}; use crate::server::{AddVersionResult, GetVersionResult, Server, SnapshotUrgency}; -use crate::storage::{Operation, StorageTxn}; +use crate::storage::{ReplicaOp, StorageTxn}; use crate::Error; use log::{info, trace, warn}; use serde::{Deserialize, Serialize}; @@ -8,7 +8,7 @@ use std::str; #[derive(Serialize, Deserialize, Debug)] struct Version { - operations: Vec, + operations: Vec, } /// Sync to the given server, pulling remote changes and pushing local changes. @@ -58,7 +58,7 @@ pub(super) fn sync( } } - let operations: Vec = txn.operations()?.to_vec(); + let operations: Vec = txn.operations()?.to_vec(); if operations.is_empty() { info!("no changes to push to server"); // nothing to sync back to the server.. @@ -136,7 +136,7 @@ fn apply_version(txn: &mut dyn StorageTxn, mut version: Version) -> anyhow::Resu // This is slightly complicated by the fact that the transform function can return None, // indicating no operation is required. If this happens for a local op, we can just omit // it. If it happens for server op, then we must copy the remaining local ops. - let mut local_operations: Vec = txn.operations()?; + let mut local_operations: Vec = txn.operations()?; for server_op in version.operations.drain(..) { trace!( "rebasing local operations onto server operation {:?}", @@ -146,7 +146,7 @@ fn apply_version(txn: &mut dyn StorageTxn, mut version: Version) -> anyhow::Resu let mut svr_op = Some(server_op); for local_op in local_operations.drain(..) { if let Some(o) = svr_op { - let (new_server_op, new_local_op) = Operation::transform(o, local_op.clone()); + let (new_server_op, new_local_op) = ReplicaOp::transform(o, local_op.clone()); trace!("local operation {:?} -> {:?}", local_op, new_local_op); svr_op = new_server_op; if let Some(o) = new_local_op { @@ -175,7 +175,7 @@ fn apply_version(txn: &mut dyn StorageTxn, mut version: Version) -> anyhow::Resu mod test { use super::*; use crate::server::test::TestServer; - use crate::storage::{InMemoryStorage, Operation}; + use crate::storage::{InMemoryStorage, ReplicaOp}; use crate::taskdb::{snapshot::SnapshotTasks, TaskDb}; use chrono::Utc; use pretty_assertions::assert_eq; @@ -197,8 +197,8 @@ mod test { // make some changes in parallel to db1 and db2.. let uuid1 = Uuid::new_v4(); - db1.apply(Operation::Create { uuid: uuid1 }).unwrap(); - db1.apply(Operation::Update { + db1.apply(ReplicaOp::Create { uuid: uuid1 }).unwrap(); + db1.apply(ReplicaOp::Update { uuid: uuid1, property: "title".into(), value: Some("my first task".into()), @@ -207,8 +207,8 @@ mod test { .unwrap(); let uuid2 = Uuid::new_v4(); - db2.apply(Operation::Create { uuid: uuid2 }).unwrap(); - db2.apply(Operation::Update { + db2.apply(ReplicaOp::Create { uuid: uuid2 }).unwrap(); + db2.apply(ReplicaOp::Update { uuid: uuid2, property: "title".into(), value: Some("my second task".into()), @@ -223,14 +223,14 @@ mod test { assert_eq!(db1.sorted_tasks(), db2.sorted_tasks()); // now make updates to the same task on both sides - db1.apply(Operation::Update { + db1.apply(ReplicaOp::Update { uuid: uuid2, property: "priority".into(), value: Some("H".into()), timestamp: Utc::now(), }) .unwrap(); - db2.apply(Operation::Update { + db2.apply(ReplicaOp::Update { uuid: uuid2, property: "project".into(), value: Some("personal".into()), @@ -259,8 +259,8 @@ mod test { // create and update a task.. let uuid = Uuid::new_v4(); - db1.apply(Operation::Create { uuid }).unwrap(); - db1.apply(Operation::Update { + db1.apply(ReplicaOp::Create { uuid }).unwrap(); + db1.apply(ReplicaOp::Update { uuid, property: "title".into(), value: Some("my first task".into()), @@ -275,9 +275,9 @@ mod test { assert_eq!(db1.sorted_tasks(), db2.sorted_tasks()); // delete and re-create the task on db1 - db1.apply(Operation::Delete { uuid }).unwrap(); - db1.apply(Operation::Create { uuid }).unwrap(); - db1.apply(Operation::Update { + db1.apply(ReplicaOp::Delete { uuid }).unwrap(); + db1.apply(ReplicaOp::Create { uuid }).unwrap(); + db1.apply(ReplicaOp::Update { uuid, property: "title".into(), value: Some("my second task".into()), @@ -286,7 +286,7 @@ mod test { .unwrap(); // and on db2, update a property of the task - db2.apply(Operation::Update { + db2.apply(ReplicaOp::Update { uuid, property: "project".into(), value: Some("personal".into()), @@ -310,8 +310,8 @@ mod test { let mut db1 = newdb(); let uuid = Uuid::new_v4(); - db1.apply(Operation::Create { uuid })?; - db1.apply(Operation::Update { + db1.apply(ReplicaOp::Create { uuid })?; + db1.apply(ReplicaOp::Update { uuid, property: "title".into(), value: Some("my first task".into()), @@ -332,7 +332,7 @@ mod test { assert_eq!(tasks[0].0, uuid); // update the taskdb and sync again - db1.apply(Operation::Update { + db1.apply(ReplicaOp::Update { uuid, property: "title".into(), value: Some("my first task, updated".into()), @@ -362,7 +362,7 @@ mod test { let mut db1 = newdb(); let uuid = Uuid::new_v4(); - db1.apply(Operation::Create { uuid }).unwrap(); + db1.apply(ReplicaOp::Create { uuid }).unwrap(); test_server.set_snapshot_urgency(SnapshotUrgency::Low); sync(&mut server, db1.storage.txn()?.as_mut(), true).unwrap(); diff --git a/taskchampion/src/taskdb/working_set.rs b/taskchampion/src/taskdb/working_set.rs index d5e0774b0..dc0680ceb 100644 --- a/taskchampion/src/taskdb/working_set.rs +++ b/taskchampion/src/taskdb/working_set.rs @@ -63,7 +63,7 @@ where #[cfg(test)] mod test { use super::*; - use crate::storage::Operation; + use crate::storage::ReplicaOp; use crate::taskdb::TaskDb; use chrono::Utc; use uuid::Uuid; @@ -94,10 +94,10 @@ mod test { // add everything to the TaskDb for uuid in &uuids { - db.apply(Operation::Create { uuid: *uuid })?; + db.apply(ReplicaOp::Create { uuid: *uuid })?; } for i in &[0usize, 1, 4] { - db.apply(Operation::Update { + db.apply(ReplicaOp::Update { uuid: uuids[*i].clone(), property: String::from("status"), value: Some("pending".into()), From fee25fa74284ebe327fdf1cdee8a919f3f5cf7d2 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sun, 19 Dec 2021 20:54:48 +0000 Subject: [PATCH 04/14] Apply SyncOps, but keep a list of ReplicaOps This changes a lot of function signatures, but basically: * TaskDB::apply now takes a SyncOp, not a ReplicaOp * Replica::update_task returns a TaskMap --- taskchampion/src/replica.rs | 20 +- taskchampion/src/server/op.rs | 36 +-- taskchampion/src/storage/op.rs | 255 ++------------------ taskchampion/src/taskdb/apply.rs | 313 +++++++++++++++++++++++++ taskchampion/src/taskdb/mod.rs | 64 ++--- taskchampion/src/taskdb/ops.rs | 233 ------------------ taskchampion/src/taskdb/sync.rs | 78 ++++-- taskchampion/src/taskdb/working_set.rs | 6 +- 8 files changed, 440 insertions(+), 565 deletions(-) create mode 100644 taskchampion/src/taskdb/apply.rs delete mode 100644 taskchampion/src/taskdb/ops.rs diff --git a/taskchampion/src/replica.rs b/taskchampion/src/replica.rs index 6bdb756e8..ac39091c8 100644 --- a/taskchampion/src/replica.rs +++ b/taskchampion/src/replica.rs @@ -1,6 +1,5 @@ -use crate::errors::Error; -use crate::server::Server; -use crate::storage::{ReplicaOp, Storage, TaskMap}; +use crate::server::{Server, SyncOp}; +use crate::storage::{Storage, TaskMap}; use crate::task::{Status, Task}; use crate::taskdb::TaskDb; use crate::workingset::WorkingSet; @@ -51,12 +50,12 @@ impl Replica { uuid: Uuid, property: S1, value: Option, - ) -> anyhow::Result<()> + ) -> anyhow::Result where S1: Into, S2: Into, { - self.taskdb.apply(ReplicaOp::Update { + self.taskdb.apply(SyncOp::Update { uuid, property: property.into(), value: value.map(|v| v.into()), @@ -100,9 +99,9 @@ impl Replica { /// Create a new task. The task must not already exist. pub fn new_task(&mut self, status: Status, description: String) -> anyhow::Result { let uuid = Uuid::new_v4(); - self.taskdb.apply(ReplicaOp::Create { uuid })?; + let taskmap = self.taskdb.apply(SyncOp::Create { uuid })?; trace!("task {} created", uuid); - let mut task = Task::new(uuid, TaskMap::new()).into_mut(self); + let mut task = Task::new(uuid, taskmap).into_mut(self); task.set_description(description)?; task.set_status(status)?; Ok(task.into_immut()) @@ -113,12 +112,7 @@ impl Replica { /// should only occur through expiration. #[allow(dead_code)] fn delete_task(&mut self, uuid: Uuid) -> anyhow::Result<()> { - // check that it already exists; this is a convenience check, as the task may already exist - // when this Create operation is finally sync'd with operations from other replicas - if self.taskdb.get_task(uuid)?.is_none() { - return Err(Error::Database(format!("Task {} does not exist", uuid)).into()); - } - self.taskdb.apply(ReplicaOp::Delete { uuid })?; + self.taskdb.apply(SyncOp::Delete { uuid })?; trace!("task {} deleted", uuid); Ok(()) } diff --git a/taskchampion/src/server/op.rs b/taskchampion/src/server/op.rs index 6f6561ceb..c2906700f 100644 --- a/taskchampion/src/server/op.rs +++ b/taskchampion/src/server/op.rs @@ -215,20 +215,20 @@ mod test { // the transform function. let mut db1 = TaskDb::new_inmemory(); if let Some(ref o) = setup { - db1.apply_sync_tmp(o.clone()).unwrap(); + db1.apply(o.clone()).unwrap(); } - db1.apply_sync_tmp(o1).unwrap(); + db1.apply(o1).unwrap(); if let Some(o) = o2p { - db1.apply_sync_tmp(o).unwrap(); + db1.apply(o).unwrap(); } let mut db2 = TaskDb::new_inmemory(); if let Some(ref o) = setup { - db2.apply_sync_tmp(o.clone()).unwrap(); + db2.apply(o.clone()).unwrap(); } - db2.apply_sync_tmp(o2).unwrap(); + db2.apply(o2).unwrap(); if let Some(o) = o1p { - db2.apply_sync_tmp(o).unwrap(); + db2.apply(o).unwrap(); } assert_eq!(db1.sorted_tasks(), db2.sorted_tasks()); @@ -380,39 +380,39 @@ mod test { // Ensure that any expected tasks already exist if let Update{ uuid, .. } = o1 { - let _ = db1.apply_sync_tmp(Create{uuid}); - let _ = db2.apply_sync_tmp(Create{uuid}); + let _ = db1.apply(Create{uuid}); + let _ = db2.apply(Create{uuid}); } if let Update{ uuid, .. } = o2 { - let _ = db1.apply_sync_tmp(Create{uuid}); - let _ = db2.apply_sync_tmp(Create{uuid}); + let _ = db1.apply(Create{uuid}); + let _ = db2.apply(Create{uuid}); } if let Delete{ uuid } = o1 { - let _ = db1.apply_sync_tmp(Create{uuid}); - let _ = db2.apply_sync_tmp(Create{uuid}); + let _ = db1.apply(Create{uuid}); + let _ = db2.apply(Create{uuid}); } if let Delete{ uuid } = o2 { - let _ = db1.apply_sync_tmp(Create{uuid}); - let _ = db2.apply_sync_tmp(Create{uuid}); + let _ = db1.apply(Create{uuid}); + let _ = db2.apply(Create{uuid}); } // if applying the initial operations fail, that indicates the operation was invalid // in the base state, so consider the case successful. - if db1.apply_sync_tmp(o1).is_err() { + if db1.apply(o1).is_err() { return Ok(()); } - if db2.apply_sync_tmp(o2).is_err() { + if db2.apply(o2).is_err() { return Ok(()); } if let Some(o) = o2p { - db1.apply_sync_tmp(o).map_err(|e| TestCaseError::Fail(format!("Applying to db1: {}", e).into()))?; + db1.apply(o).map_err(|e| TestCaseError::Fail(format!("Applying to db1: {}", e).into()))?; } if let Some(o) = o1p { - db2.apply_sync_tmp(o).map_err(|e| TestCaseError::Fail(format!("Applying to db2: {}", e).into()))?; + db2.apply(o).map_err(|e| TestCaseError::Fail(format!("Applying to db2: {}", e).into()))?; } assert_eq!(db1.sorted_tasks(), db2.sorted_tasks()); } diff --git a/taskchampion/src/storage/op.rs b/taskchampion/src/storage/op.rs index 0ab9e1104..2e0f45b26 100644 --- a/taskchampion/src/storage/op.rs +++ b/taskchampion/src/storage/op.rs @@ -126,163 +126,17 @@ impl ReplicaOp { #[cfg(test)] mod test { use super::*; - use crate::storage::InMemoryStorage; - use crate::taskdb::TaskDb; - use chrono::{Duration, Utc}; + use chrono::Utc; use pretty_assertions::assert_eq; - use proptest::prelude::*; - - // note that `tests/operation_transform_invariant.rs` tests the transform function quite - // thoroughly, so this testing is light. - - fn test_transform( - setup: Option, - o1: ReplicaOp, - o2: ReplicaOp, - exp1p: Option, - exp2p: Option, - ) { - let (o1p, o2p) = ReplicaOp::transform(o1.clone(), o2.clone()); - assert_eq!((&o1p, &o2p), (&exp1p, &exp2p)); - - // check that the two operation sequences have the same effect, enforcing the invariant of - // the transform function. - let mut db1 = TaskDb::new_inmemory(); - if let Some(ref o) = setup { - db1.apply(o.clone()).unwrap(); - } - db1.apply(o1).unwrap(); - if let Some(o) = o2p { - db1.apply(o).unwrap(); - } - - let mut db2 = TaskDb::new_inmemory(); - if let Some(ref o) = setup { - db2.apply(o.clone()).unwrap(); - } - db2.apply(o2).unwrap(); - if let Some(o) = o1p { - db2.apply(o).unwrap(); - } - - assert_eq!(db1.sorted_tasks(), db2.sorted_tasks()); - } - - #[test] - fn test_unrelated_create() { - let uuid1 = Uuid::new_v4(); - let uuid2 = Uuid::new_v4(); - - test_transform( - None, - Create { uuid: uuid1 }, - Create { uuid: uuid2 }, - Some(Create { uuid: uuid1 }), - Some(Create { uuid: uuid2 }), - ); - } - - #[test] - fn test_related_updates_different_props() { - let uuid = Uuid::new_v4(); - let timestamp = Utc::now(); - - test_transform( - Some(Create { uuid }), - Update { - uuid, - property: "abc".into(), - value: Some("true".into()), - timestamp, - }, - Update { - uuid, - property: "def".into(), - value: Some("false".into()), - timestamp, - }, - Some(Update { - uuid, - property: "abc".into(), - value: Some("true".into()), - timestamp, - }), - Some(Update { - uuid, - property: "def".into(), - value: Some("false".into()), - timestamp, - }), - ); - } - - #[test] - fn test_related_updates_same_prop() { - let uuid = Uuid::new_v4(); - let timestamp1 = Utc::now(); - let timestamp2 = timestamp1 + Duration::seconds(10); - - test_transform( - Some(Create { uuid }), - Update { - uuid, - property: "abc".into(), - value: Some("true".into()), - timestamp: timestamp1, - }, - Update { - uuid, - property: "abc".into(), - value: Some("false".into()), - timestamp: timestamp2, - }, - None, - Some(Update { - uuid, - property: "abc".into(), - value: Some("false".into()), - timestamp: timestamp2, - }), - ); - } - - #[test] - fn test_related_updates_same_prop_same_time() { - let uuid = Uuid::new_v4(); - let timestamp = Utc::now(); - - test_transform( - Some(Create { uuid }), - Update { - uuid, - property: "abc".into(), - value: Some("true".into()), - timestamp, - }, - Update { - uuid, - property: "abc".into(), - value: Some("false".into()), - timestamp, - }, - Some(Update { - uuid, - property: "abc".into(), - value: Some("true".into()), - timestamp, - }), - None, - ); - } #[test] fn test_json_create() -> anyhow::Result<()> { let uuid = Uuid::new_v4(); let op = Create { uuid }; - assert_eq!( - serde_json::to_string(&op)?, - format!(r#"{{"Create":{{"uuid":"{}"}}}}"#, uuid), - ); + let json = serde_json::to_string(&op)?; + assert_eq!(json, format!(r#"{{"Create":{{"uuid":"{}"}}}}"#, uuid)); + let deser: ReplicaOp = serde_json::from_str(&json)?; + assert_eq!(deser, op); Ok(()) } @@ -290,10 +144,10 @@ mod test { fn test_json_delete() -> anyhow::Result<()> { let uuid = Uuid::new_v4(); let op = Delete { uuid }; - assert_eq!( - serde_json::to_string(&op)?, - format!(r#"{{"Delete":{{"uuid":"{}"}}}}"#, uuid), - ); + let json = serde_json::to_string(&op)?; + assert_eq!(json, format!(r#"{{"Delete":{{"uuid":"{}"}}}}"#, uuid)); + let deser: ReplicaOp = serde_json::from_str(&json)?; + assert_eq!(deser, op); Ok(()) } @@ -309,13 +163,16 @@ mod test { timestamp, }; + let json = serde_json::to_string(&op)?; assert_eq!( - serde_json::to_string(&op)?, + json, format!( r#"{{"Update":{{"uuid":"{}","property":"abc","value":"false","timestamp":"{:?}"}}}}"#, uuid, timestamp, - ), + ) ); + let deser: ReplicaOp = serde_json::from_str(&json)?; + assert_eq!(deser, op); Ok(()) } @@ -331,90 +188,16 @@ mod test { timestamp, }; + let json = serde_json::to_string(&op)?; assert_eq!( - serde_json::to_string(&op)?, + json, format!( r#"{{"Update":{{"uuid":"{}","property":"abc","value":null,"timestamp":"{:?}"}}}}"#, uuid, timestamp, - ), + ) ); + let deser: ReplicaOp = serde_json::from_str(&json)?; + assert_eq!(deser, op); Ok(()) } - - fn uuid_strategy() -> impl Strategy { - prop_oneof![ - Just(Uuid::parse_str("83a2f9ef-f455-4195-b92e-a54c161eebfc").unwrap()), - Just(Uuid::parse_str("56e0be07-c61f-494c-a54c-bdcfdd52d2a7").unwrap()), - Just(Uuid::parse_str("4b7ed904-f7b0-4293-8a10-ad452422c7b3").unwrap()), - Just(Uuid::parse_str("9bdd0546-07c8-4e1f-a9bc-9d6299f4773b").unwrap()), - ] - } - - fn operation_strategy() -> impl Strategy { - prop_oneof![ - uuid_strategy().prop_map(|uuid| ReplicaOp::Create { uuid }), - uuid_strategy().prop_map(|uuid| ReplicaOp::Delete { uuid }), - (uuid_strategy(), "(title|project|status)").prop_map(|(uuid, property)| { - ReplicaOp::Update { - uuid, - property, - value: Some("true".into()), - timestamp: Utc::now(), - } - }), - ] - } - - proptest! { - #![proptest_config(ProptestConfig { - cases: 1024, .. ProptestConfig::default() - })] - #[test] - // check that the two operation sequences have the same effect, enforcing the invariant of - // the transform function. - fn transform_invariant_holds(o1 in operation_strategy(), o2 in operation_strategy()) { - let (o1p, o2p) = ReplicaOp::transform(o1.clone(), o2.clone()); - - let mut db1 = TaskDb::new(Box::new(InMemoryStorage::new())); - let mut db2 = TaskDb::new(Box::new(InMemoryStorage::new())); - - // Ensure that any expected tasks already exist - if let ReplicaOp::Update{ ref uuid, .. } = o1 { - let _ = db1.apply(ReplicaOp::Create{uuid: uuid.clone()}); - let _ = db2.apply(ReplicaOp::Create{uuid: uuid.clone()}); - } - - if let ReplicaOp::Update{ ref uuid, .. } = o2 { - let _ = db1.apply(ReplicaOp::Create{uuid: uuid.clone()}); - let _ = db2.apply(ReplicaOp::Create{uuid: uuid.clone()}); - } - - if let ReplicaOp::Delete{ ref uuid } = o1 { - let _ = db1.apply(ReplicaOp::Create{uuid: uuid.clone()}); - let _ = db2.apply(ReplicaOp::Create{uuid: uuid.clone()}); - } - - if let ReplicaOp::Delete{ ref uuid } = o2 { - let _ = db1.apply(ReplicaOp::Create{uuid: uuid.clone()}); - let _ = db2.apply(ReplicaOp::Create{uuid: uuid.clone()}); - } - - // if applying the initial operations fail, that indicates the operation was invalid - // in the base state, so consider the case successful. - if let Err(_) = db1.apply(o1) { - return Ok(()); - } - if let Err(_) = db2.apply(o2) { - return Ok(()); - } - - if let Some(o) = o2p { - db1.apply(o).map_err(|e| TestCaseError::Fail(format!("Applying to db1: {}", e).into()))?; - } - if let Some(o) = o1p { - db2.apply(o).map_err(|e| TestCaseError::Fail(format!("Applying to db2: {}", e).into()))?; - } - assert_eq!(db1.sorted_tasks(), db2.sorted_tasks()); - } - } } diff --git a/taskchampion/src/taskdb/apply.rs b/taskchampion/src/taskdb/apply.rs new file mode 100644 index 000000000..2824586c4 --- /dev/null +++ b/taskchampion/src/taskdb/apply.rs @@ -0,0 +1,313 @@ +use crate::errors::Error; +use crate::server::SyncOp; +use crate::storage::{ReplicaOp, StorageTxn, TaskMap}; + +/// Apply the given SyncOp to the replica, updating both the task data and adding a +/// ReplicaOp to the list of operations. Returns the TaskMap of the task after the +/// operation has been applied (or an empty TaskMap for Delete). +pub(super) fn apply(txn: &mut dyn StorageTxn, op: SyncOp) -> anyhow::Result { + match op { + SyncOp::Create { uuid } => { + let created = txn.create_task(uuid)?; + if created { + txn.add_operation(ReplicaOp::Create { uuid })?; + txn.commit()?; + Ok(TaskMap::new()) + } else { + // TODO: differentiate error types here? + Err(Error::Database(format!("Task {} already exists", uuid)).into()) + } + } + SyncOp::Delete { uuid } => { + let task = txn.get_task(uuid)?; + // (we'll need _task in the next commit) + if let Some(_task) = task { + txn.delete_task(uuid)?; + txn.add_operation(ReplicaOp::Delete { uuid })?; + txn.commit()?; + Ok(TaskMap::new()) + } else { + Err(Error::Database(format!("Task {} does not exist", uuid)).into()) + } + } + SyncOp::Update { + uuid, + property, + value, + timestamp, + } => { + let task = txn.get_task(uuid)?; + if let Some(mut task) = task { + if let Some(ref v) = value { + task.insert(property.clone(), v.clone()); + } else { + task.remove(&property); + } + txn.set_task(uuid, task.clone())?; + txn.add_operation(ReplicaOp::Update { + uuid, + property, + value, + timestamp, + })?; + txn.commit()?; + Ok(task) + } else { + Err(Error::Database(format!("Task {} does not exist", uuid)).into()) + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::taskdb::TaskDb; + use chrono::Utc; + use pretty_assertions::assert_eq; + use std::collections::HashMap; + use uuid::Uuid; + + #[test] + fn test_apply_create() -> anyhow::Result<()> { + let mut db = TaskDb::new_inmemory(); + let uuid = Uuid::new_v4(); + let op = SyncOp::Create { uuid }; + + { + let mut txn = db.storage.txn()?; + let taskmap = apply(txn.as_mut(), op)?; + assert_eq!(taskmap.len(), 0); + txn.commit()?; + } + + assert_eq!(db.sorted_tasks(), vec![(uuid, vec![]),]); + assert_eq!(db.operations(), vec![ReplicaOp::Create { uuid }]); + Ok(()) + } + + #[test] + fn test_apply_create_exists() -> anyhow::Result<()> { + let mut db = TaskDb::new_inmemory(); + let uuid = Uuid::new_v4(); + let op = SyncOp::Create { uuid }; + { + let mut txn = db.storage.txn()?; + let taskmap = apply(txn.as_mut(), op.clone())?; + assert_eq!(taskmap.len(), 0); + assert_eq!( + apply(txn.as_mut(), op).err().unwrap().to_string(), + format!("Task Database Error: Task {} already exists", uuid) + ); + txn.commit()?; + } + + // first op was applied + assert_eq!(db.sorted_tasks(), vec![(uuid, vec![])]); + assert_eq!(db.operations(), vec![ReplicaOp::Create { uuid }]); + Ok(()) + } + + #[test] + fn test_apply_create_update() -> anyhow::Result<()> { + let mut db = TaskDb::new_inmemory(); + let uuid = Uuid::new_v4(); + let now = Utc::now(); + let op1 = SyncOp::Create { uuid }; + + { + let mut txn = db.storage.txn()?; + let taskmap = apply(txn.as_mut(), op1)?; + assert_eq!(taskmap.len(), 0); + txn.commit()?; + } + + let op2 = SyncOp::Update { + uuid, + property: String::from("title"), + value: Some("my task".into()), + timestamp: now, + }; + { + let mut txn = db.storage.txn()?; + let mut taskmap = apply(txn.as_mut(), op2)?; + assert_eq!( + taskmap.drain().collect::>(), + vec![("title".into(), "my task".into())] + ); + txn.commit()?; + } + + assert_eq!( + db.sorted_tasks(), + vec![(uuid, vec![("title".into(), "my task".into())])] + ); + assert_eq!( + db.operations(), + vec![ + ReplicaOp::Create { uuid }, + ReplicaOp::Update { + uuid, + property: "title".into(), + value: Some("my task".into()), + timestamp: now + } + ] + ); + + Ok(()) + } + + #[test] + fn test_apply_create_update_delete_prop() -> anyhow::Result<()> { + let mut db = TaskDb::new_inmemory(); + let uuid = Uuid::new_v4(); + let now = Utc::now(); + let op1 = SyncOp::Create { uuid }; + { + let mut txn = db.storage.txn()?; + let taskmap = apply(txn.as_mut(), op1)?; + assert_eq!(taskmap.len(), 0); + txn.commit()?; + } + + let op2 = SyncOp::Update { + uuid, + property: String::from("title"), + value: Some("my task".into()), + timestamp: now, + }; + { + let mut txn = db.storage.txn()?; + let taskmap = apply(txn.as_mut(), op2)?; + assert_eq!(taskmap.get("title"), Some(&"my task".to_owned())); + txn.commit()?; + } + + let op3 = SyncOp::Update { + uuid, + property: String::from("priority"), + value: Some("H".into()), + timestamp: now, + }; + { + let mut txn = db.storage.txn()?; + let taskmap = apply(txn.as_mut(), op3)?; + assert_eq!(taskmap.get("priority"), Some(&"H".to_owned())); + txn.commit()?; + } + + let op4 = SyncOp::Update { + uuid, + property: String::from("title"), + value: None, + timestamp: now, + }; + { + let mut txn = db.storage.txn()?; + let taskmap = apply(txn.as_mut(), op4)?; + assert_eq!(taskmap.get("title"), None); + assert_eq!(taskmap.get("priority"), Some(&"H".to_owned())); + txn.commit()?; + } + + let mut exp = HashMap::new(); + let mut task = HashMap::new(); + task.insert(String::from("priority"), String::from("H")); + exp.insert(uuid, task); + assert_eq!( + db.sorted_tasks(), + vec![(uuid, vec![("priority".into(), "H".into())])] + ); + assert_eq!( + db.operations(), + vec![ + ReplicaOp::Create { uuid }, + ReplicaOp::Update { + uuid, + property: "title".into(), + value: Some("my task".into()), + timestamp: now, + }, + ReplicaOp::Update { + uuid, + property: "priority".into(), + value: Some("H".into()), + timestamp: now, + }, + ReplicaOp::Update { + uuid, + property: "title".into(), + value: None, + timestamp: now, + } + ] + ); + + Ok(()) + } + + #[test] + fn test_apply_update_does_not_exist() -> anyhow::Result<()> { + let mut db = TaskDb::new_inmemory(); + let uuid = Uuid::new_v4(); + let op = SyncOp::Update { + uuid, + property: String::from("title"), + value: Some("my task".into()), + timestamp: Utc::now(), + }; + { + let mut txn = db.storage.txn()?; + assert_eq!( + apply(txn.as_mut(), op).err().unwrap().to_string(), + format!("Task Database Error: Task {} does not exist", uuid) + ); + txn.commit()?; + } + + Ok(()) + } + + #[test] + fn test_apply_create_delete() -> anyhow::Result<()> { + let mut db = TaskDb::new_inmemory(); + let uuid = Uuid::new_v4(); + let op1 = SyncOp::Create { uuid }; + let op2 = SyncOp::Delete { uuid }; + + { + let mut txn = db.storage.txn()?; + let taskmap = apply(txn.as_mut(), op1)?; + assert_eq!(taskmap.len(), 0); + let taskmap = apply(txn.as_mut(), op2)?; + assert_eq!(taskmap.len(), 0); + txn.commit()?; + } + + assert_eq!(db.sorted_tasks(), vec![]); + assert_eq!( + db.operations(), + vec![ReplicaOp::Create { uuid }, ReplicaOp::Delete { uuid },] + ); + + Ok(()) + } + + #[test] + fn test_apply_delete_not_present() -> anyhow::Result<()> { + let mut db = TaskDb::new_inmemory(); + let uuid = Uuid::new_v4(); + let op = SyncOp::Delete { uuid }; + { + let mut txn = db.storage.txn()?; + assert_eq!( + apply(txn.as_mut(), op).err().unwrap().to_string(), + format!("Task Database Error: Task {} does not exist", uuid) + ); + txn.commit()?; + } + + Ok(()) + } +} diff --git a/taskchampion/src/taskdb/mod.rs b/taskchampion/src/taskdb/mod.rs index bd886c29b..69c0a91e3 100644 --- a/taskchampion/src/taskdb/mod.rs +++ b/taskchampion/src/taskdb/mod.rs @@ -1,8 +1,11 @@ use crate::server::{Server, SyncOp}; -use crate::storage::{ReplicaOp, Storage, TaskMap}; +use crate::storage::{Storage, TaskMap}; use uuid::Uuid; -mod ops; +#[cfg(test)] +use crate::storage::ReplicaOp; + +mod apply; mod snapshot; mod sync; mod working_set; @@ -28,39 +31,16 @@ impl TaskDb { TaskDb::new(Box::new(InMemoryStorage::new())) } - /// Apply an operation to the TaskDb. Aside from synchronization operations, this is the only way - /// to modify the TaskDb. In cases where an operation does not make sense, this function will do - /// nothing and return an error (but leave the TaskDb in a consistent state). - pub fn apply(&mut self, op: ReplicaOp) -> anyhow::Result<()> { - // TODO: differentiate error types here? + /// Apply an operation to the TaskDb. This will update the set of tasks and add a ReplicaOp to + /// the set of operations in the TaskDb, and return the TaskMap containing the resulting task's + /// properties (or an empty TaskMap for deletion). + /// + /// Aside from synchronization operations, this is the only way to modify the TaskDb. In cases + /// where an operation does not make sense, this function will do nothing and return an error + /// (but leave the TaskDb in a consistent state). + pub fn apply(&mut self, op: SyncOp) -> anyhow::Result { let mut txn = self.storage.txn()?; - if let err @ Err(_) = ops::apply_op(txn.as_mut(), &op) { - return err; - } - txn.add_operation(op)?; - txn.commit()?; - Ok(()) - } - - // temporary - pub fn apply_sync_tmp(&mut self, op: SyncOp) -> anyhow::Result<()> { - // create an op from SyncOp - let op = match op { - SyncOp::Create { uuid } => ReplicaOp::Create { uuid }, - SyncOp::Delete { uuid } => ReplicaOp::Delete { uuid }, - SyncOp::Update { - uuid, - property, - value, - timestamp, - } => ReplicaOp::Update { - uuid, - property, - value, - timestamp, - }, - }; - self.apply(op) + apply::apply(txn.as_mut(), op) } /// Get all tasks. @@ -172,7 +152,7 @@ impl TaskDb { mod tests { use super::*; use crate::server::test::TestServer; - use crate::storage::InMemoryStorage; + use crate::storage::{InMemoryStorage, ReplicaOp}; use chrono::Utc; use pretty_assertions::assert_eq; use proptest::prelude::*; @@ -181,14 +161,14 @@ mod tests { #[test] fn test_apply() { // this verifies that the operation is both applied and included in the list of - // operations; more detailed tests are in the `ops` module. + // operations; more detailed tests are in the `apply` module. let mut db = TaskDb::new_inmemory(); let uuid = Uuid::new_v4(); - let op = ReplicaOp::Create { uuid }; + let op = SyncOp::Create { uuid }; db.apply(op.clone()).unwrap(); assert_eq!(db.sorted_tasks(), vec![(uuid, vec![]),]); - assert_eq!(db.operations(), vec![op]); + assert_eq!(db.operations(), vec![ReplicaOp::Create { uuid }]); } fn newdb() -> TaskDb { @@ -197,7 +177,7 @@ mod tests { #[derive(Debug)] enum Action { - Op(ReplicaOp), + Op(SyncOp), Sync, } @@ -209,14 +189,14 @@ mod tests { .chunks(2) .map(|action_on| { let action = match action_on[0] { - b'C' => Action::Op(ReplicaOp::Create { uuid }), - b'U' => Action::Op(ReplicaOp::Update { + b'C' => Action::Op(SyncOp::Create { uuid }), + b'U' => Action::Op(SyncOp::Update { uuid, property: "title".into(), value: Some("foo".into()), timestamp: Utc::now(), }), - b'D' => Action::Op(ReplicaOp::Delete { uuid }), + b'D' => Action::Op(SyncOp::Delete { uuid }), b'S' => Action::Sync, _ => unreachable!(), }; diff --git a/taskchampion/src/taskdb/ops.rs b/taskchampion/src/taskdb/ops.rs deleted file mode 100644 index 45c66cb0c..000000000 --- a/taskchampion/src/taskdb/ops.rs +++ /dev/null @@ -1,233 +0,0 @@ -use crate::errors::Error; -use crate::storage::{ReplicaOp, StorageTxn}; - -pub(super) fn apply_op(txn: &mut dyn StorageTxn, op: &ReplicaOp) -> anyhow::Result<()> { - match op { - ReplicaOp::Create { uuid } => { - // insert if the task does not already exist - if !txn.create_task(*uuid)? { - return Err(Error::Database(format!("Task {} already exists", uuid)).into()); - } - } - ReplicaOp::Delete { ref uuid } => { - if !txn.delete_task(*uuid)? { - return Err(Error::Database(format!("Task {} does not exist", uuid)).into()); - } - } - ReplicaOp::Update { - ref uuid, - ref property, - ref value, - timestamp: _, - } => { - // update if this task exists, otherwise ignore - if let Some(mut task) = txn.get_task(*uuid)? { - match value { - Some(ref val) => task.insert(property.to_string(), val.clone()), - None => task.remove(property), - }; - txn.set_task(*uuid, task)?; - } else { - return Err(Error::Database(format!("Task {} does not exist", uuid)).into()); - } - } - } - - Ok(()) -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::taskdb::TaskDb; - use chrono::Utc; - use pretty_assertions::assert_eq; - use std::collections::HashMap; - use uuid::Uuid; - - #[test] - fn test_apply_create() -> anyhow::Result<()> { - let mut db = TaskDb::new_inmemory(); - let uuid = Uuid::new_v4(); - let op = ReplicaOp::Create { uuid }; - - { - let mut txn = db.storage.txn()?; - apply_op(txn.as_mut(), &op)?; - txn.commit()?; - } - - assert_eq!(db.sorted_tasks(), vec![(uuid, vec![]),]); - Ok(()) - } - - #[test] - fn test_apply_create_exists() -> anyhow::Result<()> { - let mut db = TaskDb::new_inmemory(); - let uuid = Uuid::new_v4(); - let op = ReplicaOp::Create { uuid }; - { - let mut txn = db.storage.txn()?; - apply_op(txn.as_mut(), &op)?; - assert_eq!( - apply_op(txn.as_mut(), &op).err().unwrap().to_string(), - format!("Task Database Error: Task {} already exists", uuid) - ); - txn.commit()?; - } - - // first op was applied - assert_eq!(db.sorted_tasks(), vec![(uuid, vec![])]); - - Ok(()) - } - - #[test] - fn test_apply_create_update() -> anyhow::Result<()> { - let mut db = TaskDb::new_inmemory(); - let uuid = Uuid::new_v4(); - let op1 = ReplicaOp::Create { uuid }; - - { - let mut txn = db.storage.txn()?; - apply_op(txn.as_mut(), &op1)?; - txn.commit()?; - } - - let op2 = ReplicaOp::Update { - uuid, - property: String::from("title"), - value: Some("my task".into()), - timestamp: Utc::now(), - }; - { - let mut txn = db.storage.txn()?; - apply_op(txn.as_mut(), &op2)?; - txn.commit()?; - } - - assert_eq!( - db.sorted_tasks(), - vec![(uuid, vec![("title".into(), "my task".into())])] - ); - - Ok(()) - } - - #[test] - fn test_apply_create_update_delete_prop() -> anyhow::Result<()> { - let mut db = TaskDb::new_inmemory(); - let uuid = Uuid::new_v4(); - let op1 = ReplicaOp::Create { uuid }; - { - let mut txn = db.storage.txn()?; - apply_op(txn.as_mut(), &op1)?; - txn.commit()?; - } - - let op2 = ReplicaOp::Update { - uuid, - property: String::from("title"), - value: Some("my task".into()), - timestamp: Utc::now(), - }; - { - let mut txn = db.storage.txn()?; - apply_op(txn.as_mut(), &op2)?; - txn.commit()?; - } - - let op3 = ReplicaOp::Update { - uuid, - property: String::from("priority"), - value: Some("H".into()), - timestamp: Utc::now(), - }; - { - let mut txn = db.storage.txn()?; - apply_op(txn.as_mut(), &op3)?; - txn.commit()?; - } - - let op4 = ReplicaOp::Update { - uuid, - property: String::from("title"), - value: None, - timestamp: Utc::now(), - }; - { - let mut txn = db.storage.txn()?; - apply_op(txn.as_mut(), &op4)?; - txn.commit()?; - } - - let mut exp = HashMap::new(); - let mut task = HashMap::new(); - task.insert(String::from("priority"), String::from("H")); - exp.insert(uuid, task); - assert_eq!( - db.sorted_tasks(), - vec![(uuid, vec![("priority".into(), "H".into())])] - ); - - Ok(()) - } - - #[test] - fn test_apply_update_does_not_exist() -> anyhow::Result<()> { - let mut db = TaskDb::new_inmemory(); - let uuid = Uuid::new_v4(); - let op = ReplicaOp::Update { - uuid, - property: String::from("title"), - value: Some("my task".into()), - timestamp: Utc::now(), - }; - { - let mut txn = db.storage.txn()?; - assert_eq!( - apply_op(txn.as_mut(), &op).err().unwrap().to_string(), - format!("Task Database Error: Task {} does not exist", uuid) - ); - txn.commit()?; - } - - Ok(()) - } - - #[test] - fn test_apply_create_delete() -> anyhow::Result<()> { - let mut db = TaskDb::new_inmemory(); - let uuid = Uuid::new_v4(); - let op1 = ReplicaOp::Create { uuid }; - let op2 = ReplicaOp::Delete { uuid }; - - { - let mut txn = db.storage.txn()?; - apply_op(txn.as_mut(), &op1)?; - apply_op(txn.as_mut(), &op2)?; - txn.commit()?; - } - - assert_eq!(db.sorted_tasks(), vec![]); - - Ok(()) - } - - #[test] - fn test_apply_delete_not_present() -> anyhow::Result<()> { - let mut db = TaskDb::new_inmemory(); - let uuid = Uuid::new_v4(); - let op = ReplicaOp::Delete { uuid }; - { - let mut txn = db.storage.txn()?; - assert_eq!( - apply_op(txn.as_mut(), &op).err().unwrap().to_string(), - format!("Task Database Error: Task {} does not exist", uuid) - ); - txn.commit()?; - } - - Ok(()) - } -} diff --git a/taskchampion/src/taskdb/sync.rs b/taskchampion/src/taskdb/sync.rs index f5806656a..944deda57 100644 --- a/taskchampion/src/taskdb/sync.rs +++ b/taskchampion/src/taskdb/sync.rs @@ -1,4 +1,4 @@ -use super::{ops, snapshot}; +use super::snapshot; use crate::server::{AddVersionResult, GetVersionResult, Server, SnapshotUrgency}; use crate::storage::{ReplicaOp, StorageTxn}; use crate::Error; @@ -11,6 +11,44 @@ struct Version { operations: Vec, } +/// Apply an op to the TaskDb's set of tasks (without recording it in the list of operations) +pub(super) fn apply_op(txn: &mut dyn StorageTxn, op: &ReplicaOp) -> anyhow::Result<()> { + // TODO: it'd be nice if this was integrated into apply() somehow, but that clones TaskMaps + // unnecessariliy + match op { + ReplicaOp::Create { uuid } => { + // insert if the task does not already exist + if !txn.create_task(*uuid)? { + return Err(Error::Database(format!("Task {} already exists", uuid)).into()); + } + } + ReplicaOp::Delete { ref uuid } => { + if !txn.delete_task(*uuid)? { + return Err(Error::Database(format!("Task {} does not exist", uuid)).into()); + } + } + ReplicaOp::Update { + ref uuid, + ref property, + ref value, + timestamp: _, + } => { + // update if this task exists, otherwise ignore + if let Some(mut task) = txn.get_task(*uuid)? { + match value { + Some(ref val) => task.insert(property.to_string(), val.clone()), + None => task.remove(property), + }; + txn.set_task(*uuid, task)?; + } else { + return Err(Error::Database(format!("Task {} does not exist", uuid)).into()); + } + } + } + + Ok(()) +} + /// Sync to the given server, pulling remote changes and pushing local changes. pub(super) fn sync( server: &mut Box, @@ -161,7 +199,7 @@ fn apply_version(txn: &mut dyn StorageTxn, mut version: Version) -> anyhow::Resu } } if let Some(o) = svr_op { - if let Err(e) = ops::apply_op(txn, &o) { + if let Err(e) = apply_op(txn, &o) { warn!("Invalid operation when syncing: {} (ignored)", e); } } @@ -174,8 +212,8 @@ fn apply_version(txn: &mut dyn StorageTxn, mut version: Version) -> anyhow::Resu #[cfg(test)] mod test { use super::*; - use crate::server::test::TestServer; - use crate::storage::{InMemoryStorage, ReplicaOp}; + use crate::server::{test::TestServer, SyncOp}; + use crate::storage::InMemoryStorage; use crate::taskdb::{snapshot::SnapshotTasks, TaskDb}; use chrono::Utc; use pretty_assertions::assert_eq; @@ -197,8 +235,8 @@ mod test { // make some changes in parallel to db1 and db2.. let uuid1 = Uuid::new_v4(); - db1.apply(ReplicaOp::Create { uuid: uuid1 }).unwrap(); - db1.apply(ReplicaOp::Update { + db1.apply(SyncOp::Create { uuid: uuid1 }).unwrap(); + db1.apply(SyncOp::Update { uuid: uuid1, property: "title".into(), value: Some("my first task".into()), @@ -207,8 +245,8 @@ mod test { .unwrap(); let uuid2 = Uuid::new_v4(); - db2.apply(ReplicaOp::Create { uuid: uuid2 }).unwrap(); - db2.apply(ReplicaOp::Update { + db2.apply(SyncOp::Create { uuid: uuid2 }).unwrap(); + db2.apply(SyncOp::Update { uuid: uuid2, property: "title".into(), value: Some("my second task".into()), @@ -223,14 +261,14 @@ mod test { assert_eq!(db1.sorted_tasks(), db2.sorted_tasks()); // now make updates to the same task on both sides - db1.apply(ReplicaOp::Update { + db1.apply(SyncOp::Update { uuid: uuid2, property: "priority".into(), value: Some("H".into()), timestamp: Utc::now(), }) .unwrap(); - db2.apply(ReplicaOp::Update { + db2.apply(SyncOp::Update { uuid: uuid2, property: "project".into(), value: Some("personal".into()), @@ -259,8 +297,8 @@ mod test { // create and update a task.. let uuid = Uuid::new_v4(); - db1.apply(ReplicaOp::Create { uuid }).unwrap(); - db1.apply(ReplicaOp::Update { + db1.apply(SyncOp::Create { uuid }).unwrap(); + db1.apply(SyncOp::Update { uuid, property: "title".into(), value: Some("my first task".into()), @@ -275,9 +313,9 @@ mod test { assert_eq!(db1.sorted_tasks(), db2.sorted_tasks()); // delete and re-create the task on db1 - db1.apply(ReplicaOp::Delete { uuid }).unwrap(); - db1.apply(ReplicaOp::Create { uuid }).unwrap(); - db1.apply(ReplicaOp::Update { + db1.apply(SyncOp::Delete { uuid }).unwrap(); + db1.apply(SyncOp::Create { uuid }).unwrap(); + db1.apply(SyncOp::Update { uuid, property: "title".into(), value: Some("my second task".into()), @@ -286,7 +324,7 @@ mod test { .unwrap(); // and on db2, update a property of the task - db2.apply(ReplicaOp::Update { + db2.apply(SyncOp::Update { uuid, property: "project".into(), value: Some("personal".into()), @@ -310,8 +348,8 @@ mod test { let mut db1 = newdb(); let uuid = Uuid::new_v4(); - db1.apply(ReplicaOp::Create { uuid })?; - db1.apply(ReplicaOp::Update { + db1.apply(SyncOp::Create { uuid })?; + db1.apply(SyncOp::Update { uuid, property: "title".into(), value: Some("my first task".into()), @@ -332,7 +370,7 @@ mod test { assert_eq!(tasks[0].0, uuid); // update the taskdb and sync again - db1.apply(ReplicaOp::Update { + db1.apply(SyncOp::Update { uuid, property: "title".into(), value: Some("my first task, updated".into()), @@ -362,7 +400,7 @@ mod test { let mut db1 = newdb(); let uuid = Uuid::new_v4(); - db1.apply(ReplicaOp::Create { uuid }).unwrap(); + db1.apply(SyncOp::Create { uuid }).unwrap(); test_server.set_snapshot_urgency(SnapshotUrgency::Low); sync(&mut server, db1.storage.txn()?.as_mut(), true).unwrap(); diff --git a/taskchampion/src/taskdb/working_set.rs b/taskchampion/src/taskdb/working_set.rs index dc0680ceb..dd9e57f97 100644 --- a/taskchampion/src/taskdb/working_set.rs +++ b/taskchampion/src/taskdb/working_set.rs @@ -63,7 +63,7 @@ where #[cfg(test)] mod test { use super::*; - use crate::storage::ReplicaOp; + use crate::server::SyncOp; use crate::taskdb::TaskDb; use chrono::Utc; use uuid::Uuid; @@ -94,10 +94,10 @@ mod test { // add everything to the TaskDb for uuid in &uuids { - db.apply(ReplicaOp::Create { uuid: *uuid })?; + db.apply(SyncOp::Create { uuid: *uuid })?; } for i in &[0usize, 1, 4] { - db.apply(ReplicaOp::Update { + db.apply(SyncOp::Update { uuid: uuids[*i].clone(), property: String::from("status"), value: Some("pending".into()), From cefdd83d9403ad8b182136c4cd26e993e3407c49 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sun, 19 Dec 2021 21:04:00 +0000 Subject: [PATCH 05/14] Use the latest taskmap when modifying a task The previous logic duplicated the action of applying an operation to the TaskDb with a "manual" application to the Task's local TaskMap. This now uses the updated TaskMap fetched from the DB, which will help to incorporate any other concurrent DB updates. --- taskchampion/src/task/task.rs | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/taskchampion/src/task/task.rs b/taskchampion/src/task/task.rs index d38555e19..6cc3c5126 100644 --- a/taskchampion/src/task/task.rs +++ b/taskchampion/src/task/task.rs @@ -260,10 +260,10 @@ impl<'r> TaskMut<'r> { fn lastmod(&mut self) -> anyhow::Result<()> { if !self.updated_modified { let now = format!("{}", Utc::now().timestamp()); - self.replica - .update_task(self.task.uuid, "modified", Some(now.clone()))?; trace!("task {}: set property modified={:?}", self.task.uuid, now); - self.task.taskmap.insert(String::from("modified"), now); + self.task.taskmap = + self.replica + .update_task(self.task.uuid, "modified", Some(now.clone()))?; self.updated_modified = true; } Ok(()) @@ -276,16 +276,17 @@ impl<'r> TaskMut<'r> { ) -> anyhow::Result<()> { let property = property.into(); self.lastmod()?; - self.replica - .update_task(self.task.uuid, &property, value.as_ref())?; - if let Some(v) = value { + if let Some(ref v) = value { trace!("task {}: set property {}={:?}", self.task.uuid, property, v); - self.task.taskmap.insert(property, v); } else { trace!("task {}: remove property {}", self.task.uuid, property); - self.task.taskmap.remove(&property); } + + self.task.taskmap = self + .replica + .update_task(self.task.uuid, &property, value.as_ref())?; + Ok(()) } @@ -294,18 +295,7 @@ impl<'r> TaskMut<'r> { property: &str, value: Option>, ) -> anyhow::Result<()> { - self.lastmod()?; - if let Some(value) = value { - let ts = format!("{}", value.timestamp()); - self.replica - .update_task(self.task.uuid, property, Some(ts.clone()))?; - self.task.taskmap.insert(property.to_string(), ts); - } else { - self.replica - .update_task::<_, &str>(self.task.uuid, property, None)?; - self.task.taskmap.remove(property); - } - Ok(()) + self.set_string(property, value.map(|v| v.timestamp().to_string())) } /// Used by tests to ensure that updates are properly written From 1789344cd0f6254c0e2c3f78d3f19a5e2fc1accc Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sun, 19 Dec 2021 21:13:55 +0000 Subject: [PATCH 06/14] refactor sync to use SyncOps --- taskchampion/src/storage/op.rs | 140 ++++++++++++-------------------- taskchampion/src/taskdb/sync.rs | 49 ++++++----- 2 files changed, 79 insertions(+), 110 deletions(-) diff --git a/taskchampion/src/storage/op.rs b/taskchampion/src/storage/op.rs index 2e0f45b26..25225a848 100644 --- a/taskchampion/src/storage/op.rs +++ b/taskchampion/src/storage/op.rs @@ -1,3 +1,4 @@ +use crate::server::SyncOp; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use uuid::Uuid; @@ -28,97 +29,23 @@ pub enum ReplicaOp { }, } -use ReplicaOp::*; - impl ReplicaOp { - // Transform takes two operations A and B that happened concurrently and produces two - // operations A' and B' such that `apply(apply(S, A), B') = apply(apply(S, B), A')`. This - // function is used to serialize operations in a process similar to a Git "rebase". - // - // * - // / \ - // op1 / \ op2 - // / \ - // * * - // - // this function "completes the diamond: - // - // * * - // \ / - // op2' \ / op1' - // \ / - // * - // - // such that applying op2' after op1 has the same effect as applying op1' after op2. This - // allows two different systems which have already applied op1 and op2, respectively, and thus - // reached different states, to return to the same state by applying op2' and op1', - // respectively. - pub fn transform( - operation1: ReplicaOp, - operation2: ReplicaOp, - ) -> (Option, Option) { - match (&operation1, &operation2) { - // Two creations or deletions of the same uuid reach the same state, so there's no need - // for any further operations to bring the state together. - (&Create { uuid: uuid1 }, &Create { uuid: uuid2 }) if uuid1 == uuid2 => (None, None), - (&Delete { uuid: uuid1 }, &Delete { uuid: uuid2 }) if uuid1 == uuid2 => (None, None), - - // Given a create and a delete of the same task, one of the operations is invalid: the - // create implies the task does not exist, but the delete implies it exists. Somewhat - // arbitrarily, we prefer the Create - (&Create { uuid: uuid1 }, &Delete { uuid: uuid2 }) if uuid1 == uuid2 => { - (Some(operation1), None) - } - (&Delete { uuid: uuid1 }, &Create { uuid: uuid2 }) if uuid1 == uuid2 => { - (None, Some(operation2)) - } - - // And again from an Update and a Create, prefer the Update - (&Update { uuid: uuid1, .. }, &Create { uuid: uuid2 }) if uuid1 == uuid2 => { - (Some(operation1), None) - } - (&Create { uuid: uuid1 }, &Update { uuid: uuid2, .. }) if uuid1 == uuid2 => { - (None, Some(operation2)) - } - - // Given a delete and an update, prefer the delete - (&Update { uuid: uuid1, .. }, &Delete { uuid: uuid2 }) if uuid1 == uuid2 => { - (None, Some(operation2)) - } - (&Delete { uuid: uuid1 }, &Update { uuid: uuid2, .. }) if uuid1 == uuid2 => { - (Some(operation1), None) - } - - // Two updates to the same property of the same task might conflict. - ( - &Update { - uuid: ref uuid1, - property: ref property1, - value: ref value1, - timestamp: ref timestamp1, - }, - &Update { - uuid: ref uuid2, - property: ref property2, - value: ref value2, - timestamp: ref timestamp2, - }, - ) if uuid1 == uuid2 && property1 == property2 => { - // if the value is the same, there's no conflict - if value1 == value2 { - (None, None) - } else if timestamp1 < timestamp2 { - // prefer the later modification - (None, Some(operation2)) - } else { - // prefer the later modification or, if the modifications are the same, - // just choose one of them - (Some(operation1), None) - } - } - - // anything else is not a conflict of any sort, so return the operations unchanged - (_, _) => (Some(operation1), Some(operation2)), + /// Convert this operation into a [`SyncOp`]. + pub fn into_sync(self) -> SyncOp { + match self { + Self::Create { uuid } => SyncOp::Create { uuid }, + Self::Delete { uuid } => SyncOp::Delete { uuid }, + Self::Update { + uuid, + property, + value, + timestamp, + } => SyncOp::Update { + uuid, + property, + value, + timestamp, + }, } } } @@ -200,4 +127,37 @@ mod test { assert_eq!(deser, op); Ok(()) } + + #[test] + fn test_into_sync_create() { + let uuid = Uuid::new_v4(); + assert_eq!(Create { uuid }.into_sync(), SyncOp::Create { uuid }); + } + + #[test] + fn test_into_sync_delete() { + let uuid = Uuid::new_v4(); + assert_eq!(Delete { uuid }.into_sync(), SyncOp::Delete { uuid }); + } + + #[test] + fn test_into_sync_update() { + let uuid = Uuid::new_v4(); + let timestamp = Utc::now(); + assert_eq!( + Update { + uuid, + property: "prop".into(), + value: Some("v".into()), + timestamp, + } + .into_sync(), + SyncOp::Update { + uuid, + property: "prop".into(), + value: Some("v".into()), + timestamp, + } + ); + } } diff --git a/taskchampion/src/taskdb/sync.rs b/taskchampion/src/taskdb/sync.rs index 944deda57..b6c92f093 100644 --- a/taskchampion/src/taskdb/sync.rs +++ b/taskchampion/src/taskdb/sync.rs @@ -1,6 +1,6 @@ use super::snapshot; -use crate::server::{AddVersionResult, GetVersionResult, Server, SnapshotUrgency}; -use crate::storage::{ReplicaOp, StorageTxn}; +use crate::server::{AddVersionResult, GetVersionResult, Server, SnapshotUrgency, SyncOp}; +use crate::storage::StorageTxn; use crate::Error; use log::{info, trace, warn}; use serde::{Deserialize, Serialize}; @@ -8,26 +8,26 @@ use std::str; #[derive(Serialize, Deserialize, Debug)] struct Version { - operations: Vec, + operations: Vec, } /// Apply an op to the TaskDb's set of tasks (without recording it in the list of operations) -pub(super) fn apply_op(txn: &mut dyn StorageTxn, op: &ReplicaOp) -> anyhow::Result<()> { +pub(super) fn apply_op(txn: &mut dyn StorageTxn, op: &SyncOp) -> anyhow::Result<()> { // TODO: it'd be nice if this was integrated into apply() somehow, but that clones TaskMaps // unnecessariliy match op { - ReplicaOp::Create { uuid } => { + SyncOp::Create { uuid } => { // insert if the task does not already exist if !txn.create_task(*uuid)? { return Err(Error::Database(format!("Task {} already exists", uuid)).into()); } } - ReplicaOp::Delete { ref uuid } => { + SyncOp::Delete { ref uuid } => { if !txn.delete_task(*uuid)? { return Err(Error::Database(format!("Task {} does not exist", uuid)).into()); } } - ReplicaOp::Update { + SyncOp::Update { ref uuid, ref property, ref value, @@ -72,6 +72,12 @@ pub(super) fn sync( trace!("beginning sync outer loop"); let mut base_version_id = txn.base_version()?; + let mut local_ops: Vec = txn + .operations()? + .drain(..) + .map(|op| op.into_sync()) + .collect(); + // first pull changes and "rebase" on top of them loop { trace!("beginning sync inner loop"); @@ -86,7 +92,7 @@ pub(super) fn sync( // apply this verison and update base_version in storage info!("applying version {:?} from server", version_id); - apply_version(txn, version)?; + apply_version(txn, &mut local_ops, version)?; txn.set_base_version(version_id)?; base_version_id = version_id; } else { @@ -96,17 +102,18 @@ pub(super) fn sync( } } - let operations: Vec = txn.operations()?.to_vec(); - if operations.is_empty() { + if local_ops.is_empty() { info!("no changes to push to server"); // nothing to sync back to the server.. break; } - trace!("sending {} operations to the server", operations.len()); + trace!("sending {} operations to the server", local_ops.len()); // now make a version of our local changes and push those - let new_version = Version { operations }; + let new_version = Version { + operations: local_ops, + }; let history_segment = serde_json::to_string(&new_version).unwrap().into(); info!("sending new version to server"); let (res, snapshot_urgency) = server.add_version(base_version_id, history_segment)?; @@ -114,7 +121,6 @@ pub(super) fn sync( AddVersionResult::Ok(new_version_id) => { info!("version {:?} received by server", new_version_id); txn.set_base_version(new_version_id)?; - txn.set_operations(vec![])?; // make a snapshot if the server indicates it is urgent enough let base_urgency = if avoid_snapshots { @@ -144,11 +150,16 @@ pub(super) fn sync( } } + txn.set_operations(vec![])?; txn.commit()?; Ok(()) } -fn apply_version(txn: &mut dyn StorageTxn, mut version: Version) -> anyhow::Result<()> { +fn apply_version( + txn: &mut dyn StorageTxn, + local_ops: &mut Vec, + mut version: Version, +) -> anyhow::Result<()> { // The situation here is that the server has already applied all server operations, and we // have already applied all local operations, so states have diverged by several // operations. We need to figure out what operations to apply locally and on the server in @@ -174,17 +185,16 @@ fn apply_version(txn: &mut dyn StorageTxn, mut version: Version) -> anyhow::Resu // This is slightly complicated by the fact that the transform function can return None, // indicating no operation is required. If this happens for a local op, we can just omit // it. If it happens for server op, then we must copy the remaining local ops. - let mut local_operations: Vec = txn.operations()?; for server_op in version.operations.drain(..) { trace!( "rebasing local operations onto server operation {:?}", server_op ); - let mut new_local_ops = Vec::with_capacity(local_operations.len()); + let mut new_local_ops = Vec::with_capacity(local_ops.len()); let mut svr_op = Some(server_op); - for local_op in local_operations.drain(..) { + for local_op in local_ops.drain(..) { if let Some(o) = svr_op { - let (new_server_op, new_local_op) = ReplicaOp::transform(o, local_op.clone()); + let (new_server_op, new_local_op) = SyncOp::transform(o, local_op.clone()); trace!("local operation {:?} -> {:?}", local_op, new_local_op); svr_op = new_server_op; if let Some(o) = new_local_op { @@ -203,9 +213,8 @@ fn apply_version(txn: &mut dyn StorageTxn, mut version: Version) -> anyhow::Resu warn!("Invalid operation when syncing: {} (ignored)", e); } } - local_operations = new_local_ops; + *local_ops = new_local_ops; } - txn.set_operations(local_operations)?; Ok(()) } From 103bbcdf8f2acef459ac97923cde59cc49a96cee Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sun, 19 Dec 2021 21:25:13 +0000 Subject: [PATCH 07/14] Store data necessary to undo ReplicaOps --- taskchampion/src/storage/op.rs | 42 +++++++++++++++++++------ taskchampion/src/storage/sqlite.rs | 30 ++++++++++++++---- taskchampion/src/taskdb/apply.rs | 50 ++++++++++++++++++++++++++---- 3 files changed, 100 insertions(+), 22 deletions(-) diff --git a/taskchampion/src/storage/op.rs b/taskchampion/src/storage/op.rs index 25225a848..a31366ddc 100644 --- a/taskchampion/src/storage/op.rs +++ b/taskchampion/src/storage/op.rs @@ -1,4 +1,5 @@ use crate::server::SyncOp; +use crate::storage::TaskMap; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use uuid::Uuid; @@ -9,21 +10,22 @@ use uuid::Uuid; pub enum ReplicaOp { /// Create a new task. /// - /// On application, if the task already exists, the operation does nothing. + /// On undo, the task is deleted. Create { uuid: Uuid }, /// Delete an existing task. /// - /// On application, if the task does not exist, the operation does nothing. - Delete { uuid: Uuid }, + /// On undo, the task's data is restored from old_task. + Delete { uuid: Uuid, old_task: TaskMap }, /// Update an existing task, setting the given property to the given value. If the value is /// None, then the corresponding property is deleted. /// - /// If the given task does not exist, the operation does nothing. + /// On undo, the property is set back to its previous value. Update { uuid: Uuid, property: String, + old_value: Option, value: Option, timestamp: DateTime, }, @@ -34,12 +36,13 @@ impl ReplicaOp { pub fn into_sync(self) -> SyncOp { match self { Self::Create { uuid } => SyncOp::Create { uuid }, - Self::Delete { uuid } => SyncOp::Delete { uuid }, + Self::Delete { uuid, .. } => SyncOp::Delete { uuid }, Self::Update { uuid, property, value, timestamp, + .. } => SyncOp::Update { uuid, property, @@ -56,6 +59,8 @@ mod test { use chrono::Utc; use pretty_assertions::assert_eq; + use ReplicaOp::*; + #[test] fn test_json_create() -> anyhow::Result<()> { let uuid = Uuid::new_v4(); @@ -70,9 +75,16 @@ mod test { #[test] fn test_json_delete() -> anyhow::Result<()> { let uuid = Uuid::new_v4(); - let op = Delete { uuid }; + let old_task = vec![("foo".into(), "bar".into())].drain(..).collect(); + let op = Delete { uuid, old_task }; let json = serde_json::to_string(&op)?; - assert_eq!(json, format!(r#"{{"Delete":{{"uuid":"{}"}}}}"#, uuid)); + assert_eq!( + json, + format!( + r#"{{"Delete":{{"uuid":"{}","old_task":{{"foo":"bar"}}}}}}"#, + uuid + ) + ); let deser: ReplicaOp = serde_json::from_str(&json)?; assert_eq!(deser, op); Ok(()) @@ -86,6 +98,7 @@ mod test { let op = Update { uuid, property: "abc".into(), + old_value: Some("true".into()), value: Some("false".into()), timestamp, }; @@ -94,7 +107,7 @@ mod test { assert_eq!( json, format!( - r#"{{"Update":{{"uuid":"{}","property":"abc","value":"false","timestamp":"{:?}"}}}}"#, + r#"{{"Update":{{"uuid":"{}","property":"abc","old_value":"true","value":"false","timestamp":"{:?}"}}}}"#, uuid, timestamp, ) ); @@ -111,6 +124,7 @@ mod test { let op = Update { uuid, property: "abc".into(), + old_value: None, value: None, timestamp, }; @@ -119,7 +133,7 @@ mod test { assert_eq!( json, format!( - r#"{{"Update":{{"uuid":"{}","property":"abc","value":null,"timestamp":"{:?}"}}}}"#, + r#"{{"Update":{{"uuid":"{}","property":"abc","old_value":null,"value":null,"timestamp":"{:?}"}}}}"#, uuid, timestamp, ) ); @@ -137,7 +151,14 @@ mod test { #[test] fn test_into_sync_delete() { let uuid = Uuid::new_v4(); - assert_eq!(Delete { uuid }.into_sync(), SyncOp::Delete { uuid }); + assert_eq!( + Delete { + uuid, + old_task: TaskMap::new() + } + .into_sync(), + SyncOp::Delete { uuid } + ); } #[test] @@ -148,6 +169,7 @@ mod test { Update { uuid, property: "prop".into(), + old_value: Some("foo".into()), value: Some("v".into()), timestamp, } diff --git a/taskchampion/src/storage/sqlite.rs b/taskchampion/src/storage/sqlite.rs index bf5620af5..1f1fe239a 100644 --- a/taskchampion/src/storage/sqlite.rs +++ b/taskchampion/src/storage/sqlite.rs @@ -633,8 +633,14 @@ mod test { { let mut txn = storage.txn()?; txn.set_operations(vec![ - ReplicaOp::Delete { uuid: uuid2 }, - ReplicaOp::Delete { uuid: uuid1 }, + ReplicaOp::Delete { + uuid: uuid2, + old_task: TaskMap::new(), + }, + ReplicaOp::Delete { + uuid: uuid1, + old_task: TaskMap::new(), + }, ])?; txn.commit()?; } @@ -643,7 +649,10 @@ mod test { { let mut txn = storage.txn()?; txn.add_operation(ReplicaOp::Create { uuid: uuid3 })?; - txn.add_operation(ReplicaOp::Delete { uuid: uuid3 })?; + txn.add_operation(ReplicaOp::Delete { + uuid: uuid3, + old_task: TaskMap::new(), + })?; txn.commit()?; } @@ -654,10 +663,19 @@ mod test { assert_eq!( ops, vec![ - ReplicaOp::Delete { uuid: uuid2 }, - ReplicaOp::Delete { uuid: uuid1 }, + ReplicaOp::Delete { + uuid: uuid2, + old_task: TaskMap::new() + }, + ReplicaOp::Delete { + uuid: uuid1, + old_task: TaskMap::new() + }, ReplicaOp::Create { uuid: uuid3 }, - ReplicaOp::Delete { uuid: uuid3 }, + ReplicaOp::Delete { + uuid: uuid3, + old_task: TaskMap::new() + }, ] ); } diff --git a/taskchampion/src/taskdb/apply.rs b/taskchampion/src/taskdb/apply.rs index 2824586c4..738a0550b 100644 --- a/taskchampion/src/taskdb/apply.rs +++ b/taskchampion/src/taskdb/apply.rs @@ -20,10 +20,12 @@ pub(super) fn apply(txn: &mut dyn StorageTxn, op: SyncOp) -> anyhow::Result { let task = txn.get_task(uuid)?; - // (we'll need _task in the next commit) - if let Some(_task) = task { + if let Some(task) = task { txn.delete_task(uuid)?; - txn.add_operation(ReplicaOp::Delete { uuid })?; + txn.add_operation(ReplicaOp::Delete { + uuid, + old_task: task, + })?; txn.commit()?; Ok(TaskMap::new()) } else { @@ -38,6 +40,7 @@ pub(super) fn apply(txn: &mut dyn StorageTxn, op: SyncOp) -> anyhow::Result { let task = txn.get_task(uuid)?; if let Some(mut task) = task { + let old_value = task.get(&property).cloned(); if let Some(ref v) = value { task.insert(property.clone(), v.clone()); } else { @@ -47,6 +50,7 @@ pub(super) fn apply(txn: &mut dyn StorageTxn, op: SyncOp) -> anyhow::Result anyhow::Result<()> { let mut db = TaskDb::new_inmemory(); let uuid = Uuid::new_v4(); - let op1 = SyncOp::Create { uuid }; - let op2 = SyncOp::Delete { uuid }; + let now = Utc::now(); + let op1 = SyncOp::Create { uuid }; { let mut txn = db.storage.txn()?; let taskmap = apply(txn.as_mut(), op1)?; assert_eq!(taskmap.len(), 0); + } + + let op2 = SyncOp::Update { + uuid, + property: String::from("priority"), + value: Some("H".into()), + timestamp: now, + }; + { + let mut txn = db.storage.txn()?; let taskmap = apply(txn.as_mut(), op2)?; + assert_eq!(taskmap.get("priority"), Some(&"H".to_owned())); + txn.commit()?; + } + + let op3 = SyncOp::Delete { uuid }; + { + let mut txn = db.storage.txn()?; + let taskmap = apply(txn.as_mut(), op3)?; assert_eq!(taskmap.len(), 0); txn.commit()?; } assert_eq!(db.sorted_tasks(), vec![]); + let mut old_task = TaskMap::new(); + old_task.insert("priority".into(), "H".into()); assert_eq!( db.operations(), - vec![ReplicaOp::Create { uuid }, ReplicaOp::Delete { uuid },] + vec![ + ReplicaOp::Create { uuid }, + ReplicaOp::Update { + uuid, + property: "priority".into(), + old_value: None, + value: Some("H".into()), + timestamp: now, + }, + ReplicaOp::Delete { uuid, old_task }, + ] ); Ok(()) From 1647ba9144e66ed29f3b1116965cb5e20b3c04f3 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sun, 19 Dec 2021 21:37:37 +0000 Subject: [PATCH 08/14] insert UndoPoint appropriately into the replica operations --- taskchampion/src/replica.rs | 105 +++++++++++++++++++++++++++++++- taskchampion/src/storage/op.rs | 30 ++++++--- taskchampion/src/taskdb/mod.rs | 19 ++++-- taskchampion/src/taskdb/sync.rs | 2 +- 4 files changed, 141 insertions(+), 15 deletions(-) diff --git a/taskchampion/src/replica.rs b/taskchampion/src/replica.rs index ac39091c8..57b1cfa56 100644 --- a/taskchampion/src/replica.rs +++ b/taskchampion/src/replica.rs @@ -28,12 +28,14 @@ use uuid::Uuid; /// during the garbage-collection process. pub struct Replica { taskdb: TaskDb, + added_undo_point: bool, } impl Replica { pub fn new(storage: Box) -> Replica { Replica { taskdb: TaskDb::new(storage), + added_undo_point: false, } } @@ -55,6 +57,7 @@ impl Replica { S1: Into, S2: Into, { + self.add_undo_point(false)?; self.taskdb.apply(SyncOp::Update { uuid, property: property.into(), @@ -98,6 +101,7 @@ impl Replica { /// Create a new task. The task must not already exist. pub fn new_task(&mut self, status: Status, description: String) -> anyhow::Result { + self.add_undo_point(false)?; let uuid = Uuid::new_v4(); let taskmap = self.taskdb.apply(SyncOp::Create { uuid })?; trace!("task {} created", uuid); @@ -112,6 +116,7 @@ impl Replica { /// should only occur through expiration. #[allow(dead_code)] fn delete_task(&mut self, uuid: Uuid) -> anyhow::Result<()> { + self.add_undo_point(false)?; self.taskdb.apply(SyncOp::Delete { uuid })?; trace!("task {} deleted", uuid); Ok(()) @@ -150,11 +155,24 @@ impl Replica { .rebuild_working_set(|t| t.get("status") == Some(&pending), renumber)?; Ok(()) } + + /// Add an UndoPoint, if one has not already been added by this Replica. This occurs + /// automatically when a change is made. The `force` flag allows forcing a new UndoPoint + /// even if one has laready been created by this Replica, and may be useful when a Replica + /// instance is held for a long time and used to apply more than one user-visible change. + pub fn add_undo_point(&mut self, force: bool) -> anyhow::Result<()> { + if force || !self.added_undo_point { + self.taskdb.add_undo_point()?; + self.added_undo_point = true; + } + Ok(()) + } } #[cfg(test)] mod tests { use super::*; + use crate::storage::ReplicaOp; use crate::task::Status; use pretty_assertions::assert_eq; use uuid::Uuid; @@ -187,10 +205,95 @@ mod tests { assert_eq!(t.get_description(), "past tense"); assert_eq!(t.get_status(), Status::Completed); - // check tha values have changed in storage, too + // check that values have changed in storage, too let t = rep.get_task(t.get_uuid()).unwrap().unwrap(); assert_eq!(t.get_description(), "past tense"); assert_eq!(t.get_status(), Status::Completed); + + // and check for the corresponding operations, cleaning out the timestamps + // and modified properties as these are based on the current time + let now = Utc::now(); + let clean_op = |op: ReplicaOp| { + if let ReplicaOp::Update { + uuid, + property, + mut old_value, + mut value, + .. + } = op + { + if property == "modified" { + if value.is_some() { + value = Some("just-now".into()); + } + if old_value.is_some() { + old_value = Some("just-now".into()); + } + } + ReplicaOp::Update { + uuid, + property, + old_value, + value, + timestamp: now, + } + } else { + op + } + }; + assert_eq!( + rep.taskdb + .operations() + .drain(..) + .map(clean_op) + .collect::>(), + vec![ + ReplicaOp::UndoPoint, + ReplicaOp::Create { uuid: t.get_uuid() }, + ReplicaOp::Update { + uuid: t.get_uuid(), + property: "modified".into(), + old_value: None, + value: Some("just-now".into()), + timestamp: now, + }, + ReplicaOp::Update { + uuid: t.get_uuid(), + property: "description".into(), + old_value: None, + value: Some("a task".into()), + timestamp: now, + }, + ReplicaOp::Update { + uuid: t.get_uuid(), + property: "status".into(), + old_value: None, + value: Some("P".into()), + timestamp: now, + }, + ReplicaOp::Update { + uuid: t.get_uuid(), + property: "modified".into(), + old_value: Some("just-now".into()), + value: Some("just-now".into()), + timestamp: now, + }, + ReplicaOp::Update { + uuid: t.get_uuid(), + property: "description".into(), + old_value: Some("a task".into()), + value: Some("past tense".into()), + timestamp: now, + }, + ReplicaOp::Update { + uuid: t.get_uuid(), + property: "status".into(), + old_value: Some("P".into()), + value: Some("C".into()), + timestamp: now, + }, + ] + ); } #[test] diff --git a/taskchampion/src/storage/op.rs b/taskchampion/src/storage/op.rs index a31366ddc..283c876d9 100644 --- a/taskchampion/src/storage/op.rs +++ b/taskchampion/src/storage/op.rs @@ -29,26 +29,33 @@ pub enum ReplicaOp { value: Option, timestamp: DateTime, }, + + /// Mark a point in the operations history to which the user might like to undo. Users + /// typically want to undo more than one operation at a time (for example, most changes update + /// both the `modified` property and some other task property -- the user would like to "undo" + /// both updates at the same time). Applying an UndoPoint does nothing. + UndoPoint, } impl ReplicaOp { /// Convert this operation into a [`SyncOp`]. - pub fn into_sync(self) -> SyncOp { + pub fn into_sync(self) -> Option { match self { - Self::Create { uuid } => SyncOp::Create { uuid }, - Self::Delete { uuid, .. } => SyncOp::Delete { uuid }, + Self::Create { uuid } => Some(SyncOp::Create { uuid }), + Self::Delete { uuid, .. } => Some(SyncOp::Delete { uuid }), Self::Update { uuid, property, value, timestamp, .. - } => SyncOp::Update { + } => Some(SyncOp::Update { uuid, property, value, timestamp, - }, + }), + Self::UndoPoint => None, } } } @@ -145,7 +152,7 @@ mod test { #[test] fn test_into_sync_create() { let uuid = Uuid::new_v4(); - assert_eq!(Create { uuid }.into_sync(), SyncOp::Create { uuid }); + assert_eq!(Create { uuid }.into_sync(), Some(SyncOp::Create { uuid })); } #[test] @@ -157,7 +164,7 @@ mod test { old_task: TaskMap::new() } .into_sync(), - SyncOp::Delete { uuid } + Some(SyncOp::Delete { uuid }) ); } @@ -174,12 +181,17 @@ mod test { timestamp, } .into_sync(), - SyncOp::Update { + Some(SyncOp::Update { uuid, property: "prop".into(), value: Some("v".into()), timestamp, - } + }) ); } + + #[test] + fn test_into_sync_undo_point() { + assert_eq!(UndoPoint.into_sync(), None); + } } diff --git a/taskchampion/src/taskdb/mod.rs b/taskchampion/src/taskdb/mod.rs index 69c0a91e3..4861746e5 100644 --- a/taskchampion/src/taskdb/mod.rs +++ b/taskchampion/src/taskdb/mod.rs @@ -1,10 +1,7 @@ use crate::server::{Server, SyncOp}; -use crate::storage::{Storage, TaskMap}; +use crate::storage::{ReplicaOp, Storage, TaskMap}; use uuid::Uuid; -#[cfg(test)] -use crate::storage::ReplicaOp; - mod apply; mod snapshot; mod sync; @@ -43,6 +40,13 @@ impl TaskDb { apply::apply(txn.as_mut(), op) } + /// Add an UndoPoint operation to the list of replica operations. + pub fn add_undo_point(&mut self) -> anyhow::Result<()> { + let mut txn = self.storage.txn()?; + txn.add_operation(ReplicaOp::UndoPoint)?; + txn.commit() + } + /// Get all tasks. pub fn all_tasks(&mut self) -> anyhow::Result> { let mut txn = self.storage.txn()?; @@ -171,6 +175,13 @@ mod tests { assert_eq!(db.operations(), vec![ReplicaOp::Create { uuid }]); } + #[test] + fn test_add_undo_point() { + let mut db = TaskDb::new_inmemory(); + db.add_undo_point().unwrap(); + assert_eq!(db.operations(), vec![ReplicaOp::UndoPoint]); + } + fn newdb() -> TaskDb { TaskDb::new(Box::new(InMemoryStorage::new())) } diff --git a/taskchampion/src/taskdb/sync.rs b/taskchampion/src/taskdb/sync.rs index b6c92f093..6ea11cc6f 100644 --- a/taskchampion/src/taskdb/sync.rs +++ b/taskchampion/src/taskdb/sync.rs @@ -75,7 +75,7 @@ pub(super) fn sync( let mut local_ops: Vec = txn .operations()? .drain(..) - .map(|op| op.into_sync()) + .filter_map(|op| op.into_sync()) .collect(); // first pull changes and "rebase" on top of them From 9d93928996b01280da4366baec822683956f84ef Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Tue, 21 Dec 2021 00:43:26 +0000 Subject: [PATCH 09/14] support undo operations --- taskchampion/src/replica.rs | 6 ++ taskchampion/src/storage/op.rs | 86 +++++++++++++++++++++++ taskchampion/src/taskdb/apply.rs | 78 +++++++++++++++++---- taskchampion/src/taskdb/mod.rs | 10 ++- taskchampion/src/taskdb/sync.rs | 42 +----------- taskchampion/src/taskdb/undo.rs | 114 +++++++++++++++++++++++++++++++ 6 files changed, 280 insertions(+), 56 deletions(-) create mode 100644 taskchampion/src/taskdb/undo.rs diff --git a/taskchampion/src/replica.rs b/taskchampion/src/replica.rs index 57b1cfa56..271a39436 100644 --- a/taskchampion/src/replica.rs +++ b/taskchampion/src/replica.rs @@ -145,6 +145,12 @@ impl Replica { Ok(()) } + /// Undo local operations until the most recent UndoPoint, returning false if there are no + /// local operations to undo. + pub fn undo(&mut self) -> anyhow::Result { + self.taskdb.undo() + } + /// Rebuild this replica's working set, based on whether tasks are pending or not. If /// `renumber` is true, then existing tasks may be moved to new working-set indices; in any /// case, on completion all pending tasks are in the working set and all non- pending tasks are diff --git a/taskchampion/src/storage/op.rs b/taskchampion/src/storage/op.rs index 283c876d9..bc20d99e9 100644 --- a/taskchampion/src/storage/op.rs +++ b/taskchampion/src/storage/op.rs @@ -58,11 +58,47 @@ impl ReplicaOp { Self::UndoPoint => None, } } + + /// Generate a sequence of SyncOp's to reverse the effects of this ReplicaOp. + pub fn reverse_ops(self) -> Vec { + match self { + Self::Create { uuid } => vec![SyncOp::Delete { uuid }], + Self::Delete { uuid, mut old_task } => { + let mut ops = vec![SyncOp::Create { uuid }]; + // We don't have the original update timestamp, but it doesn't + // matter because this SyncOp will just be applied and discarded. + let timestamp = Utc::now(); + for (property, value) in old_task.drain() { + ops.push(SyncOp::Update { + uuid, + property, + value: Some(value), + timestamp, + }); + } + ops + } + Self::Update { + uuid, + property, + old_value, + timestamp, + .. + } => vec![SyncOp::Update { + uuid, + property, + value: old_value, + timestamp, + }], + Self::UndoPoint => vec![], + } + } } #[cfg(test)] mod test { use super::*; + use crate::storage::taskmap_with; use chrono::Utc; use pretty_assertions::assert_eq; @@ -194,4 +230,54 @@ mod test { fn test_into_sync_undo_point() { assert_eq!(UndoPoint.into_sync(), None); } + + #[test] + fn test_reverse_create() { + let uuid = Uuid::new_v4(); + assert_eq!(Create { uuid }.reverse_ops(), vec![SyncOp::Delete { uuid }]); + } + + #[test] + fn test_reverse_delete() { + let uuid = Uuid::new_v4(); + let reversed = Delete { + uuid, + old_task: taskmap_with(vec![("prop1".into(), "v1".into())]), + } + .reverse_ops(); + assert_eq!(reversed.len(), 2); + assert_eq!(reversed[0], SyncOp::Create { uuid }); + assert!(matches!( + &reversed[1], + SyncOp::Update { uuid: u, property: p, value: Some(v), ..} + if u == &uuid && p == "prop1" && v == "v1" + )); + } + + #[test] + fn test_reverse_update() { + let uuid = Uuid::new_v4(); + let timestamp = Utc::now(); + assert_eq!( + Update { + uuid, + property: "prop".into(), + old_value: Some("foo".into()), + value: Some("v".into()), + timestamp, + } + .reverse_ops(), + vec![SyncOp::Update { + uuid, + property: "prop".into(), + value: Some("foo".into()), + timestamp, + }] + ); + } + + #[test] + fn test_reverse_undo_point() { + assert_eq!(UndoPoint.reverse_ops(), vec![]); + } } diff --git a/taskchampion/src/taskdb/apply.rs b/taskchampion/src/taskdb/apply.rs index 738a0550b..1be3864c9 100644 --- a/taskchampion/src/taskdb/apply.rs +++ b/taskchampion/src/taskdb/apply.rs @@ -5,7 +5,7 @@ use crate::storage::{ReplicaOp, StorageTxn, TaskMap}; /// Apply the given SyncOp to the replica, updating both the task data and adding a /// ReplicaOp to the list of operations. Returns the TaskMap of the task after the /// operation has been applied (or an empty TaskMap for Delete). -pub(super) fn apply(txn: &mut dyn StorageTxn, op: SyncOp) -> anyhow::Result { +pub(super) fn apply_and_record(txn: &mut dyn StorageTxn, op: SyncOp) -> anyhow::Result { match op { SyncOp::Create { uuid } => { let created = txn.create_task(uuid)?; @@ -63,6 +63,45 @@ pub(super) fn apply(txn: &mut dyn StorageTxn, op: SyncOp) -> anyhow::Result anyhow::Result<()> { + // TODO: test + // TODO: it'd be nice if this was integrated into apply() somehow, but that clones TaskMaps + // unnecessariliy + match op { + SyncOp::Create { uuid } => { + // insert if the task does not already exist + if !txn.create_task(*uuid)? { + return Err(Error::Database(format!("Task {} already exists", uuid)).into()); + } + } + SyncOp::Delete { ref uuid } => { + if !txn.delete_task(*uuid)? { + return Err(Error::Database(format!("Task {} does not exist", uuid)).into()); + } + } + SyncOp::Update { + ref uuid, + ref property, + ref value, + timestamp: _, + } => { + // update if this task exists, otherwise ignore + if let Some(mut task) = txn.get_task(*uuid)? { + match value { + Some(ref val) => task.insert(property.to_string(), val.clone()), + None => task.remove(property), + }; + txn.set_task(*uuid, task)?; + } else { + return Err(Error::Database(format!("Task {} does not exist", uuid)).into()); + } + } + } + + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -80,7 +119,7 @@ mod tests { { let mut txn = db.storage.txn()?; - let taskmap = apply(txn.as_mut(), op)?; + let taskmap = apply_and_record(txn.as_mut(), op)?; assert_eq!(taskmap.len(), 0); txn.commit()?; } @@ -97,10 +136,13 @@ mod tests { let op = SyncOp::Create { uuid }; { let mut txn = db.storage.txn()?; - let taskmap = apply(txn.as_mut(), op.clone())?; + let taskmap = apply_and_record(txn.as_mut(), op.clone())?; assert_eq!(taskmap.len(), 0); assert_eq!( - apply(txn.as_mut(), op).err().unwrap().to_string(), + apply_and_record(txn.as_mut(), op) + .err() + .unwrap() + .to_string(), format!("Task Database Error: Task {} already exists", uuid) ); txn.commit()?; @@ -121,7 +163,7 @@ mod tests { { let mut txn = db.storage.txn()?; - let taskmap = apply(txn.as_mut(), op1)?; + let taskmap = apply_and_record(txn.as_mut(), op1)?; assert_eq!(taskmap.len(), 0); txn.commit()?; } @@ -134,7 +176,7 @@ mod tests { }; { let mut txn = db.storage.txn()?; - let mut taskmap = apply(txn.as_mut(), op2)?; + let mut taskmap = apply_and_record(txn.as_mut(), op2)?; assert_eq!( taskmap.drain().collect::>(), vec![("title".into(), "my task".into())] @@ -171,7 +213,7 @@ mod tests { let op1 = SyncOp::Create { uuid }; { let mut txn = db.storage.txn()?; - let taskmap = apply(txn.as_mut(), op1)?; + let taskmap = apply_and_record(txn.as_mut(), op1)?; assert_eq!(taskmap.len(), 0); txn.commit()?; } @@ -184,7 +226,7 @@ mod tests { }; { let mut txn = db.storage.txn()?; - let taskmap = apply(txn.as_mut(), op2)?; + let taskmap = apply_and_record(txn.as_mut(), op2)?; assert_eq!(taskmap.get("title"), Some(&"my task".to_owned())); txn.commit()?; } @@ -197,7 +239,7 @@ mod tests { }; { let mut txn = db.storage.txn()?; - let taskmap = apply(txn.as_mut(), op3)?; + let taskmap = apply_and_record(txn.as_mut(), op3)?; assert_eq!(taskmap.get("priority"), Some(&"H".to_owned())); txn.commit()?; } @@ -210,7 +252,7 @@ mod tests { }; { let mut txn = db.storage.txn()?; - let taskmap = apply(txn.as_mut(), op4)?; + let taskmap = apply_and_record(txn.as_mut(), op4)?; assert_eq!(taskmap.get("title"), None); assert_eq!(taskmap.get("priority"), Some(&"H".to_owned())); txn.commit()?; @@ -268,7 +310,10 @@ mod tests { { let mut txn = db.storage.txn()?; assert_eq!( - apply(txn.as_mut(), op).err().unwrap().to_string(), + apply_and_record(txn.as_mut(), op) + .err() + .unwrap() + .to_string(), format!("Task Database Error: Task {} does not exist", uuid) ); txn.commit()?; @@ -286,7 +331,7 @@ mod tests { let op1 = SyncOp::Create { uuid }; { let mut txn = db.storage.txn()?; - let taskmap = apply(txn.as_mut(), op1)?; + let taskmap = apply_and_record(txn.as_mut(), op1)?; assert_eq!(taskmap.len(), 0); } @@ -298,7 +343,7 @@ mod tests { }; { let mut txn = db.storage.txn()?; - let taskmap = apply(txn.as_mut(), op2)?; + let taskmap = apply_and_record(txn.as_mut(), op2)?; assert_eq!(taskmap.get("priority"), Some(&"H".to_owned())); txn.commit()?; } @@ -306,7 +351,7 @@ mod tests { let op3 = SyncOp::Delete { uuid }; { let mut txn = db.storage.txn()?; - let taskmap = apply(txn.as_mut(), op3)?; + let taskmap = apply_and_record(txn.as_mut(), op3)?; assert_eq!(taskmap.len(), 0); txn.commit()?; } @@ -340,7 +385,10 @@ mod tests { { let mut txn = db.storage.txn()?; assert_eq!( - apply(txn.as_mut(), op).err().unwrap().to_string(), + apply_and_record(txn.as_mut(), op) + .err() + .unwrap() + .to_string(), format!("Task Database Error: Task {} does not exist", uuid) ); txn.commit()?; diff --git a/taskchampion/src/taskdb/mod.rs b/taskchampion/src/taskdb/mod.rs index 4861746e5..7e802313f 100644 --- a/taskchampion/src/taskdb/mod.rs +++ b/taskchampion/src/taskdb/mod.rs @@ -5,6 +5,7 @@ use uuid::Uuid; mod apply; mod snapshot; mod sync; +mod undo; mod working_set; /// A TaskDb is the backend for a replica. It manages the storage, operations, synchronization, @@ -37,7 +38,7 @@ impl TaskDb { /// (but leave the TaskDb in a consistent state). pub fn apply(&mut self, op: SyncOp) -> anyhow::Result { let mut txn = self.storage.txn()?; - apply::apply(txn.as_mut(), op) + apply::apply_and_record(txn.as_mut(), op) } /// Add an UndoPoint operation to the list of replica operations. @@ -120,6 +121,13 @@ impl TaskDb { sync::sync(server, txn.as_mut(), avoid_snapshots) } + /// Undo local operations until the most recent UndoPoint, returning false if there are no + /// local operations to undo. + pub fn undo(&mut self) -> anyhow::Result { + let mut txn = self.storage.txn()?; + undo::undo(txn.as_mut()) + } + // functions for supporting tests #[cfg(test)] diff --git a/taskchampion/src/taskdb/sync.rs b/taskchampion/src/taskdb/sync.rs index 6ea11cc6f..d479c6c03 100644 --- a/taskchampion/src/taskdb/sync.rs +++ b/taskchampion/src/taskdb/sync.rs @@ -1,4 +1,4 @@ -use super::snapshot; +use super::{apply, snapshot}; use crate::server::{AddVersionResult, GetVersionResult, Server, SnapshotUrgency, SyncOp}; use crate::storage::StorageTxn; use crate::Error; @@ -11,44 +11,6 @@ struct Version { operations: Vec, } -/// Apply an op to the TaskDb's set of tasks (without recording it in the list of operations) -pub(super) fn apply_op(txn: &mut dyn StorageTxn, op: &SyncOp) -> anyhow::Result<()> { - // TODO: it'd be nice if this was integrated into apply() somehow, but that clones TaskMaps - // unnecessariliy - match op { - SyncOp::Create { uuid } => { - // insert if the task does not already exist - if !txn.create_task(*uuid)? { - return Err(Error::Database(format!("Task {} already exists", uuid)).into()); - } - } - SyncOp::Delete { ref uuid } => { - if !txn.delete_task(*uuid)? { - return Err(Error::Database(format!("Task {} does not exist", uuid)).into()); - } - } - SyncOp::Update { - ref uuid, - ref property, - ref value, - timestamp: _, - } => { - // update if this task exists, otherwise ignore - if let Some(mut task) = txn.get_task(*uuid)? { - match value { - Some(ref val) => task.insert(property.to_string(), val.clone()), - None => task.remove(property), - }; - txn.set_task(*uuid, task)?; - } else { - return Err(Error::Database(format!("Task {} does not exist", uuid)).into()); - } - } - } - - Ok(()) -} - /// Sync to the given server, pulling remote changes and pushing local changes. pub(super) fn sync( server: &mut Box, @@ -209,7 +171,7 @@ fn apply_version( } } if let Some(o) = svr_op { - if let Err(e) = apply_op(txn, &o) { + if let Err(e) = apply::apply_op(txn, &o) { warn!("Invalid operation when syncing: {} (ignored)", e); } } diff --git a/taskchampion/src/taskdb/undo.rs b/taskchampion/src/taskdb/undo.rs new file mode 100644 index 000000000..f7636ebde --- /dev/null +++ b/taskchampion/src/taskdb/undo.rs @@ -0,0 +1,114 @@ +use super::apply; +use crate::storage::{ReplicaOp, StorageTxn}; + +/// Undo local operations until an UndoPoint. +pub(super) fn undo(txn: &mut dyn StorageTxn) -> anyhow::Result { + let mut applied = false; + let mut popped = false; + let mut local_ops = txn.operations()?; + + while let Some(op) = local_ops.pop() { + popped = true; + if op == ReplicaOp::UndoPoint { + break; + } + let rev_ops = op.reverse_ops(); + for op in rev_ops { + apply::apply_op(txn, &op)?; + applied = true; + } + } + + if popped { + txn.set_operations(local_ops)?; + txn.commit()?; + } + + Ok(applied) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::server::SyncOp; + use crate::taskdb::TaskDb; + use chrono::Utc; + use pretty_assertions::assert_eq; + use uuid::Uuid; + + #[test] + fn test_apply_create() -> anyhow::Result<()> { + let mut db = TaskDb::new_inmemory(); + let uuid1 = Uuid::new_v4(); + let uuid2 = Uuid::new_v4(); + let timestamp = Utc::now(); + + // apply a few ops, capture the DB state, make an undo point, and then apply a few more + // ops. + db.apply(SyncOp::Create { uuid: uuid1 })?; + db.apply(SyncOp::Update { + uuid: uuid1, + property: "prop".into(), + value: Some("v1".into()), + timestamp, + })?; + db.apply(SyncOp::Create { uuid: uuid2 })?; + db.apply(SyncOp::Update { + uuid: uuid2, + property: "prop".into(), + value: Some("v2".into()), + timestamp, + })?; + db.apply(SyncOp::Update { + uuid: uuid2, + property: "prop2".into(), + value: Some("v3".into()), + timestamp, + })?; + + let db_state = db.sorted_tasks(); + + db.add_undo_point()?; + db.apply(SyncOp::Delete { uuid: uuid1 })?; + db.apply(SyncOp::Update { + uuid: uuid2, + property: "prop".into(), + value: None, + timestamp, + })?; + db.apply(SyncOp::Update { + uuid: uuid2, + property: "prop2".into(), + value: Some("new-value".into()), + timestamp, + })?; + + assert_eq!(db.operations().len(), 9); + + { + let mut txn = db.storage.txn()?; + assert!(undo(txn.as_mut())?); + } + + // undo took db back to the snapshot + assert_eq!(db.operations().len(), 5); + assert_eq!(db.sorted_tasks(), db_state); + + { + let mut txn = db.storage.txn()?; + assert!(undo(txn.as_mut())?); + } + + // empty db + assert_eq!(db.operations().len(), 0); + assert_eq!(db.sorted_tasks(), vec![]); + + { + let mut txn = db.storage.txn()?; + // nothing left to undo, so undo() returns false + assert!(!undo(txn.as_mut())?); + } + + Ok(()) + } +} From caa62ba9a0c0fe3f60ff1f896315475c7fb19df5 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Tue, 21 Dec 2021 01:05:52 +0000 Subject: [PATCH 10/14] add a 'ta undo' subcommand --- cli/src/argparse/subcommand.rs | 34 ++++++++++++++++++++++++++++++++++ cli/src/invocation/cmd/mod.rs | 1 + cli/src/invocation/cmd/undo.rs | 28 ++++++++++++++++++++++++++++ cli/src/invocation/mod.rs | 7 +++++++ 4 files changed, 70 insertions(+) create mode 100644 cli/src/invocation/cmd/undo.rs diff --git a/cli/src/argparse/subcommand.rs b/cli/src/argparse/subcommand.rs index d06244bec..06cc52490 100644 --- a/cli/src/argparse/subcommand.rs +++ b/cli/src/argparse/subcommand.rs @@ -59,6 +59,7 @@ pub(crate) enum Subcommand { /// Basic operations without args Gc, Sync, + Undo, } impl Subcommand { @@ -72,6 +73,7 @@ impl Subcommand { Info::parse, Gc::parse, Sync::parse, + Undo::parse, // This must come last since it accepts arbitrary report names Report::parse, )))(input) @@ -422,6 +424,29 @@ impl Sync { } } +struct Undo; + +impl Undo { + fn parse(input: ArgList) -> IResult { + fn to_subcommand(_: &str) -> Result { + Ok(Subcommand::Undo) + } + map_res(arg_matching(literal("undo")), to_subcommand)(input) + } + + fn get_usage(u: &mut usage::Usage) { + u.subcommands.push(usage::Subcommand { + name: "undo", + syntax: "undo", + summary: "Undo the latest change made on this replica", + description: " + Undo the latest change made on this replica. + + Changes cannot be undone once they have been synchronized.", + }) + } +} + #[cfg(test)] mod test { use super::*; @@ -814,4 +839,13 @@ mod test { (&EMPTY[..], subcommand) ); } + + #[test] + fn test_undo() { + let subcommand = Subcommand::Undo; + assert_eq!( + Subcommand::parse(argv!["undo"]).unwrap(), + (&EMPTY[..], subcommand) + ); + } } diff --git a/cli/src/invocation/cmd/mod.rs b/cli/src/invocation/cmd/mod.rs index 9371f1f2c..e7606ac90 100644 --- a/cli/src/invocation/cmd/mod.rs +++ b/cli/src/invocation/cmd/mod.rs @@ -8,4 +8,5 @@ pub(crate) mod info; pub(crate) mod modify; pub(crate) mod report; pub(crate) mod sync; +pub(crate) mod undo; pub(crate) mod version; diff --git a/cli/src/invocation/cmd/undo.rs b/cli/src/invocation/cmd/undo.rs new file mode 100644 index 000000000..d3f688e7a --- /dev/null +++ b/cli/src/invocation/cmd/undo.rs @@ -0,0 +1,28 @@ +use taskchampion::Replica; +use termcolor::WriteColor; + +pub(crate) fn execute(w: &mut W, replica: &mut Replica) -> Result<(), crate::Error> { + if replica.undo()? { + writeln!(w, "Undo successful.")?; + } else { + writeln!(w, "Nothing to undo.")?; + } + Ok(()) +} + +#[cfg(test)] +mod test { + use super::*; + use crate::invocation::test::*; + use pretty_assertions::assert_eq; + + #[test] + fn test_undo() { + let mut w = test_writer(); + let mut replica = test_replica(); + + // Note that the details of the actual undo operation are tested thoroughly in the taskchampion crate + execute(&mut w, &mut replica).unwrap(); + assert_eq!(&w.into_string(), "Nothing to undo.\n") + } +} diff --git a/cli/src/invocation/mod.rs b/cli/src/invocation/mod.rs index 2f225f86a..41a8c4ce2 100644 --- a/cli/src/invocation/mod.rs +++ b/cli/src/invocation/mod.rs @@ -90,6 +90,13 @@ pub(crate) fn invoke(command: Command, settings: Settings) -> Result<(), crate:: return cmd::sync::execute(&mut w, &mut replica, &settings, &mut server); } + Command { + subcommand: Subcommand::Undo, + .. + } => { + return cmd::undo::execute(&mut w, &mut replica); + } + // handled in the first match, but here to ensure this match is exhaustive Command { subcommand: Subcommand::Help { .. }, From e328b86d97906cb2236df7b4703fb2f80f6c50fd Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Tue, 21 Dec 2021 01:10:08 +0000 Subject: [PATCH 11/14] add user docs for 'ta undo' --- docs/src/SUMMARY.md | 1 + docs/src/undo.md | 7 +++++++ 2 files changed, 8 insertions(+) create mode 100644 docs/src/undo.md diff --git a/docs/src/SUMMARY.md b/docs/src/SUMMARY.md index 061dbad5b..942db92d2 100644 --- a/docs/src/SUMMARY.md +++ b/docs/src/SUMMARY.md @@ -10,6 +10,7 @@ * [Dates and Durations](./time.md) * [Configuration](./config-file.md) * [Environment](./environment.md) + * [Undo](./undo.md) * [Synchronization](./task-sync.md) * [Running the Sync Server](./running-sync-server.md) - [Internal Details](./internals.md) diff --git a/docs/src/undo.md b/docs/src/undo.md new file mode 100644 index 000000000..350aebb7e --- /dev/null +++ b/docs/src/undo.md @@ -0,0 +1,7 @@ +# Undo + +It's easy to make a mistake: mark the wrong task as done, or hit enter before noticing a typo in a tag name. +The `ta undo` command makes it just as easy to fix the mistake, but effectively reversing the most recent change. +Multiple invocations of `ta undo` can be used to undo multiple changes. + +The limit of this functionality is that changes which have been synchronized to the server (via `ta sync`) cannot be undone. From 5fb3f700c0adc821699753e23b3289e68a09ae55 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Tue, 21 Dec 2021 01:12:30 +0000 Subject: [PATCH 12/14] add some logging for undo --- taskchampion/src/taskdb/undo.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/taskchampion/src/taskdb/undo.rs b/taskchampion/src/taskdb/undo.rs index f7636ebde..57bba0fa2 100644 --- a/taskchampion/src/taskdb/undo.rs +++ b/taskchampion/src/taskdb/undo.rs @@ -1,5 +1,6 @@ use super::apply; use crate::storage::{ReplicaOp, StorageTxn}; +use log::{debug, trace}; /// Undo local operations until an UndoPoint. pub(super) fn undo(txn: &mut dyn StorageTxn) -> anyhow::Result { @@ -12,8 +13,10 @@ pub(super) fn undo(txn: &mut dyn StorageTxn) -> anyhow::Result { if op == ReplicaOp::UndoPoint { break; } + debug!("Reversing operation {:?}", op); let rev_ops = op.reverse_ops(); for op in rev_ops { + trace!("Applying reversed operation {:?}", op); apply::apply_op(txn, &op)?; applied = true; } From 36c51d2d937f79178705fcaab520fec5a12c7b55 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Wed, 22 Dec 2021 00:31:46 +0000 Subject: [PATCH 13/14] fix clippy --- taskchampion/src/task/task.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/taskchampion/src/task/task.rs b/taskchampion/src/task/task.rs index 6cc3c5126..2e3ba8169 100644 --- a/taskchampion/src/task/task.rs +++ b/taskchampion/src/task/task.rs @@ -261,9 +261,9 @@ impl<'r> TaskMut<'r> { if !self.updated_modified { let now = format!("{}", Utc::now().timestamp()); trace!("task {}: set property modified={:?}", self.task.uuid, now); - self.task.taskmap = - self.replica - .update_task(self.task.uuid, "modified", Some(now.clone()))?; + self.task.taskmap = self + .replica + .update_task(self.task.uuid, "modified", Some(now))?; self.updated_modified = true; } Ok(()) From 8195b187c437b82cccbcb9cf92f3cabc5705db83 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Thu, 23 Dec 2021 09:06:19 -0500 Subject: [PATCH 14/14] fix docs typo --- docs/src/undo.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/src/undo.md b/docs/src/undo.md index 350aebb7e..3d6702240 100644 --- a/docs/src/undo.md +++ b/docs/src/undo.md @@ -1,7 +1,7 @@ # Undo It's easy to make a mistake: mark the wrong task as done, or hit enter before noticing a typo in a tag name. -The `ta undo` command makes it just as easy to fix the mistake, but effectively reversing the most recent change. +The `ta undo` command makes it just as easy to fix the mistake, by effectively reversing the most recent change. Multiple invocations of `ta undo` can be used to undo multiple changes. The limit of this functionality is that changes which have been synchronized to the server (via `ta sync`) cannot be undone.