From 911c194fb3ceb8ead034baaff0fa5c4f9ce4fecc Mon Sep 17 00:00:00 2001 From: Alexis Sellier Date: Fri, 30 Dec 2022 13:30:35 +0100 Subject: [PATCH] cob: Create proper `OpId` type To still access a comment's author, we add a new `author` field. --- radicle-httpd/src/api/v1/projects.rs | 6 ++- radicle/src/cob/issue.rs | 30 +++++------ radicle/src/cob/op.rs | 53 +++++++++++++++++--- radicle/src/cob/patch.rs | 26 ++++------ radicle/src/cob/store.rs | 5 +- radicle/src/cob/thread.rs | 74 +++++++++++++++------------- 6 files changed, 115 insertions(+), 79 deletions(-) diff --git a/radicle-httpd/src/api/v1/projects.rs b/radicle-httpd/src/api/v1/projects.rs index 0518e62a..1e7de595 100644 --- a/radicle-httpd/src/api/v1/projects.rs +++ b/radicle-httpd/src/api/v1/projects.rs @@ -423,9 +423,11 @@ impl<'a> FromIterator<(&'a CommentId, &'a thread::Comment)> for Comments { fn from_iter>(iter: I) -> Self { let mut comments = Vec::new(); - for (comment_id, comment) in iter { + for (_, comment) in iter { comments.push(Comment { - author: Author { id: comment_id.1 }, + author: Author { + id: comment.author(), + }, body: comment.body().to_owned(), reactions: [], timestamp: comment.timestamp(), diff --git a/radicle/src/cob/issue.rs b/radicle/src/cob/issue.rs index d988817e..b1c7dba2 100644 --- a/radicle/src/cob/issue.rs +++ b/radicle/src/cob/issue.rs @@ -20,7 +20,7 @@ use crate::storage::git as storage; use super::op::Ops; /// Issue operation. -pub type Op = crate::cob::Op; +pub type Op = cob::Op; /// Type name of an issue. pub static TYPENAME: Lazy = @@ -155,7 +155,7 @@ impl Issue { self.thread .comments() .next() - .map(|((_, pk), _)| Author::new(*pk)) + .map(|(_, c)| Author::new(c.author())) } pub fn description(&self) -> Option<&str> { @@ -192,12 +192,8 @@ impl Issue { } } Action::Thread { action } => { - self.thread.apply([cob::Op { - action, - author: op.author, - clock: op.clock, - timestamp: op.timestamp, - }])?; + self.thread + .apply([cob::Op::new(action, op.author, op.timestamp, op.clock)])?; } } } @@ -626,7 +622,7 @@ mod test { .create("My first issue", "Blah blah blah.", &[], &signer) .unwrap(); - let comment = (clock::Lamport::default(), *signer.public_key()); + let comment = OpId::initial(*signer.public_key()); let reaction = Reaction::new('🥳').unwrap(); issue.react(comment, reaction, &signer).unwrap(); @@ -648,7 +644,7 @@ mod test { let mut issue = issues .create("My first issue", "Blah blah blah.", &[], &signer) .unwrap(); - let root = (clock::Lamport::default(), author); + let root = OpId::initial(author); let c1 = issue.comment("Hi hi hi.", root, &signer).unwrap(); let c2 = issue.comment("Ha ha ha.", root, &signer).unwrap(); @@ -710,23 +706,23 @@ mod test { .unwrap(); // The initial thread op id is always the same. - let c0 = (clock::Lamport::initial(), author); + let c0 = OpId::initial(author); issue.comment("Ho ho ho.", c0, &signer).unwrap(); issue.comment("Ha ha ha.", c0, &signer).unwrap(); let id = issue.id; let issue = issues.get(&id).unwrap().unwrap(); - let ((_, a0), c0) = &issue.comments().nth(0).unwrap(); - let ((_, a1), c1) = &issue.comments().nth(1).unwrap(); - let ((_, a2), c2) = &issue.comments().nth(2).unwrap(); + let (_, c0) = &issue.comments().nth(0).unwrap(); + let (_, c1) = &issue.comments().nth(1).unwrap(); + let (_, c2) = &issue.comments().nth(2).unwrap(); assert_eq!(c0.body(), "Blah blah blah."); - assert_eq!(a0, &author); + assert_eq!(c0.author(), author); assert_eq!(c1.body(), "Ho ho ho."); - assert_eq!(a1, &author); + assert_eq!(c1.author(), author); assert_eq!(c2.body(), "Ha ha ha."); - assert_eq!(a2, &author); + assert_eq!(c2.author(), author); } #[test] diff --git a/radicle/src/cob/op.rs b/radicle/src/cob/op.rs index 502707dc..6295c769 100644 --- a/radicle/src/cob/op.rs +++ b/radicle/src/cob/op.rs @@ -1,6 +1,7 @@ use std::collections::BTreeMap; use nonempty::NonEmpty; +use serde::{Deserialize, Serialize}; use thiserror::Error; use radicle_cob::history::EntryWithClock; @@ -8,16 +9,37 @@ use radicle_crdt::clock; use radicle_crdt::clock::Lamport; use radicle_crypto::{PublicKey, Signer}; -/// Identifies an [`Op`]. -pub type OpId = (Lamport, ActorId); +/// Identifies an [`Op`] internally and within the change graph. +#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] +pub struct OpId(Lamport, ActorId); + +impl OpId { + /// Create a new operation id. + pub fn new(clock: Lamport, actor: ActorId) -> Self { + Self(clock, actor) + } + + /// Get the initial operation id for the given actor. + pub fn initial(actor: ActorId) -> Self { + Self(Lamport::initial(), actor) + } + + /// Get operation id clock. + pub fn clock(&self) -> Lamport { + self.0 + } +} + /// The author of an [`Op`]. pub type ActorId = PublicKey; /// Error decoding an operation from an entry. #[derive(Error, Debug)] -pub enum OpDecodeError { - #[error("deserialization from json failed: {0}")] - Deserialize(#[from] serde_json::Error), +pub enum OpEncodingError { + #[error("encoding failed: {0}")] + Encoding(#[from] serde_json::Error), + #[error("git: {0}")] + Git(#[from] git2::Error), } /// The `Op` is the operation that is applied onto a state to form a CRDT. @@ -36,16 +58,33 @@ pub struct Op { pub timestamp: clock::Physical, } +impl Op { + pub fn new( + action: A, + author: ActorId, + timestamp: impl Into, + clock: Lamport, + ) -> Self { + Self { + action, + author, + clock, + timestamp: timestamp.into(), + } + } +} + pub struct Ops(pub NonEmpty>); impl<'a, A> TryFrom<&'a EntryWithClock> for Ops where for<'de> A: serde::Deserialize<'de>, { - type Error = OpDecodeError; + type Error = OpEncodingError; fn try_from(entry: &'a EntryWithClock) -> Result { let mut clock = entry.clock().into(); + entry .contents() .clone() @@ -69,7 +108,7 @@ impl Op { /// Get the op id. /// This uniquely identifies each operation in the CRDT. pub fn id(&self) -> OpId { - (self.clock, self.author) + OpId(self.clock, self.author) } } diff --git a/radicle/src/cob/patch.rs b/radicle/src/cob/patch.rs index 11f09fb3..7aa42783 100644 --- a/radicle/src/cob/patch.rs +++ b/radicle/src/cob/patch.rs @@ -33,7 +33,7 @@ pub static TYPENAME: Lazy = Lazy::new(|| FromStr::from_str("xyz.radicle.patch").expect("type name is valid")); /// Patch operation. -pub type Op = crate::cob::Op; +pub type Op = cob::Op; /// Identifier for a patch. pub type PatchId = ObjectId; @@ -300,12 +300,9 @@ impl Patch { // TODO(cloudhead): Make sure we can deal with redacted revisions which are added // to out of order, like in the `Merge` case. if let Some(Redactable::Present(revision)) = self.revisions.get_mut(&revision) { - revision.discussion.apply([cob::Op { - action, - author: op.author, - clock: op.clock, - timestamp, - }])?; + revision + .discussion + .apply([cob::Op::new(action, op.author, timestamp, op.clock)])?; } else { return Err(ApplyError::Missing(revision)); } @@ -937,7 +934,7 @@ mod test { let base = oids[rng.usize(..oids.len())]; if rng.bool() { - revisions.push((clock.tick(), author)); + revisions.push(OpId::new(clock.tick(), author)); } Some((*clock, Action::Revision { base, oid })) }); @@ -947,12 +944,7 @@ mod test { let timestamp = Timestamp::now() + rng.u64(..60); for (clock, action) in gen.take(g.size()) { - changes.push(Op { - action, - author, - clock, - timestamp, - }); + changes.push(Op::new(action, author, timestamp, clock)); } for p in &mut permutations { @@ -1250,11 +1242,11 @@ mod test { assert_eq!(patch.description(), Some("Blah blah blah.")); assert_eq!(patch.version(), 0); - let ((c1, _), (c2, _)) = patch + let (r1, t1) = patch .update("I've made changes.", base, rev1_oid, &signer) .unwrap(); - assert_eq!(c1.get(), 3); - assert_eq!(c2.get(), 4); + assert_eq!(r1.clock().get(), 3); + assert_eq!(t1.clock().get(), 4); let id = patch.id; let patch = patches.get(&id).unwrap().unwrap(); diff --git a/radicle/src/cob/store.rs b/radicle/src/cob/store.rs index 81c71f3d..02192421 100644 --- a/radicle/src/cob/store.rs +++ b/radicle/src/cob/store.rs @@ -9,6 +9,7 @@ use serde::Serialize; use crate::cob; use crate::cob::common::Author; +use crate::cob::op::OpId; use crate::cob::CollaborativeObject; use crate::cob::{ActorId, Create, History, ObjectId, TypeName, Update}; use crate::crypto::PublicKey; @@ -250,11 +251,11 @@ impl Transaction { // Otherwise, it means it was the first operation of our COB. In that case // we set our clock to the initial clock value (0), and return that. if let Some(ref mut clock) = self.clock { - (clock.tick(), self.actor) + OpId::new(clock.tick(), self.actor) } else { self.clock = Some(Lamport::initial()); - (Lamport::initial(), self.actor) + OpId::initial(self.actor) } } diff --git a/radicle/src/cob/thread.rs b/radicle/src/cob/thread.rs index ba4eb225..994c3573 100644 --- a/radicle/src/cob/thread.rs +++ b/radicle/src/cob/thread.rs @@ -42,6 +42,8 @@ pub struct Edit { /// A comment on a discussion thread. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Comment { + /// Comment author. + author: ActorId, /// The comment body. edits: GMap>, /// Comment this is a reply to. @@ -51,10 +53,16 @@ pub struct Comment { impl Comment { /// Create a new comment. - pub fn new(body: String, reply_to: Option, timestamp: Timestamp) -> Self { + pub fn new( + author: ActorId, + body: String, + reply_to: Option, + timestamp: Timestamp, + ) -> Self { let edit = Edit { body, timestamp }; Self { + author, edits: GMap::singleton(Lamport::initial(), Max::from(edit)), reply_to, } @@ -82,6 +90,11 @@ impl Comment { .timestamp } + /// Return the comment author. + pub fn author(&self) -> ActorId { + self.author + } + /// Return the comment this is a reply to. Returns nothing if this is the root comment. pub fn reply_to(&self) -> Option { self.reply_to @@ -119,7 +132,7 @@ pub enum Action { /// Comment this is a reply to. /// Should be [`None`] if it's the top-level comment. /// Should be the root [`CommentId`] if it's a top-level comment. - reply_to: Option, + reply_to: Option, }, /// Edit a comment. Edit { id: CommentId, body: String }, @@ -156,6 +169,13 @@ impl Semilattice for Thread { } impl Thread { + pub fn new(id: CommentId, comment: Comment) -> Self { + Self { + comments: GMap::singleton(id, Redactable::Present(comment)), + reactions: GMap::default(), + } + } + pub fn is_initialized(&self) -> bool { !self.comments.is_empty() } @@ -211,17 +231,19 @@ impl Thread { pub fn apply(&mut self, ops: impl IntoIterator>) -> Result<(), OpError> { for op in ops.into_iter() { let id = op.id(); + let author = op.author; + let timestamp = op.timestamp; match op.action { Action::Comment { body, reply_to } => { self.comments.insert( id, - Redactable::Present(Comment::new(body, reply_to, op.timestamp)), + 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, op.timestamp); + comment.edit(op.clock, body, timestamp); } else { return Err(OpError::Missing(id)); } @@ -388,11 +410,11 @@ mod tests { fn arbitrary(g: &mut qcheck::Gen) -> Self { let author = ActorId::from([0; 32]); let rng = fastrand::Rng::with_seed(u64::arbitrary(g)); - let root = (Lamport::initial(), author); + let root = OpId::initial(author); let gen = WeightedGenerator::<(Lamport, Action), (Lamport, BTreeSet)>::new(rng.clone()) .variant(3, |(clock, comments), rng| { - comments.insert((clock.tick(), author)); + comments.insert(OpId::new(clock.tick(), author)); Some(( *clock, @@ -441,25 +463,20 @@ mod tests { Some((clock.tick(), Action::Redact { id })) }); - let mut ops = vec![Op { - action: Action::Comment { + let mut ops = vec![Op::new( + Action::Comment { body: String::default(), reply_to: None, }, author, - clock: Lamport::initial(), - timestamp: Timestamp::now(), - }]; + Timestamp::now(), + Lamport::initial(), + )]; let mut permutations: [Vec>; N] = array::from_fn(|_| Vec::new()); for (clock, action) in gen.take(g.size()) { let timestamp = Timestamp::now() + rng.u64(..60); - ops.push(Op { - action, - author, - clock, - timestamp, - }); + ops.push(Op::new(action, author, timestamp, clock)); } for p in &mut permutations { @@ -487,31 +504,21 @@ mod tests { #[test] fn test_redact_comment() { let tmp = tempfile::tempdir().unwrap(); - let (_, signer, repository) = radicle::test::setup::context(&tmp); - let store = setup::store(&signer, &repository); + let (_, signer, _) = radicle::test::setup::context(&tmp); let mut alice = Actor::new(signer); + let mut thread = Thread::default(); let a0 = alice.comment("First comment", None); let a1 = alice.comment("Second comment", Some(a0.id())); let a2 = alice.comment("Third comment", Some(a0.id())); - let (id, _, _) = store - .create("Thread created", a0.action, &alice.signer) - .unwrap(); - let comment = store - .update(id, "Thread updated", a1.action, &alice.signer) - .unwrap(); - store - .update(id, "Thread updated", a2.action, &alice.signer) - .unwrap(); + thread.apply([a0, a1.clone(), a2]).unwrap(); + assert_eq!(thread.comments().count(), 3); // Redact the second comment. - let a3 = alice.redact((comment.history().clock().into(), *alice.signer.public_key())); - store - .update(id, "Comment redacted", a3.action, &alice.signer) - .unwrap(); + let a3 = alice.redact(a1.id()); + thread.apply([a3]).unwrap(); - let (thread, _) = store.get(&id).unwrap().unwrap(); let (_, comment0) = thread.comments().nth(0).unwrap(); let (_, comment1) = thread.comments().nth(1).unwrap(); @@ -550,7 +557,6 @@ mod tests { let tmp = tempfile::tempdir().unwrap(); let (_, signer, repository) = radicle::test::setup::context(&tmp); let store = setup::store(&signer, &repository); - let mut alice = Actor::new(signer); let a0 = alice.comment("Thread root", None);