From adaed6a3ac4232b96e7ec93882821f75a9118adb Mon Sep 17 00:00:00 2001 From: Fintan Halpenny Date: Thu, 15 Dec 2022 12:15:07 +0000 Subject: [PATCH] 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 X-Clacks-Overhead: GNU Terry Pratchett --- radicle-cli/src/commands/checkout.rs | 6 +- radicle-cli/src/commands/clone.rs | 4 +- radicle-cli/src/commands/init.rs | 8 +- radicle-cli/src/commands/ls.rs | 7 +- radicle-cli/src/commands/patch/create.rs | 6 +- radicle-cli/src/commands/track.rs | 2 +- radicle-cli/src/commands/untrack.rs | 4 +- radicle/src/identity.rs | 10 +- radicle/src/identity/project.rs | 178 ++++++++++++++++++++--- radicle/src/rad.rs | 46 +++--- radicle/src/storage/git.rs | 2 +- radicle/src/test/arbitrary.rs | 6 +- 12 files changed, 211 insertions(+), 68 deletions(-) diff --git a/radicle-cli/src/commands/checkout.rs b/radicle-cli/src/commands/checkout.rs index 6adb1eec..efbdca35 100644 --- a/radicle-cli/src/commands/checkout.rs +++ b/radicle-cli/src/commands/checkout.rs @@ -84,7 +84,7 @@ pub fn execute(options: Options, profile: &Profile) -> anyhow::Result { .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 { 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 { setup_remotes( project::SetupRemote { project: id, - default_branch: payload.default_branch, + default_branch: payload.default_branch().clone(), repo: &repo, fetch: true, tracking: true, diff --git a/radicle-cli/src/commands/clone.rs b/radicle-cli/src/commands/clone.rs index 3d3fb98a..de689a8a 100644 --- a/radicle-cli/src/commands/clone.rs +++ b/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::>(); - let default_branch = proj.default_branch.clone(); + let default_branch = proj.default_branch().clone(); // Setup tracking for project delegates. setup_remotes( diff --git a/radicle-cli/src/commands/init.rs b/radicle-cli/src/commands/init.rs index b0bfb9e6..cc5a6dd3 100644 --- a/radicle-cli/src/commands/init.rs +++ b/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, - &radicle::git::refs::workdir::branch(&proj.default_branch), + proj.default_branch(), + &radicle::git::refs::workdir::branch(proj.default_branch()), )?; } diff --git a/radicle-cli/src/commands/ls.rs b/radicle-cli/src/commands/ls.rs index 11ca3c88..0bc5b21d 100644 --- a/radicle-cli/src/commands/ls.rs +++ b/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(); diff --git a/radicle-cli/src/commands/patch/create.rs b/radicle-cli/src/commands/patch/create.rs index bf5ae983..08366847 100644 --- a/radicle-cli/src/commands/patch/create.rs +++ b/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//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()), diff --git a/radicle-cli/src/commands/track.rs b/radicle-cli/src/commands/track.rs index ac9a7ece..1f9a70e4 100644 --- a/radicle-cli/src/commands/track.rs +++ b/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(); diff --git a/radicle-cli/src/commands/untrack.rs b/radicle-cli/src/commands/untrack.rs index e6b698c6..1762df8d 100644 --- a/radicle-cli/src/commands/untrack.rs +++ b/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() ); } diff --git a/radicle/src/identity.rs b/radicle/src/identity.rs index bed0ca55..05de2aa9 100644 --- a/radicle/src/identity.rs +++ b/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!?"); } } diff --git a/radicle/src/identity/project.rs b/radicle/src/identity/project.rs index cc3dd6ad..4489763d 100644 --- a/radicle/src/identity/project.rs +++ b/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(deserializer: D) -> Result + 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(self, mut map: V) -> Result + 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::>() + .join(", "), + ) + }) + } + } + const FIELDS: &[&str] = &["name", "descrption", "defaultBranch"]; + deserializer.deserialize_struct("Project", FIELDS, ProjectVisitor) + } } impl Project { - /// Validate the project data. - pub fn validate(&self) -> Result<(), ProjectError> { - if self.name.is_empty() { - return Err(ProjectError::Name("name cannot be empty")); + /// Create a new `Project` payload with the given values. + /// + /// These values are subject to validation and any errors are returned in a vector. + /// + /// # 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> { + 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 self.description.len() > doc::MAX_STRING_LENGTH { - return Err(ProjectError::Description( + + if description.len() > doc::MAX_STRING_LENGTH { + errs.push(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", - )); - } - if self.default_branch.len() > doc::MAX_STRING_LENGTH { - return Err(ProjectError::DefaultBranch( + )) + } else if default_branch.len() > doc::MAX_STRING_LENGTH { + errs.push(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>, + description: impl Into>, + default_branch: impl Into>, + ) -> Result> { + 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 } } diff --git a/radicle/src/rad.rs b/radicle/src/rad.rs index c843088b..48a5f639 100644 --- a/radicle/src/rad.rs +++ b/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( // 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 { - name: name.to_owned(), - description: description.to_owned(), - default_branch: default_branch.clone(), - }; + let proj = Project::new( + name.to_owned(), + description.to_owned(), + default_branch.clone(), + ) + .map_err(|errs| { + InitError::ProjectPayload( + errs.into_iter() + .map(|err| err.to_string()) + .collect::>() + .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( let repository = storage.repository(proj)?; let raw = repository.raw(); - let remote_head = - raw.refname_to_id(&git::refs::storage::branch(remote, &project.default_branch))?; + let remote_head = raw.refname_to_id(&git::refs::storage::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, 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, 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, - &git::refs::workdir::branch(&project.default_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.description, "Acme's repo"); - assert_eq!(project.default_branch, git::refname!("master")); + assert_eq!(project.name(), "acme"); + assert_eq!(project.description(), "Acme's repo"); + assert_eq!(project.default_branch(), &git::refname!("master")); assert_eq!(doc.delegates.first(), &Did::from(public_key)); } diff --git a/radicle/src/storage/git.rs b/radicle/src/storage/git.rs index c53d467d..64eb09f2 100644 --- a/radicle/src/storage/git.rs +++ b/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(); diff --git a/radicle/src/test/arbitrary.rs b/radicle/src/test/arbitrary.rs index 18e7fdc9..bc642466 100644 --- a/radicle/src/test/arbitrary.rs +++ b/radicle/src/test/arbitrary.rs @@ -93,11 +93,7 @@ impl Arbitrary for Project { .try_into() .unwrap(); - Project { - name, - description, - default_branch, - } + Project::new(name, description, default_branch).unwrap() } }