fix the error handling on the criterion side

milli/src/criterion.rs

@@ -53,10 +53,31 @@ impl FromStr for Criterion {
                 Ok(AscDesc::Asc(Member::Field(field))) => Ok(Criterion::Asc(field)),
                 Ok(AscDesc::Desc(Member::Field(field))) => Ok(Criterion::Desc(field)),
                 Ok(AscDesc::Asc(Member::Geo(_))) | Ok(AscDesc::Desc(Member::Geo(_))) => {
-                    Err(UserError::InvalidRankingRuleName { name: text.to_string() })?
+                    Err(UserError::InvalidReservedRankingRuleNameSort {
+                        name: "_geoPoint".to_string(),
+                    })?
                 }
                 Err(UserError::InvalidAscDescSyntax { name }) => {
-                    Err(UserError::InvalidRankingRuleName { name }.into())
+                    Err(UserError::InvalidRankingRuleName { name })?
+                }
+                Err(UserError::InvalidReservedAscDescSyntax { name })
+                    if name.starts_with("_geoPoint") =>
+                {
+                    Err(UserError::InvalidReservedRankingRuleNameSort {
+                        name: "_geoPoint".to_string(),
+                    }
+                    .into())
+                }
+                Err(UserError::InvalidReservedAscDescSyntax { name })
+                    if name.starts_with("_geoRadius") =>
+                {
+                    Err(UserError::InvalidReservedRankingRuleNameFilter {
+                        name: "_geoRadius".to_string(),
+                    }
+                    .into())
+                }
+                Err(UserError::InvalidReservedAscDescSyntax { name }) => {
+                    Err(UserError::InvalidReservedRankingRuleName { name }.into())
                 }
                 Err(error) => {
                     Err(UserError::InvalidRankingRuleName { name: error.to_string() }.into())
@@ -80,20 +101,22 @@ impl FromStr for Member {
             Some(point) => {
                 let (lat, long) = point
                     .split_once(',')
-                    .ok_or_else(|| UserError::InvalidRankingRuleName { name: text.to_string() })
+                    .ok_or_else(|| UserError::InvalidReservedAscDescSyntax {
+                        name: text.to_string(),
+                    })
                     .and_then(|(lat, long)| {
                         lat.trim()
                             .parse()
                             .and_then(|lat| long.trim().parse().map(|long| (lat, long)))
-                            .map_err(|_| UserError::InvalidRankingRuleName {
+                            .map_err(|_| UserError::InvalidReservedAscDescSyntax {
                                 name: text.to_string(),
                             })
                     })?;
                 Ok(Member::Geo([lat, long]))
             }
             None => {
-                if is_reserved_keyword(text) {
-                    return Err(UserError::InvalidReservedRankingRuleName {
+                if is_reserved_keyword(text) || text.starts_with("_geoRadius(") {
+                    return Err(UserError::InvalidReservedAscDescSyntax {
                         name: text.to_string(),
                     })?;
                 }
@@ -191,14 +214,15 @@ impl fmt::Display for Criterion {
 
 #[cfg(test)]
 mod tests {
+    use big_s::S;
+    use AscDesc::*;
+    use Member::*;
+    use UserError::*;
+
     use super::*;
 
     #[test]
     fn parse_asc_desc() {
-        use big_s::S;
-        use AscDesc::*;
-        use Member::*;
-
         let valid_req = [
             ("truc:asc", Asc(Field(S("truc")))),
             ("bidule:desc", Desc(Field(S("bidule")))),
@@ -216,28 +240,52 @@ mod tests {
         ];
 
         for (req, expected) in valid_req {
-            let res = req.parse();
-            assert!(res.is_ok(), "Failed to parse `{}`, was expecting `{:?}`", req, expected);
-            assert_eq!(expected, res.unwrap());
+            let res = req.parse::<AscDesc>();
+            assert!(
+                res.is_ok(),
+                "Failed to parse `{}`, was expecting `{:?}` but instead got `{:?}`",
+                req,
+                expected,
+                res
+            );
+            assert_eq!(res.unwrap(), expected);
         }
 
         let invalid_req = [
-            "truc:machin",
-            "truc:deesc",
-            "truc:asc:deesc",
-            "42desc",
-            "_geoPoint:asc",
-            "_geoDistance:asc",
+            ("truc:machin", InvalidAscDescSyntax { name: S("truc:machin") }),
+            ("truc:deesc", InvalidAscDescSyntax { name: S("truc:deesc") }),
+            ("truc:asc:deesc", InvalidAscDescSyntax { name: S("truc:asc:deesc") }),
+            ("42desc", InvalidAscDescSyntax { name: S("42desc") }),
+            ("_geoPoint:asc", InvalidReservedAscDescSyntax { name: S("_geoPoint") }),
+            ("_geoDistance:asc", InvalidReservedAscDescSyntax { name: S("_geoDistance") }),
+            (
                 "_geoPoint(42.12 , 59.598)",
+                InvalidAscDescSyntax { name: S("_geoPoint(42.12 , 59.598)") },
+            ),
+            (
                 "_geoPoint(42.12 , 59.598):deesc",
+                InvalidAscDescSyntax { name: S("_geoPoint(42.12 , 59.598):deesc") },
+            ),
+            (
                 "_geoPoint(42.12 , 59.598):machin",
+                InvalidAscDescSyntax { name: S("_geoPoint(42.12 , 59.598):machin") },
+            ),
+            (
                 "_geoPoint(42.12 , 59.598):asc:aasc",
+                InvalidAscDescSyntax { name: S("_geoPoint(42.12 , 59.598):asc:aasc") },
+            ),
+            (
                 "_geoPoint(42,12 , 59,598):desc",
+                InvalidReservedAscDescSyntax { name: S("_geoPoint(42,12 , 59,598)") },
+            ),
+            (
                 "_geoPoint(35, 85, 75):asc",
-            "_geoPoint(18):asc",
+                InvalidReservedAscDescSyntax { name: S("_geoPoint(35, 85, 75)") },
+            ),
+            ("_geoPoint(18):asc", InvalidReservedAscDescSyntax { name: S("_geoPoint(18)") }),
         ];
 
-        for req in invalid_req {
+        for (req, expected_error) in invalid_req {
             let res = req.parse::<AscDesc>();
             assert!(
                 res.is_err(),
@@ -245,6 +293,85 @@ mod tests {
                 req,
                 res,
             );
+            let res = res.unwrap_err();
+            assert_eq!(
+                res.to_string(),
+                expected_error.to_string(),
+                "Bad error for input {}: got `{:?}` instead of `{:?}`",
+                req,
+                res,
+                expected_error
+            );
+        }
+    }
+
+    #[test]
+    fn parse_criterion() {
+        let valid_criteria = [
+            ("words", Criterion::Words),
+            ("typo", Criterion::Typo),
+            ("proximity", Criterion::Proximity),
+            ("attribute", Criterion::Attribute),
+            ("sort", Criterion::Sort),
+            ("exactness", Criterion::Exactness),
+            ("price:asc", Criterion::Asc(S("price"))),
+            ("price:desc", Criterion::Desc(S("price"))),
+            ("price:asc:desc", Criterion::Desc(S("price:asc"))),
+            ("truc:machin:desc", Criterion::Desc(S("truc:machin"))),
+            ("hello-world!:desc", Criterion::Desc(S("hello-world!"))),
+            ("it's spacy over there:asc", Criterion::Asc(S("it's spacy over there"))),
+        ];
+
+        for (input, expected) in valid_criteria {
+            let res = input.parse::<Criterion>();
+            assert!(
+                res.is_ok(),
+                "Failed to parse `{}`, was expecting `{:?}` but instead got `{:?}`",
+                input,
+                expected,
+                res
+            );
+            assert_eq!(res.unwrap(), expected);
+        }
+
+        let invalid_criteria = [
+            ("words suffix", InvalidRankingRuleName { name: S("words suffix") }),
+            ("prefix typo", InvalidRankingRuleName { name: S("prefix typo") }),
+            ("proximity attribute", InvalidRankingRuleName { name: S("proximity attribute") }),
+            ("price", InvalidRankingRuleName { name: S("price") }),
+            ("asc:price", InvalidRankingRuleName { name: S("asc:price") }),
+            ("price:deesc", InvalidRankingRuleName { name: S("price:deesc") }),
+            ("price:aasc", InvalidRankingRuleName { name: S("price:aasc") }),
+            ("price:asc and desc", InvalidRankingRuleName { name: S("price:asc and desc") }),
+            ("price:asc:truc", InvalidRankingRuleName { name: S("price:asc:truc") }),
+            ("_geo:asc", InvalidReservedRankingRuleName { name: S("_geo") }),
+            ("_geoDistance:asc", InvalidReservedRankingRuleName { name: S("_geoDistance") }),
+            ("_geoPoint:asc", InvalidReservedRankingRuleNameSort { name: S("_geoPoint") }),
+            ("_geoPoint(42, 75):asc", InvalidReservedRankingRuleNameSort { name: S("_geoPoint") }),
+            ("_geoRadius:asc", InvalidReservedRankingRuleNameFilter { name: S("_geoRadius") }),
+            (
+                "_geoRadius(42, 75, 59):asc",
+                InvalidReservedRankingRuleNameFilter { name: S("_geoRadius") },
+            ),
+        ];
+
+        for (input, expected) in invalid_criteria {
+            let res = input.parse::<Criterion>();
+            assert!(
+                res.is_err(),
+                "Should no be able to parse `{}`, was expecting an error but instead got: `{:?}`",
+                input,
+                res
+            );
+            let res = res.unwrap_err();
+            assert_eq!(
+                res.to_string(),
+                expected.to_string(),
+                "Bad error for input {}: got `{:?}` instead of `{:?}`",
+                input,
+                res,
+                expected
+            );
         }
     }
 }

milli/src/error.rs

@@ -57,14 +57,18 @@ pub enum UserError {
     AttributeLimitReached,
     DocumentLimitReached,
     InvalidAscDescSyntax { name: String },
+    InvalidReservedAscDescSyntax { name: String },
     InvalidDocumentId { document_id: Value },
     InvalidFacetsDistribution { invalid_facets_name: HashSet<String> },
     InvalidFilter(pest::error::Error<ParserRule>),
     InvalidFilterAttribute(pest::error::Error<ParserRule>),
     InvalidSortName { name: String },
+    InvalidReservedSortName { name: String },
     InvalidGeoField { document_id: Value, object: Value },
     InvalidRankingRuleName { name: String },
     InvalidReservedRankingRuleName { name: String },
+    InvalidReservedRankingRuleNameSort { name: String },
+    InvalidReservedRankingRuleNameFilter { name: String },
     InvalidSortableAttribute { field: String, valid_fields: HashSet<String> },
     SortRankingRuleMissing,
     InvalidStoreFile,
@@ -230,10 +234,40 @@ impl fmt::Display for UserError {
                 "the document with the id: {} contains an invalid _geo field: {}",
                 document_id, object
             ),
-            Self::InvalidRankingRuleName { name } => write!(f, "invalid criterion {}", name),
+            Self::InvalidRankingRuleName { name } => write!(f, "invalid ranking rule {}", name),
+            Self::InvalidReservedAscDescSyntax { name } => {
+                write!(
+                    f,
+                    "{} is a reserved keyword and thus can't be used as a asc/desc rule",
+                    name
+                )
+            }
             Self::InvalidReservedRankingRuleName { name } => {
                 write!(f, "{} is a reserved keyword and thus can't be used as a ranking rule", name)
             }
+            Self::InvalidReservedRankingRuleNameSort { name } => {
+                write!(
+                    f,
+                    "{0} is a reserved keyword and thus can't be used as a ranking rule. \
+{0} can only be used for sorting at search time",
+                    name
+                )
+            }
+            Self::InvalidReservedRankingRuleNameFilter { name } => {
+                write!(
+                    f,
+                    "{0} is a reserved keyword and thus can't be used as a ranking rule. \
+{0} can only be used for filtering at search time",
+                    name
+                )
+            }
+            Self::InvalidReservedSortName { name } => {
+                write!(
+                    f,
+                    "{} is a reserved keyword and thus can't be used as a sort expression",
+                    name
+                )
+            }
             Self::InvalidDocumentId { document_id } => {
                 let json = serde_json::to_string(document_id).unwrap();
                 write!(