diff --git a/milli/src/criterion.rs b/milli/src/criterion.rs index b95080b4b..eb9a6c86a 100644 --- a/milli/src/criterion.rs +++ b/milli/src/criterion.rs @@ -50,9 +50,6 @@ impl FromStr for Criterion { "sort" => Ok(Criterion::Sort), "exactness" => Ok(Criterion::Exactness), text => match AscDesc::from_str(text) { - Ok(AscDesc::Asc(Member::Field(field))) if is_reserved_keyword(&field) => { - Err(UserError::InvalidReservedRankingRuleName { name: text.to_string() })? - } 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(_))) => { @@ -79,11 +76,8 @@ impl FromStr for Member { type Err = UserError; fn from_str(text: &str) -> Result { - if text.starts_with("_geoPoint(") { - let point = - text.strip_prefix("_geoPoint(") - .and_then(|point| point.strip_suffix(")")) - .ok_or_else(|| UserError::InvalidRankingRuleName { name: text.to_string() })?; + if let Some(point) = text.strip_prefix("_geoPoint(").and_then(|text| text.strip_suffix(")")) + { let (lat, long) = point .split_once(',') .ok_or_else(|| UserError::InvalidRankingRuleName { name: text.to_string() }) @@ -95,6 +89,9 @@ impl FromStr for Member { })?; Ok(Member::Geo([lat, long])) } else { + if is_reserved_keyword(text) { + return Err(UserError::InvalidReservedRankingRuleName { name: text.to_string() })?; + } Ok(Member::Field(text.to_string())) } } @@ -185,3 +182,63 @@ impl fmt::Display for Criterion { } } } + +#[cfg(test)] +mod tests { + 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")))), + ("a-b:desc", Desc(Field(S("a-b")))), + ("a:b:desc", Desc(Field(S("a:b")))), + ("a12:asc", Asc(Field(S("a12")))), + ("42:asc", Asc(Field(S("42")))), + ("_geoPoint(42, 59):asc", Asc(Geo([42., 59.]))), + ("_geoPoint(42.459, 59):desc", Desc(Geo([42.459, 59.]))), + ("_geoPoint(42, 59.895):desc", Desc(Geo([42., 59.895]))), + ("_geoPoint(42, 59.895):desc", Desc(Geo([42., 59.895]))), + ("_geoPoint(42.0002, 59.895):desc", Desc(Geo([42.0002, 59.895]))), + ("_geoPoint(42., 59.):desc", Desc(Geo([42., 59.]))), + ("truc(12, 13):desc", Desc(Field(S("truc(12, 13)")))), + ]; + + 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 invalid_req = [ + "truc:machin", + "truc:deesc", + "truc:asc:deesc", + "42desc", + "_geoPoint:asc", + "_geoDistance:asc", + "_geoPoint(42.12 , 59.598)", + "_geoPoint(42.12 , 59.598):deesc", + "_geoPoint(42.12 , 59.598):machin", + "_geoPoint(42.12 , 59.598):asc:aasc", + "_geoPoint(42,12 , 59,598):desc", + "_geoPoint(35, 85, 75):asc", + "_geoPoint(18):asc", + ]; + + for req in invalid_req { + let res = req.parse::(); + assert!( + res.is_err(), + "Should no be able to parse `{}`, was expecting an error but instead got: `{:?}`", + req, + res, + ); + } + } +} diff --git a/milli/src/error.rs b/milli/src/error.rs index 21b77b5a7..e6bd3fd62 100644 --- a/milli/src/error.rs +++ b/milli/src/error.rs @@ -13,7 +13,7 @@ use crate::{DocumentId, FieldId}; pub type Object = Map; pub fn is_reserved_keyword(keyword: &str) -> bool { - ["_geo", "_geoDistance"].contains(&keyword) + ["_geo", "_geoDistance", "_geoPoint", "_geoRadius"].contains(&keyword) } #[derive(Debug)]