diff --git a/radicle/src/cob/patch.rs b/radicle/src/cob/patch.rs index adec0021..e0c13f39 100644 --- a/radicle/src/cob/patch.rs +++ b/radicle/src/cob/patch.rs @@ -421,8 +421,11 @@ impl store::FromHistory for Patch { revision, description, } => { - if let Some(Redactable::Present(revision)) = self.revisions.get_mut(&revision) { - revision.description.set(description, op.clock); + if let Some(redactable) = self.revisions.get_mut(&revision) { + // If the revision was redacted concurrently, there's nothing to do. + if let Redactable::Present(revision) = redactable { + revision.description.set(description, op.clock); + } } else { return Err(Error::Missing(revision)); } @@ -432,12 +435,6 @@ impl store::FromHistory for Patch { base, oid, } => { - // Since revisions are keyed by content hash, we shouldn't re-insert a revision - // if it already exists, otherwise this will be resolved via the `merge` - // operation of `Redactable`. - if self.revisions.contains_key(&id) { - continue; - } self.revisions.insert( id, Redactable::Present(Revision::new( @@ -451,6 +448,7 @@ impl store::FromHistory for Patch { ); } Action::Redact { revision } => { + // Redactions must have observed a revision to be valid. if let Some(revision) = self.revisions.get_mut(&revision) { revision.merge(Redactable::Redacted); } else { @@ -1347,6 +1345,7 @@ mod test { use crate::cob::test::Actor; use crate::crypto::test::signer::MockSigner; use crate::test; + use crate::test::arbitrary; use crate::test::arbitrary::gen; use crate::test::storage::MockRepository; @@ -1744,9 +1743,9 @@ mod test { } #[test] - fn test_revision_redact_reinsert() { - let base = git::Oid::from_str("cb18e95ada2bb38aadd8e6cef0963ce37a87add3").unwrap(); - let oid = git::Oid::from_str("518d5069f94c03427f694bb494ac1cd7d1339380").unwrap(); + fn test_revision_edit_redact() { + let base = arbitrary::oid(); + let oid = arbitrary::oid(); let repo = gen::(1); let mut alice = Actor::new(MockSigner::default()); let mut p1 = Patch::default(); @@ -1757,41 +1756,15 @@ mod test { base, oid, }); - let a2 = alice.op(Action::Redact { revision: a1.id() }); - - p1.apply([a1.clone(), a2.clone(), a1.clone()], &repo) - .unwrap(); - p2.apply([a1.clone(), a1, a2], &repo).unwrap(); - - assert_eq!(p1, p2); - } - - #[test] - fn test_revision_merge_reinsert() { - let base = git::Oid::from_str("cb18e95ada2bb38aadd8e6cef0963ce37a87add3").unwrap(); - let oid = git::Oid::from_str("518d5069f94c03427f694bb494ac1cd7d1339380").unwrap(); - let id = gen::(1); - let mut alice = Actor::new(MockSigner::default()); - let mut doc = gen::>(1); - doc.delegates.push(alice.signer.public_key().into()); - let repo = MockRepository::new(id, doc); - - let mut p1 = Patch::default(); - let mut p2 = Patch::default(); - - let a1 = alice.op(Action::Revision { - description: String::new(), - base, - oid, - }); - let a2 = alice.op(Action::Merge { - revision: a1.id(), - commit: oid, + let a2 = alice.op(Action::Redact { revision: a1.id }); + let a3 = alice.op(Action::EditRevision { + revision: a1.id, + description: String::from("Edited"), }); - p1.apply([a1.clone(), a2.clone(), a1.clone()], &repo) + p1.apply([a1.clone(), a2.clone(), a3.clone()], &repo) .unwrap(); - p2.apply([a1.clone(), a1, a2], &repo).unwrap(); + p2.apply([a1, a3, a2], &repo).unwrap(); assert_eq!(p1, p2); } diff --git a/radicle/src/cob/test.rs b/radicle/src/cob/test.rs index dddc0c56..47a923aa 100644 --- a/radicle/src/cob/test.rs +++ b/radicle/src/cob/test.rs @@ -167,7 +167,11 @@ impl Actor { clock: clock::Lamport, identity: Oid, ) -> Op { - let data = encoding::encode(&action).unwrap(); + let data = encoding::encode(serde_json::json!({ + "action": action, + "nonce": fastrand::u64(..), + })) + .unwrap(); let oid = git::raw::Oid::hash_object(git::raw::ObjectType::Blob, &data).unwrap(); let id = oid.into(); let author = *self.signer.public_key(); diff --git a/radicle/src/cob/thread.rs b/radicle/src/cob/thread.rs index bfe72ae7..319f465b 100644 --- a/radicle/src/cob/thread.rs +++ b/radicle/src/cob/thread.rs @@ -34,6 +34,12 @@ pub enum Error { /// Validation error. #[error("validation failed: {0}")] Validate(&'static str), + /// Error with comment operation. + #[error("comment {0} is invalid")] + Comment(EntryId), + /// Error with edit operation. + #[error("edit {0} is invalid")] + Edit(EntryId), } /// Identifies a comment. @@ -288,27 +294,40 @@ impl cob::store::FromHistory for Thread { match op.action { Action::Comment { body, reply_to } => { - // Since comments are keyed by content hash, we shouldn't re-insert a comment - // if it already exists, otherwise this will be resolved via the `merge` - // operation of `Redactable`. - if self.comments.contains_key(&id) { - continue; + if body.is_empty() { + return Err(Error::Comment(op.id)); } - + // Nb. If a comment is already present, it must be redacted, because the + // underlying store guarantees exactly-once delivery of ops. self.comments.insert( id, Redactable::Present(Comment::new(author, body, reply_to, timestamp)), ); } Action::Edit { id, body } => { - if let Some(Redactable::Present(comment)) = self.comments.get_mut(&id) { - comment.edit(op.clock, body, timestamp); + if body.is_empty() { + return Err(Error::Edit(op.id)); + } + // It's possible for a comment to be redacted before we're able to edit it, in + // case of a concurrent update. + // + // However, it's *not* possible for the comment to be absent. Therefore we treat + // that as an error. + if let Some(redactable) = self.comments.get_mut(&id) { + if let Redactable::Present(comment) = redactable { + comment.edit(op.clock, body, timestamp); + } } else { return Err(Error::Missing(id)); } } Action::Redact { id } => { - self.comments.insert(id, Redactable::Redacted); + // Redactions must have observed a comment to be valid. + if let Some(comment) = self.comments.get_mut(&id) { + comment.merge(Redactable::Redacted); + } else { + return Err(Error::Missing(id)); + } } Action::React { to, @@ -348,6 +367,7 @@ mod tests { use crate::cob::test; use crate::crypto::test::signer::MockSigner; use crate::crypto::Signer; + use crate::test::arbitrary; use crate::test::arbitrary::gen; use crate::test::storage::MockRepository; @@ -451,7 +471,7 @@ mod tests { ); comments.insert(comment.id); - Some((*clock, comment)) + Some((clock.tick(), comment)) }) .variant(2, |(actor, clock, comments), rng| { if comments.is_empty() { @@ -465,8 +485,7 @@ mod tests { .collect::() .as_str(), ); - - Some((*clock, edit)) + Some((clock.tick(), edit)) }) .variant(2, |(actor, clock, comments), rng| { if comments.is_empty() { @@ -488,7 +507,7 @@ mod tests { Some((clock.tick(), redact)) }); - let mut ops = vec![Actor::::default().comment("", None)]; + let mut ops = vec![Actor::::default().comment("Root", None)]; let mut permutations: [Vec>; N] = array::from_fn(|_| Vec::new()); for (_, op) in gen.take(g.size()) { @@ -661,21 +680,37 @@ mod tests { } #[test] - fn test_comment_edit_reinsert() { + fn test_comment_redact_missing() { let repo = gen::(1); - let mut alice = Actor::::default(); - let mut t1 = Thread::default(); - let mut t2 = Thread::default(); + let mut t = Thread::default(); + let id = arbitrary::entry_id(); - let a1 = alice.comment("Hello.", None); - let a2 = alice.edit(a1.id(), "Hello World."); + t.apply([alice.redact(id)], &repo).unwrap_err(); + } - t1.apply([a1.clone(), a2.clone(), a1.clone()], &repo) - .unwrap(); - t2.apply([a1.clone(), a1, a2], &repo).unwrap(); + #[test] + fn test_comment_edit_missing() { + let repo = gen::(1); + let mut alice = Actor::::default(); + let mut t = Thread::default(); + let id = arbitrary::entry_id(); - assert_eq!(t1, t2); + t.apply([alice.edit(id, "Edited")], &repo).unwrap_err(); + } + + #[test] + fn test_comment_edit_redacted() { + let repo = gen::(1); + let mut alice = Actor::::default(); + let mut t = Thread::default(); + + let a1 = alice.comment("Hi", None); + let a2 = alice.redact(a1.id); + let a3 = alice.edit(a1.id, "Edited"); + + t.apply([a1, a2, a3], &repo).unwrap(); + assert_eq!(t.comments().count(), 0); } #[test] diff --git a/radicle/src/test/arbitrary.rs b/radicle/src/test/arbitrary.rs index 0749f001..c8f65905 100644 --- a/radicle/src/test/arbitrary.rs +++ b/radicle/src/test/arbitrary.rs @@ -9,7 +9,6 @@ use nonempty::NonEmpty; use qcheck::Arbitrary; use crate::collections::HashMap; -use crate::git; use crate::identity::{ doc::{Doc, Id}, project::Project, @@ -19,12 +18,17 @@ use crate::node::Address; use crate::storage; use crate::storage::refs::{Refs, SignedRefs}; use crate::test::storage::{MockRepository, MockStorage}; +use crate::{cob, git}; pub fn oid() -> storage::Oid { let oid_bytes: [u8; 20] = gen(1); storage::Oid::try_from(oid_bytes.as_slice()).unwrap() } +pub fn entry_id() -> cob::EntryId { + self::oid().into() +} + pub fn refstring(len: usize) -> git::RefString { let mut buf = Vec::::new(); for _ in 0..len {