Skip to content
This repository was archived by the owner on Feb 28, 2021. It is now read-only.

Adjust project name validations #247

Merged
merged 1 commit into from
Mar 10, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cli/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ pub trait CommandT {
/// Show information for a registered project.
pub struct ShowProject {
/// The name of the project
project_name: String32,
project_name: ProjectName,
/// The org in which the project is registered.
org_id: OrgId,
}
Expand Down Expand Up @@ -192,7 +192,7 @@ impl CommandT for UnregisterOrg {
/// Register a project with the given name under the given org.
pub struct RegisterProject {
/// Name of the project to register.
project_name: String32,
project_name: ProjectName,

/// Org under which to register the project.
org_id: OrgId,
Expand Down
2 changes: 1 addition & 1 deletion client/examples/project_registration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ async fn go() -> Result<(), Error> {
let node_host = url::Host::parse("127.0.0.1").unwrap();
let client = Client::create(node_host).await?;

let project_name = ProjectName::from_string("radicle-registry".to_string()).unwrap();
let project_name = ProjectName::try_from("radicle-registry").unwrap();
let org_id = OrgId::try_from("monadic").unwrap();

// Choose some random project hash and create a checkpoint
Expand Down
6 changes: 3 additions & 3 deletions core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ pub use string32::String32;
mod org_id;
pub use org_id::OrgId;

mod project_name;
pub use project_name::ProjectName;

mod error;
pub use error::RegistryError;

Expand All @@ -58,9 +61,6 @@ pub type Balance = u128;
/// The id of a project. Used as storage key.
pub type ProjectId = (ProjectName, OrgId);

/// The name a project is registered with.
pub type ProjectName = String32;

/// Org
///
/// Different from [state::Org] in which this type gathers
Expand Down
208 changes: 208 additions & 0 deletions core/src/project_name.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,208 @@
// Radicle Registry
// Copyright (C) 2019 Monadic GmbH <[email protected]>
//
// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License version 3 as
// published by the Free Software Foundation.
//
// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.

/// The name associated to a [`Project`].
///
/// https://github.com/radicle-dev/registry-spec/blob/master/body.tex#L306
use alloc::prelude::v1::*;
use core::convert::{From, Into, TryFrom};
use parity_scale_codec as codec;

#[derive(codec::Encode, Clone, Debug, Eq, PartialEq)]
pub struct ProjectName(String);

impl ProjectName {
fn from_string(input: String) -> Result<Self, InvalidProjectNameError> {
// Must be at least 1 character.
if input.is_empty() {
return Err(InvalidProjectNameError("must be at least 1 character"));
}
// Must be no longer than 32.
if input.len() > 32 {
return Err(InvalidProjectNameError("must not exceed 32 characters"));
}

// Must only contain a-z, 0-9, '-', '_' and '.' characters.
{
let check_charset = |c: char| {
c.is_ascii_digit() || c.is_ascii_lowercase() || c == '-' || c == '_' || c == '.'
};

if !input.chars().all(check_charset) {
return Err(InvalidProjectNameError(
"must only include a-z, 0-9, '-', '_' and '.'",
));
}
}

// Must not equal '.' or '..'.
if input == "." || input == ".." {
return Err(InvalidProjectNameError("must not be equal to '.' or '..'"));
}

let id = Self(input);

Ok(id)
}
}

impl codec::Decode for ProjectName {
fn decode<I: codec::Input>(input: &mut I) -> Result<Self, codec::Error> {
let decoded: String = String::decode(input)?;

match Self::try_from(decoded) {
Ok(id) => Ok(id),
Err(err) => Err(codec::Error::from(err.what())),
}
}
}

impl Into<String> for ProjectName {
fn into(self) -> String {
self.0
}
}

impl TryFrom<String> for ProjectName {
type Error = InvalidProjectNameError;

fn try_from(input: String) -> Result<Self, Self::Error> {
Self::from_string(input)
}
}

impl TryFrom<&str> for ProjectName {
type Error = InvalidProjectNameError;

fn try_from(input: &str) -> Result<Self, Self::Error> {
Self::from_string(input.to_string())
}
}

impl core::str::FromStr for ProjectName {
type Err = InvalidProjectNameError;

fn from_str(s: &str) -> Result<Self, Self::Err> {
Self::from_string(s.to_string())
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's too bad we need both FromStr and TryFrom<&str>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm honestly curious, why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They provide different APIS (from_str and try_from("") respectively) I saw a FromStr implementation. It seems the main reason for FromStr being around and distinct from TryFrom<T> is for parse semantics: rust-lang/rfcs#2143 (comment)

I'm not certain about our requirements, but happy to settle on of the methods.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d prefer to have one way to do it with TryFrom but I’m ok if we leave it. If we change it here we should align it with the org ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did an audit of the codebase for occurrences of from_str and found none. Lo and behold, structopt derivation needs it anyway. I suspect for the parse semantics mentioned above. Will leave it for now and we can revise later.


#[cfg(feature = "std")]
impl core::fmt::Display for ProjectName {
fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
write!(f, "{}", self.0)
}
}

/// Error type when conversion from an inordinate input failed.
#[derive(codec::Encode, Clone, Debug, Eq, PartialEq)]
pub struct InvalidProjectNameError(&'static str);

impl InvalidProjectNameError {
/// Error description
///
/// This function returns an actual error str when running in `std`
/// environment, but `""` on `no_std`.
#[cfg(feature = "std")]
pub fn what(&self) -> &'static str {
self.0
}

/// Error description
///
/// This function returns an actual error str when running in `std`
/// environment, but `""` on `no_std`.
#[cfg(not(feature = "std"))]
pub fn what(&self) -> &'static str {
""
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use thiserror here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need input what we need want and what is possible in the no_std context.


#[cfg(feature = "std")]
impl std::fmt::Display for InvalidProjectNameError {
fn fmt(&self, f: &mut core::fmt::Formatter) -> std::fmt::Result {
write!(f, "InvalidProjectNameError({})", self.0)
}
}

#[cfg(feature = "std")]
impl std::error::Error for InvalidProjectNameError {
fn description(&self) -> &str {
self.0
}
}

impl From<&'static str> for InvalidProjectNameError {
#[cfg(feature = "std")]
fn from(s: &'static str) -> Self {
Self(s)
}

#[cfg(not(feature = "std"))]
fn from(s: &'static str) -> Self {
InvalidProjectNameError(s)
}
}

#[cfg(test)]
mod test {
use super::ProjectName;
use parity_scale_codec::{Decode, Encode};

#[test]
fn name_too_short() {
assert!(ProjectName::from_string("".into()).is_err());
}

#[test]
fn name_too_long() {
let input = std::iter::repeat("X").take(33).collect::<String>();
let too_long = ProjectName::from_string(input);
assert!(too_long.is_err());
}

#[test]
fn name_invalid_characters() {
let invalid_characters = ProjectName::from_string("AZ+*".into());
assert!(invalid_characters.is_err());
}

#[test]
fn name_is_dot() {
let dot = ProjectName::from_string(".".into());
assert!(dot.is_err());
}

#[test]
fn name_is_double_dot() {
let dot = ProjectName::from_string("..".into());
assert!(dot.is_err());
}

#[test]
fn name_valid() {
let valid = ProjectName::from_string("--radicle_registry001".into());
assert!(valid.is_ok());
}

#[test]
fn encode_then_decode() {
let id = ProjectName::from_string("monadic".into()).unwrap();
let encoded = id.encode();
let decoded = <ProjectName>::decode(&mut &encoded[..]).unwrap();

assert_eq!(id, decoded)
}
}
2 changes: 1 addition & 1 deletion runtime/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ mod test {
/// is identify as to the original input id.
fn projects_decode_key_identity() {
let org_id = OrgId::try_from("monadic").unwrap();
let project_name = ProjectName::from_string("Radicle".into()).unwrap();
let project_name = ProjectName::try_from("radicle".to_string()).unwrap();
let project_id: ProjectId = (project_name, org_id);
let hashed_key = store::Projects::storage_map_final_key(project_id.clone());
let decoded_key = store::Projects::decode_key(&hashed_key).unwrap();
Expand Down
7 changes: 6 additions & 1 deletion test-utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,13 +89,18 @@ pub fn random_org_id() -> OrgId {
OrgId::try_from(random_string(size).to_lowercase()).unwrap()
}

pub fn random_project_name() -> ProjectName {
let size = rand::thread_rng().gen_range(1, 33);
ProjectName::try_from(random_string(size).to_lowercase()).unwrap()
}

/// Create a [core::message::RegisterProject] with random parameters to register a project with.
pub fn random_register_project_message(
org_id: OrgId,
checkpoint_id: CheckpointId,
) -> message::RegisterProject {
message::RegisterProject {
project_name: random_string32(),
project_name: random_project_name(),
org_id,
checkpoint_id,
metadata: Bytes128::random(),
Expand Down