diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 05c76e407b3..cc77ba78f69 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -16,7 +16,7 @@ use tokio::runtime::Handle; use url::Url; use crate::controllers::cargo_prelude::*; -use crate::models::krate::MAX_NAME_LENGTH; +use crate::models::krate::{InvalidDependencyName, MAX_NAME_LENGTH}; use crate::models::{ insert_version_owner_action, Category, Crate, DependencyKind, Keyword, NewCrate, NewVersion, Rights, VersionAction, @@ -238,7 +238,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult max_features { @@ -253,7 +253,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult AppResult<()> { } for feature in &dep.features { - Crate::valid_feature(feature)?; + Crate::validate_feature(feature).map_err(|error| cargo_err(&error))?; } if let Some(registry) = &dep.registry { @@ -623,7 +623,7 @@ pub fn validate_dependency(dep: &EncodableCrateDependency) -> AppResult<()> { if let Some(toml_name) = &dep.explicit_name_in_toml { if !Crate::valid_dependency_name(toml_name) { - return Err(cargo_err(&Crate::invalid_dependency_name_msg(toml_name))); + return Err(cargo_err(&InvalidDependencyName(toml_name.into()))); } } diff --git a/src/models/krate.rs b/src/models/krate.rs index 30cb3381546..d0da6bf6a2c 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -220,32 +220,18 @@ impl Crate { .unwrap_or(false) } - pub fn invalid_dependency_name_msg(dep: &str) -> String { - format!( - "\"{dep}\" is an invalid dependency name (dependency \ - names must start with a letter or underscore, contain only \ - letters, numbers, hyphens, or underscores and have at most \ - {MAX_NAME_LENGTH} characters)" - ) - } - /// Validates the THIS parts of `features = ["THIS", "and/THIS", "dep:THIS", "dep?/THIS"]`. /// 1. The name must be non-empty. /// 2. The first character must be a Unicode XID start character, `_`, or a digit. /// 3. The remaining characters must be Unicode XID characters, `_`, `+`, `-`, or `.`. - pub fn valid_feature_name(name: &str) -> AppResult<()> { + pub fn validate_feature_name(name: &str) -> Result<(), InvalidFeature> { if name.is_empty() { - return Err(cargo_err("feature cannot be an empty")); + return Err(InvalidFeature::Empty); } let mut chars = name.chars(); if let Some(ch) = chars.next() { if !(unicode_xid::UnicodeXID::is_xid_start(ch) || ch == '_' || ch.is_ascii_digit()) { - return Err(cargo_err(&format!( - "invalid character `{}` in feature `{}`, \ - the first character must be a Unicode XID start character or digit \ - (most letters or `_` or `0` to `9`)", - ch, name, - ))); + return Err(InvalidFeature::Start(ch, name.into())); } } for ch in chars { @@ -254,12 +240,7 @@ impl Crate { || ch == '-' || ch == '.') { - return Err(cargo_err(&format!( - "invalid character `{}` in feature `{}`, \ - characters must be Unicode XID characters, `+`, `-`, or `.` \ - (numbers, `+`, `-`, `_`, `.`, or most letters)", - ch, name, - ))); + return Err(InvalidFeature::Char(ch, name.into())); } } @@ -267,20 +248,22 @@ impl Crate { } /// Validates a whole feature string, `features = ["THIS", "and/THIS", "dep:THIS", "dep?/THIS"]`. - pub fn valid_feature(name: &str) -> AppResult<()> { + pub fn validate_feature(name: &str) -> Result<(), InvalidFeature> { if let Some((dep, dep_feat)) = name.split_once('/') { let dep = dep.strip_suffix('?').unwrap_or(dep); if !Crate::valid_dependency_name(dep) { - return Err(cargo_err(&Crate::invalid_dependency_name_msg(dep))); + let err = InvalidDependencyName(dep.into()); + return Err(InvalidFeature::DependencyName(err)); } - Crate::valid_feature_name(dep_feat) + Crate::validate_feature_name(dep_feat) } else if let Some((_, dep)) = name.split_once("dep:") { if !Crate::valid_dependency_name(dep) { - return Err(cargo_err(&Crate::invalid_dependency_name_msg(dep))); + let err = InvalidDependencyName(dep.into()); + return Err(InvalidFeature::DependencyName(err)); } return Ok(()); } else { - Crate::valid_feature_name(name) + Crate::validate_feature_name(name) } } @@ -528,6 +511,34 @@ impl CrateVersions for [Crate] { } } +#[derive(Debug, Eq, PartialEq, thiserror::Error)] +pub enum InvalidFeature { + #[error("feature cannot be empty")] + Empty, + #[error( + "invalid character `{0}` in feature `{1}`, the first character must be \ + a Unicode XID start character or digit (most letters or `_` or `0` to \ + `9`)" + )] + Start(char, String), + #[error( + "invalid character `{0}` in feature `{1}`, characters must be Unicode \ + XID characters, `+`, `-`, or `.` (numbers, `+`, `-`, `_`, `.`, or most \ + letters)" + )] + Char(char, String), + #[error(transparent)] + DependencyName(#[from] InvalidDependencyName), +} + +#[derive(Debug, Eq, PartialEq, thiserror::Error)] +#[error( + "\"{0}\" is an invalid dependency name (dependency names must start with a \ + letter or underscore, contain only letters, numbers, hyphens, or \ + underscores and have at most {MAX_NAME_LENGTH} characters)" +)] +pub struct InvalidDependencyName(pub String); + #[cfg(test)] mod tests { use crate::models::Crate; @@ -559,29 +570,57 @@ mod tests { assert!(Crate::valid_dependency_name("_foo")); assert!(!Crate::valid_dependency_name("-foo")); } + #[test] fn valid_feature_names() { - assert!(Crate::valid_feature("foo").is_ok()); - assert!(Crate::valid_feature("1foo").is_ok()); - assert!(Crate::valid_feature("_foo").is_ok()); - assert!(Crate::valid_feature("_foo-_+.1").is_ok()); - assert!(Crate::valid_feature("_foo-_+.1").is_ok()); - assert!(Crate::valid_feature("").is_err()); - assert!(Crate::valid_feature("/").is_err()); - assert!(Crate::valid_feature("%/%").is_err()); - assert!(Crate::valid_feature("a/a").is_ok()); - assert!(Crate::valid_feature("32-column-tables").is_ok()); - assert!(Crate::valid_feature("c++20").is_ok()); - assert!(Crate::valid_feature("krate/c++20").is_ok()); - assert!(Crate::valid_feature("c++20/wow").is_err()); - assert!(Crate::valid_feature("foo?/bar").is_ok()); - assert!(Crate::valid_feature("dep:foo").is_ok()); - assert!(Crate::valid_feature("dep:foo?/bar").is_err()); - assert!(Crate::valid_feature("foo/?bar").is_err()); - assert!(Crate::valid_feature("foo?bar").is_err()); - assert!(Crate::valid_feature("bar.web").is_ok()); - assert!(Crate::valid_feature("foo/bar.web").is_ok()); - assert!(Crate::valid_feature("dep:0foo").is_err()); - assert!(Crate::valid_feature("0foo?/bar.web").is_err()); + use super::InvalidDependencyName; + use super::InvalidFeature::*; + + assert_ok!(Crate::validate_feature("foo")); + assert_ok!(Crate::validate_feature("1foo")); + assert_ok!(Crate::validate_feature("_foo")); + assert_ok!(Crate::validate_feature("_foo-_+.1")); + assert_ok!(Crate::validate_feature("_foo-_+.1")); + assert_err_eq!(Crate::validate_feature(""), Empty); + assert_err_eq!( + Crate::validate_feature("/"), + InvalidDependencyName("".into()).into() + ); + assert_err_eq!( + Crate::validate_feature("%/%"), + InvalidDependencyName("%".into()).into() + ); + assert_ok!(Crate::validate_feature("a/a")); + assert_ok!(Crate::validate_feature("32-column-tables")); + assert_ok!(Crate::validate_feature("c++20")); + assert_ok!(Crate::validate_feature("krate/c++20")); + assert_err_eq!( + Crate::validate_feature("c++20/wow"), + InvalidDependencyName("c++20".into()).into() + ); + assert_ok!(Crate::validate_feature("foo?/bar")); + assert_ok!(Crate::validate_feature("dep:foo")); + assert_err_eq!( + Crate::validate_feature("dep:foo?/bar"), + InvalidDependencyName("dep:foo".into()).into() + ); + assert_err_eq!( + Crate::validate_feature("foo/?bar"), + Start('?', "?bar".into()) + ); + assert_err_eq!( + Crate::validate_feature("foo?bar"), + Char('?', "foo?bar".into()) + ); + assert_ok!(Crate::validate_feature("bar.web")); + assert_ok!(Crate::validate_feature("foo/bar.web")); + assert_err_eq!( + Crate::validate_feature("dep:0foo"), + InvalidDependencyName("0foo".into()).into() + ); + assert_err_eq!( + Crate::validate_feature("0foo?/bar.web"), + InvalidDependencyName("0foo".into()).into() + ); } } diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__features__empty_feature_name.snap b/src/tests/krate/publish/snapshots/all__krate__publish__features__empty_feature_name.snap index 508aa219266..63b2f9fd1de 100644 --- a/src/tests/krate/publish/snapshots/all__krate__publish__features__empty_feature_name.snap +++ b/src/tests/krate/publish/snapshots/all__krate__publish__features__empty_feature_name.snap @@ -5,7 +5,7 @@ expression: response.into_json() { "errors": [ { - "detail": "feature cannot be an empty" + "detail": "feature cannot be empty" } ] }