make taskdb.apply for create/delete not fail if already exists/doesn't exist

taskchampion/src/replica.rs

@@ -99,18 +99,25 @@ impl Replica {
             .map(move |tm| Task::new(uuid, tm)))
     }
 
-    /// Create a new task.  The task must not already exist.
+    /// Create a new task.
     pub fn new_task(&mut self, status: Status, description: String) -> anyhow::Result<Task> {
-        self.add_undo_point(false)?;
         let uuid = Uuid::new_v4();
-        let taskmap = self.taskdb.apply(SyncOp::Create { uuid })?;
+        let mut task = self.create_task(uuid)?.into_mut(self);
-        trace!("task {} created", uuid);
-        let mut task = Task::new(uuid, taskmap).into_mut(self);
         task.set_description(description)?;
         task.set_status(status)?;
+        trace!("task {} created", uuid);
         Ok(task.into_immut())
     }
 
+    /// Create a new, empty task with the given UUID.  This is useful for importing tasks, but
+    /// otherwise should be avoided in favor of `new_task`.  If the task already exists, this
+    /// does nothing and returns the existing task.
+    pub fn create_task(&mut self, uuid: Uuid) -> anyhow::Result<Task> {
+        self.add_undo_point(false)?;
+        let taskmap = self.taskdb.apply(SyncOp::Create { uuid })?;
+        Ok(Task::new(uuid, taskmap))
+    }
+
     /// Delete a task.  The task must exist.  Note that this is different from setting status to
     /// Deleted; this is the final purge of the task.  This is not a public method as deletion
     /// should only occur through expiration.

taskchampion/src/taskdb/apply.rs

@@ -4,7 +4,8 @@ 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).
+/// operation has been applied (or an empty TaskMap for Delete).  It is not an error
+/// to create an existing task, nor to delete a nonexistent task.
 pub(super) fn apply_and_record(txn: &mut dyn StorageTxn, op: SyncOp) -> anyhow::Result<TaskMap> {
     match op {
         SyncOp::Create { uuid } => {
@@ -14,8 +15,9 @@ pub(super) fn apply_and_record(txn: &mut dyn StorageTxn, op: SyncOp) -> anyhow::
                 txn.commit()?;
                 Ok(TaskMap::new())
             } else {
-                // TODO: differentiate error types here?
+                Ok(txn
-                Err(Error::Database(format!("Task {} already exists", uuid)).into())
+                    .get_task(uuid)?
+                    .expect("create_task failed but task does not exist"))
             }
         }
         SyncOp::Delete { uuid } => {
@@ -29,7 +31,7 @@ pub(super) fn apply_and_record(txn: &mut dyn StorageTxn, op: SyncOp) -> anyhow::
                 txn.commit()?;
                 Ok(TaskMap::new())
             } else {
-                Err(Error::Database(format!("Task {} does not exist", uuid)).into())
+                Ok(TaskMap::new())
             }
         }
         SyncOp::Update {
@@ -105,6 +107,7 @@ pub(super) fn apply_op(txn: &mut dyn StorageTxn, op: &SyncOp) -> anyhow::Result<
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::storage::TaskMap;
     use crate::taskdb::TaskDb;
     use chrono::Utc;
     use pretty_assertions::assert_eq;
@@ -133,24 +136,33 @@ mod tests {
     fn test_apply_create_exists() -> anyhow::Result<()> {
         let mut db = TaskDb::new_inmemory();
         let uuid = Uuid::new_v4();
+        {
+            let mut txn = db.storage.txn()?;
+            txn.create_task(uuid)?;
+            let mut taskmap = TaskMap::new();
+            taskmap.insert("foo".into(), "bar".into());
+            txn.set_task(uuid, taskmap)?;
+            txn.commit()?;
+        }
+
         let op = SyncOp::Create { uuid };
         {
             let mut txn = db.storage.txn()?;
             let taskmap = apply_and_record(txn.as_mut(), op.clone())?;
-            assert_eq!(taskmap.len(), 0);
+
-            assert_eq!(
+            assert_eq!(taskmap.len(), 1);
-                apply_and_record(txn.as_mut(), op)
+            assert_eq!(taskmap.get("foo").unwrap(), "bar");
-                    .err()
+
-                    .unwrap()
-                    .to_string(),
-                format!("Task Database Error: Task {} already exists", uuid)
-            );
             txn.commit()?;
         }
 
-        // first op was applied
+        // create did not delete the old task..
-        assert_eq!(db.sorted_tasks(), vec![(uuid, vec![])]);
+        assert_eq!(
-        assert_eq!(db.operations(), vec![ReplicaOp::Create { uuid }]);
+            db.sorted_tasks(),
+            vec![(uuid, vec![("foo".into(), "bar".into())])]
+        );
+        // create was done "manually" above, and no new op was added
+        assert_eq!(db.operations(), vec![]);
         Ok(())
     }
 
@@ -384,13 +396,8 @@ mod tests {
         let op = SyncOp::Delete { uuid };
         {
             let mut txn = db.storage.txn()?;
-            assert_eq!(
+            let taskmap = apply_and_record(txn.as_mut(), op)?;
-                apply_and_record(txn.as_mut(), op)
+            assert_eq!(taskmap.len(), 0);
-                    .err()
-                    .unwrap()
-                    .to_string(),
-                format!("Task Database Error: Task {} does not exist", uuid)
-            );
             txn.commit()?;
         }