Refactor valid_feature function to return

AppResult. Signed-off-by: hi-rustin <[email protected]>
rust-lang · Nov 8, 2023 · c4f3be2 · c4f3be2
1 parent 3740cf0
commit c4f3be2
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 35 deletions.
diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs
@@ -244,9 +244,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult<Json<GoodCra
             }
 
             for value in values.iter() {
-                if !Crate::valid_feature(value) {
-                    return Err(cargo_err(&format!("\"{value}\" is an invalid feature name")));
-                }
+                Crate::valid_feature(value)?;
             }
         }
 
@@ -577,11 +575,7 @@ pub fn validate_dependency(dep: &EncodableCrateDependency) -> AppResult<()> {
     Crate::valid_name(&dep.name)?;
 
     for feature in &dep.features {
-        if !Crate::valid_feature(feature) {
-            return Err(cargo_err(&format_args!(
-                "\"{feature}\" is an invalid feature name",
-            )));
-        }
+        Crate::valid_feature(feature)?;
     }
 
     if let Some(registry) = &dep.registry {

diff --git a/src/models/krate.rs b/src/models/krate.rs
@@ -288,15 +288,15 @@ impl Crate {
         Ok(())
     }
 
-    /// Validates a whole feature string, `features = ["THIS", "ALL/THIS"]`.
-    pub fn valid_feature(name: &str) -> bool {
+    /// Validates a whole feature string, `features = ["THIS", "and/THIS", "dep:THIS", "dep?/THIS"]`.
+    pub fn valid_feature(name: &str) -> AppResult<()> {
         match name.split_once('/') {
             Some((dep, dep_feat)) => {
                 let dep = dep.strip_suffix('?').unwrap_or(dep);
-                Crate::valid_dependency_name(dep).is_ok()
-                    && Crate::valid_feature_name(dep_feat).is_ok()
+                Crate::valid_dependency_name(dep)?;
+                Crate::valid_feature_name(dep_feat)
             }
-            None => Crate::valid_feature_name(name.strip_prefix("dep:").unwrap_or(name)).is_ok(),
+            None => Crate::valid_feature_name(name.strip_prefix("dep:").unwrap_or(name)),
         }
     }
 
@@ -567,25 +567,25 @@ mod tests {
 
     #[test]
     fn valid_feature_names() {
-        assert!(Crate::valid_feature("foo"));
-        assert!(Crate::valid_feature("1foo"));
-        assert!(Crate::valid_feature("_foo"));
-        assert!(Crate::valid_feature("_foo-_+.1"));
-        assert!(Crate::valid_feature("_foo-_+.1"));
-        assert!(!Crate::valid_feature(""));
-        assert!(!Crate::valid_feature("/"));
-        assert!(!Crate::valid_feature("%/%"));
-        assert!(Crate::valid_feature("a/a"));
-        assert!(Crate::valid_feature("32-column-tables"));
-        assert!(Crate::valid_feature("c++20"));
-        assert!(Crate::valid_feature("krate/c++20"));
-        assert!(!Crate::valid_feature("c++20/wow"));
-        assert!(Crate::valid_feature("foo?/bar"));
-        assert!(Crate::valid_feature("dep:foo"));
-        assert!(!Crate::valid_feature("dep:foo?/bar"));
-        assert!(!Crate::valid_feature("foo/?bar"));
-        assert!(!Crate::valid_feature("foo?bar"));
-        assert!(Crate::valid_feature("bar.web"));
-        assert!(Crate::valid_feature("foo/bar.web"));
+        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());
     }
 }
diff --git a/...ests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_feature_name.snap b/...ests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_feature_name.snap
@@ -5,7 +5,7 @@ expression: response.into_json()
 {
   "errors": [
     {
-      "detail": "\"🍺\" is an invalid feature name"
+      "detail": "invalid character `🍺` in feature `🍺`, the first character must be a Unicode XID start character or digit (most letters or `_` or `0` to `9`)"
     }
   ]
 }
diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature.snap b/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature.snap
@@ -5,7 +5,7 @@ expression: response.into_json()
 {
   "errors": [
     {
-      "detail": "\"!bar\" is an invalid feature name"
+      "detail": "invalid character `!` in feature `!bar`, the first character must be a Unicode XID start character or digit (most letters or `_` or `0` to `9`)"
     }
   ]
 }