From 0ee67bb7d174f1ef974bb578d565914322a535a2 Mon Sep 17 00:00:00 2001 From: Tamo Date: Thu, 30 Sep 2021 12:28:40 +0200 Subject: [PATCH 1/2] improve the reserved keyword error message for the filters --- milli/src/search/facet/filter_condition.rs | 97 +++++++++++++++++----- milli/src/search/facet/grammar.pest | 2 +- 2 files changed, 79 insertions(+), 20 deletions(-) diff --git a/milli/src/search/facet/filter_condition.rs b/milli/src/search/facet/filter_condition.rs index 1e5bf9ad0..f0a51fe0a 100644 --- a/milli/src/search/facet/filter_condition.rs +++ b/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(); diff --git a/milli/src/search/facet/grammar.pest b/milli/src/search/facet/grammar.pest index d07d5bca5..8bfdeb667 100644 --- a/milli/src/search/facet/grammar.pest +++ b/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} From d9eba9d1451a89ad98d171d1ea8c2ad5f381df1b Mon Sep 17 00:00:00 2001 From: Tamo Date: Thu, 30 Sep 2021 12:50:40 +0200 Subject: [PATCH 2/2] improve and test the sort error message --- milli/src/asc_desc.rs | 70 ++++++++++++++++++++++++++++++------------- 1 file changed, 49 insertions(+), 21 deletions(-) diff --git a/milli/src/asc_desc.rs b/milli/src/asc_desc.rs index b8323292c..09bd0082a 100644 --- a/milli/src/asc_desc.rs +++ b/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 { match text.rsplit_once(':') { Some((left, "asc")) => Ok(AscDesc::Asc(left.parse()?)), @@ -156,10 +154,10 @@ impl From 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() + ); + } + } }