Radish alpha
h
Radicle Heartwood Protocol & Stack
Radicle
Git (anonymous pull)
Log in to clone via SSH
cob: Add CobAction::produces_identifier and validation
Lorenz Leutgeb committed 1 year ago
commit f30760d6bb86d2978a5ed4df8ee45b9aa97778b4
parent e4d23fe56a7f7329ac98991bc55bb043ddede617
5 files changed +94 -7
modified 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)]
modified 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() {
@@ -1689,6 +1693,29 @@ mod test {
    }

    #[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<Issue, test::storage::git::Repository> =
+
            Transaction::<Issue, _>::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;
        use cob::change::Storage as _;
modified 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.
modified radicle/src/cob/store.rs
@@ -22,6 +22,23 @@ pub trait CobAction: Debug {
    fn parents(&self) -> Vec<git::Oid> {
        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<T: Cob + cob::Evaluate<R>, R> {
    actions: Vec<T::Action>,
    embeds: Vec<Embed<Uri>>,

+
    // 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<usize>,
+

    repo: PhantomData<R>,
    type_name: TypeName,
}
@@ -349,6 +375,7 @@ impl<T: Cob + CobWithType + cob::Evaluate<R>, R> Default for Transaction<T, R> {
        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<Item = T::Action>) -> 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<I: IntoIterator<Item = T::Action>>(&mut self, actions: I) -> Result<(), Error> {
+
        for action in actions {
+
            self.push(action)?;
+
        }
        Ok(())
    }

modified 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<Action> for nonempty::NonEmpty<Action> {
    fn from(action: Action) -> Self {