From 11dfe387616e8cb70822b55ed81a2e75aebf6022 Mon Sep 17 00:00:00 2001 From: Tamo Date: Thu, 7 Oct 2021 15:42:08 +0200 Subject: [PATCH] Update the check on the latitude and longitude Latitude are not supposed to go beyound 90 degrees or below -90. The same goes for longitude with 180 or -180. This was badly implemented in the filters, and was not implemented for the AscDesc rules. --- milli/src/asc_desc.rs | 49 ++++++++++++++++++-- milli/src/search/facet/filter_condition.rs | 54 +++++++++++++++++----- 2 files changed, 87 insertions(+), 16 deletions(-) diff --git a/milli/src/asc_desc.rs b/milli/src/asc_desc.rs index 09bd0082a..ebd550e60 100644 --- a/milli/src/asc_desc.rs +++ b/milli/src/asc_desc.rs @@ -12,6 +12,8 @@ use crate::{CriterionError, Error, UserError}; /// You must always cast it to a sort error or a criterion error. #[derive(Debug)] pub enum AscDescError { + BadLat, + BadLng, InvalidSyntax { name: String }, ReservedKeyword { name: String }, } @@ -19,6 +21,12 @@ pub enum AscDescError { impl fmt::Display for AscDescError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { + Self::BadLat => { + write!(f, "Latitude must be contained between -90 and 90 degrees.",) + } + Self::BadLng => { + write!(f, "Longitude must be contained between -180 and 180 degrees.",) + } Self::InvalidSyntax { name } => { write!(f, "invalid asc/desc syntax for {}.", name) } @@ -36,6 +44,9 @@ impl fmt::Display for AscDescError { impl From for CriterionError { fn from(error: AscDescError) -> Self { match error { + AscDescError::BadLat | AscDescError::BadLng => { + CriterionError::ReservedNameForSort { name: "_geoPoint".to_string() } + } AscDescError::InvalidSyntax { name } => CriterionError::InvalidName { name }, AscDescError::ReservedKeyword { name } if name.starts_with("_geoPoint") => { CriterionError::ReservedNameForSort { name: "_geoPoint".to_string() } @@ -60,16 +71,21 @@ impl FromStr for Member { fn from_str(text: &str) -> Result { match text.strip_prefix("_geoPoint(").and_then(|text| text.strip_suffix(")")) { Some(point) => { - let (lat, long) = point + let (lat, lng) = point .split_once(',') .ok_or_else(|| AscDescError::ReservedKeyword { name: text.to_string() }) - .and_then(|(lat, long)| { + .and_then(|(lat, lng)| { lat.trim() .parse() - .and_then(|lat| long.trim().parse().map(|long| (lat, long))) + .and_then(|lat| lng.trim().parse().map(|lng| (lat, lng))) .map_err(|_| AscDescError::ReservedKeyword { name: text.to_string() }) })?; - Ok(Member::Geo([lat, long])) + if !(-90.0..=90.0).contains(&lat) { + return Err(AscDescError::BadLat)?; + } else if !(-180.0..=180.0).contains(&lng) { + return Err(AscDescError::BadLng)?; + } + Ok(Member::Geo([lat, lng])) } None => { if is_reserved_keyword(text) || text.starts_with("_geoRadius(") { @@ -139,6 +155,8 @@ impl FromStr for AscDesc { #[derive(Debug)] pub enum SortError { + BadLat, + BadLng, BadGeoPointUsage { name: String }, InvalidName { name: String }, ReservedName { name: String }, @@ -149,6 +167,8 @@ pub enum SortError { impl From for SortError { fn from(error: AscDescError) -> Self { match error { + AscDescError::BadLat => SortError::BadLat, + AscDescError::BadLng => SortError::BadLng, AscDescError::InvalidSyntax { name } => SortError::InvalidName { name }, AscDescError::ReservedKeyword { name } if name.starts_with("_geoPoint") => { SortError::BadGeoPointUsage { name } @@ -167,6 +187,12 @@ impl From for SortError { impl fmt::Display for SortError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { + Self::BadLat => { + write!(f, "Latitude must be contained between -90 and 90 degrees.",) + } + Self::BadLng => { + write!(f, "Longitude must be contained between -180 and 180 degrees.",) + } Self::BadGeoPointUsage { name } => { write!( f, @@ -225,6 +251,8 @@ mod tests { ("_geoPoint(42.459, 59):desc", Desc(Geo([42.459, 59.]))), ("_geoPoint(42, 59.895):desc", Desc(Geo([42., 59.895]))), ("_geoPoint(42, 59.895):desc", Desc(Geo([42., 59.895]))), + ("_geoPoint(90.000000000, 180):desc", Desc(Geo([90., 180.]))), + ("_geoPoint(-90, -180.0000000000):asc", Asc(Geo([-90., -180.]))), ("_geoPoint(42.0002, 59.895):desc", Desc(Geo([42.0002, 59.895]))), ("_geoPoint(42., 59.):desc", Desc(Geo([42., 59.]))), ("truc(12, 13):desc", Desc(Field(S("truc(12, 13)")))), @@ -268,6 +296,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", BadLat), + ("_geoPoint(90.000001, 0):asc", BadLat), + ("_geoPoint(0, -180.000001):desc", BadLng), + ("_geoPoint(159.256, 130):asc", BadLat), + ("_geoPoint(12, -2021):desc", BadLng), ]; for (req, expected_error) in invalid_req { @@ -313,6 +346,14 @@ mod tests { AscDescError::ReservedKeyword { name: S("_geoRadius(12, 13)") }, S("`_geoRadius` is a reserved keyword and thus can't be used as a sort expression. Use the `_geoPoint(latitude, longitude)` built-in rule to sort on `_geo` field coordinates."), ), + ( + AscDescError::BadLat, + S("Latitude must be contained between -90 and 90 degrees."), + ), + ( + AscDescError::BadLng, + S("Longitude must be contained between -180 and 180 degrees."), + ), ]; for (asc_desc_error, expected_message) in errors { diff --git a/milli/src/search/facet/filter_condition.rs b/milli/src/search/facet/filter_condition.rs index f0a51fe0a..f1055b2f8 100644 --- a/milli/src/search/facet/filter_condition.rs +++ b/milli/src/search/facet/filter_condition.rs @@ -206,17 +206,19 @@ impl FilterCondition { )))?; } let (lat, lng, distance) = (¶meters[0], ¶meters[1], parameters[2].0); - if let Some(span) = (!(-181.0..181.).contains(&lat.0)) - .then(|| &lat.1) - .or((!(-181.0..181.).contains(&lng.0)).then(|| &lng.1)) - { + if !(-90.0..=90.0).contains(&lat.0) { return Err(UserError::InvalidFilter(PestError::new_from_span( ErrorVariant::CustomError { - message: format!( - "Latitude and longitude must be contained between -180 to 180 degrees." - ), + message: format!("Latitude must be contained between -90 and 90 degrees."), }, - span.clone(), + 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, GeoLowerThan([lat.0, lng.0], distance))) @@ -858,6 +860,18 @@ mod tests { let expected = Operator(0, GeoLowerThan([12., 13.0005], 2000.)); assert_eq!(condition, expected); + // basic test with latitude and longitude at the max angle + let condition = + FilterCondition::from_str(&rtxn, &index, "_geoRadius(90, 180, 2000)").unwrap(); + let expected = Operator(0, GeoLowerThan([90., 180.], 2000.)); + assert_eq!(condition, expected); + + // basic test with latitude and longitude at the min angle + let condition = + FilterCondition::from_str(&rtxn, &index, "_geoRadius(-90, -180, 2000)").unwrap(); + let expected = Operator(0, GeoLowerThan([-90., -180.], 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(); @@ -906,20 +920,36 @@ mod tests { assert!(error.to_string().contains("The `_geoRadius` filter expect three arguments: `_geoRadius(latitude, longitude, radius)`")); // georadius have a bad latitude - let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius(-200, 150, 10)"); + let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius(-100, 150, 10)"); assert!(result.is_err()); let error = result.unwrap_err(); assert!(error .to_string() - .contains("Latitude and longitude must be contained between -180 to 180 degrees.")); + .contains("Latitude must be contained between -90 and 90 degrees.")); + + // georadius have a bad latitude + let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius(-90.0000001, 150, 10)"); + assert!(result.is_err()); + let error = result.unwrap_err(); + assert!(error + .to_string() + .contains("Latitude must be contained between -90 and 90 degrees.")); // georadius have a bad longitude - let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius(-10, 181, 10)"); + let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius(-10, 250, 10)"); assert!(result.is_err()); let error = result.unwrap_err(); assert!(error .to_string() - .contains("Latitude and longitude must be contained between -180 to 180 degrees.")); + .contains("Longitude must be contained between -180 and 180 degrees.")); + + // georadius have a bad longitude + let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius(-10, 180.000001, 10)"); + assert!(result.is_err()); + let error = result.unwrap_err(); + assert!(error + .to_string() + .contains("Longitude must be contained between -180 and 180 degrees.")); } #[test]