Better error msg for invalid feature name

0xPoe · Turbo87 · commit dd9610333b96 · 2023-11-15T16:42:48.000+01:00
Signed-off-by: hi-rustin &lt;rustin.liu@gmail.com&gt;
diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs
@@ -238,12 +238,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult<Json<GoodCra
         }
 
         for (key, values) in features.iter() {
-            if !Crate::valid_feature_name(key) {
-                return Err(cargo_err(&format!(
-                    "\"{key}\" is an invalid feature name (feature names must contain only Unicode XID characters, `+`, `-`, or `.` \
-                (numbers, `+`, `-`, `_`, `.`, or most letters)"
-                )));
-            }
+            Crate::valid_feature_name(key)?;
 
             let num_features = values.len();
             if num_features > max_features {
@@ -258,9 +253,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)?;
             }
         }
 
@@ -603,11 +596,7 @@ pub fn validate_dependency(dep: &EncodableCrateDependency) -> AppResult<()> {
     }
 
     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
@@ -224,14 +224,19 @@ impl Crate {
     /// 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) -> bool {
+    pub fn valid_feature_name(name: &str) -> AppResult<()> {
         if name.is_empty() {
-            return false;
+            return Err(cargo_err("feature cannot be an 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 false;
+                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,
+                )));
             }
         }
         for ch in chars {
@@ -240,20 +245,41 @@ impl Crate {
                 || ch == '-'
                 || ch == '.')
             {
-                return false;
+                return Err(cargo_err(&format!(
+                    "invalid character `{}` in feature `{}`, \
+                characters must be Unicode XID characters, `+`, `-`, or `.` \
+                (numbers, `+`, `-`, `_`, `.`, or most letters)",
+                    ch, name,
+                )));
             }
         }
 
-        true
+        Ok(())
     }
 
     /// Validates a whole feature string, `features = ["THIS", "and/THIS", "dep:THIS", "dep?/THIS"]`.
-    pub fn valid_feature(name: &str) -> bool {
+    pub fn valid_feature(name: &str) -> AppResult<()> {
         if let Some((dep, dep_feat)) = name.split_once('/') {
             let dep = dep.strip_suffix('?').unwrap_or(dep);
-            Crate::valid_dependency_name(dep) && Crate::valid_feature_name(dep_feat)
+            if !Crate::valid_dependency_name(dep) {
+                return Err(cargo_err(&format_args!(
+                    "\"{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)"
+                )));
+            }
+            Crate::valid_feature_name(dep_feat)
         } else if let Some((_, dep)) = name.split_once("dep:") {
-            Crate::valid_dependency_name(dep)
+            if !Crate::valid_dependency_name(dep) {
+                return Err(cargo_err(&format_args!(
+                    "\"{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)"
+                )));
+            }
+            return Ok(());
         } else {
             Crate::valid_feature_name(name)
         }
@@ -534,30 +560,29 @@ mod tests {
         assert!(Crate::valid_dependency_name("_foo"));
         assert!(!Crate::valid_dependency_name("-foo"));
     }
-
     #[test]
     fn valid_feature_names() {
-        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("foo"));
-        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("dep:0foo"));
-        assert!(!Crate::valid_feature("0foo?/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());
+        assert!(Crate::valid_feature("dep:0foo").is_err());
+        assert!(Crate::valid_feature("0foo?/bar.web").is_err());
     }
 }
diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__dependencies__invalid_feature_name.snap b/src/tests/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`)"
     }
   ]
 }
diff --git a/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature_name.snap b/src/tests/krate/publish/snapshots/all__krate__publish__features__invalid_feature_name.snap
@@ -5,7 +5,7 @@ expression: response.into_json()
 {
   "errors": [
     {
-      "detail": "\"~foo\" is an invalid feature name (feature names must contain only Unicode XID characters, `+`, `-`, or `.` (numbers, `+`, `-`, `_`, `.`, or most letters)"
+      "detail": "invalid character `~` in feature `~foo`, the first character must be a Unicode XID start character or digit (most letters or `_` or `0` to `9`)"
     }
   ]
 }

Original file line number	Diff line number	Diff line change
`@@ -238,12 +238,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult<Json<GoodCra`
`238`	`238`	`}`
`239`	`239`
`240`	`240`	`for (key, values) in features.iter() {`
`241`		`- if !Crate::valid_feature_name(key) {`
`242`		`- return Err(cargo_err(&format!(`
`243`		- "\"{key}\" is an invalid feature name (feature names must contain only Unicode XID characters, `+`, `-`, or `.` \
`244`		- (numbers, `+`, `-`, `_`, `.`, or most letters)"
`245`		`- )));`
`246`		`- }`
	`241`	`+ Crate::valid_feature_name(key)?;`
`247`	`242`
`248`	`243`	`let num_features = values.len();`
`249`	`244`	`if num_features > max_features {`
`@@ -258,9 +253,7 @@ pub async fn publish(app: AppState, req: BytesRequest) -> AppResult<Json<GoodCra`
`258`	`253`	`}`
`259`	`254`
`260`	`255`	`for value in values.iter() {`
`261`		`- if !Crate::valid_feature(value) {`
`262`		`- return Err(cargo_err(&format!("\"{value}\" is an invalid feature name")));`
`263`		`- }`
	`256`	`+ Crate::valid_feature(value)?;`
`264`	`257`	`}`
`265`	`258`	`}`
`266`	`259`
`@@ -603,11 +596,7 @@ pub fn validate_dependency(dep: &EncodableCrateDependency) -> AppResult<()> {`
`603`	`596`	`}`
`604`	`597`
`605`	`598`	`for feature in &dep.features {`
`606`		`- if !Crate::valid_feature(feature) {`
`607`		`- return Err(cargo_err(&format_args!(`
`608`		`- "\"{feature}\" is an invalid feature name",`
`609`		`- )));`
`610`		`- }`
	`599`	`+ Crate::valid_feature(feature)?;`
`611`	`600`	`}`
`612`	`601`
`613`	`602`	`if let Some(registry) = &dep.registry {`
Original file line number	Diff line number	Diff line change
`@@ -5,7 +5,7 @@ expression: response.into_json()`
`5`	`5`	`{`
`6`	`6`	`"errors": [`
`7`	`7`	`{`
`8`		`- "detail": "\"🍺\" is an invalid feature name"`
	`8`	+ "detail": "invalid character `🍺` in feature `🍺`, the first character must be a Unicode XID start character or digit (most letters or `_` or `0` to `9`)"
`9`	`9`	`}`
`10`	`10`	`]`
`11`	`11`	`}`
Original file line number	Diff line number	Diff line change
`@@ -5,7 +5,7 @@ expression: response.into_json()`
`5`	`5`	`{`
`6`	`6`	`"errors": [`
`7`	`7`	`{`
`8`		`- "detail": "\"!bar\" is an invalid feature name"`
	`8`	+ "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`)"
`9`	`9`	`}`
`10`	`10`	`]`
`11`	`11`	`}`
Original file line number	Diff line number	Diff line change
`@@ -5,7 +5,7 @@ expression: response.into_json()`
`5`	`5`	`{`
`6`	`6`	`"errors": [`
`7`	`7`	`{`
`8`		- "detail": "\"~foo\" is an invalid feature name (feature names must contain only Unicode XID characters, `+`, `-`, or `.` (numbers, `+`, `-`, `_`, `.`, or most letters)"
	`8`	+ "detail": "invalid character `~` in feature `~foo`, the first character must be a Unicode XID start character or digit (most letters or `_` or `0` to `9`)"
`9`	`9`	`}`
`10`	`10`	`]`
`11`	`11`	`}`