From 469d92c569a41e0f98a81e9597ccfec02466282c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=88=98=E7=80=9A=E9=AA=8B?= Date: Sun, 10 Oct 2021 14:50:59 +0800 Subject: [PATCH] tweak error handling --- milli/src/error.rs | 6 +- milli/src/search/facet/filter_condition.rs | 75 ++++------------------ milli/src/search/facet/filter_parser.rs | 71 +++++++++++++++++--- 3 files changed, 74 insertions(+), 78 deletions(-) diff --git a/milli/src/error.rs b/milli/src/error.rs index b80238468..3ae18165f 100644 --- a/milli/src/error.rs +++ b/milli/src/error.rs @@ -210,6 +210,8 @@ impl fmt::Display for UserError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { //TODO + Self::InvalidFilterAttributeNom => write!(f, "parser error "), + Self::InvalidFilterValue => write!(f, "parser error "), Self::InvalidFilterNom { input } => write!(f, "parser error {}", input), Self::AttributeLimitReached => f.write_str("maximum number of attributes reached"), Self::CriterionError(error) => write!(f, "{}", error), @@ -228,10 +230,6 @@ impl fmt::Display for UserError { "the document with the id: {} contains an invalid _geo field: {}", document_id, object ), - Self::InvalidAscDescSyntax { name } => { - write!(f, "invalid asc/desc syntax for {}", name) - } - Self::InvalidCriterionName { name } => write!(f, "invalid criterion {}", name), Self::InvalidDocumentId { document_id } => { let json = serde_json::to_string(document_id).unwrap(); write!( diff --git a/milli/src/search/facet/filter_condition.rs b/milli/src/search/facet/filter_condition.rs index c728e0acd..c4fa8e561 100644 --- a/milli/src/search/facet/filter_condition.rs +++ b/milli/src/search/facet/filter_condition.rs @@ -5,7 +5,7 @@ use std::ops::Bound::{self, Excluded, Included}; use either::Either; use heed::types::DecodeIgnore; use log::debug; -use nom::error::VerboseError; +use nom::error::{convert_error, VerboseError}; use roaring::RoaringBitmap; use self::FilterCondition::*; @@ -87,7 +87,15 @@ impl FilterCondition { ParseContext { fields_ids_map: &fields_ids_map, filterable_fields: &filterable_fields }; match ctx.parse_expression::>(expression) { Ok((_, fc)) => Ok(fc), - Err(e) => Err(Error::UserError(UserError::InvalidFilterNom { input: e.to_string() })), + Err(e) => { + match e { + nom::Err::Error(x) => { + println!("verbose err:\n{}", convert_error(expression, x)) + } + _ => unreachable!(), + } + Err(Error::UserError(UserError::InvalidFilterNom { input: "whatever".to_string() })) + } } } pub fn negate(self) -> FilterCondition { @@ -101,67 +109,6 @@ impl FilterCondition { Empty => Empty, } } - - fn geo_radius( - fields_ids_map: &FieldsIdsMap, - filterable_fields: &HashSet, - item: Pair, - ) -> Result { - if !filterable_fields.contains("_geo") { - return Err(UserError::InvalidFilterAttribute(PestError::new_from_span( - ErrorVariant::CustomError { - message: format!( - "attribute `_geo` is not filterable, available filterable attributes are: {}", - filterable_fields.iter().join(", "), - ), - }, - item.as_span(), - )))?; - } - let mut items = item.into_inner(); - let fid = match fields_ids_map.id("_geo") { - Some(fid) => fid, - None => return Ok(Empty), - }; - let parameters_item = items.next().unwrap(); - // We don't need more than 3 parameters, but to handle errors correctly we are still going - // to extract the first 4 parameters - let param_span = parameters_item.as_span(); - let parameters = parameters_item - .into_inner() - .take(4) - .map(|param| (param.clone(), param.as_span())) - .map(|(param, span)| pest_parse(param).0.map(|arg| (arg, span))) - .collect::, _>>() - .map_err(UserError::InvalidFilter)?; - if parameters.len() != 3 { - return Err(UserError::InvalidFilter(PestError::new_from_span( - ErrorVariant::CustomError { - message: format!("The `_geoRadius` filter expect three arguments: `_geoRadius(latitude, longitude, radius)`"), - }, - // we want to point to the last parameters and if there was no parameters we - // point to the parenthesis - parameters.last().map(|param| param.1.clone()).unwrap_or(param_span), - )))?; - } - let (lat, lng, distance) = (¶meters[0], ¶meters[1], parameters[2].0); - if !(-90.0..=90.0).contains(&lat.0) { - return Err(UserError::InvalidFilter(PestError::new_from_span( - ErrorVariant::CustomError { - message: format!("Latitude must be contained between -90 and 90 degrees."), - }, - lat.1.clone(), - )))?; - } else if !(-180.0..=180.0).contains(&lng.0) { - return Err(UserError::InvalidFilter(PestError::new_from_span( - ErrorVariant::CustomError { - message: format!("Longitude must be contained between -180 and 180 degrees."), - }, - lng.1.clone(), - )))?; - } - Ok(Operator(fid, Operator::GeoLowerThan([lat.0, lng.0], distance))) - } } impl FilterCondition { @@ -348,7 +295,7 @@ impl FilterCondition { numbers_db, strings_db, field_id, - &GeoLowerThan(point.clone(), *distance), + &Operator::GeoLowerThan(point.clone(), *distance), )?; let geo_faceted_doc_ids = index.geo_faceted_documents_ids(rtxn)?; return Ok(geo_faceted_doc_ids - result); diff --git a/milli/src/search/facet/filter_parser.rs b/milli/src/search/facet/filter_parser.rs index 53a51ca49..419c148d7 100644 --- a/milli/src/search/facet/filter_parser.rs +++ b/milli/src/search/facet/filter_parser.rs @@ -9,10 +9,11 @@ use nom::{ bytes::complete::{tag, take_while1}, character::complete::{char, multispace0}, combinator::map, - error::ErrorKind, error::ParseError, error::VerboseError, + error::{ContextError, ErrorKind}, multi::many0, + multi::separated_list1, sequence::{delimited, preceded, tuple}, IResult, }; @@ -43,6 +44,8 @@ impl Operator { LowerThan(n) => (GreaterThanOrEqual(n), None), LowerThanOrEqual(n) => (GreaterThan(n), None), Between(n, m) => (LowerThan(n), Some(GreaterThan(m))), + GeoLowerThan(point, distance) => (GeoGreaterThan(point, distance), None), + GeoGreaterThan(point, distance) => (GeoLowerThan(point, distance), None), } } } @@ -193,13 +196,52 @@ impl<'a> ParseContext<'a> { Ok((input, res)) } + fn parse_geo_radius(&'a self, input: &'a str) -> IResult<&'a str, FilterCondition, E> + where + E: ParseError<&'a str>, + { + let (input, args) = preceded( + tag("_geoRadius"), + delimited( + tag("("), + separated_list1(tag(","), self.ws(|c| self.parse_value(c))), + tag(")"), + ), + )(input)?; + + if args.len() != 3 { + let e = E::from_char(input, '('); + return Err(nom::Err::Failure(e)); + } + let lat = self.parse_numeric(args[0])?; + let lng = self.parse_numeric(args[1])?; + let dis = self.parse_numeric(args[2])?; + + let fid = match self.fields_ids_map.id("_geo") { + Some(fid) => fid, + None => return Ok((input, FilterCondition::Empty)), + }; + + if let Some(span) = (!(-181.0..181.).contains(&lat)) + .then(|| &lat) + .or((!(-181.0..181.).contains(&lng)).then(|| &lng)) + { + let e = E::from_char(input, '('); + return Err(nom::Err::Failure(e)); + } + + let res = FilterCondition::Operator(fid, GeoLowerThan([lat, lng], dis)); + Ok((input, res)) + } + fn parse_condition(&'a self, input: &'a str) -> IResult<&'a str, FilterCondition, E> where E: ParseError<&'a str>, { + let l0 = |c| self.parse_geo_radius(c); let l1 = |c| self.parse_simple_condition(c); let l2 = |c| self.parse_range_condition(c); - let (input, condition) = alt((l1, l2))(input)?; + let (input, condition) = alt((l0, l1, l2))(input)?; Ok((input, condition)) } @@ -220,6 +262,15 @@ impl<'a> ParseContext<'a> { let key = |input| take_while1(Self::is_key_component)(input); alt((key, delimited(char('"'), key, char('"'))))(input) } + + fn parse_value(&'a self, input: &'a str) -> IResult<&'a str, &'a str, E> + where + E: ParseError<&'a str>, + { + let key = |input| take_while1(Self::is_key_component)(input); + alt((key, delimited(char('"'), key, char('"'))))(input) + } + fn is_key_component(c: char) -> bool { c.is_alphanumeric() || ['_', '-', '.'].contains(&c) } @@ -431,13 +482,13 @@ mod tests { // basic test let condition = FilterCondition::from_str(&rtxn, &index, "_geoRadius(12, 13.0005, 2000)").unwrap(); - let expected = Operator(0, GeoLowerThan([12., 13.0005], 2000.)); + let expected = FilterCondition::Operator(0, GeoLowerThan([12., 13.0005], 2000.)); assert_eq!(condition, expected); // test the negation of the GeoLowerThan let condition = FilterCondition::from_str(&rtxn, &index, "NOT _geoRadius(50, 18, 2000.500)").unwrap(); - let expected = Operator(0, GeoGreaterThan([50., 18.], 2000.500)); + let expected = FilterCondition::Operator(0, GeoGreaterThan([50., 18.], 2000.500)); assert_eq!(condition, expected); // composition of multiple operations @@ -446,13 +497,13 @@ mod tests { &index, "(NOT _geoRadius(1, 2, 300) AND _geoRadius(1.001, 2.002, 1000.300)) OR price <= 10", ) - .unwrap(); - let expected = Or( - Box::new(And( - Box::new(Operator(0, GeoGreaterThan([1., 2.], 300.))), - Box::new(Operator(0, GeoLowerThan([1.001, 2.002], 1000.300))), + .unwrap_or_else(|e| FilterCondition::Empty); + let expected = FilterCondition::Or( + Box::new(FilterCondition::And( + Box::new(FilterCondition::Operator(0, GeoGreaterThan([1., 2.], 300.))), + Box::new(FilterCondition::Operator(0, GeoLowerThan([1.001, 2.002], 1000.300))), )), - Box::new(Operator(1, LowerThanOrEqual(10.))), + Box::new(FilterCondition::Operator(1, LowerThanOrEqual(10.))), ); assert_eq!(condition, expected);