Add support for Delete operations

README.md

@@ -25,12 +25,16 @@ See below for details on how tasks can "expire" from the task database.
 Every change to the task database is captured as an operation.
 Each operation has one of the forms 
  * `Create(uuid)`
+ * `Delete(uuid)`
  * `Update(uuid, property, value, timestamp)`
 
-The former form creates a new task.
+The Create form creates a new task.
 It is invalid to create a task that already exists.
 
-The latter form updates the given property of the given task, where property and value are both strings.
+Similarly, the Delete form deletes an existing task.
+It is invalid to delete a task that does not exist.
+
+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.
 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.

src/operation.rs

@@ -5,11 +5,16 @@ use uuid::Uuid;
 /// An Operation defines a single change to the task database
 #[derive(PartialEq, Clone, Debug, Serialize, Deserialize)]
 pub enum Operation {
-    /// Create a new task; if the task already exists in the DB.
+    /// 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.
     ///
@@ -52,9 +57,36 @@ impl Operation {
         operation2: Operation,
     ) -> (Option<Operation>, Option<Operation>) {
         match (&operation1, &operation2) {
-            // Two creations of the same uuid reach the same state, so there's no need for any
+            // Two creations or deletions of the same uuid reach the same state, so there's no need
-            // further operations to bring the state together.
+            // 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.
             (
@@ -103,6 +135,7 @@ mod test {
     // thoroughly, so this testing is light.
 
     fn test_transform(
+        setup: Option<Operation>,
         o1: Operation,
         o2: Operation,
         exp1p: Option<Operation>,
@@ -114,15 +147,23 @@ mod test {
         // check that the two operation sequences have the same effect, enforcing the invariant of
         // the transform function.
         let mut db1 = DB::new();
-        db1.apply(o1);
+        if let Some(ref o) = setup {
+            db1.apply(o.clone()).unwrap();
+        }
+        db1.apply(o1).unwrap();
         if let Some(o) = o2p {
-            db1.apply(o);
+            db1.apply(o).unwrap();
         }
+
         let mut db2 = DB::new();
-        db2.apply(o2);
+        if let Some(ref o) = setup {
-        if let Some(o) = o1p {
+            db2.apply(o.clone()).unwrap();
-            db2.apply(o);
         }
+        db2.apply(o2).unwrap();
+        if let Some(o) = o1p {
+            db2.apply(o).unwrap();
+        }
+
         assert_eq!(db1.tasks(), db2.tasks());
     }
 
@@ -132,6 +173,7 @@ mod test {
         let uuid2 = Uuid::new_v4();
 
         test_transform(
+            None,
             Create { uuid: uuid1 },
             Create { uuid: uuid2 },
             Some(Create { uuid: uuid1 }),
@@ -145,6 +187,7 @@ mod test {
         let timestamp = Utc::now();
 
         test_transform(
+            Some(Create { uuid }),
             Update {
                 uuid,
                 property: "abc".into(),
@@ -179,6 +222,7 @@ mod test {
         let timestamp2 = timestamp1 + Duration::seconds(10);
 
         test_transform(
+            Some(Create { uuid }),
             Update {
                 uuid,
                 property: "abc".into(),
@@ -207,6 +251,7 @@ mod test {
         let timestamp = Utc::now();
 
         test_transform(
+            Some(Create { uuid }),
             Update {
                 uuid,
                 property: "abc".into(),

src/taskdb.rs

@@ -1,3 +1,4 @@
+use crate::errors::Error;
 use crate::operation::Operation;
 use crate::server::{Server, VersionAdd};
 use serde::{Deserialize, Serialize};
@@ -41,15 +42,30 @@ impl DB {
     /// Apply an operation to the DB.  Aside from synchronization operations, this
     /// is the only way to modify the DB.  In cases where an operation does not
     /// make sense, this function will ignore the operation.
-    pub fn apply(&mut self, op: Operation) {
+    pub fn apply(&mut self, op: Operation) -> Result<(), Error> {
+        if let err @ Err(_) = self.apply_op(&op) {
+            return err;
+        }
+        self.operations.push(op);
+        Ok(())
+    }
+
+    fn apply_op(&mut self, op: &Operation) -> Result<(), Error> {
         match op {
-            Operation::Create { uuid } => {
+            &Operation::Create { uuid } => {
                 // insert if the task does not already exist
                 if let ent @ Entry::Vacant(_) = self.tasks.entry(uuid) {
                     ent.or_insert(HashMap::new());
+                } else {
+                    return Err(Error::DBError(format!("Task {} already exists", uuid)));
                 }
             }
-            Operation::Update {
+            &Operation::Delete { ref uuid } => {
+                if let None = self.tasks.remove(uuid) {
+                    return Err(Error::DBError(format!("Task {} does not exist", uuid)));
+                }
+            }
+            &Operation::Update {
                 ref uuid,
                 ref property,
                 ref value,
@@ -57,18 +73,17 @@ impl DB {
             } => {
                 // update if this task exists, otherwise ignore
                 if let Some(task) = self.tasks.get_mut(uuid) {
-                    DB::apply_update(task, property, value);
+                    match value {
+                        Some(ref val) => task.insert(property.to_string(), val.clone()),
+                        None => task.remove(property),
+                    };
+                } else {
+                    return Err(Error::DBError(format!("Task {} does not exist", uuid)));
                 }
             }
-        };
+        }
-        self.operations.push(op);
-    }
 
-    fn apply_update(task: &mut TaskMap, property: &str, value: &Option<String>) {
+        Ok(())
-        match value {
-            Some(ref val) => task.insert(property.to_string(), val.clone()),
-            None => task.remove(property),
-        };
     }
 
     /// Get a read-only reference to the underlying set of tasks.
@@ -152,7 +167,9 @@ impl DB {
                 }
             }
             if let Some(o) = svr_op {
-                self.apply(o);
+                if let Err(e) = self.apply_op(&o) {
+                    println!("Invalid operation when syncing: {} (ignored)", e);
+                }
             }
             self.operations = new_local_ops;
         }
@@ -171,7 +188,7 @@ mod tests {
         let mut db = DB::new();
         let uuid = Uuid::new_v4();
         let op = Operation::Create { uuid };
-        db.apply(op.clone());
+        db.apply(op.clone()).unwrap();
 
         let mut exp = HashMap::new();
         exp.insert(uuid, HashMap::new());
@@ -184,13 +201,16 @@ mod tests {
         let mut db = DB::new();
         let uuid = Uuid::new_v4();
         let op = Operation::Create { uuid };
-        db.apply(op.clone());
+        db.apply(op.clone()).unwrap();
-        db.apply(op.clone());
+        assert_eq!(
+            db.apply(op.clone()).err().unwrap(),
+            Error::DBError(format!("Task {} already exists", uuid))
+        );
 
         let mut exp = HashMap::new();
         exp.insert(uuid, HashMap::new());
         assert_eq!(db.tasks(), &exp);
-        assert_eq!(db.operations, vec![op.clone(), op]);
+        assert_eq!(db.operations, vec![op]);
     }
 
     #[test]
@@ -198,14 +218,14 @@ mod tests {
         let mut db = DB::new();
         let uuid = Uuid::new_v4();
         let op1 = Operation::Create { uuid };
-        db.apply(op1.clone());
+        db.apply(op1.clone()).unwrap();
         let op2 = Operation::Update {
             uuid,
             property: String::from("title"),
             value: Some("my task".into()),
             timestamp: Utc::now(),
         };
-        db.apply(op2.clone());
+        db.apply(op2.clone()).unwrap();
 
         let mut exp = HashMap::new();
         let mut task = HashMap::new();
@@ -220,7 +240,7 @@ mod tests {
         let mut db = DB::new();
         let uuid = Uuid::new_v4();
         let op1 = Operation::Create { uuid };
-        db.apply(op1.clone());
+        db.apply(op1.clone()).unwrap();
 
         let op2 = Operation::Update {
             uuid,
@@ -228,7 +248,7 @@ mod tests {
             value: Some("my task".into()),
             timestamp: Utc::now(),
         };
-        db.apply(op2.clone());
+        db.apply(op2.clone()).unwrap();
 
         let op3 = Operation::Update {
             uuid,
@@ -236,7 +256,7 @@ mod tests {
             value: Some("H".into()),
             timestamp: Utc::now(),
         };
-        db.apply(op3.clone());
+        db.apply(op3.clone()).unwrap();
 
         let op4 = Operation::Update {
             uuid,
@@ -244,7 +264,7 @@ mod tests {
             value: None,
             timestamp: Utc::now(),
         };
-        db.apply(op4.clone());
+        db.apply(op4.clone()).unwrap();
 
         let mut exp = HashMap::new();
         let mut task = HashMap::new();
@@ -264,9 +284,41 @@ mod tests {
             value: Some("my task".into()),
             timestamp: Utc::now(),
         };
-        db.apply(op.clone());
+        assert_eq!(
+            db.apply(op).err().unwrap(),
+            Error::DBError(format!("Task {} does not exist", uuid))
+        );
 
         assert_eq!(db.tasks(), &HashMap::new());
-        assert_eq!(db.operations, vec![op]);
+        assert_eq!(db.operations, vec![]);
+    }
+
+    #[test]
+    fn test_apply_create_delete() {
+        let mut db = DB::new();
+        let uuid = Uuid::new_v4();
+        let op1 = Operation::Create { uuid };
+        db.apply(op1.clone()).unwrap();
+
+        let op2 = Operation::Delete { uuid };
+        db.apply(op2.clone()).unwrap();
+
+        assert_eq!(db.tasks(), &HashMap::new());
+        assert_eq!(db.operations, vec![op1, op2]);
+    }
+
+    #[test]
+    fn test_apply_delete_not_present() {
+        let mut db = DB::new();
+        let uuid = Uuid::new_v4();
+
+        let op1 = Operation::Delete { uuid };
+        assert_eq!(
+            db.apply(op1).err().unwrap(),
+            Error::DBError(format!("Task {} does not exist", uuid))
+        );
+
+        assert_eq!(db.tasks(), &HashMap::new());
+        assert_eq!(db.operations, vec![]);
     }
 }

tests/operation_transform_invariant.rs

@@ -16,6 +16,7 @@ fn uuid_strategy() -> impl Strategy<Value = Uuid> {
 fn operation_strategy() -> impl Strategy<Value = Operation> {
     prop_oneof![
         uuid_strategy().prop_map(|uuid| Operation::Create { uuid }),
+        uuid_strategy().prop_map(|uuid| Operation::Delete { uuid }),
         (uuid_strategy(), "(title|project|status)").prop_map(|(uuid, property)| {
             Operation::Update {
                 uuid,
@@ -28,32 +29,50 @@ fn operation_strategy() -> impl Strategy<Value = Operation> {
 }
 
 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) = Operation::transform(o1.clone(), o2.clone());
 
-        // check that the two operation sequences have the same effect, enforcing the invariant of
-        // the transform function.  This needs some care as if either of the operations is
-        // an Update then we must ensure the task already exists in the DB.
         let mut db1 = DB::new();
 
+        // Ensure that any expected tasks already exist
         if let Operation::Update{ ref uuid, .. } = o1 {
-            db1.apply(Operation::Create{uuid: uuid.clone()});
+            let _ = db1.apply(Operation::Create{uuid: uuid.clone()});
         }
 
         if let Operation::Update{ ref uuid, .. } = o2 {
-            db1.apply(Operation::Create{uuid: uuid.clone()});
+            let _ = db1.apply(Operation::Create{uuid: uuid.clone()});
+        }
+
+        if let Operation::Delete{ ref uuid } = o1 {
+            let _ = db1.apply(Operation::Create{uuid: uuid.clone()});
+        }
+
+        if let Operation::Delete{ ref uuid } = o2 {
+            let _ = db1.apply(Operation::Create{uuid: uuid.clone()});
         }
 
         let mut db2 = db1.clone();
 
-        db1.apply(o1);
+        // if applying the initial operations fail, that indicates the operation was invalid
-        if let Some(o) = o2p {
+        // in the base state, so consider the case successful.
-            db1.apply(o);
+        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()))?;
         }
-        db2.apply(o2);
         if let Some(o) = o1p {
-            db2.apply(o);
+            db2.apply(o).map_err(|e| TestCaseError::Fail(format!("Applying to db2: {}", e).into()))?;
         }
         assert_eq!(db1.tasks(), db2.tasks());
     }

tests/sync.rs

@@ -14,22 +14,24 @@ fn test_sync() {
 
     // make some changes in parallel to db1 and db2..
     let uuid1 = Uuid::new_v4();
-    db1.apply(Operation::Create { uuid: uuid1 });
+    db1.apply(Operation::Create { uuid: uuid1 }).unwrap();
     db1.apply(Operation::Update {
         uuid: uuid1,
         property: "title".into(),
         value: Some("my first task".into()),
         timestamp: Utc::now(),
-    });
+    })
+    .unwrap();
 
     let uuid2 = Uuid::new_v4();
-    db2.apply(Operation::Create { uuid: uuid2 });
+    db2.apply(Operation::Create { uuid: uuid2 }).unwrap();
     db2.apply(Operation::Update {
         uuid: uuid2,
         property: "title".into(),
         value: Some("my second task".into()),
         timestamp: Utc::now(),
-    });
+    })
+    .unwrap();
 
     // and synchronize those around
     db1.sync("me", &mut server);
@@ -43,13 +45,15 @@ fn test_sync() {
         property: "priority".into(),
         value: Some("H".into()),
         timestamp: Utc::now(),
-    });
+    })
+    .unwrap();
     db2.apply(Operation::Update {
         uuid: uuid2,
         property: "project".into(),
         value: Some("personal".into()),
         timestamp: Utc::now(),
-    });
+    })
+    .unwrap();
 
     // and synchronize those around
     db1.sync("me", &mut server);