From e47d7ca2aab86c69266a03c0dec6e56f5500095c Mon Sep 17 00:00:00 2001 From: Alexis Sellier Date: Thu, 15 Dec 2022 09:43:48 +0100 Subject: [PATCH] cob: Implement comment edit action This implementation intentionally preserves all edits such that they may be displayed in an application as an edit history. Signed-off-by: Alexis Sellier --- radicle-cli/src/terminal/io.rs | 2 +- radicle-httpd/src/api/v1/projects.rs | 6 +- radicle/src/cob/issue.rs | 24 ++--- radicle/src/cob/patch.rs | 7 +- radicle/src/cob/thread.rs | 134 ++++++++++++++++++++------- 5 files changed, 124 insertions(+), 49 deletions(-) diff --git a/radicle-cli/src/terminal/io.rs b/radicle-cli/src/terminal/io.rs index 340405e3..9802cc78 100644 --- a/radicle-cli/src/terminal/io.rs +++ b/radicle-cli/src/terminal/io.rs @@ -382,7 +382,7 @@ pub fn comment_select(issue: &Issue) -> Option { .items( &issue .comments() - .map(|(_, i)| i.body.clone()) + .map(|(_, i)| i.body().to_owned()) .collect::>(), ) .default(0) diff --git a/radicle-httpd/src/api/v1/projects.rs b/radicle-httpd/src/api/v1/projects.rs index f8141017..7de08df1 100644 --- a/radicle-httpd/src/api/v1/projects.rs +++ b/radicle-httpd/src/api/v1/projects.rs @@ -395,10 +395,10 @@ impl<'a> FromIterator<(&'a CommentId, &'a thread::Comment)> for Comments { for (comment_id, comment) in iter { comments.push(Comment { author: Author { id: comment_id.1 }, - body: comment.body.to_owned(), + body: comment.body().to_owned(), reactions: [], - timestamp: comment.timestamp, - reply_to: comment.reply_to, + timestamp: comment.timestamp(), + reply_to: comment.reply_to(), }); } diff --git a/radicle/src/cob/issue.rs b/radicle/src/cob/issue.rs index 86398bd2..ea54bb9d 100644 --- a/radicle/src/cob/issue.rs +++ b/radicle/src/cob/issue.rs @@ -34,6 +34,8 @@ pub type IssueId = ObjectId; pub enum Error { #[error("apply failed")] Apply, + #[error("thread apply failed: {0}")] + Thread(#[from] thread::OpError), #[error("store: {0}")] Store(#[from] store::Error), } @@ -150,7 +152,7 @@ impl Issue { } pub fn description(&self) -> Option<&str> { - self.thread.comments().next().map(|(_, c)| c.body.as_str()) + self.thread.comments().next().map(|(_, c)| c.body()) } pub fn comments(&self) -> impl Iterator { @@ -180,7 +182,7 @@ impl Issue { author: op.author, clock: op.clock, timestamp: op.timestamp, - }]); + }])?; } } } @@ -537,8 +539,8 @@ mod test { let (_, reply1) = &issue.replies(&root).nth(0).unwrap(); let (_, reply2) = &issue.replies(&root).nth(1).unwrap(); - assert_eq!(reply1.body, "Hi hi hi."); - assert_eq!(reply2.body, "Ha ha ha."); + assert_eq!(reply1.body(), "Hi hi hi."); + assert_eq!(reply2.body(), "Ha ha ha."); issue.comment("Re: Hi.", c1, &signer).unwrap(); issue.comment("Re: Ha.", c2, &signer).unwrap(); @@ -546,11 +548,11 @@ mod test { issue.comment("Re: Ha. Ha. Ha.", c2, &signer).unwrap(); let issue = issues.get(&id).unwrap().unwrap(); - assert_eq!(&issue.replies(&c1).nth(0).unwrap().1.body, "Re: Hi."); - assert_eq!(&issue.replies(&c2).nth(0).unwrap().1.body, "Re: Ha."); - assert_eq!(&issue.replies(&c2).nth(1).unwrap().1.body, "Re: Ha. Ha."); + assert_eq!(issue.replies(&c1).nth(0).unwrap().1.body(), "Re: Hi."); + assert_eq!(issue.replies(&c2).nth(0).unwrap().1.body(), "Re: Ha."); + assert_eq!(issue.replies(&c2).nth(1).unwrap().1.body(), "Re: Ha. Ha."); assert_eq!( - &issue.replies(&c2).nth(2).unwrap().1.body, + issue.replies(&c2).nth(2).unwrap().1.body(), "Re: Ha. Ha. Ha." ); } @@ -600,11 +602,11 @@ mod test { let ((_, a1), c1) = &issue.comments().nth(1).unwrap(); let ((_, a2), c2) = &issue.comments().nth(2).unwrap(); - assert_eq!(&c0.body, "Blah blah blah."); + assert_eq!(c0.body(), "Blah blah blah."); assert_eq!(a0, &author); - assert_eq!(&c1.body, "Ho ho ho."); + assert_eq!(c1.body(), "Ho ho ho."); assert_eq!(a1, &author); - assert_eq!(&c2.body, "Ha ha ha."); + assert_eq!(c2.body(), "Ha ha ha."); assert_eq!(a2, &author); } diff --git a/radicle/src/cob/patch.rs b/radicle/src/cob/patch.rs index 1a1d2689..b5468bdc 100644 --- a/radicle/src/cob/patch.rs +++ b/radicle/src/cob/patch.rs @@ -56,6 +56,9 @@ pub enum ApplyError { /// that hasn't happened yet. #[error("causal dependency {0:?} missing")] Missing(OpId), + /// Error applying an op to the patch thread. + #[error("thread apply failed: {0}")] + Thread(#[from] thread::OpError), } /// Error updating or creating patches. @@ -302,7 +305,7 @@ impl Patch { author: op.author, clock: op.clock, timestamp, - }]); + }])?; } else { return Err(ApplyError::Missing(revision)); } @@ -373,7 +376,7 @@ impl Revision { pub fn description(&self) -> Option<&str> { let (_, comment) = self.discussion.root()?; - Some(comment.body.as_str()) + Some(comment.body()) } } diff --git a/radicle/src/cob/thread.rs b/radicle/src/cob/thread.rs index fcbe2495..8a176a68 100644 --- a/radicle/src/cob/thread.rs +++ b/radicle/src/cob/thread.rs @@ -1,9 +1,9 @@ use std::cmp::Ordering; -use std::collections::BTreeMap; use std::ops::{Deref, DerefMut}; use radicle_crdt as crdt; use serde::{Deserialize, Serialize}; +use thiserror::Error; use crate::cob; use crate::cob::common::{Reaction, Timestamp}; @@ -11,35 +11,91 @@ use crate::cob::{ActorId, Op, OpId}; use crate::crypto::Signer; use crdt::clock::Lamport; -use crdt::gmap::GMap; -use crdt::lwwset::LWWSet; -use crdt::redactable::Redactable; -use crdt::Semilattice; +use crdt::{GMap, LWWSet, Max, Redactable, Semilattice}; + +/// Error applying an operation onto a state. +#[derive(Error, Debug)] +pub enum OpError { + /// Causal dependency missing. + /// + /// This error indicates that the operations are not being applied + /// in causal order, which is a requirement for this CRDT. + /// + /// For example, this can occur if an operation references anothern operation + /// that hasn't happened yet. + #[error("causal dependency {0:?} missing")] + Missing(OpId), +} /// Identifies a comment. pub type CommentId = OpId; +/// A comment edit is just some text and an edit time. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] +pub struct Edit { + /// When the edit was made. + pub timestamp: Timestamp, + /// Edit contents. Replaces previous edits. + pub body: String, +} + /// A comment on a discussion thread. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct Comment { /// The comment body. - pub body: String, + edits: GMap>, /// Comment this is a reply to. /// Should always be set, except for the root comment. - pub reply_to: Option, - /// When the comment was authored. - pub timestamp: Timestamp, + reply_to: Option, } impl Comment { /// Create a new comment. pub fn new(body: String, reply_to: Option, timestamp: Timestamp) -> Self { + let edit = Edit { body, timestamp }; + Self { - body, + edits: GMap::singleton(Lamport::initial(), Max::from(edit)), reply_to, - timestamp, } } + + /// Get the comment body. If there are multiple edits, gets the value at the latest edit. + pub fn body(&self) -> &str { + // SAFETY: There is always at least one edit. This is guaranteed by the [`Comment`] + // constructor. + #[allow(clippy::unwrap_used)] + self.edits.values().last().unwrap().get().body.as_str() + } + + /// Get the comment timestamp, which is the time of the *original* edit. To get the timestamp + /// of the latest edit, use the [`Comment::edits`] function. + pub fn timestamp(&self) -> Timestamp { + // SAFETY: There is always at least one edit. This is guaranteed by the [`Comment`] + // constructor. + #[allow(clippy::unwrap_used)] + self.edits + .first_key_value() + .map(|(_, v)| v) + .unwrap() + .get() + .timestamp + } + + /// 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 + } + + /// Return the ordered list of edits for this comment, including the original version. + pub fn edits(&self) -> impl Iterator { + self.edits.values().map(Max::get) + } + + /// Add an edit. + pub fn edit(&mut self, clock: Lamport, body: String, timestamp: Timestamp) { + self.edits.insert(clock, Edit { body, timestamp }.into()) + } } impl PartialOrd for Comment { @@ -65,11 +121,13 @@ pub enum Action { /// Should be the root [`CommentId`] if it's a top-level comment. reply_to: Option, }, + /// Edit a comment. + Edit { id: CommentId, body: String }, /// Redact a change. Not all changes can be redacted. - Redact { id: OpId }, + Redact { id: CommentId }, /// React to a change. React { - to: OpId, + to: CommentId, reaction: Reaction, active: bool, }, @@ -97,19 +155,19 @@ impl Semilattice for Thread { } } -impl Deref for Thread { - type Target = BTreeMap>; - - fn deref(&self) -> &Self::Target { - &self.comments - } -} - impl Thread { pub fn is_initialized(&self) -> bool { !self.comments.is_empty() } + pub fn is_empty(&self) -> bool { + self.len() == 0 + } + + pub fn len(&self) -> usize { + self.comments.len() + } + pub fn comment(&self, id: &CommentId) -> Option<&Comment> { if let Some(Redactable::Present(comment)) = self.comments.get(id) { Some(comment) @@ -150,7 +208,7 @@ impl Thread { .map(|(a, r)| (a, r)) } - pub fn apply(&mut self, ops: impl IntoIterator>) { + pub fn apply(&mut self, ops: impl IntoIterator>) -> Result<(), OpError> { for op in ops.into_iter() { let id = op.id(); @@ -161,6 +219,13 @@ impl Thread { Redactable::Present(Comment::new(body, reply_to, op.timestamp)), ); } + Action::Edit { id, body } => { + if let Some(Redactable::Present(comment)) = self.comments.get_mut(&id) { + comment.edit(op.clock, body, op.timestamp); + } else { + return Err(OpError::Missing(id)); + } + } Action::Redact { id } => { self.comments.insert(id, Redactable::Redacted); } @@ -181,6 +246,7 @@ impl Thread { } } } + Ok(()) } pub fn comments(&self) -> impl Iterator + '_ { @@ -279,11 +345,13 @@ mod tests { fn from_history(history: &cob::History) -> Result<(Self, Lamport), cob::store::Error> { let obj = history.traverse(Thread::default(), |mut acc, entry| { if let Ok(Ops(ops)) = Ops::try_from(entry) { - acc.apply(ops); - ControlFlow::Continue(acc) + if acc.apply(ops).is_err() { + return ControlFlow::Break(acc); + } } else { - ControlFlow::Break(acc) + return ControlFlow::Break(acc); } + ControlFlow::Continue(acc) }); Ok((obj, history.clock().into())) } @@ -412,8 +480,8 @@ mod tests { let (_, comment1) = thread.comments().nth(1).unwrap(); assert_eq!(thread.comments().count(), 2); - assert_eq!(comment0.body, "First comment"); - assert_eq!(comment1.body, "Third comment"); // Second comment was redacted. + assert_eq!(comment0.body(), "First comment"); + assert_eq!(comment1.body(), "Third comment"); // Second comment was redacted. } #[test] @@ -430,7 +498,9 @@ mod tests { let a2 = alice.comment("Second comment", Some(a0.id())); let mut expected = Thread::default(); - expected.apply([a0.clone(), a1.clone(), a2.clone()]); + expected + .apply([a0.clone(), a1.clone(), a2.clone()]) + .unwrap(); let (id, _, _) = store .create("Thread created", a0.action, &alice.signer) @@ -525,13 +595,13 @@ mod tests { let [p1, p2, p3] = log.permutations; let mut t1 = t.clone(); - t1.apply(p1); + t1.apply(p1).unwrap(); let mut t2 = t.clone(); - t2.apply(p2); + t2.apply(p2).unwrap(); let mut t3 = t; - t3.apply(p3); + t3.apply(p3).unwrap(); assert_eq!(t1, t2); assert_eq!(t2, t3);