From 113a061bee3f40134fab7a01f80af24ee4ed349b Mon Sep 17 00:00:00 2001 From: Tamo Date: Wed, 22 Sep 2021 15:09:07 +0200 Subject: [PATCH] fix the error handling on the criterion side --- milli/src/criterion.rs | 181 +++++++++++++++++++++++++++++++++++------ milli/src/error.rs | 36 +++++++- 2 files changed, 189 insertions(+), 28 deletions(-) diff --git a/milli/src/criterion.rs b/milli/src/criterion.rs index c526a7e32..33112508c 100644 --- a/milli/src/criterion.rs +++ b/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::(); + 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", - "_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", + ("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", + 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::(); 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::(); + 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::(); + 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 + ); } } } diff --git a/milli/src/error.rs b/milli/src/error.rs index fe0ac2cf7..2e2d3088e 100644 --- a/milli/src/error.rs +++ b/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 }, InvalidFilter(pest::error::Error), InvalidFilterAttribute(pest::error::Error), 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 }, 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!(