From 8234f9fdf3886bebcfc1dfeef239c1bcbc442d1c Mon Sep 17 00:00:00 2001 From: Tamo Date: Thu, 4 Nov 2021 17:24:55 +0100 Subject: [PATCH] recreate most filter error except for the geosearch --- filter_parser/src/error.rs | 4 + filter_parser/src/lib.rs | 4 + milli/src/error.rs | 4 +- milli/src/search/facet/filter_condition.rs | 93 +++++++++++++++------- 4 files changed, 73 insertions(+), 32 deletions(-) diff --git a/filter_parser/src/error.rs b/filter_parser/src/error.rs index a0ea2efac..a1bbac47a 100644 --- a/filter_parser/src/error.rs +++ b/filter_parser/src/error.rs @@ -71,6 +71,10 @@ impl<'a> Error<'a> { Self { context, kind } } + pub fn new_from_external(context: Span<'a>, error: impl std::error::Error) -> Self { + Self::new_from_kind(context, ErrorKind::External(error.to_string())) + } + pub fn char(self) -> char { match self.kind { ErrorKind::Char(c) => c, diff --git a/filter_parser/src/lib.rs b/filter_parser/src/lib.rs index 9335ef185..31aa973ab 100644 --- a/filter_parser/src/lib.rs +++ b/filter_parser/src/lib.rs @@ -69,6 +69,10 @@ impl<'a> Token<'a> { pub fn new(position: Span<'a>) -> Self { Self { position, inner: &position } } + + pub fn as_external_error(&self, error: impl std::error::Error) -> Error<'a> { + Error::new_from_external(self.position, error) + } } impl<'a> From> for Token<'a> { diff --git a/milli/src/error.rs b/milli/src/error.rs index c0ce101c8..3d744da5c 100644 --- a/milli/src/error.rs +++ b/milli/src/error.rs @@ -59,7 +59,7 @@ pub enum UserError { InvalidDocumentId { document_id: Value }, InvalidFacetsDistribution { invalid_facets_name: HashSet }, InvalidGeoField { document_id: Value, object: Value }, - InvalidFilter { input: String }, + InvalidFilter(String), InvalidSortName { name: String }, InvalidSortableAttribute { field: String, valid_fields: HashSet }, SortRankingRuleMissing, @@ -207,7 +207,7 @@ impl StdError for InternalError {} impl fmt::Display for UserError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - Self::InvalidFilter { input } => write!(f, "parser error {}", input), + Self::InvalidFilter(input) => write!(f, "{}", input), Self::AttributeLimitReached => f.write_str("maximum number of attributes reached"), Self::CriterionError(error) => write!(f, "{}", error), Self::DocumentLimitReached => f.write_str("maximum number of documents reached"), diff --git a/milli/src/search/facet/filter_condition.rs b/milli/src/search/facet/filter_condition.rs index d0c32c8f4..13622a134 100644 --- a/milli/src/search/facet/filter_condition.rs +++ b/milli/src/search/facet/filter_condition.rs @@ -1,4 +1,4 @@ -use std::fmt::Debug; +use std::fmt::{Debug, Display}; use std::ops::Bound::{self, Excluded, Included}; use std::str::FromStr; @@ -20,18 +20,50 @@ pub struct Filter<'a> { condition: FilterCondition<'a>, } -fn parse(tok: &Token) -> Result { +#[derive(Debug)] +enum FilterError<'a> { + AttributeNotFilterable { attribute: &'a str, filterable: String }, + BadGeo(&'a str), + Reserved(&'a str), +} +impl<'a> std::error::Error for FilterError<'a> {} + +impl<'a> Display for FilterError<'a> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::AttributeNotFilterable { attribute, filterable } => write!( + f, + "Attribute `{}` is not filterable. Available filterable attributes are: `{}`.", + attribute, + filterable, + ), + Self::Reserved(keyword) => write!( + f, + "`{}` is a reserved keyword and thus can't be used as a filter expression.", + keyword + ), + Self::BadGeo(keyword) => write!(f, "`{}` is a reserved keyword and thus can't be used as a filter expression. Use the _geoRadius(latitude, longitude, distance) built-in rule to filter on _geo field coordinates.", keyword), + } + } +} + +impl<'a> From> for Error { + fn from(error: FPError<'a>) -> Self { + Self::UserError(UserError::InvalidFilter(error.to_string())) + } +} + +fn parse(tok: &Token) -> Result +where + T: FromStr, + T::Err: std::error::Error, +{ match tok.inner.parse::() { Ok(t) => Ok(t), - Err(_e) => Err(UserError::InvalidFilter { - input: format!( - "Could not parse `{}` at line {} and offset {}", - tok.inner, - tok.position.location_line(), - tok.position.get_column() - ), + Err(e) => { + Err(UserError::InvalidFilter(FPError::new_from_external(tok.position, e).to_string()) + .into()) } - .into()), } } @@ -90,7 +122,7 @@ impl<'a> Filter<'a> { pub fn from_str(expression: &'a str) -> Result { let condition = match FilterCondition::parse(expression) { Ok(fc) => Ok(fc), - Err(e) => Err(Error::UserError(UserError::InvalidFilter { input: e.to_string() })), + Err(e) => Err(Error::UserError(UserError::InvalidFilter(e.to_string()))), }?; Ok(Self { condition }) } @@ -299,25 +331,26 @@ impl<'a> Filter<'a> { Self::evaluate_operator(rtxn, index, numbers_db, strings_db, fid, &op) } else { match fid.inner { - // TODO update the error messages according to the spec - "_geo" => { - return Err(UserError::InvalidFilter { input: format!("Tried to use _geo in a filter, you probably wanted to use _geoRadius(latitude, longitude, radius)") })?; + attribute @ "_geo" => { + return Err(fid.as_external_error(FilterError::BadGeo(attribute)))?; } - "_geoDistance" => { - return Err(UserError::InvalidFilter { - input: format!("Reserved field _geoDistance"), - })?; + attribute if attribute.starts_with("_geoPoint(") => { + return Err(fid.as_external_error(FilterError::BadGeo("_geoPoint")))?; } - fid if fid.starts_with("_geoPoint(") => { - return Err(UserError::InvalidFilter { input: format!("_geoPoint only available in sort. You wanted to use _geoRadius") })?; + attribute @ "_geoDistance" => { + return Err(fid.as_external_error(FilterError::Reserved(attribute)))?; } - fid => { - return Err(UserError::InvalidFilter { - input: format!( - "Bad filter {}, available filters are {:?}", - fid, filterable_fields - ), - })?; + attribute => { + return Err(fid.as_external_error( + FilterError::AttributeNotFilterable { + attribute, + filterable: filterable_fields + .iter() + .map(|(_, s)| s) + .collect::>() + .join(" "), + }, + ))?; } } } @@ -356,9 +389,9 @@ impl<'a> Filter<'a> { Ok(result) } else { // TODO TAMO: update the error message - return Err(UserError::InvalidFilter { - input: format!("You tried to use _geo in a filter, you probably wanted to use _geoRadius"), - })?; + return Err(UserError::InvalidFilter(format!( + "You tried to use _geo in a filter, you probably wanted to use _geoRadius" + )))?; } } FilterCondition::GeoGreaterThan { point, radius } => {