Merge pull request #7537 from Turbo87/feature-validation

models/krate: Improve feature validation

Author: 0xPoe

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<Json<GoodCra
         }
 
         for (key, values) in features.iter() {
-            Crate::valid_feature_name(key)?;
+            Crate::validate_feature_name(key).map_err(|error| cargo_err(&error))?;
 
             let num_features = values.len();
             if num_features > max_features {
@@ -253,7 +253,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult<Json<GoodCra
             }
 
             for value in values.iter() {
-                Crate::valid_feature(value)?;
+                Crate::validate_feature(value).map_err(|error| cargo_err(&error))?;
             }
         }
 
@@ -596,7 +596,7 @@ pub fn validate_dependency(dep: &EncodableCrateDependency) -> 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())));
         }
     }
 

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,33 +240,30 @@ 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()));
             }
         }
 
         Ok(())
     }
 
     /// 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()
+        );
     }
 }

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"
     }
   ]
 }