radicle: validated construction of Project

The `validate` method of `Project` was unused, which is problematic if
the attributes are supposed to be validated based on some
constraints. The issue is that `Project` provided public construction
of its fields with enforcing validation.

This change makes `Project`'s attributes private and introduces a
smart constructor `new` which is fallible if any of the constraints
are not met. The error type was changed to be a vector of errors since
all of these errors can be collected for better error reporting.

Deserialization also provided a way of constructing a `Project`
without ensuring any of the constraints were checked. To prevent this
a hand-rolled `Deserialize` is provided that goes through
`Project::new`.

To access the fields of `Project`, accessor functions are added and
all call sites are updated to use them.

Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
X-Clacks-Overhead: GNU Terry Pratchett

radicle-cli/src/commands/checkout.rs

@@ -84,7 +84,7 @@ pub fn execute(options: Options, profile: &Profile) -> anyhow::Result<PathBuf> {
         .identity_of(profile.id())
         .context("project could not be found in local storage")?;
     let payload = doc.project()?;
-    let path = PathBuf::from(payload.name.clone());
+    let path = PathBuf::from(payload.name().clone());
 
     if path.exists() {
         anyhow::bail!("the local path {:?} already exists", path.as_path());
@@ -93,7 +93,7 @@ pub fn execute(options: Options, profile: &Profile) -> anyhow::Result<PathBuf> {
     term::headline(&format!(
         "Initializing local checkout for 🌱 {} ({})",
         term::format::highlight(options.id),
-        payload.name,
+        payload.name(),
     ));
 
     let spinner = term::spinner("Performing checkout...");
@@ -119,7 +119,7 @@ pub fn execute(options: Options, profile: &Profile) -> anyhow::Result<PathBuf> {
     setup_remotes(
         project::SetupRemote {
             project: id,
-            default_branch: payload.default_branch,
+            default_branch: payload.default_branch().clone(),
             repo: &repo,
             fetch: true,
             tracking: true,

radicle-cli/src/commands/clone.rs

@@ -97,7 +97,7 @@ pub fn clone(id: Id, _interactive: Interactive, ctx: impl term::Context) -> anyh
         .map_err(|_e| anyhow!("couldn't load project {} from local state", id))?;
     let proj = doc.project()?;
 
-    let path = Path::new(&proj.name);
+    let path = Path::new(proj.name());
     let repo = rad::checkout(id, profile.id(), path, &profile.storage)?;
     let delegates = doc
         .delegates
@@ -105,7 +105,7 @@ pub fn clone(id: Id, _interactive: Interactive, ctx: impl term::Context) -> anyh
         .map(|d| **d)
         .filter(|id| id != profile.id())
         .collect::<Vec<_>>();
-    let default_branch = proj.default_branch.clone();
+    let default_branch = proj.default_branch().clone();
 
     // Setup tracking for project delegates.
     setup_remotes(

radicle-cli/src/commands/init.rs

@@ -205,7 +205,7 @@ pub fn init(options: Options, profile: &profile::Profile) -> anyhow::Result<()>
 
             spinner.message(format!(
                 "Project {} created",
-                term::format::highlight(&proj.name)
+                term::format::highlight(proj.name())
             ));
             spinner.finish();
 
@@ -214,13 +214,13 @@ pub fn init(options: Options, profile: &profile::Profile) -> anyhow::Result<()>
                 term::blank();
             }
 
-            if options.set_upstream || git::branch_remote(&repo, &proj.default_branch).is_err() {
+            if options.set_upstream || git::branch_remote(&repo, proj.default_branch()).is_err() {
                 // Setup eg. `master` -> `rad/master`
                 radicle::git::set_upstream(
                     &repo,
                     &radicle::rad::REMOTE_NAME,
-                    &proj.default_branch,
+                    proj.default_branch(),
-                    &radicle::git::refs::workdir::branch(&proj.default_branch),
+                    &radicle::git::refs::workdir::branch(proj.default_branch()),
                 )?;
             }
 

radicle-cli/src/commands/ls.rs

@@ -3,7 +3,6 @@ use std::ffi::OsString;
 use crate::terminal as term;
 use crate::terminal::args::{Args, Error, Help};
 
-use radicle::prelude::*;
 use radicle::storage::{ReadRepository, WriteStorage};
 
 pub const HELP: Help = Help {
@@ -50,13 +49,13 @@ pub fn run(_options: Options, ctx: impl term::Context) -> anyhow::Result<()> {
     storage.projects()?.into_iter().for_each(|id| {
         let Ok(repo) = storage.repository(id) else { return };
         let Ok((_, head)) = repo.head() else { return };
-        let Ok(Project { name, description, .. }) = repo.project_of(profile.id()) else { return };
+        let Ok(proj) = repo.project_of(profile.id()) else { return };
         let head = term::format::oid(head);
         table.push([
-            term::format::bold(name),
+            term::format::bold(proj.name()),
             term::format::tertiary(id),
             term::format::secondary(head),
-            term::format::italic(description),
+            term::format::italic(proj.description()),
         ]);
     });
     table.render();

radicle-cli/src/commands/patch/create.rs

@@ -47,7 +47,7 @@ pub fn run(
 
     term::headline(&format!(
         "🌱 Creating patch for {}",
-        term::format::highlight(&project.name)
+        term::format::highlight(project.name())
     ));
 
     let signer = term::signer(profile)?;
@@ -92,7 +92,7 @@ pub fn run(
     // branch, as well as your own (eg. `rad/master`).
     let mut spinner = term::spinner("Analyzing remotes...");
     let targets =
-        common::find_merge_targets(&head_oid, project.default_branch.as_refstr(), storage)?;
+        common::find_merge_targets(&head_oid, project.default_branch().as_refstr(), storage)?;
 
     // eg. `refs/namespaces/<peer>/refs/heads/master`
     let (target_peer, target_oid) = match targets.not_merged.as_slice() {
@@ -177,7 +177,7 @@ pub fn run(
     term::info!(
         "{}/{} ({}) <- {}/{} ({})",
         term::format::dim(target_peer.id),
-        term::format::highlight(project.default_branch.to_string()),
+        term::format::highlight(project.default_branch().to_string()),
         term::format::secondary(term::format::oid(*target_oid)),
         term::format::dim(term::format::node(patches.public_key())),
         term::format::highlight(head_branch.to_string()),

radicle-cli/src/commands/track.rs

@@ -98,7 +98,7 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> {
 
     term::info!(
         "Establishing 🌱 tracking relationship for {}",
-        term::format::highlight(project.name)
+        term::format::highlight(project.name())
     );
     term::blank();
 

radicle-cli/src/commands/untrack.rs

@@ -73,13 +73,13 @@ pub fn run(options: Options, ctx: impl term::Context) -> anyhow::Result<()> {
     if untrack(id, &profile)? {
         term::success!(
             "Tracking relationships for {} ({}) removed",
-            term::format::highlight(project.name),
+            term::format::highlight(project.name()),
             &id.to_human()
         );
     } else {
         term::info!(
             "Tracking relationships for {} ({}) doesn't exist",
-            term::format::highlight(project.name),
+            term::format::highlight(project.name()),
             &id.to_human()
         );
     }

radicle/src/identity.rs

@@ -194,11 +194,12 @@ mod test {
         // want to have to create a repository object twice. Perhaps there should
         // be a way of getting a project from a repo.
         let mut doc = storage.get(alice.public_key(), id).unwrap().unwrap();
-        let mut prj = doc.project().unwrap();
+        let prj = doc.project().unwrap();
         let repo = storage.repository(id).unwrap();
 
         // Make a change to the description and sign it.
-        prj.description += "!";
+        let desc = prj.description().to_owned() + "!";
+        let prj = prj.update(None, desc, None).unwrap();
         doc.payload.insert(PayloadId::project(), prj.clone().into());
         doc.sign(&alice)
             .and_then(|(_, sig)| {
@@ -241,7 +242,8 @@ mod test {
             .unwrap();
 
         // Update description again with signatures by Eve and Bob.
-        prj.description += "?";
+        let desc = prj.description().to_owned() + "?";
+        let prj = prj.update(None, desc, None).unwrap();
         doc.payload.insert(PayloadId::project(), prj.into());
         let (current, head) = doc
             .sign(&bob)
@@ -271,6 +273,6 @@ mod test {
         assert_eq!(identity.doc, doc);
 
         let doc = storage.get(alice.public_key(), id).unwrap().unwrap();
-        assert_eq!(doc.project().unwrap().description, "Acme's repository!?");
+        assert_eq!(doc.project().unwrap().description(), "Acme's repository!?");
     }
 }

radicle/src/identity/project.rs

@@ -1,4 +1,9 @@
-use serde::{Deserialize, Serialize};
+use std::fmt;
+
+use serde::{
+    de::{self, MapAccess, Visitor},
+    Deserialize, Serialize,
+};
 use thiserror::Error;
 
 use crate::crypto;
@@ -20,42 +25,171 @@ pub enum ProjectError {
 }
 
 /// A "project" payload in an identity document.
-#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
+#[derive(Debug, Clone, PartialEq, Eq, Serialize)]
 #[serde(rename_all = "camelCase")]
 pub struct Project {
     /// Project name.
-    pub name: String,
+    name: String,
     /// Project description.
-    pub description: String,
+    description: String,
     /// Project default branch.
-    pub default_branch: BranchName,
+    default_branch: BranchName,
+}
+
+impl<'de> Deserialize<'de> for Project {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        #[derive(Deserialize)]
+        #[serde(field_identifier, rename_all = "camelCase")]
+        enum Field {
+            Name,
+            Description,
+            DefaultBranch,
+        }
+
+        struct ProjectVisitor;
+
+        impl<'de> Visitor<'de> for ProjectVisitor {
+            type Value = Project;
+
+            fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+                formatter.write_str("xyz.radicle.project")
+            }
+
+            fn visit_map<V>(self, mut map: V) -> Result<Self::Value, V::Error>
+            where
+                V: MapAccess<'de>,
+            {
+                let mut name = None;
+                let mut description = None;
+                let mut default_branch = None;
+
+                while let Some(key) = map.next_key()? {
+                    match key {
+                        Field::Name => {
+                            if name.is_some() {
+                                return Err(de::Error::duplicate_field("name"));
+                            }
+                            name = Some(map.next_value()?);
+                        }
+                        Field::Description => {
+                            if description.is_some() {
+                                return Err(de::Error::duplicate_field("description"));
+                            }
+                            description = Some(map.next_value()?);
+                        }
+                        Field::DefaultBranch => {
+                            if default_branch.is_some() {
+                                return Err(de::Error::duplicate_field("defaultBranch"));
+                            }
+                            default_branch = Some(map.next_value()?);
+                        }
+                    }
+                }
+                let name = name.ok_or_else(|| de::Error::missing_field("name"))?;
+                let description =
+                    description.ok_or_else(|| de::Error::missing_field("description"))?;
+                let default_branch =
+                    default_branch.ok_or_else(|| de::Error::missing_field("defaultBranch"))?;
+                Project::new(name, description, default_branch).map_err(|errs| {
+                    de::Error::custom(
+                        errs.into_iter()
+                            .map(|err| err.to_string())
+                            .collect::<Vec<_>>()
+                            .join(", "),
+                    )
+                })
+            }
+        }
+        const FIELDS: &[&str] = &["name", "descrption", "defaultBranch"];
+        deserializer.deserialize_struct("Project", FIELDS, ProjectVisitor)
+    }
 }
 
 impl Project {
-    /// Validate the project data.
+    /// Create a new `Project` payload with the given values.
-    pub fn validate(&self) -> Result<(), ProjectError> {
+    ///
-        if self.name.is_empty() {
+    /// These values are subject to validation and any errors are returned in a vector.
-            return Err(ProjectError::Name("name cannot be empty"));
+    ///
+    /// # Validation Rules
+    ///
+    ///   * `name`'s length must not be empty and must not exceed 255.
+    ///   * `description`'s length must not exceed 255.
+    ///   * `default_branch`'s length must not be empty and must not exceed 255.
+    pub fn new(
+        name: String,
+        description: String,
+        default_branch: BranchName,
+    ) -> Result<Self, Vec<ProjectError>> {
+        let mut errs = Vec::new();
+
+        if name.is_empty() {
+            errs.push(ProjectError::Name("name cannot be empty"));
+        } else if name.len() > doc::MAX_STRING_LENGTH {
+            errs.push(ProjectError::Name("name cannot exceed 255 bytes"));
         }
-        if self.name.len() > doc::MAX_STRING_LENGTH {
+
-            return Err(ProjectError::Name("name cannot exceed 255 bytes"));
+        if description.len() > doc::MAX_STRING_LENGTH {
-        }
+            errs.push(ProjectError::Description(
-        if self.description.len() > doc::MAX_STRING_LENGTH {
-            return Err(ProjectError::Description(
                 "description cannot exceed 255 bytes",
             ));
         }
-        if self.default_branch.is_empty() {
+
-            return Err(ProjectError::DefaultBranch(
+        if default_branch.is_empty() {
+            errs.push(ProjectError::DefaultBranch(
                 "default branch cannot be empty",
-            ));
+            ))
-        }
+        } else if default_branch.len() > doc::MAX_STRING_LENGTH {
-        if self.default_branch.len() > doc::MAX_STRING_LENGTH {
+            errs.push(ProjectError::DefaultBranch(
-            return Err(ProjectError::DefaultBranch(
                 "default branch cannot exceed 255 bytes",
-            ));
+            ))
         }
-        Ok(())
+
+        if errs.is_empty() {
+            Ok(Self {
+                name,
+                description,
+                default_branch,
+            })
+        } else {
+            Err(errs)
+        }
+    }
+
+    /// Update the `Project` payload with new values, if provided.
+    ///
+    /// When any of the values are set to `None` then the original
+    /// value will be used, and so the value will pass validation.
+    ///
+    /// Otherwise, the new value is used and will be subject to the
+    /// original validation rules (see [`Project::new`]).
+    pub fn update(
+        self,
+        name: impl Into<Option<String>>,
+        description: impl Into<Option<String>>,
+        default_branch: impl Into<Option<BranchName>>,
+    ) -> Result<Self, Vec<ProjectError>> {
+        let name = name.into().unwrap_or(self.name);
+        let description = description.into().unwrap_or(self.description);
+        let default_branch = default_branch.into().unwrap_or(self.default_branch);
+        Self::new(name, description, default_branch)
+    }
+
+    #[inline]
+    pub fn name(&self) -> &String {
+        &self.name
+    }
+
+    #[inline]
+    pub fn description(&self) -> &String {
+        &self.description
+    }
+
+    #[inline]
+    pub fn default_branch(&self) -> &BranchName {
+        &self.default_branch
     }
 }
 

radicle/src/rad.rs

@@ -33,6 +33,8 @@ pub enum InitError {
     Doc(#[from] DocError),
     #[error("project: {0}")]
     Project(#[from] storage::git::ProjectError),
+    #[error("project payload: {0}")]
+    ProjectPayload(String),
     #[error("git: {0}")]
     Git(#[from] git2::Error),
     #[error("i/o: {0}")]
@@ -59,11 +61,19 @@ pub fn init<G: Signer>(
     // TODO: Better error when project id already exists in storage, but remote doesn't.
     let pk = signer.public_key();
     let delegate = identity::Did::from(*pk);
-    let proj = Project {
+    let proj = Project::new(
-        name: name.to_owned(),
+        name.to_owned(),
-        description: description.to_owned(),
+        description.to_owned(),
-        default_branch: default_branch.clone(),
+        default_branch.clone(),
-    };
+    )
+    .map_err(|errs| {
+        InitError::ProjectPayload(
+            errs.into_iter()
+                .map(|err| err.to_string())
+                .collect::<Vec<_>>()
+                .join(", "),
+        )
+    })?;
     let doc = identity::Doc::initial(proj, delegate).verified()?;
     let (project, _) = Repository::init(&doc, pk, storage, signer)?;
     let url = git::Url::from(project.id).with_namespace(*pk);
@@ -129,10 +139,12 @@ pub fn fork_remote<G: Signer, S: storage::WriteStorage>(
     let repository = storage.repository(proj)?;
 
     let raw = repository.raw();
-    let remote_head =
+    let remote_head = raw.refname_to_id(&git::refs::storage::branch(
-        raw.refname_to_id(&git::refs::storage::branch(remote, &project.default_branch))?;
+        remote,
+        project.default_branch(),
+    ))?;
     raw.reference(
-        &git::refs::storage::branch(me, &project.default_branch),
+        &git::refs::storage::branch(me, project.default_branch()),
         remote_head,
         false,
         &format!("creating default branch for {me}"),
@@ -271,9 +283,9 @@ pub fn checkout<P: AsRef<Path>, S: storage::ReadStorage>(
     let project = doc.project()?;
 
     let mut opts = git2::RepositoryInitOptions::new();
-    opts.no_reinit(true).description(&project.description);
+    opts.no_reinit(true).description(project.description());
 
-    let repo = git2::Repository::init_opts(path.as_ref().join(&project.name), &opts)?;
+    let repo = git2::Repository::init_opts(path.as_ref().join(project.name()), &opts)?;
     let url = git::Url::from(proj).with_namespace(*remote);
 
     // Configure and fetch all refs from remote.
@@ -283,17 +295,17 @@ pub fn checkout<P: AsRef<Path>, S: storage::ReadStorage>(
     {
         // Setup default branch.
         let remote_head_ref =
-            git::refs::workdir::remote_branch(&REMOTE_NAME, &project.default_branch);
+            git::refs::workdir::remote_branch(&REMOTE_NAME, project.default_branch());
 
         let remote_head_commit = repo.find_reference(&remote_head_ref)?.peel_to_commit()?;
-        let _ = repo.branch(&project.default_branch, &remote_head_commit, true)?;
+        let _ = repo.branch(project.default_branch(), &remote_head_commit, true)?;
 
         // Setup remote tracking for default branch.
         git::set_upstream(
             &repo,
             &REMOTE_NAME,
-            &project.default_branch,
+            project.default_branch(),
-            &git::refs::workdir::branch(&project.default_branch),
+            &git::refs::workdir::branch(project.default_branch()),
         )?;
     }
 
@@ -403,9 +415,9 @@ mod tests {
         );
 
         assert_eq!(remotes[&public_key].refs, refs);
-        assert_eq!(project.name, "acme");
+        assert_eq!(project.name(), "acme");
-        assert_eq!(project.description, "Acme's repo");
+        assert_eq!(project.description(), "Acme's repo");
-        assert_eq!(project.default_branch, git::refname!("master"));
+        assert_eq!(project.default_branch(), &git::refname!("master"));
         assert_eq!(doc.delegates.first(), &Did::from(public_key));
     }
 

radicle/src/storage/git.rs

@@ -520,7 +520,7 @@ impl ReadRepository for Repository {
         let (_, doc) = self.project_identity()?;
         let doc = doc.verified()?;
         let project = doc.project()?;
-        let branch_ref = Qualified::from(lit::refs_heads(&project.default_branch));
+        let branch_ref = Qualified::from(lit::refs_heads(&project.default_branch()));
         let raw = self.raw();
 
         let mut heads = Vec::new();

radicle/src/test/arbitrary.rs

@@ -93,11 +93,7 @@ impl Arbitrary for Project {
             .try_into()
             .unwrap();
 
-        Project {
+        Project::new(name, description, default_branch).unwrap()
-            name,
-            description,
-            default_branch,
-        }
     }
 }