From f30760d6bb86d2978a5ed4df8ee45b9aa97778b4 Mon Sep 17 00:00:00 2001 From: Lorenz Leutgeb Date: Mon, 28 Apr 2025 08:46:20 +0200 Subject: [PATCH] cob: Add CobAction::produces_identifier and validation Transactions will now fail/deny when adding multiple actions that produce an identifier. --- radicle/src/cob/identity.rs | 6 ++++- radicle/src/cob/issue.rs | 31 ++++++++++++++++++++++-- radicle/src/cob/patch.rs | 10 ++++++++ radicle/src/cob/store.rs | 48 ++++++++++++++++++++++++++++++++++--- radicle/src/cob/thread.rs | 6 ++++- 5 files changed, 94 insertions(+), 7 deletions(-) diff --git a/radicle/src/cob/identity.rs b/radicle/src/cob/identity.rs index e1162dfc..73dc74e3 100644 --- a/radicle/src/cob/identity.rs +++ b/radicle/src/cob/identity.rs @@ -77,7 +77,11 @@ pub enum Action { RevisionRedact { revision: RevisionId }, } -impl CobAction for Action {} +impl CobAction for Action { + fn produces_identifier(&self) -> bool { + matches!(self, Self::Revision { .. }) + } +} /// Error applying an operation onto a state. #[derive(Error, Debug)] diff --git a/radicle/src/cob/issue.rs b/radicle/src/cob/issue.rs index 825c12f2..a53d20c6 100644 --- a/radicle/src/cob/issue.rs +++ b/radicle/src/cob/issue.rs @@ -935,7 +935,11 @@ pub enum Action { }, } -impl CobAction for Action {} +impl CobAction for Action { + fn produces_identifier(&self) -> bool { + matches!(self, Self::Comment { .. }) + } +} #[cfg(test)] #[allow(clippy::unwrap_used)] @@ -946,8 +950,8 @@ mod test { use crate::cob::{store::CobWithType, ActorId, Reaction}; use crate::git::Oid; use crate::issue::cache::Issues as _; - use crate::test; use crate::test::arbitrary; + use crate::{assert_matches, test}; #[test] fn test_concurrency() { @@ -1688,6 +1692,29 @@ mod test { assert_eq!(issue.comments().count(), 1); } + #[test] + fn test_invalid_tx_reference() { + let test::setup::NodeWithRepo { node, repo, .. } = test::setup::NodeWithRepo::default(); + let mut issues = Cache::no_cache(&*repo).unwrap(); + let issue = issues + .create( + "My first issue", + "Blah blah blah.", + &[], + &[], + [], + &node.signer, + ) + .unwrap(); + + // Comments require references, so adding two of them to the same transaction errors. + let mut tx: Transaction = + Transaction::::default(); + tx.comment("First reply", *issue.id, vec![]).unwrap(); + let err = tx.comment("Second reply", *issue.id, vec![]).unwrap_err(); + assert_matches!(err, cob::store::Error::ClashingIdentifiers(_, _)); + } + #[test] fn test_invalid_cob() { use crate::crypto::test::signer::MockSigner; diff --git a/radicle/src/cob/patch.rs b/radicle/src/cob/patch.rs index e700a490..1666c1d3 100644 --- a/radicle/src/cob/patch.rs +++ b/radicle/src/cob/patch.rs @@ -320,6 +320,16 @@ impl CobAction for Action { _ => vec![], } } + + fn produces_identifier(&self) -> bool { + matches!( + self, + Self::Revision { .. } + | Self::RevisionComment { .. } + | Self::Review { .. } + | Self::ReviewComment { .. } + ) + } } /// Output of a merge. diff --git a/radicle/src/cob/store.rs b/radicle/src/cob/store.rs index 0511f0c7..bd67b2d3 100644 --- a/radicle/src/cob/store.rs +++ b/radicle/src/cob/store.rs @@ -22,6 +22,23 @@ pub trait CobAction: Debug { fn parents(&self) -> Vec { Vec::new() } + + /// The outcome of some actions are to be referred later. + /// For example, one action may create a comment, followed by another + /// action that may create a reply to the comment, referring to it. + /// Since actions are stored as part of [`crate::cob::op::Op`], + /// and operations are the smallest identifiable units, + /// this may lead to ambiguity. + /// It would not be possible to to, say, address one particular comment out + /// of two, if the corresponding actions of creations were part of the + /// same operation. + /// To help avoid this, implementations signal whether specific actions + /// require "their own" identifier. + /// This allows checking for multiple such actions before creating an + /// operation. + fn produces_identifier(&self) -> bool { + false + } } /// A collaborative object. Can be materialized from an operation history. @@ -111,6 +128,8 @@ pub enum Error { #[source] err: git::raw::Error, }, + #[error("transaction already contains action {0} which produces an identifier, denying to add action {1} which also produces an identifier")] + ClashingIdentifiers(String, String), } /// Storage for collaborative objects of a specific type `T` in a single repository. @@ -340,6 +359,13 @@ pub struct Transaction, R> { actions: Vec, embeds: Vec>, + // Internal state kept for validation of the transaction. + // If an action that produces an identifier is added to + // the transaction, then this will track its index, + // so that adding a second action that produces an identifier + // can fail with a useful error. + produces_identifier: Option, + repo: PhantomData, type_name: TypeName, } @@ -349,6 +375,7 @@ impl, R> Default for Transaction { Self { actions: Vec::new(), embeds: Vec::new(), + produces_identifier: None, repo: PhantomData, type_name: T::type_name().clone(), } @@ -363,6 +390,7 @@ where Self { actions, embeds, + produces_identifier: None, repo: PhantomData, type_name, } @@ -405,15 +433,29 @@ where { /// Add an action to this transaction. pub fn push(&mut self, action: T::Action) -> Result<(), Error> { + if action.produces_identifier() { + if let Some(index) = self.produces_identifier { + return Err(Error::ClashingIdentifiers( + serde_json::to_string(&self.actions[index])?, + serde_json::to_string(&action)?, + )); + } else { + self.produces_identifier = Some(self.actions.len()) + } + } + self.actions.push(action); Ok(()) } /// Add actions to this transaction. - pub fn extend(&mut self, actions: impl IntoIterator) -> Result<(), Error> { - self.actions.extend(actions); - + /// Note that we cannot implement [`std::iter::Extend`] because [`Self::push`] + /// validates the action being pushed, and therefore is falliable. + pub fn extend>(&mut self, actions: I) -> Result<(), Error> { + for action in actions { + self.push(action)?; + } Ok(()) } diff --git a/radicle/src/cob/thread.rs b/radicle/src/cob/thread.rs index 08670dc6..7ae4c3bf 100644 --- a/radicle/src/cob/thread.rs +++ b/radicle/src/cob/thread.rs @@ -272,7 +272,11 @@ pub enum Action { }, } -impl cob::store::CobAction for Action {} +impl cob::store::CobAction for Action { + fn produces_identifier(&self) -> bool { + matches!(self, Self::Comment { .. }) + } +} impl From for nonempty::NonEmpty { fn from(action: Action) -> Self {