diff --git a/milli/src/asc_desc.rs b/milli/src/asc_desc.rs index ebb28c27d..bbc49ea7d 100644 --- a/milli/src/asc_desc.rs +++ b/milli/src/asc_desc.rs @@ -7,45 +7,31 @@ use serde::{Deserialize, Serialize}; use thiserror::Error; use crate::error::is_reserved_keyword; +use crate::search::facet::BadGeoError; use crate::{CriterionError, Error, UserError}; /// This error type is never supposed to be shown to the end user. /// You must always cast it to a sort error or a criterion error. -#[derive(Debug)] +#[derive(Error, Debug)] pub enum AscDescError { - InvalidLatitude, - InvalidLongitude, + #[error(transparent)] + GeoError(BadGeoError), + #[error("Invalid syntax for the asc/desc parameter: expected expression ending by `:asc` or `:desc`, found `{name}`.")] InvalidSyntax { name: String }, + #[error("`{name}` is a reserved keyword and thus can't be used as a asc/desc rule.")] ReservedKeyword { name: String }, } -impl fmt::Display for AscDescError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match self { - Self::InvalidLatitude => { - write!(f, "Latitude must be contained between -90 and 90 degrees.",) - } - Self::InvalidLongitude => { - write!(f, "Longitude must be contained between -180 and 180 degrees.",) - } - Self::InvalidSyntax { name } => { - write!(f, "Invalid syntax for the asc/desc parameter: expected expression ending by `:asc` or `:desc`, found `{}`.", name) - } - Self::ReservedKeyword { name } => { - write!( - f, - "`{}` is a reserved keyword and thus can't be used as a asc/desc rule.", - name - ) - } - } +impl From for AscDescError { + fn from(geo_error: BadGeoError) -> Self { + AscDescError::GeoError(geo_error) } } impl From for CriterionError { fn from(error: AscDescError) -> Self { match error { - AscDescError::InvalidLatitude | AscDescError::InvalidLongitude => { + AscDescError::GeoError(_) => { CriterionError::ReservedNameForSort { name: "_geoPoint".to_string() } } AscDescError::InvalidSyntax { name } => CriterionError::InvalidName { name }, @@ -85,9 +71,9 @@ impl FromStr for Member { .map_err(|_| AscDescError::ReservedKeyword { name: text.to_string() }) })?; if !(-90.0..=90.0).contains(&lat) { - return Err(AscDescError::InvalidLatitude)?; + return Err(BadGeoError::Lat(lat))?; } else if !(-180.0..=180.0).contains(&lng) { - return Err(AscDescError::InvalidLongitude)?; + return Err(BadGeoError::Lng(lng))?; } Ok(Member::Geo([lat, lng])) } @@ -162,10 +148,8 @@ impl FromStr for AscDesc { #[derive(Error, Debug)] pub enum SortError { - #[error("{}", AscDescError::InvalidLatitude)] - InvalidLatitude, - #[error("{}", AscDescError::InvalidLongitude)] - InvalidLongitude, + #[error(transparent)] + ParseGeoError { error: BadGeoError }, #[error("Invalid syntax for the geo parameter: expected expression formated like \ `_geoPoint(latitude, longitude)` and ending by `:asc` or `:desc`, found `{name}`.")] BadGeoPointUsage { name: String }, @@ -184,8 +168,7 @@ pub enum SortError { impl From for SortError { fn from(error: AscDescError) -> Self { match error { - AscDescError::InvalidLatitude => SortError::InvalidLatitude, - AscDescError::InvalidLongitude => SortError::InvalidLongitude, + AscDescError::GeoError(error) => SortError::ParseGeoError { error }, AscDescError::InvalidSyntax { name } => SortError::InvalidName { name }, AscDescError::ReservedKeyword { name } if name.starts_with("_geoPoint") => { SortError::BadGeoPointUsage { name } @@ -277,11 +260,11 @@ mod tests { ), ("_geoPoint(35, 85, 75):asc", ReservedKeyword { name: S("_geoPoint(35, 85, 75)") }), ("_geoPoint(18):asc", ReservedKeyword { name: S("_geoPoint(18)") }), - ("_geoPoint(200, 200):asc", InvalidLatitude), - ("_geoPoint(90.000001, 0):asc", InvalidLatitude), - ("_geoPoint(0, -180.000001):desc", InvalidLongitude), - ("_geoPoint(159.256, 130):asc", InvalidLatitude), - ("_geoPoint(12, -2021):desc", InvalidLongitude), + ("_geoPoint(200, 200):asc", GeoError(BadGeoError::Lat(200.))), + ("_geoPoint(90.000001, 0):asc", GeoError(BadGeoError::Lat(90.000001))), + ("_geoPoint(0, -180.000001):desc", GeoError(BadGeoError::Lng(-180.000001))), + ("_geoPoint(159.256, 130):asc", GeoError(BadGeoError::Lat(159.256))), + ("_geoPoint(12, -2021):desc", GeoError(BadGeoError::Lng(-2021.))), ]; for (req, expected_error) in invalid_req { diff --git a/milli/src/search/facet/filter.rs b/milli/src/search/facet/filter.rs index 3cf11819f..a4ac53950 100644 --- a/milli/src/search/facet/filter.rs +++ b/milli/src/search/facet/filter.rs @@ -21,18 +21,51 @@ pub struct Filter<'a> { condition: FilterCondition<'a>, } +#[derive(Debug)] +pub enum BadGeoError { + Lat(f64), + Lng(f64), + BoundingBoxTopIsBelowBottom(f64, f64), +} + +impl std::error::Error for BadGeoError {} + +impl Display for BadGeoError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::BoundingBoxTopIsBelowBottom(top, bottom) => { + write!(f, "The top latitude `{top}` is below the bottom latitude `{bottom}`.") + } + Self::Lat(lat) => write!( + f, + "Bad latitude `{}`. Latitude must be contained between -90 and 90 degrees. ", + lat + ), + Self::Lng(lng) => write!( + f, + "Bad longitude `{}`. Longitude must be contained between -180 and 180 degrees. ", + lng + ), + } + } +} + #[derive(Debug)] enum FilterError<'a> { AttributeNotFilterable { attribute: &'a str, filterable_fields: HashSet }, - BadGeo(&'a str), - BadGeoLat(f64), - BadGeoLng(f64), - BadGeoBoundingBoxTopIsBelowBottom(f64, f64), + ParseGeoError(BadGeoError), + ReservedGeo(&'a str), Reserved(&'a str), TooDeep, } impl<'a> std::error::Error for FilterError<'a> {} +impl<'a> From for FilterError<'a> { + fn from(geo_error: BadGeoError) -> Self { + FilterError::ParseGeoError(geo_error) + } +} + impl<'a> Display for FilterError<'a> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { @@ -44,7 +77,11 @@ impl<'a> Display for FilterError<'a> { attribute, ) } else { - let filterables_list = filterable_fields.iter().map(AsRef::as_ref).collect::>().join(" "); + let filterables_list = filterable_fields + .iter() + .map(AsRef::as_ref) + .collect::>() + .join(" "); write!( f, @@ -53,20 +90,19 @@ impl<'a> Display for FilterError<'a> { filterables_list, ) } - }, - Self::TooDeep => write!(f, + } + Self::TooDeep => write!( + f, "Too many filter conditions, can't process more than {} filters.", MAX_FILTER_DEPTH ), + Self::ReservedGeo(keyword) => write!(f, "`{}` is a reserved keyword and thus can't be used as a filter expression. Use the `_geoRadius(latitude, longitude, distance)` or `_geoBoundingBox([latitude, longitude], [latitude, longitude])` built-in rules to filter on `_geo` field coordinates.", keyword), 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)` or `_geoBoundingBox([latitude, longitude], [latitude, longitude])` built-in rules to filter on `_geo` field coordinates.", keyword), - Self::BadGeoBoundingBoxTopIsBelowBottom(top, bottom) => write!(f, "The top latitude `{top}` is below the bottom latitude `{bottom}`."), - Self::BadGeoLat(lat) => write!(f, "Bad latitude `{}`. Latitude must be contained between -90 and 90 degrees. ", lat), - Self::BadGeoLng(lng) => write!(f, "Bad longitude `{}`. Longitude must be contained between -180 and 180 degrees. ", lng), + Self::ParseGeoError(error) => write!(f, "{}", error), } } } @@ -298,10 +334,10 @@ impl<'a> Filter<'a> { } else { match fid.value() { attribute @ "_geo" => { - Err(fid.as_external_error(FilterError::BadGeo(attribute)))? + Err(fid.as_external_error(FilterError::ReservedGeo(attribute)))? } attribute if attribute.starts_with("_geoPoint(") => { - Err(fid.as_external_error(FilterError::BadGeo("_geoPoint")))? + Err(fid.as_external_error(FilterError::ReservedGeo("_geoPoint")))? } attribute @ "_geoDistance" => { Err(fid.as_external_error(FilterError::Reserved(attribute)))? @@ -353,14 +389,10 @@ impl<'a> Filter<'a> { let base_point: [f64; 2] = [point[0].parse_finite_float()?, point[1].parse_finite_float()?]; if !(-90.0..=90.0).contains(&base_point[0]) { - return Err( - point[0].as_external_error(FilterError::BadGeoLat(base_point[0])) - )?; + return Err(point[0].as_external_error(BadGeoError::Lat(base_point[0])))?; } if !(-180.0..=180.0).contains(&base_point[1]) { - return Err( - point[1].as_external_error(FilterError::BadGeoLng(base_point[1])) - )?; + return Err(point[1].as_external_error(BadGeoError::Lng(base_point[1])))?; } let radius = radius.parse_finite_float()?; let rtree = match index.geo_rtree(rtxn)? { @@ -398,27 +430,26 @@ impl<'a> Filter<'a> { bottom_right_point[1].parse_finite_float()?, ]; if !(-90.0..=90.0).contains(&top_left[0]) { - return Err(top_left_point[0] - .as_external_error(FilterError::BadGeoLat(top_left[0])))?; + return Err( + top_left_point[0].as_external_error(BadGeoError::Lat(top_left[0])) + )?; } if !(-180.0..=180.0).contains(&top_left[1]) { - return Err(top_left_point[1] - .as_external_error(FilterError::BadGeoLng(top_left[1])))?; + return Err( + top_left_point[1].as_external_error(BadGeoError::Lng(top_left[1])) + )?; } if !(-90.0..=90.0).contains(&bottom_right[0]) { return Err(bottom_right_point[0] - .as_external_error(FilterError::BadGeoLat(bottom_right[0])))?; + .as_external_error(BadGeoError::Lat(bottom_right[0])))?; } if !(-180.0..=180.0).contains(&bottom_right[1]) { return Err(bottom_right_point[1] - .as_external_error(FilterError::BadGeoLng(bottom_right[1])))?; + .as_external_error(BadGeoError::Lng(bottom_right[1])))?; } if top_left[0] < bottom_right[0] { return Err(bottom_right_point[1].as_external_error( - FilterError::BadGeoBoundingBoxTopIsBelowBottom( - top_left[0], - bottom_right[0], - ), + BadGeoError::BoundingBoxTopIsBelowBottom(top_left[0], bottom_right[0]), ))?; } diff --git a/milli/src/search/facet/mod.rs b/milli/src/search/facet/mod.rs index 73054b84a..c88d4e9e7 100644 --- a/milli/src/search/facet/mod.rs +++ b/milli/src/search/facet/mod.rs @@ -4,7 +4,7 @@ use heed::types::{ByteSlice, DecodeIgnore}; use heed::{BytesDecode, RoTxn}; pub use self::facet_distribution::{FacetDistribution, DEFAULT_VALUES_PER_FACET}; -pub use self::filter::Filter; +pub use self::filter::{BadGeoError, Filter}; use crate::heed_codec::facet::{FacetGroupKeyCodec, FacetGroupValueCodec}; use crate::heed_codec::ByteSliceRefCodec; mod facet_distribution;