Merge #383

383: Add check on latitude and longitude r=irevoire a=irevoire

Latitudes are not supposed to go beyond 90 degrees or below -90.
The same goes for longitudes with 180 or -180.

This was badly implemented in the filters, and was not implemented for the `AscDesc` rules.

Co-authored-by: Tamo <tamo@meilisearch.com>
Co-authored-by: Irevoire <tamo@meilisearch.com>

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 {
+    InvalidLatitude,
+    InvalidLongitude,
     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::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 asc/desc syntax for {}.", name)
             }
@@ -36,6 +44,9 @@ impl fmt::Display for AscDescError {
 impl From<AscDescError> for CriterionError {
     fn from(error: AscDescError) -> Self {
         match error {
+            AscDescError::InvalidLatitude | AscDescError::InvalidLongitude => {
+                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<Member, Self::Err> {
         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::InvalidLatitude)?;
+                } else if !(-180.0..=180.0).contains(&lng) {
+                    return Err(AscDescError::InvalidLongitude)?;
+                }
+                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 {
+    InvalidLatitude,
+    InvalidLongitude,
     BadGeoPointUsage { name: String },
     InvalidName { name: String },
     ReservedName { name: String },
@@ -149,6 +167,8 @@ pub enum SortError {
 impl From<AscDescError> for SortError {
     fn from(error: AscDescError) -> Self {
         match error {
+            AscDescError::InvalidLatitude => SortError::InvalidLatitude,
+            AscDescError::InvalidLongitude => SortError::InvalidLongitude,
             AscDescError::InvalidSyntax { name } => SortError::InvalidName { name },
             AscDescError::ReservedKeyword { name } if name.starts_with("_geoPoint") => {
                 SortError::BadGeoPointUsage { name }
@@ -167,6 +187,8 @@ impl From<AscDescError> for SortError {
 impl fmt::Display for SortError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
+            Self::InvalidLatitude => write!(f, "{}", AscDescError::InvalidLatitude),
+            Self::InvalidLongitude => write!(f, "{}", AscDescError::InvalidLongitude),
             Self::BadGeoPointUsage { name } => {
                 write!(
                     f,
@@ -225,6 +247,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 +292,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),
         ];
 
         for (req, expected_error) in invalid_req {
@@ -313,6 +342,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::InvalidLatitude,
+                S("Latitude must be contained between -90 and 90 degrees."),
+            ),
+            (
+                AscDescError::InvalidLongitude,
+                S("Longitude must be contained between -180 and 180 degrees."),
+            ),
         ];
 
         for (asc_desc_error, expected_message) in errors {

milli/src/search/facet/filter_condition.rs

@@ -206,17 +206,19 @@ impl FilterCondition {
             )))?;
         }
         let (lat, lng, distance) = (&parameters[0], &parameters[1], parameters[2].0);
-        if let Some(span) = (!(-181.0..181.).contains(&lat.0))
+        if !(-90.0..=90.0).contains(&lat.0) {
-            .then(|| &lat.1)
-            .or((!(-181.0..181.).contains(&lng.0)).then(|| &lng.1))
-        {
             return Err(UserError::InvalidFilter(PestError::new_from_span(
                 ErrorVariant::CustomError {
-                    message: format!(
+                    message: format!("Latitude must be contained between -90 and 90 degrees."),
-                        "Latitude and longitude must be contained between -180 to 180 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]