From 457b82f72325f554919b94d808628014e484494f Mon Sep 17 00:00:00 2001 From: cloudhead Date: Tue, 5 Sep 2023 18:29:41 +0200 Subject: [PATCH] cob: Evaluate COB state eagerly Instead of returning histories from `radicle-cob`, we return an evaluated object which we build during initial graph traversal. We do this so that branches can be correctly pruned when operations are invalid at the application level. This way, new operations are not building on top of invalid ones contained in the history. To achieve this, we introduce a new `Evaluate` trait that is implemented by all COBs, and we make change graph evaluation fallible. Doing this means that we get `apply` errors returned for free when an invalid transaction is applied, and no longer need to check for action validity in multiple places. We also implement a new `Dag::prune` method to avoid having to copy graph nodes during traversal. --- radicle-cob/src/backend/git/change.rs | 44 ++- radicle-cob/src/change.rs | 4 +- radicle-cob/src/change/store.rs | 2 +- radicle-cob/src/change_graph.rs | 74 +++-- radicle-cob/src/history.rs | 10 - radicle-cob/src/lib.rs | 7 +- radicle-cob/src/object.rs | 4 +- radicle-cob/src/object/collaboration.rs | 50 ++- .../src/object/collaboration/create.rs | 15 +- radicle-cob/src/object/collaboration/error.rs | 26 +- radicle-cob/src/object/collaboration/get.rs | 14 +- radicle-cob/src/object/collaboration/list.rs | 22 +- .../src/object/collaboration/remove.rs | 8 +- .../src/object/collaboration/update.rs | 48 +-- radicle-cob/src/object/storage.rs | 7 +- radicle-cob/src/test/storage.rs | 25 +- radicle-cob/src/tests.rs | 45 ++- radicle-dag/src/lib.rs | 130 ++++++++ radicle-remote-helper/src/list.rs | 7 +- radicle/src/cob.rs | 4 +- radicle/src/cob/common.rs | 1 + radicle/src/cob/identity.rs | 43 ++- radicle/src/cob/issue.rs | 301 ++++++++++++++++-- radicle/src/cob/patch.rs | 88 +++-- radicle/src/cob/store.rs | 223 ++++++------- radicle/src/cob/test.rs | 18 +- radicle/src/cob/thread.rs | 52 ++- radicle/src/storage/git.rs | 1 - radicle/src/storage/git/cob.rs | 22 +- radicle/src/test.rs | 6 + 30 files changed, 878 insertions(+), 423 deletions(-) diff --git a/radicle-cob/src/backend/git/change.rs b/radicle-cob/src/backend/git/change.rs index f0ecae45..4ca3e3ce 100644 --- a/radicle-cob/src/backend/git/change.rs +++ b/radicle-cob/src/backend/git/change.rs @@ -2,6 +2,7 @@ use std::collections::BTreeMap; use std::convert::TryFrom; +use std::iter; use std::path::PathBuf; use git_ext::author::Author; @@ -95,7 +96,7 @@ impl change::Storage for git2::Repository { fn store( &self, resource: Self::Parent, - parents: Vec, + mut parents: Vec, signer: &Signer, spec: store::Template, ) -> Result @@ -118,11 +119,19 @@ impl change::Storage for git2::Repository { ExtendedSignature::new(*key, sig) }; + // Make sure there are no duplicates in the parents list. + parents.dedup(); + parents.sort(); + let (id, timestamp) = write_commit( self, - resource, - parents.clone(), - tips, + *resource, + // Commit to tips, extra parents and resource. + tips.iter() + .cloned() + .chain(parents.clone()) + .chain(iter::once(resource)) + .map(git2::Oid::from), message, signature.clone(), tree, @@ -133,7 +142,7 @@ impl change::Storage for git2::Repository { revision: revision.into(), signature, resource, - parents, + parents: tips.into_iter().chain(parents).collect(), manifest, contents, timestamp, @@ -242,31 +251,14 @@ fn load_contents(repo: &git2::Repository, tree: &git2::Tree) -> Result( +fn write_commit( repo: &git2::Repository, - resource: O, - parents: Vec, - tips: Vec, + resource: git2::Oid, + parents: impl IntoIterator, message: String, signature: ExtendedSignature, tree: git2::Tree, -) -> Result<(Oid, Timestamp), error::Create> -where - O: AsRef, -{ - let resource = *resource.as_ref(); - // Add extra parents ensuring there are no duplicates. - let mut parents = parents.iter().map(|o| *o.as_ref()).collect::>(); - parents.sort(); - parents.dedup(); - - let parents = tips - .iter() - .map(|o| *o.as_ref()) - .chain(parents.into_iter()) - .chain(std::iter::once(resource)) - .collect::>(); - +) -> Result<(Oid, Timestamp), error::Create> { let trailers: Vec = vec![trailers::ResourceCommitTrailer::from(resource).into()]; let author = repo.signature()?; let timestamp = author.when().seconds(); diff --git a/radicle-cob/src/change.rs b/radicle-cob/src/change.rs index 1ae1b51b..8458064d 100644 --- a/radicle-cob/src/change.rs +++ b/radicle-cob/src/change.rs @@ -7,7 +7,5 @@ pub use store::{Contents, EntryId, Storage, Template, Timestamp}; use crate::signatures::ExtendedSignature; -/// A single change in the change graph. The layout of changes in the repository -/// is specified in the RFC (docs/rfc/0662-collaborative-objects.adoc) -/// under "Change Commits". +/// A single change in the change graph. pub type Entry = store::Entry; diff --git a/radicle-cob/src/change/store.rs b/radicle-cob/src/change/store.rs index 0cb588f2..43925c74 100644 --- a/radicle-cob/src/change/store.rs +++ b/radicle-cob/src/change/store.rs @@ -59,7 +59,7 @@ pub type Timestamp = u64; /// A unique identifier for a history entry. pub type EntryId = Oid; -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq, Eq)] pub struct Entry { /// The content address of the `Change` itself. pub id: Id, diff --git a/radicle-cob/src/change_graph.rs b/radicle-cob/src/change_graph.rs index a559f924..a1fca829 100644 --- a/radicle-cob/src/change_graph.rs +++ b/radicle-cob/src/change_graph.rs @@ -7,10 +7,18 @@ use git_ext::Oid; use radicle_dag::Dag; use crate::{ - change, object, signatures::ExtendedSignature, CollaborativeObject, Entry, History, ObjectId, - TypeName, + change, object, object::collaboration::Evaluate, signatures::ExtendedSignature, + CollaborativeObject, Entry, EntryId, History, ObjectId, TypeName, }; +#[derive(Debug, thiserror::Error)] +pub enum EvaluateError { + #[error("unable to initialize object: {0}")] + Init(Box), + #[error("invalid signature for entry '{0}'")] + Signature(EntryId), +} + /// The graph of changes for a particular collaborative object pub(super) struct ChangeGraph { object_id: ObjectId, @@ -30,6 +38,7 @@ impl ChangeGraph { S: change::Storage, { log::info!("loading object '{}' '{}'", typename, oid); + let mut builder = GraphBuilder::default(); let mut edges_to_process: Vec<(Oid, Oid)> = Vec::new(); @@ -81,40 +90,45 @@ impl ChangeGraph { /// Given a graph evaluate it to produce a collaborative object. This will /// filter out branches of the graph which do not have valid signatures. - pub(crate) fn evaluate(self) -> CollaborativeObject { + pub(crate) fn evaluate>( + mut self, + store: &S, + ) -> Result, EvaluateError> { let root = *self.object_id; - let root_node = self + let root = self .graph .get(&root) .expect("ChangeGraph::evaluate: root must be part of change graph"); - let manifest = root_node.manifest.clone(); - let graph = self - .graph - .fold(&[root], Dag::new(), |mut graph, _, change| { - // Check the change signatures are valid. - if !change.valid_signatures() { - return ControlFlow::Break(graph); - } - let entry = change.value.clone(); - let id = *entry.id(); - - graph.node(id, entry); - - for k in &change.dependents { - graph.dependency(*k, id); - } - for k in &change.dependencies { - graph.dependency(id, *k); - } - ControlFlow::Continue(graph) - }); - - CollaborativeObject { - manifest, - history: History::new((*root).into(), graph), - id: self.object_id, + if !root.valid_signatures() { + return Err(EvaluateError::Signature(root.id)); } + // Evaluate the root separately, since we can't have a COB without a valid root. + // Then, traverse the graph starting from the root's dependents. + let mut object = + T::init(&root.value, store).map_err(|e| EvaluateError::Init(Box::new(e)))?; + let children = Vec::from_iter(root.dependents.iter().cloned()); + let manifest = root.manifest.clone(); + let root = root.id; + + self.graph.prune(&children, |_, entry| { + // Check the entry signatures are valid. + if !entry.valid_signatures() { + return ControlFlow::Break(()); + } + // Apply the entry to the state, and if there's an error, prune that branch. + if object.apply(entry, store).is_err() { + return ControlFlow::Break(()); + } + ControlFlow::Continue(()) + }); + + Ok(CollaborativeObject { + manifest, + object, + history: History::new(root, self.graph), + id: self.object_id, + }) } /// Get the tips of the collaborative object diff --git a/radicle-cob/src/history.rs b/radicle-cob/src/history.rs index eba86026..3275488f 100644 --- a/radicle-cob/src/history.rs +++ b/radicle-cob/src/history.rs @@ -39,16 +39,6 @@ impl History { Self::new(id, Dag::root(id, root)) } - /// Get the current history timestamp. - /// This is the latest timestamp of any tip. - pub fn timestamp(&self) -> Timestamp { - self.graph - .tips() - .map(|(_, n)| n.timestamp) - .max() - .unwrap_or_default() - } - /// Get all the tips of the graph. pub fn tips(&self) -> BTreeSet { self.graph.tips().map(|(_, entry)| *entry.id()).collect() diff --git a/radicle-cob/src/lib.rs b/radicle-cob/src/lib.rs index 83e8319a..03cc94d0 100644 --- a/radicle-cob/src/lib.rs +++ b/radicle-cob/src/lib.rs @@ -91,7 +91,8 @@ pub use type_name::TypeName; pub mod object; pub use object::{ - create, get, info, list, remove, update, CollaborativeObject, Create, ObjectId, Update, Updated, + create, get, info, list, remove, update, CollaborativeObject, Create, Evaluate, ObjectId, + Update, Updated, }; #[cfg(test)] @@ -116,9 +117,9 @@ mod tests; /// [`git2::Repository`]. It is expected that the underlying storage /// for `object::Storage` will also be `git2::Repository`, but if not /// please open an issue to change the definition of `Store` :) -pub trait Store +pub trait Store where - Self: object::Storage + Self: object::Storage + change::Storage< StoreError = git::change::error::Create, LoadError = git::change::error::Load, diff --git a/radicle-cob/src/object.rs b/radicle-cob/src/object.rs index 3220b8ae..69991338 100644 --- a/radicle-cob/src/object.rs +++ b/radicle-cob/src/object.rs @@ -9,8 +9,8 @@ use thiserror::Error; pub mod collaboration; pub use collaboration::{ - create, get, info, list, parse_refstr, remove, update, CollaborativeObject, Create, Update, - Updated, + create, get, info, list, parse_refstr, remove, update, CollaborativeObject, Create, Evaluate, + Update, Updated, }; pub mod storage; diff --git a/radicle-cob/src/object/collaboration.rs b/radicle-cob/src/object/collaboration.rs index e9edde02..ac00f575 100644 --- a/radicle-cob/src/object/collaboration.rs +++ b/radicle-cob/src/object/collaboration.rs @@ -1,11 +1,12 @@ // Copyright © 2022 The Radicle Link Contributors - -use std::collections::BTreeSet; +use std::convert::Infallible; +use std::fmt::Debug; use git_ext::Oid; +use nonempty::NonEmpty; use crate::change::store::{Manifest, Version}; -use crate::{change, History, ObjectId, TypeName}; +use crate::{change, Entry, History, ObjectId, TypeName}; pub mod error; @@ -28,16 +29,22 @@ pub use update::{update, Update, Updated}; /// A collaborative object #[derive(Debug, Clone, PartialEq, Eq)] -pub struct CollaborativeObject { +pub struct CollaborativeObject { /// The manifest of this object - pub(crate) manifest: Manifest, - /// The CRDT history we know about for this object - pub(crate) history: History, + pub manifest: Manifest, + /// The materialized object resulting from traversing the history. + pub object: T, + /// The history DAG. + pub history: History, /// The id of the object - pub(crate) id: ObjectId, + pub id: ObjectId, } -impl CollaborativeObject { +impl CollaborativeObject { + pub fn object(&self) -> &T { + &self.object + } + pub fn history(&self) -> &History { &self.history } @@ -53,9 +60,30 @@ impl CollaborativeObject { pub fn manifest(&self) -> &Manifest { &self.manifest } +} - fn tips(&self) -> BTreeSet { - self.history.tips().into_iter().map(Oid::from).collect() +/// An object that can be built by evaluating a history. +pub trait Evaluate: Sized + Debug + 'static { + type Error: std::error::Error + Send + Sync + 'static; + + /// Initialize the object with the first (root) history entry. + fn init(entry: &Entry, store: &R) -> Result; + + /// Apply a history entry to the evaluated state. + fn apply(&mut self, entry: &Entry, store: &R) -> Result<(), Self::Error>; +} + +impl Evaluate for NonEmpty { + type Error = Infallible; + + fn init(entry: &Entry, _store: &R) -> Result { + Ok(Self::new(entry.clone())) + } + + fn apply(&mut self, entry: &Entry, _store: &R) -> Result<(), Self::Error> { + self.push(entry.clone()); + + Ok(()) } } diff --git a/radicle-cob/src/object/collaboration/create.rs b/radicle-cob/src/object/collaboration/create.rs index 9f8db102..3ecafe96 100644 --- a/radicle-cob/src/object/collaboration/create.rs +++ b/radicle-cob/src/object/collaboration/create.rs @@ -1,8 +1,10 @@ // Copyright © 2022 The Radicle Link Contributors use nonempty::NonEmpty; +use radicle_crypto::PublicKey; use crate::Embed; +use crate::Evaluate; use crate::Store; use super::*; @@ -51,16 +53,17 @@ impl Create { /// /// The `args` are the metadata for this [`CollaborativeObject`]. See /// [`Create`] for further information. -pub fn create( +pub fn create( storage: &S, signer: &G, resource: Oid, parents: Vec, - identifier: &S::Identifier, + identifier: &PublicKey, args: Create, -) -> Result +) -> Result, error::Create> where - S: Store, + T: Evaluate, + S: Store, G: crypto::Signer, { let type_name = args.type_name.clone(); @@ -69,9 +72,10 @@ where .store(resource, parents, signer, args.template()) .map_err(error::Create::from)?; let object_id = init_change.id().into(); + let object = T::init(&init_change, storage).map_err(error::Create::evaluate)?; storage - .update(identifier, &type_name, &object_id, &init_change.id) + .update(identifier, &type_name, &object_id, &object_id) .map_err(|err| error::Create::Refs { err: Box::new(err) })?; let history = History::new_from_root(init_change); @@ -79,6 +83,7 @@ where Ok(CollaborativeObject { manifest: Manifest::new(type_name, version), history, + object, id: object_id, }) } diff --git a/radicle-cob/src/object/collaboration/error.rs b/radicle-cob/src/object/collaboration/error.rs index 829e1001..795ec261 100644 --- a/radicle-cob/src/object/collaboration/error.rs +++ b/radicle-cob/src/object/collaboration/error.rs @@ -6,8 +6,8 @@ use crate::git; #[derive(Debug, Error)] pub enum Create { - #[error("Invalid automerge history")] - InvalidAutomergeHistory, + #[error(transparent)] + Evaluate(Box), #[error(transparent)] CreateChange(#[from] git::change::error::Create), #[error("failed to updated references for during object creation")] @@ -21,6 +21,12 @@ pub enum Create { SignerIsNotAuthor, } +impl Create { + pub(crate) fn evaluate(err: impl std::error::Error + Send + Sync + 'static) -> Self { + Self::Evaluate(Box::new(err)) + } +} + #[derive(Debug, Error)] #[error("failed to remove object: {err}")] pub struct Remove { @@ -30,6 +36,8 @@ pub struct Remove { #[derive(Debug, Error)] pub enum Retrieve { + #[error("object failed to evaluate: {0}")] + Evaluate(Box), #[error(transparent)] Git(#[from] git2::Error), #[error("failed to get references during object retrieval")] @@ -41,8 +49,16 @@ pub enum Retrieve { Io(#[from] std::io::Error), } +impl Retrieve { + pub(crate) fn evaluate(err: impl std::error::Error + Send + Sync + 'static) -> Self { + Self::Evaluate(Box::new(err)) + } +} + #[derive(Debug, Error)] pub enum Update { + #[error("object failed to evaluate: {0}")] + Evaluate(Box), #[error("no object found")] NoSuchObject, #[error(transparent)] @@ -59,3 +75,9 @@ pub enum Update { #[error("signer must belong to the author")] SignerIsNotAuthor, } + +impl Update { + pub(crate) fn evaluate(err: impl std::error::Error + Send + Sync + 'static) -> Self { + Self::Evaluate(Box::new(err)) + } +} diff --git a/radicle-cob/src/object/collaboration/get.rs b/radicle-cob/src/object/collaboration/get.rs index 2ef1e2c2..281f86c9 100644 --- a/radicle-cob/src/object/collaboration/get.rs +++ b/radicle-cob/src/object/collaboration/get.rs @@ -1,6 +1,6 @@ // Copyright © 2022 The Radicle Link Contributors -use crate::{change_graph::ChangeGraph, CollaborativeObject, ObjectId, Store, TypeName}; +use crate::{change_graph::ChangeGraph, CollaborativeObject, Evaluate, ObjectId, Store, TypeName}; use super::error; @@ -13,16 +13,20 @@ use super::error; /// The `typename` is the type of object to be found, while the /// `object_id` is the identifier for the particular object under that /// type. -pub fn get( +pub fn get( storage: &S, typename: &TypeName, oid: &ObjectId, -) -> Result, error::Retrieve> +) -> Result>, error::Retrieve> where - S: Store, + T: Evaluate, + S: Store, { let tip_refs = storage .objects(typename, oid) .map_err(|err| error::Retrieve::Refs { err: Box::new(err) })?; - Ok(ChangeGraph::load(storage, tip_refs.iter(), typename, oid).map(|graph| graph.evaluate())) + + ChangeGraph::load(storage, tip_refs.iter(), typename, oid) + .map(|graph| graph.evaluate(storage).map_err(error::Retrieve::evaluate)) + .transpose() } diff --git a/radicle-cob/src/object/collaboration/list.rs b/radicle-cob/src/object/collaboration/list.rs index 05bf6625..c5a2b7da 100644 --- a/radicle-cob/src/object/collaboration/list.rs +++ b/radicle-cob/src/object/collaboration/list.rs @@ -1,6 +1,6 @@ // Copyright © 2022 The Radicle Link Contributors -use crate::{change_graph::ChangeGraph, CollaborativeObject, Store, TypeName}; +use crate::{change_graph::ChangeGraph, CollaborativeObject, Evaluate, Store, TypeName}; use super::error; @@ -11,12 +11,13 @@ use super::error; /// [`Store`] for further information. /// /// The `typename` is the type of objects to be listed. -pub fn list( +pub fn list( storage: &S, typename: &TypeName, -) -> Result, error::Retrieve> +) -> Result>, error::Retrieve> where - S: Store, + T: Evaluate, + S: Store, { let references = storage .types(typename) @@ -24,17 +25,20 @@ where log::trace!("loaded {} references", references.len()); let mut result = Vec::new(); for (oid, tip_refs) in references { - log::trace!("loading object '{}'", oid); + log::trace!("loading object '{oid}'"); let loaded = ChangeGraph::load(storage, tip_refs.iter(), typename, &oid) - .map(|graph| graph.evaluate()); + .map(|graph| graph.evaluate(storage).map_err(error::Retrieve::evaluate)); match loaded { - Some(obj) => { - log::trace!("object '{}' found", oid); + Some(Ok(obj)) => { + log::trace!("object '{oid}' found"); result.push(obj); } + Some(Err(e)) => { + log::trace!("object '{oid}' failed to load: {e}") + } None => { - log::trace!("object '{}' not found", oid); + log::trace!("object '{oid}' not found"); } } } diff --git a/radicle-cob/src/object/collaboration/remove.rs b/radicle-cob/src/object/collaboration/remove.rs index 13e8d9fd..9bcbde69 100644 --- a/radicle-cob/src/object/collaboration/remove.rs +++ b/radicle-cob/src/object/collaboration/remove.rs @@ -1,5 +1,7 @@ // Copyright © 2022 The Radicle Link Contributors +use radicle_crypto::PublicKey; + use crate::{ObjectId, Store, TypeName}; use super::error; @@ -13,14 +15,14 @@ use super::error; /// The `typename` is the type of object to be found, while the /// `object_id` is the identifier for the particular object under that /// type. -pub fn remove( +pub fn remove( storage: &S, - identifier: &S::Identifier, + identifier: &PublicKey, typename: &TypeName, oid: &ObjectId, ) -> Result<(), error::Remove> where - S: Store, + S: Store, { storage .remove(identifier, typename, oid) diff --git a/radicle-cob/src/object/collaboration/update.rs b/radicle-cob/src/object/collaboration/update.rs index 6662f956..551fb1a4 100644 --- a/radicle-cob/src/object/collaboration/update.rs +++ b/radicle-cob/src/object/collaboration/update.rs @@ -2,21 +2,22 @@ use git_ext::Oid; use nonempty::NonEmpty; +use radicle_crypto::PublicKey; use crate::{ - change, change_graph::ChangeGraph, history::EntryId, CollaborativeObject, Embed, ObjectId, - Store, TypeName, + change, change_graph::ChangeGraph, history::EntryId, CollaborativeObject, Embed, Evaluate, + ObjectId, Store, TypeName, }; use super::error; /// Result of an `update` operation. #[derive(Debug)] -pub struct Updated { +pub struct Updated { /// The new head commit of the DAG. pub head: Oid, /// The newly updated collaborative object. - pub object: CollaborativeObject, + pub object: CollaborativeObject, /// Entry parents. pub parents: Vec, } @@ -54,16 +55,17 @@ pub struct Update { /// /// The `args` are the metadata for this [`CollaborativeObject`] /// udpate. See [`Update`] for further information. -pub fn update( +pub fn update( storage: &S, signer: &G, resource: Oid, parents: Vec, - identifier: &S::Identifier, + identifier: &PublicKey, args: Update, -) -> Result +) -> Result, error::Update> where - S: Store, + T: Evaluate, + S: Store, G: crypto::Signer, { let Update { @@ -78,32 +80,42 @@ where .objects(typename, &object_id) .map_err(|err| error::Update::Refs { err: Box::new(err) })?; - let mut object = ChangeGraph::load(storage, existing_refs.iter(), typename, &object_id) - .map(|graph| graph.evaluate()) + let graph = ChangeGraph::load(storage, existing_refs.iter(), typename, &object_id) .ok_or(error::Update::NoSuchObject)?; + let mut object: CollaborativeObject = + graph.evaluate(storage).map_err(error::Update::evaluate)?; - let change = storage.store( + // Create a commit for this change, but don't update any references yet. + let entry = storage.store( resource, parents, signer, change::Template { - tips: object.tips().iter().cloned().collect(), + tips: object.history.tips().into_iter().collect(), embeds, contents: changes, type_name: typename.clone(), message, }, )?; + let head = entry.id; + let parents = entry.parents.to_vec(); + // Try to apply this change to our object. This prevents storing invalid updates. + // Note that if this returns with an error, we are left with an unreachable + // commit object created above. This is fine, as it will eventually get + // garbage-collected by Git. + object + .object + .apply(&entry, storage) + .map_err(error::Update::evaluate)?; + object.history.extend(entry); + + // Here we actually update the references to point to the new update. storage - .update(identifier, typename, &object_id, &change.id) + .update(identifier, typename, &object_id, &head) .map_err(|err| error::Update::Refs { err: Box::new(err) })?; - let parents = change.parents.to_vec(); - let head = change.id; - - object.history.extend(change); - Ok(Updated { object, head, diff --git a/radicle-cob/src/object/storage.rs b/radicle-cob/src/object/storage.rs index 9a6dfe38..c8a11928 100644 --- a/radicle-cob/src/object/storage.rs +++ b/radicle-cob/src/object/storage.rs @@ -4,6 +4,7 @@ use std::{collections::BTreeMap, error::Error}; use git_ext::ref_format::RefString; use git_ext::Oid; +use radicle_crypto::PublicKey; use crate::change::EntryId; use crate::{ObjectId, TypeName}; @@ -58,8 +59,6 @@ pub trait Storage { type UpdateError: Error + Send + Sync + 'static; type RemoveError: Error + Send + Sync + 'static; - type Identifier; - /// Get all references which point to a head of the change graph for a /// particular object fn objects( @@ -75,7 +74,7 @@ pub trait Storage { /// Update a ref to a particular collaborative object fn update( &self, - identifier: &Self::Identifier, + identifier: &PublicKey, typename: &TypeName, object_id: &ObjectId, entry: &EntryId, @@ -84,7 +83,7 @@ pub trait Storage { /// Remove a ref to a particular collaborative object fn remove( &self, - identifier: &Self::Identifier, + identifier: &PublicKey, typename: &TypeName, object_id: &ObjectId, ) -> Result<(), Self::RemoveError>; diff --git a/radicle-cob/src/test/storage.rs b/radicle-cob/src/test/storage.rs index 32c3b4fd..abab9a9d 100644 --- a/radicle-cob/src/test/storage.rs +++ b/radicle-cob/src/test/storage.rs @@ -1,5 +1,6 @@ use std::{collections::BTreeMap, convert::TryFrom as _}; +use radicle_crypto::PublicKey; use tempfile::TempDir; use crate::{ @@ -8,8 +9,6 @@ use crate::{ ObjectId, Store, }; -use super::identity::Urn; - pub mod error { use thiserror::Error; @@ -56,7 +55,7 @@ impl Storage { } } -impl Store for Storage {} +impl Store for Storage {} impl change::Storage for Storage { type StoreError = ::StoreError; @@ -106,8 +105,6 @@ impl object::Storage for Storage { type UpdateError = git2::Error; type RemoveError = git2::Error; - type Identifier = Urn; - fn objects( &self, typename: &crate::TypeName, @@ -151,17 +148,12 @@ impl object::Storage for Storage { fn update( &self, - identifier: &Self::Identifier, + identifier: &PublicKey, typename: &crate::TypeName, object_id: &ObjectId, entry: &change::EntryId, ) -> Result<(), Self::UpdateError> { - let name = format!( - "refs/rad/{}/cobs/{}/{}", - identifier.to_path(), - typename, - object_id - ); + let name = format!("refs/rad/{}/cobs/{}/{}", identifier, typename, object_id); self.raw .reference(&name, (*entry).into(), true, "new change")?; Ok(()) @@ -169,16 +161,11 @@ impl object::Storage for Storage { fn remove( &self, - identifier: &Self::Identifier, + identifier: &PublicKey, typename: &crate::TypeName, object_id: &ObjectId, ) -> Result<(), Self::RemoveError> { - let name = format!( - "refs/rad/{}/cobs/{}/{}", - identifier.to_path(), - typename, - object_id - ); + let name = format!("refs/rad/{}/cobs/{}/{}", identifier, typename, object_id); self.raw.find_reference(&name)?.delete()?; Ok(()) diff --git a/radicle-cob/src/tests.rs b/radicle-cob/src/tests.rs index 1a8d1908..57b4baf8 100644 --- a/radicle-cob/src/tests.rs +++ b/radicle-cob/src/tests.rs @@ -1,13 +1,13 @@ use std::ops::ControlFlow; use crypto::test::signer::MockSigner; +use crypto::{PublicKey, Signer}; use git_ext::ref_format::{refname, Component, RefString}; -use nonempty::nonempty; +use nonempty::{nonempty, NonEmpty}; use qcheck::Arbitrary; -use radicle_crypto::Signer; use crate::{ - create, get, list, object, test::arbitrary::Invalid, update, Create, ObjectId, TypeName, + create, get, list, object, test::arbitrary::Invalid, update, Create, Entry, ObjectId, TypeName, Update, Updated, Version, }; @@ -24,12 +24,12 @@ fn roundtrip() { person: terry, }; let typename = "xyz.rad.issue".parse::().unwrap(); - let cob = create( + let cob = create::, _, _>( &storage, &signer, proj.project.content_id, vec![], - &proj.identifier(), + signer.public_key(), Create { contents: nonempty!(Vec::new()), type_name: typename.clone(), @@ -58,12 +58,12 @@ fn list_cobs() { person: terry, }; let typename = "xyz.rad.issue".parse::().unwrap(); - let issue_1 = create( + let issue_1 = create::, _, _>( &storage, &signer, proj.project.content_id, vec![], - &proj.identifier(), + signer.public_key(), Create { contents: nonempty!(b"issue 1".to_vec()), type_name: typename.clone(), @@ -79,7 +79,7 @@ fn list_cobs() { &signer, proj.project.content_id, vec![], - &proj.identifier(), + signer.public_key(), Create { contents: nonempty!(b"issue 2".to_vec()), type_name: typename.clone(), @@ -110,12 +110,12 @@ fn update_cob() { person: terry, }; let typename = "xyz.rad.issue".parse::().unwrap(); - let cob = create( + let cob = create::, _, _>( &storage, &signer, proj.project.content_id, vec![], - &proj.identifier(), + signer.public_key(), Create { contents: nonempty!(Vec::new()), type_name: typename.clone(), @@ -126,7 +126,7 @@ fn update_cob() { ) .unwrap(); - let not_expected = get(&storage, &typename, cob.id()) + let not_expected = get::, _>(&storage, &typename, cob.id()) .unwrap() .expect("BUG: cob was missing"); @@ -135,7 +135,7 @@ fn update_cob() { &signer, proj.project.content_id, vec![], - &proj.identifier(), + signer.public_key(), Update { changes: nonempty!(b"issue 1".to_vec()), object_id: *cob.id(), @@ -151,7 +151,7 @@ fn update_cob() { .expect("BUG: cob was missing"); assert_ne!(object, not_expected); - assert_eq!(object, expected); + assert_eq!(object, expected, "{object:#?} {expected:#?}"); } #[test] @@ -171,12 +171,12 @@ fn traverse_cobs() { person: neil, }; let typename = "xyz.rad.issue".parse::().unwrap(); - let cob = create( + let cob = create::, _, _>( &storage, &terry_signer, terry_proj.project.content_id, vec![], - &terry_proj.identifier(), + terry_signer.public_key(), Create { contents: nonempty!(b"issue 1".to_vec()), type_name: typename.clone(), @@ -188,19 +188,19 @@ fn traverse_cobs() { .unwrap(); copy_to( storage.as_raw(), - &terry_proj, + terry_signer.public_key(), &neil_proj, &typename, *cob.id(), ) .unwrap(); - let Updated { object, .. } = update( + let Updated { object, .. } = update::, _, _>( &storage, &neil_signer, neil_proj.project.content_id, vec![], - &neil_proj.identifier(), + neil_signer.public_key(), Update { changes: nonempty!(b"issue 2".to_vec()), object_id: *cob.id(), @@ -306,18 +306,13 @@ fn gen(size: usize) -> T { fn copy_to( repo: &git2::Repository, - from: &test::RemoteProject, + from: &PublicKey, to: &test::RemoteProject, typename: &TypeName, object: ObjectId, ) -> Result<(), git2::Error> { let original = { - let name = format!( - "refs/rad/{}/cobs/{}/{}", - from.identifier().to_path(), - typename, - object - ); + let name = format!("refs/rad/{}/cobs/{}/{}", from, typename, object); let r = repo.find_reference(&name)?; r.target().unwrap() }; diff --git a/radicle-dag/src/lib.rs b/radicle-dag/src/lib.rs index 7cf0f9a5..f0100c2d 100644 --- a/radicle-dag/src/lib.rs +++ b/radicle-dag/src/lib.rs @@ -186,6 +186,37 @@ impl Dag { /// Fold over the graph in topological order, pruning branches along the way. /// /// To continue traversing a branch, return [`ControlFlow::Continue`] from the + /// filter function. To stop traversal of a branch and prune it, + /// return [`ControlFlow::Break`]. + pub fn prune(&mut self, roots: &[K], mut filter: F) + where + F: for<'r> FnMut(&'r K, &'r Node) -> ControlFlow<()>, + { + let mut visited = BTreeSet::new(); + let mut queue = VecDeque::::from_iter(roots.iter().cloned()); + + while let Some(next) = queue.pop_front() { + if !visited.insert(next) { + continue; + } + if let Some(node) = self.graph.get(&next) { + match filter(&next, node) { + ControlFlow::Continue(()) => { + queue.extend(node.dependents.iter().cloned()); + } + ControlFlow::Break(()) => { + // When pruning a node, we remove all transitive dependents on + // that node. + self.remove(&next); + } + } + } + } + } + + /// Fold over the graph in topological order, skipping certain branches. + /// + /// To continue traversing a branch, return [`ControlFlow::Continue`] from the /// filter function. To stop traversal of a branch, return [`ControlFlow::Break`]. pub fn fold(&self, roots: &[K], mut acc: A, mut filter: F) -> A where @@ -208,6 +239,7 @@ impl Dag { // When filtering out a node, we filter out all transitive dependents on // that node by adding them to the already visited list. visited.extend(self.descendants_of(node)); + acc = a; } } @@ -216,6 +248,30 @@ impl Dag { acc } + /// Remove a node from the graph, and all its dependents. + pub fn remove(&mut self, key: &K) -> Option> { + if let Some(node) = self.graph.remove(key) { + self.tips.remove(key); + self.roots.remove(key); + + for k in &node.dependencies { + if let Some(dependency) = self.graph.get_mut(k) { + dependency.dependents.remove(key); + + if dependency.dependents.is_empty() { + self.tips.insert(*k); + } + } + } + for k in &node.dependents { + self.remove(k); + } + Some(node) + } else { + None + } + } + fn descendants_of(&self, from: &Node) -> Vec { let mut visited = BTreeSet::new(); let mut stack = VecDeque::new(); @@ -535,4 +591,78 @@ mod tests { }); assert_eq!(acc, vec!["R", "A2"]); } + + #[test] + fn test_remove() { + let mut dag = Dag::new(); + + dag.node("R", ()); + dag.node("A1", ()); + dag.node("A2", ()); + dag.node("A3", ()); + dag.node("B1", ()); + dag.node("C1", ()); + dag.node("D1", ()); + + dag.dependency("A1", "R"); + dag.dependency("A2", "R"); + dag.dependency("A3", "A2"); + dag.dependency("B1", "A1"); + dag.dependency("B1", "A2"); + dag.dependency("C1", "B1"); + dag.dependency("C1", "A3"); + dag.dependency("D1", "C1"); + dag.dependency("D1", "A2"); + + dag.remove(&"C1"); + assert!(dag.get(&"C1").is_none()); + assert!(dag.get(&"D1").is_none()); + assert!(!dag.tips.contains(&"D1")); + assert_eq!(dag.tips.iter().collect::>(), vec![&"A3", &"B1"]); + + dag.remove(&"A3"); + assert_eq!(dag.tips.iter().collect::>(), vec![&"B1"]); + + dag.remove(&"A1"); + assert!(dag.get(&"A1").is_none()); + assert!(dag.get(&"B1").is_none()); + assert!(dag.get(&"A2").is_some()); + assert_eq!(dag.tips.iter().collect::>(), vec![&"A2"]); + + dag.remove(&"R"); + assert!(dag.is_empty()); + assert!(dag.tips.is_empty()); + assert!(dag.roots.is_empty()); + } + + #[test] + fn test_prune() { + let mut dag = Dag::new(); + + dag.node("R", ()); + dag.node("A1", ()); + dag.node("A2", ()); + dag.node("B1", ()); + dag.node("C1", ()); + dag.node("D1", ()); + + dag.dependency("A1", "R"); + dag.dependency("A2", "R"); + dag.dependency("B1", "A1"); + dag.dependency("C1", "B1"); + dag.dependency("D1", "C1"); + dag.dependency("D1", "A2"); + + let a1 = dag.get(&"A1").unwrap(); + assert_eq!(dag.descendants_of(a1), vec!["B1", "C1", "D1"]); + + dag.prune(&["R"], |key, _| { + if key == &"B1" { + ControlFlow::Break(()) + } else { + ControlFlow::Continue(()) + } + }); + assert_eq!(dag.sorted(|a, b| a.cmp(b)), vec!["R", "A1", "A2"]); + } } diff --git a/radicle-remote-helper/src/list.rs b/radicle-remote-helper/src/list.rs index 540657a2..992a7699 100644 --- a/radicle-remote-helper/src/list.rs +++ b/radicle-remote-helper/src/list.rs @@ -23,7 +23,10 @@ pub enum Error { } /// List refs for fetching (`git fetch` and `git ls-remote`). -pub fn for_fetch(url: &Url, stored: &R) -> Result<(), Error> { +pub fn for_fetch( + url: &Url, + stored: &R, +) -> Result<(), Error> { if let Some(namespace) = url.namespace { // Listing namespaced refs. for (name, oid) in stored.references_of(&namespace)? { @@ -68,7 +71,7 @@ pub fn for_push(profile: &Profile, stored: &R) -> Result<(), } /// List canonical patch references. These are magic refs that can be used to pull patch updates. -fn patch_refs(stored: &R) -> Result<(), Error> { +fn patch_refs(stored: &R) -> Result<(), Error> { let patches = radicle::cob::patch::Patches::open(stored)?; for patch in patches.all()? { let Ok((id, patch)) = patch else { diff --git a/radicle/src/cob.rs b/radicle/src/cob.rs index fbd8ec04..cbb9ce68 100644 --- a/radicle/src/cob.rs +++ b/radicle/src/cob.rs @@ -1,3 +1,4 @@ +#![warn(clippy::unwrap_used)] pub mod common; pub mod identity; pub mod issue; @@ -11,7 +12,8 @@ pub mod test; pub use cob::{ change, history::EntryId, object, object::collaboration::error, CollaborativeObject, Contents, - Create, Embed, Entry, History, Manifest, ObjectId, Store, TypeName, Update, Updated, Version, + Create, Embed, Entry, Evaluate, History, Manifest, ObjectId, Store, TypeName, Update, Updated, + Version, }; pub use cob::{create, get, list, remove, update}; pub use common::*; diff --git a/radicle/src/cob/common.rs b/radicle/src/cob/common.rs index 8de83e17..5ce0e04b 100644 --- a/radicle/src/cob/common.rs +++ b/radicle/src/cob/common.rs @@ -304,6 +304,7 @@ impl From for Authorization { } #[cfg(test)] +#[allow(clippy::unwrap_used)] mod test { use super::*; diff --git a/radicle/src/cob/identity.rs b/radicle/src/cob/identity.rs index ae73bc44..98ac90bd 100644 --- a/radicle/src/cob/identity.rs +++ b/radicle/src/cob/identity.rs @@ -11,8 +11,8 @@ use thiserror::Error; use crate::{ cob::{ - self, - store::{self, FromHistory as _, HistoryAction, Transaction}, + self, op, + store::{self, Cob, CobAction, Transaction}, Reaction, Timestamp, }, identity::{doc::DocError, Did, Identity, IdentityError}, @@ -102,7 +102,7 @@ pub enum Action { }, } -impl HistoryAction for Action {} +impl CobAction for Action {} /// Error applying an operation onto a state. #[derive(Error, Debug)] @@ -160,6 +160,8 @@ pub enum Error { Apply(#[from] ApplyError), #[error("store: {0}")] Store(#[from] store::Error), + #[error("op decoding failed: {0}")] + Op(#[from] op::OpEncodingError), } /// Propose a new [`Doc`] for an [`Identity`]. The proposal can be @@ -315,7 +317,7 @@ impl Proposal { } } -impl store::FromHistory for Proposal { +impl store::Cob for Proposal { type Action = Action; type Error = ApplyError; @@ -323,13 +325,13 @@ impl store::FromHistory for Proposal { &TYPENAME } - fn init(op: Op, repo: &R) -> Result { + fn from_root(op: Op, repo: &R) -> Result { let mut identity = Self::default(); - identity.apply(op, repo)?; + identity.op(op, repo)?; Ok(identity) } - fn apply(&mut self, op: Op, _repo: &R) -> Result<(), Self::Error> { + fn op(&mut self, op: Op, _repo: &R) -> Result<(), ApplyError> { let id = op.id; let author = Author::new(op.author); let timestamp = op.timestamp; @@ -438,6 +440,23 @@ impl store::FromHistory for Proposal { } } +impl cob::Evaluate for Proposal { + type Error = Error; + + fn init(entry: &cob::Entry, repo: &R) -> Result { + let op = Op::try_from(entry)?; + let object = Proposal::from_root(op, repo)?; + + Ok(object) + } + + fn apply(&mut self, entry: &cob::Entry, repo: &R) -> Result<(), Self::Error> { + let op = Op::try_from(entry)?; + + self.op(op, repo).map_err(Error::Apply) + } +} + mod lookup { use super::*; @@ -550,7 +569,7 @@ impl Revision { } } -impl store::Transaction { +impl store::Transaction { pub fn accept( &mut self, revision: RevisionId, @@ -640,13 +659,13 @@ where ) -> Result where G: Signer, - F: FnOnce(&mut Transaction) -> Result<(), store::Error>, + F: FnOnce(&mut Transaction) -> Result<(), store::Error>, { - let mut tx = Transaction::new(*signer.public_key()); + let mut tx = Transaction::default(); operations(&mut tx)?; - let (ops, commit) = tx.commit(message, self.id, &mut self.store.raw, signer)?; - self.proposal.apply(ops, self.store.as_ref())?; + let (proposal, commit) = tx.commit(message, self.id, &mut self.store.raw, signer)?; + self.proposal = proposal; Ok(commit) } diff --git a/radicle/src/cob/issue.rs b/radicle/src/cob/issue.rs index b9ef0f36..62884968 100644 --- a/radicle/src/cob/issue.rs +++ b/radicle/src/cob/issue.rs @@ -9,10 +9,10 @@ use thiserror::Error; use crate::cob; use crate::cob::common::{Author, Authorization, Label, Reaction, Timestamp, Uri}; use crate::cob::store::Transaction; -use crate::cob::store::{FromHistory as _, HistoryAction}; +use crate::cob::store::{Cob, CobAction}; use crate::cob::thread; use crate::cob::thread::{Comment, CommentId, Thread}; -use crate::cob::{store, ActorId, Embed, EntryId, ObjectId, TypeName}; +use crate::cob::{op, store, ActorId, Embed, EntryId, ObjectId, TypeName}; use crate::crypto::Signer; use crate::git; use crate::identity::doc::{Doc, DocError}; @@ -35,20 +35,22 @@ pub enum Error { /// Error loading the identity document. #[error("identity doc failed to load: {0}")] Doc(#[from] DocError), - #[error("description missing")] - DescriptionMissing, #[error("thread apply failed: {0}")] Thread(#[from] thread::Error), #[error("store: {0}")] Store(#[from] store::Error), - #[error("history: {0}")] - History(Box), /// Action not authorized. #[error("{0} not authorized to apply {1:?}")] NotAuthorized(ActorId, Action), + /// Action not allowed. + #[error("action is not allowed: {0}")] + NotAllowed(EntryId), /// General error initializing an issue. #[error("initialization failed: {0}")] Init(&'static str), + /// Error decoding an operation. + #[error("op decoding failed: {0}")] + Op(#[from] op::OpEncodingError), } /// Reason why an issue was closed. @@ -113,7 +115,7 @@ pub struct Issue { pub(super) thread: Thread, } -impl store::FromHistory for Issue { +impl store::Cob for Issue { type Action = Action; type Error = Error; @@ -121,7 +123,7 @@ impl store::FromHistory for Issue { &TYPENAME } - fn init(op: Op, repo: &R) -> Result { + fn from_root(op: Op, repo: &R) -> Result { let mut actions = op.actions.into_iter(); let Some(Action::Comment { body, reply_to: None, embeds }) = actions.next() else { return Err(Error::Init("the first action must be of type `comment`")); @@ -136,10 +138,10 @@ impl store::FromHistory for Issue { Ok(issue) } - fn apply(&mut self, op: Op, repo: &R) -> Result<(), Error> { + fn op(&mut self, op: Op, repo: &R) -> Result<(), Error> { let doc = repo.identity_doc_at(op.identity)?.verified()?; for action in op.actions { - match self.authorization(&action, &op.author, &doc) { + match self.authorization(&action, &op.author, &doc)? { Authorization::Allow => { self.action(action, op.id, op.author, op.timestamp, op.identity, repo)?; } @@ -155,6 +157,23 @@ impl store::FromHistory for Issue { } } +impl cob::Evaluate for Issue { + type Error = Error; + + fn init(entry: &cob::Entry, repo: &R) -> Result { + let op = Op::try_from(entry)?; + let object = Issue::from_root(op, repo)?; + + Ok(object) + } + + fn apply(&mut self, entry: &cob::Entry, repo: &R) -> Result<(), Self::Error> { + let op = Op::try_from(entry)?; + + self.op(op, repo) + } +} + impl Issue { /// Construct a new issue. pub fn new(thread: Thread) -> Self { @@ -222,14 +241,13 @@ impl Issue { action: &Action, actor: &ActorId, doc: &Doc, - ) -> Authorization { + ) -> Result { if doc.is_delegate(actor) { // A delegate is authorized to do all actions. - return Authorization::Allow; + return Ok(Authorization::Allow); } let author: ActorId = *self.author().id().as_key(); - - match action { + let outcome = match action { // Only delegate can assign someone to an issue. Action::Assign { .. } => Authorization::Deny, // Issue authors can edit their own issues. @@ -245,15 +263,20 @@ impl Issue { Action::Comment { .. } => Authorization::Allow, // All roles can edit or redact their own comments. Action::CommentEdit { id, .. } | Action::CommentRedact { id, .. } => { - if let Some(comment) = self.comment(id) { - Authorization::from(*actor == comment.author()) + if let Some(comment) = self.thread.comments.get(id) { + if let Some(comment) = comment { + Authorization::from(*actor == comment.author()) + } else { + Authorization::Unknown + } } else { - Authorization::Unknown + return Err(Error::Thread(thread::Error::Missing(*id))); } } // All roles can react to a comment on an issue. Action::CommentReact { .. } => Authorization::Allow, - } + }; + Ok(outcome) } } @@ -301,6 +324,10 @@ impl Issue { thread::edit(&mut self.thread, entry, id, timestamp, body, embeds)?; } Action::CommentRedact { id } => { + let (root, _) = self.root(); + if id == *root { + return Err(Error::NotAllowed(entry)); + } thread::redact(&mut self.thread, entry, id)?; } Action::CommentReact { @@ -323,7 +350,7 @@ impl Deref for Issue { } } -impl store::Transaction { +impl store::Transaction { /// Assign DIDs to the issue. pub fn assign(&mut self, assignees: impl IntoIterator) -> Result<(), store::Error> { self.push(Action::Assign { @@ -355,6 +382,11 @@ impl store::Transaction { }) } + /// Redact a comment. + pub fn redact_comment(&mut self, id: CommentId) -> Result<(), store::Error> { + self.push(Action::CommentRedact { id }) + } + /// Lifecycle an issue. pub fn lifecycle(&mut self, state: State) -> Result<(), store::Error> { self.push(Action::Lifecycle { state }) @@ -473,9 +505,7 @@ where embeds: impl IntoIterator, signer: &G, ) -> Result { - let Some((id, _)) = self.thread.comments().next() else { - return Err(Error::DescriptionMissing); - }; + let (id, _) = self.root(); let id = *id; self.transaction("Edit description", signer, |tx| { tx.edit_comment(id, description, embeds.into_iter().collect()) @@ -495,15 +525,20 @@ where embeds: impl IntoIterator, signer: &G, ) -> Result { - assert!( - self.thread.comment(&reply_to).is_some(), - "Comment {reply_to} not found" - ); self.transaction("Comment", signer, |tx| { tx.comment(body, reply_to, embeds.into_iter().collect()) }) } + /// Redact a comment. + pub fn redact_comment( + &mut self, + id: CommentId, + signer: &G, + ) -> Result { + self.transaction("Redact comment", signer, |tx| tx.redact_comment(id)) + } + /// Label an issue. pub fn label( &mut self, @@ -532,13 +567,13 @@ where ) -> Result where G: Signer, - F: FnOnce(&mut Transaction) -> Result<(), store::Error>, + F: FnOnce(&mut Transaction) -> Result<(), store::Error>, { - let mut tx = Transaction::new(*signer.public_key()); + let mut tx = Transaction::default(); operations(&mut tx)?; - let (ops, commit) = tx.commit(message, self.id, &mut self.store.raw, signer)?; - self.issue.apply(ops, self.store.as_ref())?; + let (issue, commit) = tx.commit(message, self.id, &mut self.store.raw, signer)?; + self.issue = issue; Ok(commit) } @@ -710,9 +745,10 @@ pub enum Action { }, } -impl HistoryAction for Action {} +impl CobAction for Action {} #[cfg(test)] +#[allow(clippy::unwrap_used)] mod test { use pretty_assertions::assert_eq; @@ -1186,6 +1222,38 @@ mod test { assert_eq!(c2.author(), author); } + #[test] + fn test_issue_comment_redact() { + let test::setup::NodeWithRepo { node, repo, .. } = test::setup::NodeWithRepo::default(); + let mut issues = Issues::open(&*repo).unwrap(); + let mut issue = issues + .create( + "My first issue", + "Blah blah blah.", + &[], + &[], + [], + &node.signer, + ) + .unwrap(); + + // The root thread op id is always the same. + let (c0, _) = issue.root(); + let c0 = *c0; + + let comment = issue + .comment("Ho ho ho.", c0, vec![], &node.signer) + .unwrap(); + issue.reload().unwrap(); + assert_eq!(issue.comments().count(), 2); + + issue.redact_comment(comment, &node.signer).unwrap(); + assert_eq!(issue.comments().count(), 1); + + // Can't redact root comment. + issue.redact_comment(*issue.id, &node.signer).unwrap_err(); + } + #[test] fn test_issue_state_serde() { assert_eq!( @@ -1358,4 +1426,175 @@ mod test { assert_eq!(e1.content, Uri::from(embed1_edited.oid())); assert_eq!(b1.content(), &embed1_edited.content); } + + #[test] + fn test_invalid_actions() { + let test::setup::NodeWithRepo { node, repo, .. } = test::setup::NodeWithRepo::default(); + let mut issues = Issues::open(&*repo).unwrap(); + let mut issue = issues + .create( + "My first issue", + "Blah blah blah.", + &[], + &[], + [], + &node.signer, + ) + .unwrap(); + let missing = arbitrary::oid(); + + issue + .comment("Invalid", missing, [], &node.signer) + .unwrap_err(); + assert_eq!(issue.comments().count(), 1); + issue.reload().unwrap(); + assert_eq!(issue.comments().count(), 1); + + let cob = cob::get::(&*repo, Issue::type_name(), issue.id()) + .unwrap() + .unwrap(); + + assert_eq!(cob.history().len(), 1); + assert_eq!( + cob.history().tips().into_iter().collect::>(), + vec![*issue.id] + ); + } + + #[test] + fn test_invalid_tx() { + let test::setup::NodeWithRepo { node, repo, .. } = test::setup::NodeWithRepo::default(); + let mut issues = Issues::open(&*repo).unwrap(); + let mut issue = issues + .create( + "My first issue", + "Blah blah blah.", + &[], + &[], + [], + &node.signer, + ) + .unwrap(); + let missing = arbitrary::oid(); + + // An invalid comment which points to a missing parent. + // Even creating it via a transaction will trigger an error. + let mut tx = Transaction::::default(); + tx.comment("Invalid comment", missing, vec![]).unwrap(); + tx.commit("Add comment", issue.id, &mut issue.store.raw, &node.signer) + .unwrap_err(); + + issue.reload().unwrap(); + assert_eq!(issue.comments().count(), 1); + } + + #[test] + fn test_invalid_cob() { + use crate::crypto::test::signer::MockSigner; + use cob::change::Storage as _; + use cob::object::Storage as _; + use nonempty::NonEmpty; + + let test::setup::NodeWithRepo { node, repo, .. } = test::setup::NodeWithRepo::default(); + let eve = MockSigner::default(); + let identity = repo.identity().unwrap().head; + let missing = arbitrary::oid(); + let type_name = Issue::type_name().clone(); + let mut issues = Issues::open(&*repo).unwrap(); + let mut issue = issues + .create( + "My first issue", + "Blah blah blah.", + &[], + &[], + [], + &node.signer, + ) + .unwrap(); + + // Initially, there is one node in the DAG. + let cob = cob::get::, _>(&*repo, &type_name, issue.id()) + .unwrap() + .unwrap(); + + assert_eq!(cob.history.len(), 1); + assert_eq!(cob.object.len(), 1); + + // We have a valid issue. Now we're going to add an invalid action to it, by bypassing + // the COB API. We do this using a different key, so that valid actions by + // our issue author don't overwrite the invalid action, since there is + // only one ref per COB per user. + let action = Action::CommentRedact { id: missing }; + let action = cob::store::encoding::encode(action).unwrap(); + let contents = NonEmpty::new(action); + let invalid = repo + .store( + identity, + vec![], + &eve, + cob::change::Template { + tips: vec![*issue.id], + embeds: vec![], + contents: contents.clone(), + type_name: type_name.clone(), + message: String::from("Add invalid operation"), + }, + ) + .unwrap(); + + repo.update(eve.public_key(), &type_name, &issue.id, &invalid.id) + .unwrap(); + + // If we fetch the COB with its history, *without* trying to interpret it as an issue, + // we'll see that all entries, including the invalid one are there. + let cob = cob::get::, _>(&*repo, &type_name, issue.id()) + .unwrap() + .unwrap(); + + assert_eq!(cob.history.len(), 2); + assert_eq!(cob.object.len(), 2); + assert_eq!(cob.object.last().contents(), &contents); + + // However, if we try to fetch it as an *issue*, the invalid comment is pruned. + let cob = cob::get::(&*repo, &type_name, issue.id()) + .unwrap() + .unwrap(); + assert_eq!(cob.history.len(), 1); + assert_eq!(cob.object.comments().count(), 1); + assert!(cob.object.comment(&issue.id).is_some()); + + // Additionally, when adding a *valid* comment, it does not build upon the bad operation. + issue.reload().unwrap(); + issue + .comment("Valid comment", *issue.id, vec![], &node.signer) + .unwrap(); + issue.reload().unwrap(); + assert_eq!(issue.comments().count(), 2); + assert_eq!(issue.thread.timeline().count(), 2); + assert_eq!(issue.comments().last().unwrap().1.body(), "Valid comment"); + + // The actual DAG contains 3 nodes, but only 2 were loaded as an issue. + let cob = cob::get::, _>(&*repo, &type_name, issue.id()) + .unwrap() + .unwrap(); + + assert_eq!(cob.history.len(), 3); + assert_eq!(cob.object.len(), 3); + + // If Eve now writes a valid comment via the `Issue` type, it will overwrite her invalid + // one, since it won't be loaded as a tip. + issue + .comment("Eve's comment", *issue.id, vec![], &eve) + .unwrap(); + + let cob = cob::get::, _>(&*repo, &type_name, issue.id()) + .unwrap() + .unwrap(); + + // There are three nodes still, but they are all valid comments. + // The invalid comment of Eve was replaced with a valid one. + assert_eq!(issue.comments().count(), 3); + assert_eq!(cob.history.len(), 3); + assert_eq!(cob.object.len(), 3); + } } diff --git a/radicle/src/cob/patch.rs b/radicle/src/cob/patch.rs index ed1f778e..b0e0d82d 100644 --- a/radicle/src/cob/patch.rs +++ b/radicle/src/cob/patch.rs @@ -15,11 +15,11 @@ use thiserror::Error; use crate::cob; use crate::cob::common::{Author, Authorization, Label, Reaction, Timestamp}; use crate::cob::store::Transaction; -use crate::cob::store::{FromHistory as _, HistoryAction}; +use crate::cob::store::{Cob, CobAction}; use crate::cob::thread; use crate::cob::thread::Thread; use crate::cob::thread::{Comment, CommentId, Reactions}; -use crate::cob::{store, ActorId, EntryId, ObjectId, TypeName}; +use crate::cob::{op, store, ActorId, EntryId, ObjectId, TypeName}; use crate::crypto::{PublicKey, Signer}; use crate::git; use crate::identity; @@ -84,9 +84,6 @@ pub type RevisionIx = usize; /// Error applying an operation onto a state. #[derive(Debug, Error)] pub enum Error { - /// Error trying to delete the protected root revision. - #[error("refusing to delete root revision: {0}")] - RootRevision(RevisionId), /// Causal dependency missing. /// /// This error indicates that the operations are not being applied @@ -111,11 +108,14 @@ pub enum Error { /// Store error. #[error("store: {0}")] Store(#[from] store::Error), - #[error("history error: {0}")] - History(Box), + #[error("op decoding failed: {0}")] + Op(#[from] op::OpEncodingError), /// Action not authorized by the author #[error("{0} not authorized to apply {1:?}")] NotAuthorized(ActorId, Action), + /// An illegal action. + #[error("action is not allowed: {0}")] + NotAllowed(EntryId), /// Initialization failed. #[error("initialization failed: {0}")] Init(&'static str), @@ -264,7 +264,7 @@ pub enum Action { }, } -impl HistoryAction for Action { +impl CobAction for Action { fn parents(&self) -> Vec { match self { Self::Revision { base, oid, .. } => { @@ -376,24 +376,21 @@ pub struct Patch { } impl Patch { - /// Create a valid patch - pub fn new( - title: String, - target: MergeTarget, - (revision_id, revision): (RevisionId, Revision), - ) -> Self { + /// Construct a new patch object from a revision. + pub fn new(title: String, target: MergeTarget, (id, revision): (RevisionId, Revision)) -> Self { Self { title, state: State::default(), target, labels: BTreeSet::default(), merges: BTreeMap::default(), - revisions: BTreeMap::from([(revision_id, Some(revision))]), + revisions: BTreeMap::from_iter([(id, Some(revision))]), assignees: BTreeSet::default(), - timeline: vec![revision_id.into_inner()], + timeline: vec![id.into_inner()], reviews: BTreeMap::default(), } } + /// Title of the patch. pub fn title(&self) -> &str { self.title.as_str() @@ -764,6 +761,11 @@ impl Patch { } } Action::RevisionRedact { revision } => { + // Not allowed to delete the root revision. + let (root, _) = self.root(); + if revision == root { + return Err(Error::NotAllowed(entry)); + } // Redactions must have observed a revision to be valid. if let Some(r) = self.revisions.get_mut(&revision) { // If the revision has already been merged, ignore the redaction. We @@ -1020,7 +1022,7 @@ impl Patch { } } -impl store::FromHistory for Patch { +impl store::Cob for Patch { type Action = Action; type Error = Error; @@ -1028,7 +1030,7 @@ impl store::FromHistory for Patch { &TYPENAME } - fn init(op: Op, repo: &R) -> Result { + fn from_root(op: Op, repo: &R) -> Result { let mut actions = op.actions.into_iter(); let Some(Action::Revision { description, base, oid, resolves }) = actions.next() else { return Err(Error::Init("the first action must be of type `revision`")); @@ -1053,7 +1055,7 @@ impl store::FromHistory for Patch { Ok(patch) } - fn apply(&mut self, op: Op, repo: &R) -> Result<(), Error> { + fn op(&mut self, op: Op, repo: &R) -> Result<(), Error> { debug_assert!(!self.timeline.contains(&op.id)); self.timeline.push(op.id); @@ -1080,6 +1082,23 @@ impl store::FromHistory for Patch { } } +impl cob::Evaluate for Patch { + type Error = Error; + + fn init(entry: &cob::Entry, repo: &R) -> Result { + let op = Op::try_from(entry)?; + let object = Patch::from_root(op, repo)?; + + Ok(object) + } + + fn apply(&mut self, entry: &cob::Entry, repo: &R) -> Result<(), Self::Error> { + let op = Op::try_from(entry)?; + + self.op(op, repo) + } +} + mod lookup { use super::*; @@ -1462,7 +1481,7 @@ impl Review { } } -impl store::Transaction { +impl store::Transaction { pub fn edit(&mut self, title: impl ToString, target: MergeTarget) -> Result<(), store::Error> { self.push(Action::Edit { title: title.to_string(), @@ -1685,7 +1704,7 @@ pub struct PatchMut<'a, 'g, R> { impl<'a, 'g, R> PatchMut<'a, 'g, R> where - R: ReadRepository + SignRepository + cob::Store, + R: ReadRepository + SignRepository + cob::Store + 'static, { pub fn new(id: ObjectId, patch: Patch, store: &'g mut Patches<'a, R>) -> Self { Self { id, patch, store } @@ -1713,13 +1732,13 @@ where ) -> Result where G: Signer, - F: FnOnce(&mut Transaction) -> Result<(), store::Error>, + F: FnOnce(&mut Transaction) -> Result<(), store::Error>, { - let mut tx = Transaction::new(*signer.public_key()); + let mut tx = Transaction::default(); operations(&mut tx)?; - let (op, commit) = tx.commit(message, self.id, &mut self.store.raw, signer)?; - self.patch.apply(op, self.store.as_ref())?; + let (patch, commit) = tx.commit(message, self.id, &mut self.store.raw, signer)?; + self.patch = patch; Ok(commit) } @@ -1765,9 +1784,6 @@ where revision: RevisionId, signer: &G, ) -> Result { - if revision.0 == *self.id { - return Err(Error::RootRevision(revision)); - } self.transaction("Redact revision", signer, |tx| tx.redact(revision)) } @@ -2011,7 +2027,7 @@ impl<'a, R> Deref for Patches<'a, R> { impl<'a, R> Patches<'a, R> where - R: ReadRepository + cob::Store, + R: ReadRepository + cob::Store + 'static, { /// Open an patches store. pub fn open(repository: &'a R) -> Result { @@ -2090,7 +2106,7 @@ where impl<'a, R> Patches<'a, R> where - R: ReadRepository + SignRepository + cob::Store, + R: ReadRepository + SignRepository + cob::Store + 'static, { /// Open a new patch. pub fn create<'g, G: Signer>( @@ -2180,6 +2196,7 @@ where } #[cfg(test)] +#[allow(clippy::unwrap_used)] mod test { use std::str::FromStr; @@ -2447,11 +2464,11 @@ mod test { let mut patch = Patch::from_ops([a1, a2], &repo).unwrap(); assert_eq!(patch.revisions().count(), 2); - patch.apply(a3, &repo).unwrap(); + patch.op(a3, &repo).unwrap(); assert_eq!(patch.revisions().count(), 1); - patch.apply(a4, &repo).unwrap(); - patch.apply(a5, &repo).unwrap(); + patch.op(a4, &repo).unwrap(); + patch.op(a5, &repo).unwrap(); } #[test] @@ -2478,7 +2495,7 @@ mod test { time, &alice, ); - h0.commit( + let r1 = h0.commit( &Action::Revision { description: String::from("New"), base, @@ -2493,7 +2510,7 @@ mod test { let mut h1 = h0.clone(); h1.commit( &Action::RevisionRedact { - revision: RevisionId(*h0.root().id()), + revision: RevisionId(r1), }, &alice, ); @@ -2755,6 +2772,7 @@ mod test { assert_eq!(patch.revisions().count(), 1); // The patch's root must always exist. + assert_eq!(patch.latest(), patch.root()); assert!(patch.redact(patch.latest().0, &alice.signer).is_err()); } diff --git a/radicle/src/cob/store.rs b/radicle/src/cob/store.rs index 51ddb439..79ff35a3 100644 --- a/radicle/src/cob/store.rs +++ b/radicle/src/cob/store.rs @@ -3,31 +3,19 @@ #![allow(clippy::type_complexity)] use std::fmt::Debug; use std::marker::PhantomData; -use std::ops::ControlFlow; use nonempty::NonEmpty; use serde::{Deserialize, Serialize}; -use crate::cob::common::Timestamp; use crate::cob::op::Op; -use crate::cob::{ - ActorId, Create, Embed, EntryId, History, ObjectId, TypeName, Update, Updated, Version, -}; +use crate::cob::{Create, Embed, EntryId, ObjectId, TypeName, Update, Updated, Version}; use crate::git; use crate::prelude::*; use crate::storage::git as storage; use crate::storage::SignRepository; use crate::{cob, identity}; -#[derive(Debug, thiserror::Error)] -pub enum HistoryError { - #[error("apply: {0}")] - Apply(T::Error), - #[error("operation decoding failed: {0}")] - Op(#[from] cob::op::OpEncodingError), -} - -pub trait HistoryAction: Debug { +pub trait CobAction: Debug { /// Parent objects this action depends on. For example, patch revisions /// have the commit objects as their parent. fn parents(&self) -> Vec { @@ -35,33 +23,33 @@ pub trait HistoryAction: Debug { } } -/// A type that can be materialized from an event history. -/// All collaborative objects implement this trait. -pub trait FromHistory: Sized + PartialEq + Debug { +/// A collaborative object. Can be materialized from an operation history. +pub trait Cob: Sized + PartialEq + Debug { /// The underlying action composing each operation. - type Action: HistoryAction + for<'de> Deserialize<'de> + Serialize; + type Action: CobAction + for<'de> Deserialize<'de> + Serialize; /// Error returned by `apply` function. type Error: std::error::Error + Send + Sync + 'static; /// The object type name. fn type_name() -> &'static TypeName; - /// Initialize a collarorative object - fn init(op: Op, repo: &R) -> Result; + /// Initialize a collarorative object from a root operation. + fn from_root(op: Op, repo: &R) -> Result; - /// Apply a list of operations to the state. - fn apply( + /// Apply an operation to the state. + fn op( &mut self, op: Op, repo: &R, - ) -> Result<(), Self::Error>; + ) -> Result<(), ::Error>; + #[cfg(test)] /// Create an object from a history. fn from_history( - history: &History, + history: &crate::cob::History, repo: &R, - ) -> Result> { - self::from_history::(history, repo) + ) -> Result> { + test::from_history::(history, repo) } #[cfg(test)] @@ -75,43 +63,14 @@ pub trait FromHistory: Sized + PartialEq + Debug { let Some(init) = ops.next() else { panic!("FromHistory::from_ops: operations list is empty"); }; - let mut state = Self::init(init, repo)?; + let mut state = Self::from_root(init, repo)?; for op in ops { - state.apply(op, repo)?; + state.op(op, repo)?; } Ok(state) } } -/// Turn a history into a concrete type, by traversing the history and applying each operation -/// to the state, skipping branches that return errors. -pub fn from_history( - history: &History, - repo: &R, -) -> Result> { - let root = history.root(); - let children = history.children_of(root.id()); - let op = Op::try_from(root)?; - let initial = T::init(op, repo).map_err(HistoryError::Apply)?; - let obj = history.traverse(initial, &children, |mut acc, _, entry| { - match Op::try_from(entry) { - Ok(op) => { - if let Err(err) = acc.apply(op, repo) { - log::warn!("Error applying op to `{}` state: {err}", T::type_name()); - return ControlFlow::Break(acc); - } - } - Err(err) => { - log::warn!("Error decoding ops for `{}` state: {err}", T::type_name()); - return ControlFlow::Break(acc); - } - } - ControlFlow::Continue(acc) - }); - - Ok(obj) -} - /// Store error. #[derive(Debug, thiserror::Error)] pub enum Error { @@ -137,14 +96,6 @@ pub enum Error { #[source] err: git::Error, }, - #[error("history: {0}")] - History(Box), -} - -impl Error { - fn history(e: impl std::error::Error + Send + Sync + 'static) -> Self { - Self::History(Box::new(e)) - } } /// Storage for collaborative objects of a specific type `T` in a single repository. @@ -176,7 +127,7 @@ impl<'a, T, R: ReadRepository> Store<'a, T, R> { impl<'a, T, R> Store<'a, T, R> where R: ReadRepository + SignRepository + cob::Store, - T: FromHistory + 'static, + T: Cob + cob::Evaluate, T::Action: Serialize, { /// Update an object. @@ -187,7 +138,7 @@ where actions: impl Into>, embeds: Vec, signer: &G, - ) -> Result { + ) -> Result, Error> { let actions = actions.into(); let parents = actions.iter().flat_map(T::Action::parents).collect(); let changes = actions.try_map(encoding::encode)?; @@ -205,7 +156,6 @@ where changes, }, )?; - self.repo.sign_refs(signer).map_err(Error::SignRefs)?; Ok(updated) @@ -222,7 +172,7 @@ where let actions = actions.into(); let parents = actions.iter().flat_map(T::Action::parents).collect(); let contents = actions.try_map(encoding::encode)?; - let cob = cob::create( + let cob = cob::create::( self.repo, signer, self.identity, @@ -236,11 +186,9 @@ where contents, }, )?; - let object = T::from_history(cob.history(), self.repo).map_err(Error::history)?; - self.repo.sign_refs(signer).map_err(Error::SignRefs)?; - Ok((*cob.id(), object)) + Ok((*cob.id(), cob.object)) } /// Remove an object. @@ -268,30 +216,21 @@ where impl<'a, T, R> Store<'a, T, R> where R: ReadRepository + cob::Store, - T: FromHistory + 'static, + T: cob::Evaluate + Cob, T::Action: Serialize, { /// Get an object. pub fn get(&self, id: &ObjectId) -> Result, Error> { - let cob = cob::get(self.repo, T::type_name(), id)?; - - if let Some(cob) = cob { - let obj = T::from_history(cob.history(), self.repo).map_err(Error::history)?; - - Ok(Some(obj)) - } else { - Ok(None) - } + cob::get::(self.repo, T::type_name(), id) + .map(|r| r.map(|cob| cob.object)) + .map_err(Error::from) } /// Return all objects. pub fn all(&self) -> Result> + 'a, Error> { - let raw = cob::list(self.repo, T::type_name())?; + let raw = cob::list::(self.repo, T::type_name())?; - Ok(raw.into_iter().map(|o| { - let obj = T::from_history(o.history(), self.repo).map_err(Error::history)?; - Ok((*o.id(), obj)) - })) + Ok(raw.into_iter().map(|o| Ok((*o.id(), o.object)))) } /// Return true if the list of issues is empty. @@ -301,7 +240,7 @@ where /// Return objects count. pub fn count(&self) -> Result { - let raw = cob::list(self.repo, T::type_name())?; + let raw = cob::list::(self.repo, T::type_name())?; Ok(raw.len()) } @@ -309,24 +248,25 @@ where /// Allows operations to be batched atomically. #[derive(Debug)] -pub struct Transaction { - actor: ActorId, +pub struct Transaction, R> { actions: Vec, embeds: Vec, + repo: PhantomData, } -impl Transaction { - /// Create a new transaction. - pub fn new(actor: ActorId) -> Self { +impl, R> Default for Transaction { + fn default() -> Self { Self { - actor, actions: Vec::new(), embeds: Vec::new(), + repo: PhantomData, } } +} +impl, R> Transaction { /// Create a new transaction to be used as the initial set of operations for a COB. - pub fn initial( + pub fn initial( message: &str, store: &mut Store, signer: &G, @@ -338,16 +278,13 @@ impl Transaction { R: ReadRepository + SignRepository + cob::Store, T::Action: Serialize + Clone, { - let actor = *signer.public_key(); - let mut tx = Transaction::new(actor); - + let mut tx = Transaction::default(); operations(&mut tx)?; let actions = NonEmpty::from_vec(tx.actions) - .expect("Transaction::initial: transaction must contain at least one operation"); - let (id, cob) = store.create(message, actions, tx.embeds, signer)?; + .expect("Transaction::initial: transaction must contain at least one action"); - Ok((id, cob)) + store.create(message, actions, tx.embeds, signer) } /// Add an operation to this transaction. @@ -367,40 +304,23 @@ impl Transaction { /// Commit transaction. /// /// Returns an operation that can be applied onto an in-memory state. - pub fn commit( + pub fn commit( self, msg: &str, id: ObjectId, store: &mut Store, signer: &G, - ) -> Result<(cob::Op, EntryId), Error> + ) -> Result<(T, EntryId), Error> where R: ReadRepository + SignRepository + cob::Store, T::Action: Serialize + Clone, { let actions = NonEmpty::from_vec(self.actions) .expect("Transaction::commit: transaction must not be empty"); - let Updated { - head, - object, - parents, - } = store.update(id, msg, actions.clone(), self.embeds, signer)?; - let id = head; - let author = self.actor; - let timestamp = Timestamp::from_secs(object.history().timestamp()); - let identity = store.identity; - let manifest = object.manifest().clone(); - let op = cob::Op { - id, - actions, - author, - timestamp, - parents, - identity, - manifest, - }; + let Updated { head, object, .. } = + store.update(id, msg, actions.clone(), self.embeds, signer)?; - Ok((op, id)) + Ok((object.object, head)) } } @@ -409,18 +329,11 @@ pub fn ops( id: &ObjectId, type_name: &TypeName, repo: &R, -) -> Result>>, Error> { - let cob = cob::get(repo, type_name, id)?; +) -> Result>>, Error> { + let cob = cob::get::, _>(repo, type_name, id)?; if let Some(cob) = cob { - let root = cob.history().root(); - let ops = cob - .history() - .traverse(Vec::new(), &[root.id], |mut ops, _, entry| { - ops.push(Op::from(entry.clone())); - ControlFlow::Continue(ops) - }); - Ok(ops) + Ok(cob.object.map(Op::from)) } else { Err(Error::NotFound(type_name.clone(), *id)) } @@ -442,3 +355,47 @@ pub mod encoding { Ok(buf) } } + +#[cfg(test)] +pub mod test { + use super::*; + + #[derive(Debug, thiserror::Error)] + pub enum HistoryError { + #[error("apply: {0}")] + Apply(T::Error), + #[error("operation decoding failed: {0}")] + Op(#[from] cob::op::OpEncodingError), + } + + /// Turn a history into a concrete type, by traversing the history and applying each operation + /// to the state, skipping branches that return errors. + pub fn from_history( + history: &crate::cob::History, + repo: &R, + ) -> Result> { + use std::ops::ControlFlow; + + let root = history.root(); + let children = history.children_of(root.id()); + let op = Op::try_from(root)?; + let initial = T::from_root(op, repo).map_err(HistoryError::Apply)?; + let obj = history.traverse(initial, &children, |mut acc, _, entry| { + match Op::try_from(entry) { + Ok(op) => { + if let Err(err) = acc.op(op, repo) { + log::warn!("Error applying op to `{}` state: {err}", T::type_name()); + return ControlFlow::Break(acc); + } + } + Err(err) => { + log::warn!("Error decoding ops for `{}` state: {err}", T::type_name()); + return ControlFlow::Break(acc); + } + } + ControlFlow::Continue(acc) + }); + + Ok(obj) + } +} diff --git a/radicle/src/cob/test.rs b/radicle/src/cob/test.rs index 0cc0fe04..8d4d1224 100644 --- a/radicle/src/cob/test.rs +++ b/radicle/src/cob/test.rs @@ -1,3 +1,4 @@ +#![allow(clippy::unwrap_used)] use std::marker::PhantomData; use std::ops::Deref; @@ -20,7 +21,7 @@ use crate::prelude::Did; use crate::storage::ReadRepository; use crate::test::arbitrary; -use super::store::FromHistory; +use super::store::Cob; use super::thread; /// Convenience type for building histories. @@ -53,7 +54,7 @@ impl HistoryBuilder { } } -impl HistoryBuilder +impl HistoryBuilder where T::Action: for<'de> Deserialize<'de> + Serialize + Eq + 'static, { @@ -129,7 +130,7 @@ impl Deref for HistoryBuilder { } /// Create a new test history. -pub fn history( +pub fn history( actions: &[T::Action], time: Timestamp, signer: &G, @@ -159,7 +160,7 @@ impl Actor { impl Actor { /// Create a new operation. - pub fn op_with( + pub fn op_with( &mut self, actions: impl IntoIterator, identity: Oid, @@ -193,10 +194,7 @@ impl Actor { } /// Create a new operation. - pub fn op( - &mut self, - actions: impl IntoIterator, - ) -> Op + pub fn op(&mut self, actions: impl IntoIterator) -> Op where T::Action: Clone + Serialize, { @@ -222,7 +220,7 @@ impl Actor { oid: git::Oid, repo: &R, ) -> Result { - Patch::init( + Patch::from_root( self.op::([ patch::Action::Revision { description: description.to_string(), @@ -244,7 +242,7 @@ impl Actor { /// /// Doesn't encode in the same way as we do in production, but attempts to include the same data /// that feeds into the hash entropy, so that changing any input will change the resulting oid. -pub fn encoded( +pub fn encoded( action: &T::Action, timestamp: Timestamp, parents: impl IntoIterator, diff --git a/radicle/src/cob/thread.rs b/radicle/src/cob/thread.rs index 587257c8..27749e98 100644 --- a/radicle/src/cob/thread.rs +++ b/radicle/src/cob/thread.rs @@ -8,7 +8,8 @@ use thiserror::Error; use crate::cob; use crate::cob::common::{Reaction, Timestamp, Uri}; -use crate::cob::{ActorId, Embed, EntryId, Op}; +use crate::cob::store::Cob; +use crate::cob::{op, ActorId, Embed, EntryId, Op}; use crate::git; use crate::prelude::ReadRepository; @@ -38,6 +39,8 @@ pub enum Error { /// Object initialization failed. #[error("initialization failed: {0}")] Init(&'static str), + #[error("op decoding failed: {0}")] + Op(#[from] op::OpEncodingError), } /// Identifies a comment. @@ -229,7 +232,7 @@ pub enum Action { }, } -impl cob::store::HistoryAction for Action {} +impl cob::store::CobAction for Action {} impl From for nonempty::NonEmpty { fn from(action: Action) -> Self { @@ -241,9 +244,9 @@ impl From for nonempty::NonEmpty { #[derive(Debug, Clone, PartialEq, Eq)] pub struct Thread> { /// The comments under the thread. - comments: BTreeMap>, + pub(crate) comments: BTreeMap>, /// Comment timeline. - timeline: Vec, + pub(crate) timeline: Vec, } impl Default for Thread { @@ -299,6 +302,10 @@ impl Thread { .map(|comment| (id, comment)) }) } + + pub fn timeline(&self) -> impl DoubleEndedIterator + '_ { + self.timeline.iter() + } } impl Thread { @@ -350,7 +357,7 @@ impl Thread> { } } -impl cob::store::FromHistory for Thread { +impl cob::store::Cob for Thread { type Action = Action; type Error = Error; @@ -358,7 +365,7 @@ impl cob::store::FromHistory for Thread { &TYPENAME } - fn init(op: Op, repo: &R) -> Result { + fn from_root(op: Op, repo: &R) -> Result { let author = op.author; let entry = op.id; let timestamp = op.timestamp; @@ -385,7 +392,7 @@ impl cob::store::FromHistory for Thread { Ok(thread) } - fn apply(&mut self, op: Op, repo: &R) -> Result<(), Error> { + fn op(&mut self, op: Op, repo: &R) -> Result<(), Error> { for action in op.actions { self.action(action, op.id, op.author, op.timestamp, op.identity, repo)?; } @@ -393,6 +400,23 @@ impl cob::store::FromHistory for Thread { } } +impl cob::Evaluate for Thread { + type Error = Error; + + fn init(entry: &cob::Entry, repo: &R) -> Result { + let op = Op::try_from(entry)?; + let object = ::from_root(op, repo)?; + + Ok(object) + } + + fn apply(&mut self, entry: &cob::Entry, repo: &R) -> Result<(), Self::Error> { + let op = Op::try_from(entry)?; + + self.op(op, repo) + } +} + pub fn comment( thread: &mut Thread>, id: EntryId, @@ -406,6 +430,11 @@ pub fn comment( if body.is_empty() { return Err(Error::Comment(id)); } + if let Some(id) = reply_to { + if !thread.comments.contains_key(&id) { + return Err(Error::Missing(id)); + } + } debug_assert!(!thread.timeline.contains(&id)); thread.timeline.push(id); @@ -522,6 +551,7 @@ pub fn unresolve( } #[cfg(test)] +#[allow(clippy::unwrap_used)] mod tests { use std::ops::{Deref, DerefMut}; @@ -530,7 +560,7 @@ mod tests { use super::*; use crate as radicle; - use crate::cob::store::FromHistory; + use crate::cob::store::Cob; use crate::cob::test; use crate::crypto::test::signer::MockSigner; use crate::crypto::Signer; @@ -609,7 +639,7 @@ mod tests { // Redact the second comment. let a3 = alice.redact(a1.id()); - thread.apply(a3, &repo).unwrap(); + thread.op(a3, &repo).unwrap(); let (_, comment0) = thread.comments().nth(0).unwrap(); let (_, comment1) = thread.comments().nth(1).unwrap(); @@ -806,7 +836,7 @@ mod tests { let mut t = Thread::default(); let id = arbitrary::entry_id(); - t.apply(alice.redact(id), &repo).unwrap_err(); + t.op(alice.redact(id), &repo).unwrap_err(); } #[test] @@ -816,7 +846,7 @@ mod tests { let mut t = Thread::default(); let id = arbitrary::entry_id(); - t.apply(alice.edit(id, "Edited"), &repo).unwrap_err(); + t.op(alice.edit(id, "Edited"), &repo).unwrap_err(); } #[test] diff --git a/radicle/src/storage/git.rs b/radicle/src/storage/git.rs index 8e5d5b91..74cec577 100644 --- a/radicle/src/storage/git.rs +++ b/radicle/src/storage/git.rs @@ -492,7 +492,6 @@ impl ReadRepository for Repository { let oid = e.target().ok_or(Error::InvalidRef)?; let (_, category, _, _) = refname.non_empty_components(); - // Only sign known ref categories. if [ git::name::HEADS, git::name::TAGS, diff --git a/radicle/src/storage/git/cob.rs b/radicle/src/storage/git/cob.rs index 2667e905..fa830544 100644 --- a/radicle/src/storage/git/cob.rs +++ b/radicle/src/storage/git/cob.rs @@ -15,7 +15,7 @@ use crate::storage::{ }; use crate::{ git, identity, - identity::{doc::DocError, IdentityError}, + identity::{doc::DocError, IdentityError, PublicKey}, }; use super::{RemoteId, Repository}; @@ -80,8 +80,6 @@ impl cob::object::Storage for Repository { type UpdateError = git2::Error; type RemoveError = git2::Error; - type Identifier = RemoteId; - fn objects( &self, typename: &cob::TypeName, @@ -136,7 +134,7 @@ impl cob::object::Storage for Repository { fn update( &self, - identifier: &Self::Identifier, + identifier: &PublicKey, typename: &cob::TypeName, object_id: &cob::ObjectId, entry: &cob::EntryId, @@ -156,7 +154,7 @@ impl cob::object::Storage for Repository { fn remove( &self, - identifier: &Self::Identifier, + identifier: &PublicKey, typename: &cob::TypeName, object_id: &cob::ObjectId, ) -> Result<(), Self::RemoveError> { @@ -216,12 +214,16 @@ impl<'a, R: storage::WriteRepository> change::Storage for DraftStore<'a, R> { } } -impl<'a> SignRepository for DraftStore<'a> { +impl<'a, R: storage::ReadRepository> SignRepository for DraftStore<'a, R> { fn sign_refs( &self, signer: &G, ) -> Result, Error> { - self.repo.sign_refs(signer) + // Since this is a draft store, we do not actually want to sign the refs. + // Instead, we just return the existing signed refs. + let remote = self.repo.remote(signer.public_key())?; + + Ok(remote.refs) } } @@ -344,8 +346,6 @@ impl<'a, R: storage::WriteRepository> cob::object::Storage for DraftStore<'a, R> type UpdateError = git2::Error; type RemoveError = git2::Error; - type Identifier = RemoteId; - fn objects( &self, typename: &cob::TypeName, @@ -382,7 +382,7 @@ impl<'a, R: storage::WriteRepository> cob::object::Storage for DraftStore<'a, R> fn update( &self, - identifier: &Self::Identifier, + identifier: &PublicKey, typename: &cob::TypeName, object_id: &cob::ObjectId, entry: &cob::history::EntryId, @@ -402,7 +402,7 @@ impl<'a, R: storage::WriteRepository> cob::object::Storage for DraftStore<'a, R> fn remove( &self, - identifier: &Self::Identifier, + identifier: &PublicKey, typename: &cob::TypeName, object_id: &cob::ObjectId, ) -> Result<(), Self::RemoveError> { diff --git a/radicle/src/test.rs b/radicle/src/test.rs index 50129a26..7c9b69d7 100644 --- a/radicle/src/test.rs +++ b/radicle/src/test.rs @@ -155,6 +155,12 @@ pub mod setup { } } + impl std::ops::DerefMut for NodeRepo { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.repo + } + } + /// A repository checkout. pub struct NodeRepoCheckout { checkout: git::raw::Repository,