Merge #380

380: Reserved keyword error message r=Kerollmops a=irevoire

And I missed _another_ reserved keyword error message in the filter :(

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

milli/src/asc_desc.rs

@@ -20,12 +20,12 @@ impl fmt::Display for AscDescError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
             Self::InvalidSyntax { name } => {
-                write!(f, "invalid asc/desc syntax for {}", name)
+                write!(f, "invalid asc/desc syntax for {}.", name)
             }
             Self::ReservedKeyword { name } => {
                 write!(
                     f,
-                    "{} is a reserved keyword and thus can't be used as a asc/desc rule",
+                    "{} is a reserved keyword and thus can't be used as a asc/desc rule.",
                     name
                 )
             }
@@ -128,8 +128,6 @@ impl AscDesc {
 impl FromStr for AscDesc {
     type Err = AscDescError;
 
-    /// Since we don't know if this was deserialized for a criterion or a sort we just return a
-    /// string and let the caller create his own error.
     fn from_str(text: &str) -> Result<AscDesc, Self::Err> {
         match text.rsplit_once(':') {
             Some((left, "asc")) => Ok(AscDesc::Asc(left.parse()?)),
@@ -156,10 +154,10 @@ impl From<AscDescError> for SortError {
                 SortError::BadGeoPointUsage { name }
             }
             AscDescError::ReservedKeyword { name } if &name == "_geo" => {
-                SortError::ReservedNameForSettings { name: "_geoPoint".to_string() }
+                SortError::ReservedNameForSettings { name }
             }
             AscDescError::ReservedKeyword { name } if name.starts_with("_geoRadius") => {
-                SortError::ReservedNameForFilter { name: "_geoRadius".to_string() }
+                SortError::ReservedNameForFilter { name: String::from("_geoRadius") }
             }
             AscDescError::ReservedKeyword { name } => SortError::ReservedName { name },
         }
@@ -173,34 +171,26 @@ impl fmt::Display for SortError {
                 write!(
                     f,
                     "invalid syntax for the `_geoPoint` parameter: `{}`. \
-Usage: `_geoPoint(latitude, longitude):asc`",
+                    Usage: `_geoPoint(latitude, longitude):asc`.",
                     name
                 )
             }
             Self::InvalidName { name } => {
-                write!(f, "invalid syntax for the sort parameter {}", name)
+                write!(f, "invalid syntax for the sort parameter `{}`.", name)
             }
             Self::ReservedName { name } => {
                 write!(
                     f,
-                    "{} is a reserved keyword and thus can't be used as a sort expression",
+                    "{} is a reserved keyword and thus can't be used as a sort expression.",
                     name
                 )
             }
-            Self::ReservedNameForSettings { name } => {
+            Self::ReservedNameForSettings { name } | Self::ReservedNameForFilter { name } => {
                 write!(
                     f,
-                    "{} is a reserved keyword and thus can't be used as a sort expression. \
-{} can only be used in the settings",
-                    name, name
-                )
-            }
-            Self::ReservedNameForFilter { name } => {
-                write!(
-                    f,
-                    "{} is a reserved keyword and thus can't be used as a sort expression. \
-{} can only be used for filtering at search time",
-                    name, name
+                    "`{}` 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.",
+                    name,
                 )
             }
         }
@@ -299,4 +289,42 @@ mod tests {
             );
         }
     }
+
+    #[test]
+    fn sort_error_message() {
+        let errors = [
+            (
+                AscDescError::InvalidSyntax { name: S("truc:machin") },
+                S("invalid syntax for the sort parameter `truc:machin`."),
+            ),
+            (
+                AscDescError::InvalidSyntax { name: S("hello:world") },
+                S("invalid syntax for the sort parameter `hello:world`."),
+            ),
+            (
+                AscDescError::ReservedKeyword { name: S("_geo") },
+                S("`_geo` 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::ReservedKeyword { name: S("_geoDistance") },
+                S("_geoDistance is a reserved keyword and thus can't be used as a sort expression.")
+            ),
+            (
+                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."),
+            ),
+        ];
+
+        for (asc_desc_error, expected_message) in errors {
+            let sort_error = SortError::from(asc_desc_error);
+            assert_eq!(
+                sort_error.to_string(),
+                expected_message,
+                "was expecting {} for the error {:?} but instead got {}",
+                expected_message,
+                sort_error,
+                sort_error.to_string()
+            );
+        }
+    }
 }

milli/src/search/facet/filter_condition.rs

@@ -600,24 +600,33 @@ fn field_id(
     // lexing ensures that we at least have a key
     let key = items.next().unwrap();
     if key.as_rule() == Rule::reserved {
-        return Err(PestError::new_from_span(
-                ErrorVariant::CustomError {
-                    message: format!(
-                                 "`{}` is a reserved keyword and therefore can't be used as a filter expression. \
-                    Available filterable attributes are: {}",
-                    key.as_str(),
-                    filterable_fields.iter().join(", "),
-                             ),
-                },
-                key.as_span(),
-        ));
+        let message = match key.as_str() {
+            key if key.starts_with("_geoPoint") => {
+                format!(
+                    "`_geoPoint` 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.",
+                )
+            }
+            key @ "_geo" => {
+                format!(
+                    "`{}` 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.",
+                    key
+                )
+            }
+            key => format!(
+                "`{}` is a reserved keyword and thus can't be used as a filter expression.",
+                key
+            ),
+        };
+        return Err(PestError::new_from_span(ErrorVariant::CustomError { message }, key.as_span()));
     }
 
     if !filterable_fields.contains(key.as_str()) {
         return Err(PestError::new_from_span(
             ErrorVariant::CustomError {
                 message: format!(
-                    "attribute `{}` is not filterable, available filterable attributes are: {}",
+                    "attribute `{}` is not filterable, available filterable attributes are: {}.",
                     key.as_str(),
                     filterable_fields.iter().join(", "),
                 ),
@@ -688,13 +697,6 @@ mod tests {
         let condition = FilterCondition::from_str(&rtxn, &index, "NOT channel = ponce").unwrap();
         let expected = Operator(0, Operator::NotEqual(None, S("ponce")));
         assert_eq!(condition, expected);
-
-        let result = FilterCondition::from_str(&rtxn, &index, "_geo = France");
-        assert!(result.is_err());
-        let error = result.unwrap_err();
-        assert!(error.to_string().contains(
-            "`_geo` is a reserved keyword and therefore can't be used as a filter expression."
-        ));
     }
 
     #[test]
@@ -777,6 +779,49 @@ mod tests {
         assert_eq!(condition, expected);
     }
 
+    #[test]
+    fn reserved_field_names() {
+        let path = tempfile::tempdir().unwrap();
+        let mut options = EnvOpenOptions::new();
+        options.map_size(10 * 1024 * 1024); // 10 MB
+        let index = Index::new(options, &path).unwrap();
+        let rtxn = index.read_txn().unwrap();
+
+        let error = FilterCondition::from_str(&rtxn, &index, "_geo = 12").unwrap_err();
+        assert!(error
+            .to_string()
+            .contains("`_geo` 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."),
+            "{}",
+            error.to_string()
+        );
+
+        let error =
+            FilterCondition::from_str(&rtxn, &index, r#"_geoDistance <= 1000"#).unwrap_err();
+        assert!(error
+            .to_string()
+            .contains("`_geoDistance` is a reserved keyword and thus can't be used as a filter expression."),
+            "{}",
+            error.to_string()
+        );
+
+        let error = FilterCondition::from_str(&rtxn, &index, r#"_geoPoint > 5"#).unwrap_err();
+        assert!(error
+            .to_string()
+            .contains("`_geoPoint` 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."),
+            "{}",
+            error.to_string()
+        );
+
+        let error =
+            FilterCondition::from_str(&rtxn, &index, r#"_geoPoint(12, 16) > 5"#).unwrap_err();
+        assert!(error
+            .to_string()
+            .contains("`_geoPoint` 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."),
+            "{}",
+            error.to_string()
+        );
+    }
+
     #[test]
     fn geo_radius() {
         let path = tempfile::tempdir().unwrap();
@@ -788,6 +833,20 @@ mod tests {
         let mut wtxn = index.write_txn().unwrap();
         let mut builder = Settings::new(&mut wtxn, &index, 0);
         builder.set_searchable_fields(vec![S("_geo"), S("price")]); // to keep the fields order
+        builder.execute(|_, _| ()).unwrap();
+        wtxn.commit().unwrap();
+
+        let rtxn = index.read_txn().unwrap();
+        // _geo is not filterable
+        let result = FilterCondition::from_str(&rtxn, &index, "_geoRadius(12, 12, 10)");
+        assert!(result.is_err());
+        let error = result.unwrap_err();
+        assert!(error
+            .to_string()
+            .contains("attribute `_geo` is not filterable, available filterable attributes are:"),);
+
+        let mut wtxn = index.write_txn().unwrap();
+        let mut builder = Settings::new(&mut wtxn, &index, 0);
         builder.set_filterable_fields(hashset! { S("_geo"), S("price") });
         builder.execute(|_, _| ()).unwrap();
         wtxn.commit().unwrap();

milli/src/search/facet/grammar.pest

@@ -8,7 +8,7 @@ char =  _{ !(PEEK | "\\") ~ ANY
     | "\\" ~ (PEEK | "\\" | "/" | "b" | "f" | "n" | "r" | "t")
     | "\\" ~ ("u" ~ ASCII_HEX_DIGIT{4})}
 
-reserved = { "_geo" | "_geoDistance" | "_geoPoint" | ("_geoPoint" ~ parameters) }
+reserved = { "_geoDistance" | ("_geoPoint" ~ parameters) | "_geo" }
 // we deliberately choose to allow empty parameters to generate more specific error message later
 parameters = {("(" ~ (value ~ ",")* ~ value? ~ ")") | ""}
 condition = _{between | eq | greater | less | geq | leq | neq}