Merge pull request #346 from jeffa5/non-counter-increment

Prevent increment on non-counter

automerge/src/decoding.rs

@@ -52,7 +52,60 @@ pub enum Error {
     Io(#[from] io::Error),
 }
 
-#[derive(thiserror::Error, Debug)]
+impl PartialEq<Error> for Error {
+    fn eq(&self, other: &Error) -> bool {
+        match (self, other) {
+            (
+                Self::WrongType {
+                    expected_one_of: l_expected_one_of,
+                    found: l_found,
+                },
+                Self::WrongType {
+                    expected_one_of: r_expected_one_of,
+                    found: r_found,
+                },
+            ) => l_expected_one_of == r_expected_one_of && l_found == r_found,
+            (Self::BadChangeFormat(l0), Self::BadChangeFormat(r0)) => l0 == r0,
+            (
+                Self::WrongByteLength {
+                    expected: l_expected,
+                    found: l_found,
+                },
+                Self::WrongByteLength {
+                    expected: r_expected,
+                    found: r_found,
+                },
+            ) => l_expected == r_expected && l_found == r_found,
+            (
+                Self::ColumnsNotInAscendingOrder {
+                    last: l_last,
+                    found: l_found,
+                },
+                Self::ColumnsNotInAscendingOrder {
+                    last: r_last,
+                    found: r_found,
+                },
+            ) => l_last == r_last && l_found == r_found,
+            (
+                Self::InvalidChecksum {
+                    found: l_found,
+                    calculated: l_calculated,
+                },
+                Self::InvalidChecksum {
+                    found: r_found,
+                    calculated: r_calculated,
+                },
+            ) => l_found == r_found && l_calculated == r_calculated,
+            (Self::InvalidChange(l0), Self::InvalidChange(r0)) => l0 == r0,
+            (Self::ChangeDecompressFailed(l0), Self::ChangeDecompressFailed(r0)) => l0 == r0,
+            (Self::Leb128(_l0), Self::Leb128(_r0)) => true,
+            (Self::Io(l0), Self::Io(r0)) => l0.kind() == r0.kind(),
+            _ => core::mem::discriminant(self) == core::mem::discriminant(other),
+        }
+    }
+}
+
+#[derive(thiserror::Error, PartialEq, Debug)]
 pub enum InvalidChangeError {
     #[error("Change contained an operation with action 'set' which did not have a 'value'")]
     SetOpWithoutValue,

automerge/src/encoding.rs

@@ -21,6 +21,14 @@ pub enum Error {
     Io(#[from] io::Error),
 }
 
+impl PartialEq<Error> for Error {
+    fn eq(&self, other: &Error) -> bool {
+        match (self, other) {
+            (Self::Io(error1), Self::Io(error2)) => error1.kind() == error2.kind(),
+        }
+    }
+}
+
 /// Encodes booleans by storing the count of the same value.
 ///
 /// The sequence of numbers describes the count of false values on even indices (0-indexed) and the

automerge/src/error.rs

@@ -3,7 +3,7 @@ use crate::value::DataType;
 use crate::{decoding, encoding, ChangeHash};
 use thiserror::Error;
 
-#[derive(Error, Debug)]
+#[derive(Error, Debug, PartialEq)]
 pub enum AutomergeError {
     #[error("invalid obj id format `{0}`")]
     InvalidObjIdFormat(String),
@@ -23,6 +23,8 @@ pub enum AutomergeError {
     DuplicateSeqNumber(u64, ActorId),
     #[error("invalid hash {0}")]
     InvalidHash(ChangeHash),
+    #[error("increment operations must be against a counter value")]
+    MissingCounter,
     #[error("general failure")]
     Fail,
 }

automerge/src/transaction/inner.rs

@@ -264,6 +264,12 @@ impl TransactionInner {
             return Ok(None);
         }
 
+        // increment operations are only valid against counter values.
+        // if there are multiple values (from conflicts) then we just need one of them to be a counter.
+        if matches!(action, OpType::Increment(_)) && query.ops.iter().all(|op| !op.is_counter()) {
+            return Err(AutomergeError::MissingCounter);
+        }
+
         let pred = query.ops.iter().map(|op| op.id).collect();
 
         let op = Op {
@@ -299,6 +305,12 @@ impl TransactionInner {
             return Ok(None);
         }
 
+        // increment operations are only valid against counter values.
+        // if there are multiple values (from conflicts) then we just need one of them to be a counter.
+        if matches!(action, OpType::Increment(_)) && query.ops.iter().all(|op| !op.is_counter()) {
+            return Err(AutomergeError::MissingCounter);
+        }
+
         let op = Op {
             id,
             action,

automerge/tests/test.rs

@@ -1,5 +1,7 @@
 use automerge::transaction::Transactable;
-use automerge::{ActorId, AutoCommit, Automerge, ObjType, ScalarValue, Value, ROOT};
+use automerge::{
+    ActorId, AutoCommit, Automerge, AutomergeError, ObjType, ScalarValue, Value, ROOT,
+};
 
 mod helpers;
 #[allow(unused_imports)]
@@ -7,6 +9,8 @@ use helpers::{
     mk_counter, new_doc, new_doc_with_actor, pretty_print, realize, realize_obj, sorted_actors,
     RealizedObject,
 };
+use pretty_assertions::assert_eq;
+
 #[test]
 fn no_conflict_on_repeated_assignment() {
     let mut doc = AutoCommit::new();
@@ -925,3 +929,67 @@ fn list_counter_del() -> Result<(), automerge::AutomergeError> {
 
     Ok(())
 }
+
+#[test]
+fn increment_non_counter_map() {
+    let mut doc = AutoCommit::new();
+    // can't increment nothing
+    assert_eq!(
+        doc.increment(ROOT, "nothing", 2),
+        Err(AutomergeError::MissingCounter)
+    );
+
+    // can't increment a non-counter
+    doc.put(ROOT, "non-counter", "mystring").unwrap();
+    assert_eq!(
+        doc.increment(ROOT, "non-counter", 2),
+        Err(AutomergeError::MissingCounter)
+    );
+
+    // can increment a counter still
+    doc.put(ROOT, "counter", ScalarValue::counter(1)).unwrap();
+    assert_eq!(doc.increment(ROOT, "counter", 2), Ok(()));
+
+    // can increment a counter that is part of a conflict
+    let mut doc1 = AutoCommit::new();
+    doc1.set_actor(ActorId::from([1]));
+    let mut doc2 = AutoCommit::new();
+    doc2.set_actor(ActorId::from([2]));
+
+    doc1.put(ROOT, "key", ScalarValue::counter(1)).unwrap();
+    doc2.put(ROOT, "key", "mystring").unwrap();
+    doc1.merge(&mut doc2).unwrap();
+
+    assert_eq!(doc1.increment(ROOT, "key", 2), Ok(()));
+}
+
+#[test]
+fn increment_non_counter_list() {
+    let mut doc = AutoCommit::new();
+    let list = doc.put_object(ROOT, "list", ObjType::List).unwrap();
+
+    // can't increment a non-counter
+    doc.insert(&list, 0, "mystring").unwrap();
+    assert_eq!(
+        doc.increment(&list, 0, 2),
+        Err(AutomergeError::MissingCounter)
+    );
+
+    // can increment a counter
+    doc.insert(&list, 0, ScalarValue::counter(1)).unwrap();
+    assert_eq!(doc.increment(&list, 0, 2), Ok(()));
+
+    // can increment a counter that is part of a conflict
+    let mut doc1 = AutoCommit::new();
+    doc1.set_actor(ActorId::from([1]));
+    let list = doc1.put_object(ROOT, "list", ObjType::List).unwrap();
+    doc1.insert(&list, 0, ()).unwrap();
+    let mut doc2 = doc1.fork();
+    doc2.set_actor(ActorId::from([2]));
+
+    doc1.put(&list, 0, ScalarValue::counter(1)).unwrap();
+    doc2.put(&list, 0, "mystring").unwrap();
+    doc1.merge(&mut doc2).unwrap();
+
+    assert_eq!(doc1.increment(&list, 0, 2), Ok(()));
+}