From e405702733f43df31c3d48dae53b194a87ce8d06 Mon Sep 17 00:00:00 2001 From: Filip Bachul Date: Tue, 7 Feb 2023 18:57:27 +0100 Subject: [PATCH 01/11] chore: introduce new error ParseGeoError type --- milli/src/search/facet/filter.rs | 93 ++++++++++++++++++++------------ 1 file changed, 60 insertions(+), 33 deletions(-) diff --git a/milli/src/search/facet/filter.rs b/milli/src/search/facet/filter.rs index 3cf11819f..548ed699e 100644 --- a/milli/src/search/facet/filter.rs +++ b/milli/src/search/facet/filter.rs @@ -22,12 +22,30 @@ pub struct Filter<'a> { } #[derive(Debug)] -enum FilterError<'a> { - AttributeNotFilterable { attribute: &'a str, filterable_fields: HashSet }, - BadGeo(&'a str), +enum ParseGeoError { + BadGeo(String), BadGeoLat(f64), BadGeoLng(f64), BadGeoBoundingBoxTopIsBelowBottom(f64, f64), +} + +impl std::error::Error for ParseGeoError {} + +impl Display for ParseGeoError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + 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), + } + } +} + +#[derive(Debug)] +enum FilterError<'a> { + AttributeNotFilterable { attribute: &'a str, filterable_fields: HashSet }, + ParseGeoError(ParseGeoError), Reserved(&'a str), TooDeep, } @@ -44,7 +62,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,8 +75,9 @@ 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 ), @@ -63,10 +86,7 @@ impl<'a> Display for FilterError<'a> { "`{}` 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), } } } @@ -297,12 +317,13 @@ impl<'a> Filter<'a> { } } else { match fid.value() { - attribute @ "_geo" => { - Err(fid.as_external_error(FilterError::BadGeo(attribute)))? - } - attribute if attribute.starts_with("_geoPoint(") => { - Err(fid.as_external_error(FilterError::BadGeo("_geoPoint")))? - } + attribute @ "_geo" => Err(fid.as_external_error( + FilterError::ParseGeoError(ParseGeoError::BadGeo(attribute.to_owned())), + ))?, + attribute if attribute.starts_with("_geoPoint(") => Err(fid + .as_external_error(FilterError::ParseGeoError( + ParseGeoError::BadGeo("_geoPoint".to_owned()), + )))?, attribute @ "_geoDistance" => { Err(fid.as_external_error(FilterError::Reserved(attribute)))? } @@ -353,14 +374,14 @@ 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(FilterError::ParseGeoError( + ParseGeoError::BadGeoLat(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(FilterError::ParseGeoError( + ParseGeoError::BadGeoLng(base_point[1]), + )))?; } let radius = radius.parse_finite_float()?; let rtree = match index.geo_rtree(rtxn)? { @@ -398,26 +419,32 @@ 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( + FilterError::ParseGeoError(ParseGeoError::BadGeoLat(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( + FilterError::ParseGeoError(ParseGeoError::BadGeoLng(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])))?; + return Err(bottom_right_point[0].as_external_error( + FilterError::ParseGeoError(ParseGeoError::BadGeoLat(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])))?; + return Err(bottom_right_point[1].as_external_error( + FilterError::ParseGeoError(ParseGeoError::BadGeoLng(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], + FilterError::ParseGeoError( + ParseGeoError::BadGeoBoundingBoxTopIsBelowBottom( + top_left[0], + bottom_right[0], + ), ), ))?; } From 825923f6fc7b2c725dc4b932dedd23181836e65c Mon Sep 17 00:00:00 2001 From: Filip Bachul Date: Tue, 7 Feb 2023 18:58:09 +0100 Subject: [PATCH 02/11] export ParseGeoError --- milli/src/search/facet/filter.rs | 2 +- milli/src/search/facet/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/milli/src/search/facet/filter.rs b/milli/src/search/facet/filter.rs index 548ed699e..35352c764 100644 --- a/milli/src/search/facet/filter.rs +++ b/milli/src/search/facet/filter.rs @@ -22,7 +22,7 @@ pub struct Filter<'a> { } #[derive(Debug)] -enum ParseGeoError { +pub enum ParseGeoError { BadGeo(String), BadGeoLat(f64), BadGeoLng(f64), diff --git a/milli/src/search/facet/mod.rs b/milli/src/search/facet/mod.rs index 73054b84a..0d0f96851 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::{Filter, ParseGeoError}; use crate::heed_codec::facet::{FacetGroupKeyCodec, FacetGroupValueCodec}; use crate::heed_codec::ByteSliceRefCodec; mod facet_distribution; From 4c910376020a336f0bfda26587f7c9911671e47b Mon Sep 17 00:00:00 2001 From: Filip Bachul Date: Tue, 7 Feb 2023 19:15:06 +0100 Subject: [PATCH 03/11] use ParseGeoError in sort parser --- milli/src/asc_desc.rs | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/milli/src/asc_desc.rs b/milli/src/asc_desc.rs index ebb28c27d..e821a847e 100644 --- a/milli/src/asc_desc.rs +++ b/milli/src/asc_desc.rs @@ -7,14 +7,15 @@ use serde::{Deserialize, Serialize}; use thiserror::Error; use crate::error::is_reserved_keyword; +use crate::search::facet::ParseGeoError; 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)] pub enum AscDescError { - InvalidLatitude, - InvalidLongitude, + InvalidLatitude(ParseGeoError), + InvalidLongitude(ParseGeoError), InvalidSyntax { name: String }, ReservedKeyword { name: String }, } @@ -22,11 +23,11 @@ pub enum AscDescError { 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::InvalidLatitude(error) => { + write!(f, "{error}",) } - Self::InvalidLongitude => { - write!(f, "Longitude must be contained between -180 and 180 degrees.",) + Self::InvalidLongitude(error) => { + write!(f, "{error}",) } Self::InvalidSyntax { name } => { write!(f, "Invalid syntax for the asc/desc parameter: expected expression ending by `:asc` or `:desc`, found `{}`.", name) @@ -45,7 +46,7 @@ impl fmt::Display for AscDescError { impl From for CriterionError { fn from(error: AscDescError) -> Self { match error { - AscDescError::InvalidLatitude | AscDescError::InvalidLongitude => { + AscDescError::InvalidLatitude(_) | AscDescError::InvalidLongitude(_) => { CriterionError::ReservedNameForSort { name: "_geoPoint".to_string() } } AscDescError::InvalidSyntax { name } => CriterionError::InvalidName { name }, @@ -85,9 +86,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(AscDescError::InvalidLatitude(ParseGeoError::BadGeoLat(lat)))?; } else if !(-180.0..=180.0).contains(&lng) { - return Err(AscDescError::InvalidLongitude)?; + return Err(AscDescError::InvalidLongitude(ParseGeoError::BadGeoLng(lng)))?; } Ok(Member::Geo([lat, lng])) } @@ -162,10 +163,10 @@ impl FromStr for AscDesc { #[derive(Error, Debug)] pub enum SortError { - #[error("{}", AscDescError::InvalidLatitude)] - InvalidLatitude, - #[error("{}", AscDescError::InvalidLongitude)] - InvalidLongitude, + #[error("{}", error)] + InvalidLatitude { error: ParseGeoError }, + #[error("{}", error)] + InvalidLongitude { error: ParseGeoError }, #[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 +185,8 @@ pub enum SortError { impl From for SortError { fn from(error: AscDescError) -> Self { match error { - AscDescError::InvalidLatitude => SortError::InvalidLatitude, - AscDescError::InvalidLongitude => SortError::InvalidLongitude, + AscDescError::InvalidLatitude(error) => SortError::InvalidLatitude { error }, + AscDescError::InvalidLongitude(error) => SortError::InvalidLongitude { error }, AscDescError::InvalidSyntax { name } => SortError::InvalidName { name }, AscDescError::ReservedKeyword { name } if name.starts_with("_geoPoint") => { SortError::BadGeoPointUsage { name } @@ -277,11 +278,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", InvalidLatitude(ParseGeoError::BadGeoLat(200.))), + ("_geoPoint(90.000001, 0):asc", InvalidLatitude(ParseGeoError::BadGeoLat(90.000001))), + ("_geoPoint(0, -180.000001):desc", InvalidLongitude(ParseGeoError::BadGeoLng(-180.000001))), + ("_geoPoint(159.256, 130):asc", InvalidLatitude(ParseGeoError::BadGeoLat(159.256))), + ("_geoPoint(12, -2021):desc", InvalidLongitude(ParseGeoError::BadGeoLng(-2021.))), ]; for (req, expected_error) in invalid_req { From 83c765ce6cdf37a120fe0cccbca8553aa4c3e50b Mon Sep 17 00:00:00 2001 From: Filip Bachul Date: Thu, 9 Feb 2023 16:35:15 +0100 Subject: [PATCH 04/11] implement From for FilterError --- milli/src/search/facet/filter.rs | 60 ++++++++++++++++---------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/milli/src/search/facet/filter.rs b/milli/src/search/facet/filter.rs index 35352c764..2740669b7 100644 --- a/milli/src/search/facet/filter.rs +++ b/milli/src/search/facet/filter.rs @@ -51,6 +51,12 @@ enum FilterError<'a> { } impl<'a> std::error::Error for FilterError<'a> {} +impl<'a> From for FilterError<'a> { + fn from(geo_error: ParseGeoError) -> Self { + FilterError::ParseGeoError(geo_error) + } +} + impl<'a> Display for FilterError<'a> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { @@ -317,13 +323,13 @@ impl<'a> Filter<'a> { } } else { match fid.value() { - attribute @ "_geo" => Err(fid.as_external_error( - FilterError::ParseGeoError(ParseGeoError::BadGeo(attribute.to_owned())), - ))?, - attribute if attribute.starts_with("_geoPoint(") => Err(fid - .as_external_error(FilterError::ParseGeoError( - ParseGeoError::BadGeo("_geoPoint".to_owned()), - )))?, + attribute @ "_geo" => { + Err(fid.as_external_error(ParseGeoError::BadGeo(attribute.to_owned())))? + } + attribute if attribute.starts_with("_geoPoint(") => { + Err(fid + .as_external_error(ParseGeoError::BadGeo("_geoPoint".to_owned())))? + } attribute @ "_geoDistance" => { Err(fid.as_external_error(FilterError::Reserved(attribute)))? } @@ -374,14 +380,14 @@ 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::ParseGeoError( - ParseGeoError::BadGeoLat(base_point[0]), - )))?; + return Err( + point[0].as_external_error(ParseGeoError::BadGeoLat(base_point[0])) + )?; } if !(-180.0..=180.0).contains(&base_point[1]) { - return Err(point[1].as_external_error(FilterError::ParseGeoError( - ParseGeoError::BadGeoLng(base_point[1]), - )))?; + return Err( + point[1].as_external_error(ParseGeoError::BadGeoLng(base_point[1])) + )?; } let radius = radius.parse_finite_float()?; let rtree = match index.geo_rtree(rtxn)? { @@ -419,32 +425,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::ParseGeoError(ParseGeoError::BadGeoLat(top_left[0])), - ))?; + return Err(top_left_point[0] + .as_external_error(ParseGeoError::BadGeoLat(top_left[0])))?; } if !(-180.0..=180.0).contains(&top_left[1]) { - return Err(top_left_point[1].as_external_error( - FilterError::ParseGeoError(ParseGeoError::BadGeoLng(top_left[1])), - ))?; + return Err(top_left_point[1] + .as_external_error(ParseGeoError::BadGeoLng(top_left[1])))?; } if !(-90.0..=90.0).contains(&bottom_right[0]) { - return Err(bottom_right_point[0].as_external_error( - FilterError::ParseGeoError(ParseGeoError::BadGeoLat(bottom_right[0])), - ))?; + return Err(bottom_right_point[0] + .as_external_error(ParseGeoError::BadGeoLat(bottom_right[0])))?; } if !(-180.0..=180.0).contains(&bottom_right[1]) { - return Err(bottom_right_point[1].as_external_error( - FilterError::ParseGeoError(ParseGeoError::BadGeoLng(bottom_right[1])), - ))?; + return Err(bottom_right_point[1] + .as_external_error(ParseGeoError::BadGeoLng(bottom_right[1])))?; } if top_left[0] < bottom_right[0] { return Err(bottom_right_point[1].as_external_error( - FilterError::ParseGeoError( - ParseGeoError::BadGeoBoundingBoxTopIsBelowBottom( - top_left[0], - bottom_right[0], - ), + ParseGeoError::BadGeoBoundingBoxTopIsBelowBottom( + top_left[0], + bottom_right[0], ), ))?; } From 7481559e8b1ee969323802d422d95f1c01d0db30 Mon Sep 17 00:00:00 2001 From: Filip Bachul Date: Thu, 9 Feb 2023 17:37:18 +0100 Subject: [PATCH 05/11] move BadGeo to FilterError --- milli/src/search/facet/filter.rs | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/milli/src/search/facet/filter.rs b/milli/src/search/facet/filter.rs index 2740669b7..3e95909a0 100644 --- a/milli/src/search/facet/filter.rs +++ b/milli/src/search/facet/filter.rs @@ -23,7 +23,6 @@ pub struct Filter<'a> { #[derive(Debug)] pub enum ParseGeoError { - BadGeo(String), BadGeoLat(f64), BadGeoLng(f64), BadGeoBoundingBoxTopIsBelowBottom(f64, f64), @@ -34,10 +33,19 @@ impl std::error::Error for ParseGeoError {} impl Display for ParseGeoError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - 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::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 + ), } } } @@ -46,6 +54,7 @@ impl Display for ParseGeoError { enum FilterError<'a> { AttributeNotFilterable { attribute: &'a str, filterable_fields: HashSet }, ParseGeoError(ParseGeoError), + ReservedGeo(&'a str), Reserved(&'a str), TooDeep, } @@ -87,6 +96,7 @@ impl<'a> Display for FilterError<'a> { "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.", @@ -324,11 +334,10 @@ impl<'a> Filter<'a> { } else { match fid.value() { attribute @ "_geo" => { - Err(fid.as_external_error(ParseGeoError::BadGeo(attribute.to_owned())))? + Err(fid.as_external_error(FilterError::ReservedGeo(attribute)))? } attribute if attribute.starts_with("_geoPoint(") => { - Err(fid - .as_external_error(ParseGeoError::BadGeo("_geoPoint".to_owned())))? + Err(fid.as_external_error(FilterError::ReservedGeo("_geoPoint")))? } attribute @ "_geoDistance" => { Err(fid.as_external_error(FilterError::Reserved(attribute)))? From c0b77773bad9547a76a53e8b1e659c600158b223 Mon Sep 17 00:00:00 2001 From: Filip Bachul Date: Thu, 9 Feb 2023 17:41:36 +0100 Subject: [PATCH 06/11] fmt asc_desc --- milli/src/asc_desc.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/milli/src/asc_desc.rs b/milli/src/asc_desc.rs index e821a847e..fc29367ec 100644 --- a/milli/src/asc_desc.rs +++ b/milli/src/asc_desc.rs @@ -280,7 +280,10 @@ mod tests { ("_geoPoint(18):asc", ReservedKeyword { name: S("_geoPoint(18)") }), ("_geoPoint(200, 200):asc", InvalidLatitude(ParseGeoError::BadGeoLat(200.))), ("_geoPoint(90.000001, 0):asc", InvalidLatitude(ParseGeoError::BadGeoLat(90.000001))), - ("_geoPoint(0, -180.000001):desc", InvalidLongitude(ParseGeoError::BadGeoLng(-180.000001))), + ( + "_geoPoint(0, -180.000001):desc", + InvalidLongitude(ParseGeoError::BadGeoLng(-180.000001)), + ), ("_geoPoint(159.256, 130):asc", InvalidLatitude(ParseGeoError::BadGeoLat(159.256))), ("_geoPoint(12, -2021):desc", InvalidLongitude(ParseGeoError::BadGeoLng(-2021.))), ]; From c810af3ebfa1be46efd6eb0e866cbe9088e8b064 Mon Sep 17 00:00:00 2001 From: Filip Bachul Date: Thu, 9 Feb 2023 18:33:41 +0100 Subject: [PATCH 07/11] implement From for AscDescError --- milli/src/asc_desc.rs | 40 ++++++++++++++++++---------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/milli/src/asc_desc.rs b/milli/src/asc_desc.rs index fc29367ec..ecac4194a 100644 --- a/milli/src/asc_desc.rs +++ b/milli/src/asc_desc.rs @@ -14,19 +14,21 @@ use crate::{CriterionError, Error, UserError}; /// You must always cast it to a sort error or a criterion error. #[derive(Debug)] pub enum AscDescError { - InvalidLatitude(ParseGeoError), - InvalidLongitude(ParseGeoError), + GeoError(ParseGeoError), InvalidSyntax { name: String }, ReservedKeyword { name: String }, } +impl From for AscDescError { + fn from(geo_error: ParseGeoError) -> Self { + AscDescError::GeoError(geo_error) + } +} + impl fmt::Display for AscDescError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - Self::InvalidLatitude(error) => { - write!(f, "{error}",) - } - Self::InvalidLongitude(error) => { + Self::GeoError(error) => { write!(f, "{error}",) } Self::InvalidSyntax { name } => { @@ -46,7 +48,7 @@ impl fmt::Display for AscDescError { 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 }, @@ -86,9 +88,9 @@ impl FromStr for Member { .map_err(|_| AscDescError::ReservedKeyword { name: text.to_string() }) })?; if !(-90.0..=90.0).contains(&lat) { - return Err(AscDescError::InvalidLatitude(ParseGeoError::BadGeoLat(lat)))?; + return Err(ParseGeoError::BadGeoLat(lat))?; } else if !(-180.0..=180.0).contains(&lng) { - return Err(AscDescError::InvalidLongitude(ParseGeoError::BadGeoLng(lng)))?; + return Err(ParseGeoError::BadGeoLng(lng))?; } Ok(Member::Geo([lat, lng])) } @@ -164,9 +166,7 @@ impl FromStr for AscDesc { #[derive(Error, Debug)] pub enum SortError { #[error("{}", error)] - InvalidLatitude { error: ParseGeoError }, - #[error("{}", error)] - InvalidLongitude { error: ParseGeoError }, + ParseGeoError { error: ParseGeoError }, #[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 }, @@ -185,8 +185,7 @@ pub enum SortError { impl From for SortError { fn from(error: AscDescError) -> Self { match error { - AscDescError::InvalidLatitude(error) => SortError::InvalidLatitude { error }, - AscDescError::InvalidLongitude(error) => SortError::InvalidLongitude { error }, + AscDescError::GeoError(error) => SortError::ParseGeoError { error }, AscDescError::InvalidSyntax { name } => SortError::InvalidName { name }, AscDescError::ReservedKeyword { name } if name.starts_with("_geoPoint") => { SortError::BadGeoPointUsage { name } @@ -278,14 +277,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(ParseGeoError::BadGeoLat(200.))), - ("_geoPoint(90.000001, 0):asc", InvalidLatitude(ParseGeoError::BadGeoLat(90.000001))), - ( - "_geoPoint(0, -180.000001):desc", - InvalidLongitude(ParseGeoError::BadGeoLng(-180.000001)), - ), - ("_geoPoint(159.256, 130):asc", InvalidLatitude(ParseGeoError::BadGeoLat(159.256))), - ("_geoPoint(12, -2021):desc", InvalidLongitude(ParseGeoError::BadGeoLng(-2021.))), + ("_geoPoint(200, 200):asc", GeoError(ParseGeoError::BadGeoLat(200.))), + ("_geoPoint(90.000001, 0):asc", GeoError(ParseGeoError::BadGeoLat(90.000001))), + ("_geoPoint(0, -180.000001):desc", GeoError(ParseGeoError::BadGeoLng(-180.000001))), + ("_geoPoint(159.256, 130):asc", GeoError(ParseGeoError::BadGeoLat(159.256))), + ("_geoPoint(12, -2021):desc", GeoError(ParseGeoError::BadGeoLng(-2021.))), ]; for (req, expected_error) in invalid_req { From 7f25007d31ab3a87afcae0984c5e605d79f54022 Mon Sep 17 00:00:00 2001 From: filip Date: Mon, 13 Feb 2023 18:02:33 +0100 Subject: [PATCH 08/11] Update milli/src/asc_desc.rs Co-authored-by: Tamo --- milli/src/asc_desc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/milli/src/asc_desc.rs b/milli/src/asc_desc.rs index ecac4194a..3d6ec2d24 100644 --- a/milli/src/asc_desc.rs +++ b/milli/src/asc_desc.rs @@ -165,7 +165,7 @@ impl FromStr for AscDesc { #[derive(Error, Debug)] pub enum SortError { - #[error("{}", error)] + #[error(transparent)] ParseGeoError { error: ParseGeoError }, #[error("Invalid syntax for the geo parameter: expected expression formated like \ `_geoPoint(latitude, longitude)` and ending by `:asc` or `:desc`, found `{name}`.")] From 849de089d2efca18aa2efb8f0dcdc440bc0f6f36 Mon Sep 17 00:00:00 2001 From: Filip Bachul Date: Tue, 14 Feb 2023 00:08:42 +0100 Subject: [PATCH 09/11] add thiserror for AscDescError --- milli/src/asc_desc.rs | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/milli/src/asc_desc.rs b/milli/src/asc_desc.rs index 3d6ec2d24..f8595a0ee 100644 --- a/milli/src/asc_desc.rs +++ b/milli/src/asc_desc.rs @@ -12,10 +12,13 @@ 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 { + #[error(transparent)] GeoError(ParseGeoError), + #[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 }, } @@ -25,25 +28,6 @@ impl From for AscDescError { } } -impl fmt::Display for AscDescError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match self { - Self::GeoError(error) => { - write!(f, "{error}",) - } - 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 CriterionError { fn from(error: AscDescError) -> Self { From d7ad39ad778e8299c9255277d4c13d83cc6078cc Mon Sep 17 00:00:00 2001 From: Filip Bachul Date: Tue, 14 Feb 2023 00:14:17 +0100 Subject: [PATCH 10/11] fix: clippy error --- milli/src/asc_desc.rs | 24 ++++++++++---------- milli/src/search/facet/filter.rs | 38 ++++++++++++++++---------------- milli/src/search/facet/mod.rs | 2 +- 3 files changed, 32 insertions(+), 32 deletions(-) diff --git a/milli/src/asc_desc.rs b/milli/src/asc_desc.rs index f8595a0ee..2e3892707 100644 --- a/milli/src/asc_desc.rs +++ b/milli/src/asc_desc.rs @@ -7,7 +7,7 @@ use serde::{Deserialize, Serialize}; use thiserror::Error; use crate::error::is_reserved_keyword; -use crate::search::facet::ParseGeoError; +use crate::search::facet::BadGeoError; use crate::{CriterionError, Error, UserError}; /// This error type is never supposed to be shown to the end user. @@ -15,15 +15,15 @@ use crate::{CriterionError, Error, UserError}; #[derive(Error, Debug)] pub enum AscDescError { #[error(transparent)] - GeoError(ParseGeoError), + 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 From for AscDescError { - fn from(geo_error: ParseGeoError) -> Self { +impl From for AscDescError { + fn from(geo_error: BadGeoError) -> Self { AscDescError::GeoError(geo_error) } } @@ -72,9 +72,9 @@ impl FromStr for Member { .map_err(|_| AscDescError::ReservedKeyword { name: text.to_string() }) })?; if !(-90.0..=90.0).contains(&lat) { - return Err(ParseGeoError::BadGeoLat(lat))?; + return Err(BadGeoError::Lat(lat))?; } else if !(-180.0..=180.0).contains(&lng) { - return Err(ParseGeoError::BadGeoLng(lng))?; + return Err(BadGeoError::Lng(lng))?; } Ok(Member::Geo([lat, lng])) } @@ -150,7 +150,7 @@ impl FromStr for AscDesc { #[derive(Error, Debug)] pub enum SortError { #[error(transparent)] - ParseGeoError { error: ParseGeoError }, + 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 }, @@ -261,11 +261,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", GeoError(ParseGeoError::BadGeoLat(200.))), - ("_geoPoint(90.000001, 0):asc", GeoError(ParseGeoError::BadGeoLat(90.000001))), - ("_geoPoint(0, -180.000001):desc", GeoError(ParseGeoError::BadGeoLng(-180.000001))), - ("_geoPoint(159.256, 130):asc", GeoError(ParseGeoError::BadGeoLat(159.256))), - ("_geoPoint(12, -2021):desc", GeoError(ParseGeoError::BadGeoLng(-2021.))), + ("_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 3e95909a0..585d59a29 100644 --- a/milli/src/search/facet/filter.rs +++ b/milli/src/search/facet/filter.rs @@ -22,26 +22,26 @@ pub struct Filter<'a> { } #[derive(Debug)] -pub enum ParseGeoError { - BadGeoLat(f64), - BadGeoLng(f64), - BadGeoBoundingBoxTopIsBelowBottom(f64, f64), +pub enum BadGeoError { + Lat(f64), + Lng(f64), + BoundingBoxTopIsBelowBottom(f64, f64), } -impl std::error::Error for ParseGeoError {} +impl std::error::Error for BadGeoError {} -impl Display for ParseGeoError { +impl Display for BadGeoError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - Self::BadGeoBoundingBoxTopIsBelowBottom(top, bottom) => { + Self::BoundingBoxTopIsBelowBottom(top, bottom) => { write!(f, "The top latitude `{top}` is below the bottom latitude `{bottom}`.") } - Self::BadGeoLat(lat) => write!( + Self::Lat(lat) => write!( f, "Bad latitude `{}`. Latitude must be contained between -90 and 90 degrees. ", lat ), - Self::BadGeoLng(lng) => write!( + Self::Lng(lng) => write!( f, "Bad longitude `{}`. Longitude must be contained between -180 and 180 degrees. ", lng @@ -53,15 +53,15 @@ impl Display for ParseGeoError { #[derive(Debug)] enum FilterError<'a> { AttributeNotFilterable { attribute: &'a str, filterable_fields: HashSet }, - ParseGeoError(ParseGeoError), + 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: ParseGeoError) -> Self { +impl<'a> From for FilterError<'a> { + fn from(geo_error: BadGeoError) -> Self { FilterError::ParseGeoError(geo_error) } } @@ -390,12 +390,12 @@ impl<'a> Filter<'a> { [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(ParseGeoError::BadGeoLat(base_point[0])) + 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(ParseGeoError::BadGeoLng(base_point[1])) + point[1].as_external_error(BadGeoError::Lng(base_point[1])) )?; } let radius = radius.parse_finite_float()?; @@ -435,23 +435,23 @@ impl<'a> Filter<'a> { ]; if !(-90.0..=90.0).contains(&top_left[0]) { return Err(top_left_point[0] - .as_external_error(ParseGeoError::BadGeoLat(top_left[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(ParseGeoError::BadGeoLng(top_left[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(ParseGeoError::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(ParseGeoError::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( - ParseGeoError::BadGeoBoundingBoxTopIsBelowBottom( + 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 0d0f96851..fb93ae00b 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, ParseGeoError}; +pub use self::filter::{Filter, BadGeoError}; use crate::heed_codec::facet::{FacetGroupKeyCodec, FacetGroupValueCodec}; use crate::heed_codec::ByteSliceRefCodec; mod facet_distribution; From a53536836b79fd3b6e8ba9b1301bb46177a27cd4 Mon Sep 17 00:00:00 2001 From: Filip Bachul Date: Tue, 14 Feb 2023 17:03:44 +0100 Subject: [PATCH 11/11] fmt --- milli/src/asc_desc.rs | 1 - milli/src/search/facet/filter.rs | 23 +++++++++-------------- milli/src/search/facet/mod.rs | 2 +- 3 files changed, 10 insertions(+), 16 deletions(-) diff --git a/milli/src/asc_desc.rs b/milli/src/asc_desc.rs index 2e3892707..bbc49ea7d 100644 --- a/milli/src/asc_desc.rs +++ b/milli/src/asc_desc.rs @@ -28,7 +28,6 @@ impl From for AscDescError { } } - impl From for CriterionError { fn from(error: AscDescError) -> Self { match error { diff --git a/milli/src/search/facet/filter.rs b/milli/src/search/facet/filter.rs index 585d59a29..a4ac53950 100644 --- a/milli/src/search/facet/filter.rs +++ b/milli/src/search/facet/filter.rs @@ -389,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(BadGeoError::Lat(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(BadGeoError::Lng(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)? { @@ -434,12 +430,14 @@ 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(BadGeoError::Lat(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(BadGeoError::Lng(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] @@ -451,10 +449,7 @@ impl<'a> Filter<'a> { } if top_left[0] < bottom_right[0] { return Err(bottom_right_point[1].as_external_error( - BadGeoError::BoundingBoxTopIsBelowBottom( - 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 fb93ae00b..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, BadGeoError}; +pub use self::filter::{BadGeoError, Filter}; use crate::heed_codec::facet::{FacetGroupKeyCodec, FacetGroupValueCodec}; use crate::heed_codec::ByteSliceRefCodec; mod facet_distribution;