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,