diff --git a/filter-parser/src/error.rs b/filter-parser/src/error.rs index 4d9d89859..6de9526fd 100644 --- a/filter-parser/src/error.rs +++ b/filter-parser/src/error.rs @@ -159,7 +159,7 @@ impl<'a> Display for Error<'a> { writeln!(f, "The `_geoBoundingBox` filter expects two pairs of arguments: `_geoBoundingBox([latitude, longitude], [latitude, longitude])`.")? } ErrorKind::ReservedGeo(name) => { - writeln!(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` coordinates.", name.escape_debug())? + writeln!(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` coordinates.", name.escape_debug())? } ErrorKind::MisusedGeoRadius => { writeln!(f, "The `_geoRadius` filter is an operation and can't be used as a value.")? diff --git a/filter-parser/src/lib.rs b/filter-parser/src/lib.rs index 8e21ff6be..4bf9751c4 100644 --- a/filter-parser/src/lib.rs +++ b/filter-parser/src/lib.rs @@ -382,6 +382,34 @@ fn parse_geo_point(input: Span) -> IResult { Err(nom::Err::Failure(Error::new_from_kind(input, ErrorKind::ReservedGeo("_geoPoint")))) } +/// geoPoint = WS* "_geoDistance(float WS* "," WS* float WS* "," WS* float) +fn parse_geo_distance(input: Span) -> IResult { + // we want to forbid space BEFORE the _geoDistance but not after + tuple(( + multispace0, + tag("_geoDistance"), + // if we were able to parse `_geoDistance` we are going to return a Failure whatever happens next. + cut(delimited(char('('), separated_list1(tag(","), ws(recognize_float)), char(')'))), + ))(input) + .map_err(|e| e.map(|_| Error::new_from_kind(input, ErrorKind::ReservedGeo("_geoDistance"))))?; + // if we succeeded we still return a `Failure` because `geoDistance` filters are not allowed + Err(nom::Err::Failure(Error::new_from_kind(input, ErrorKind::ReservedGeo("_geoDistance")))) +} + +/// geo = WS* "_geo(float WS* "," WS* float WS* "," WS* float) +fn parse_geo(input: Span) -> IResult { + // we want to forbid space BEFORE the _geo but not after + tuple(( + multispace0, + word_exact("_geo"), + // if we were able to parse `_geo` we are going to return a Failure whatever happens next. + cut(delimited(char('('), separated_list1(tag(","), ws(recognize_float)), char(')'))), + ))(input) + .map_err(|e| e.map(|_| Error::new_from_kind(input, ErrorKind::ReservedGeo("_geo"))))?; + // if we succeeded we still return a `Failure` because `_geo` filter is not allowed + Err(nom::Err::Failure(Error::new_from_kind(input, ErrorKind::ReservedGeo("_geo")))) +} + fn parse_error_reserved_keyword(input: Span) -> IResult { match parse_condition(input) { Ok(result) => Ok(result), @@ -418,6 +446,8 @@ fn parse_primary(input: Span, depth: usize) -> IResult { parse_not_exists, parse_to, // the next lines are only for error handling and are written at the end to have the less possible performance impact + parse_geo, + parse_geo_distance, parse_geo_point, parse_error_reserved_keyword, ))(input) @@ -621,15 +651,35 @@ pub mod tests { "###); insta::assert_display_snapshot!(p("_geoPoint(12, 13, 14)"), @r###" - `_geoPoint` 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` coordinates. + `_geoPoint` 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` coordinates. 1:22 _geoPoint(12, 13, 14) "###); insta::assert_display_snapshot!(p("position <= _geoPoint(12, 13, 14)"), @r###" - `_geoPoint` 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` coordinates. + `_geoPoint` 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` coordinates. 13:34 position <= _geoPoint(12, 13, 14) "###); + insta::assert_display_snapshot!(p("_geoDistance(12, 13, 14)"), @r###" + `_geoDistance` 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` coordinates. + 1:25 _geoDistance(12, 13, 14) + "###); + + insta::assert_display_snapshot!(p("position <= _geoDistance(12, 13, 14)"), @r###" + `_geoDistance` 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` coordinates. + 13:37 position <= _geoDistance(12, 13, 14) + "###); + + insta::assert_display_snapshot!(p("_geo(12, 13, 14)"), @r###" + `_geo` 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` coordinates. + 1:17 _geo(12, 13, 14) + "###); + + insta::assert_display_snapshot!(p("position <= _geo(12, 13, 14)"), @r###" + `_geo` 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` coordinates. + 13:29 position <= _geo(12, 13, 14) + "###); + insta::assert_display_snapshot!(p("position <= _geoRadius(12, 13, 14)"), @r###" The `_geoRadius` filter is an operation and can't be used as a value. 13:35 position <= _geoRadius(12, 13, 14) diff --git a/filter-parser/src/value.rs b/filter-parser/src/value.rs index 2296c0769..fb8f35503 100644 --- a/filter-parser/src/value.rs +++ b/filter-parser/src/value.rs @@ -7,8 +7,8 @@ use nom::{InputIter, InputLength, InputTake, Slice}; use crate::error::{ExpectedValueKind, NomErrorExt}; use crate::{ - parse_geo_bounding_box, parse_geo_point, parse_geo_radius, Error, ErrorKind, IResult, Span, - Token, + parse_geo, parse_geo_bounding_box, parse_geo_distance, parse_geo_point, parse_geo_radius, + Error, ErrorKind, IResult, Span, Token, }; /// This function goes through all characters in the [Span] if it finds any escaped character (`\`). @@ -88,11 +88,16 @@ pub fn parse_value(input: Span) -> IResult { // then, we want to check if the user is misusing a geo expression // This expression can’t finish without error. // We want to return an error in case of failure. - if let Err(err) = parse_geo_point(input) { - if err.is_failure() { - return Err(err); + let geo_reserved_parse_functions = [parse_geo_point, parse_geo_distance, parse_geo]; + + for parser in geo_reserved_parse_functions { + if let Err(err) = parser(input) { + if err.is_failure() { + return Err(err); + } } } + match parse_geo_radius(input) { Ok(_) => { return Err(nom::Err::Failure(Error::new_from_kind(input, ErrorKind::MisusedGeoRadius))) diff --git a/meilisearch/tests/search/errors.rs b/meilisearch/tests/search/errors.rs index 9e4dbdcf5..0aaf87773 100644 --- a/meilisearch/tests/search/errors.rs +++ b/meilisearch/tests/search/errors.rs @@ -672,7 +672,7 @@ async fn filter_reserved_geo_attribute_array() { index.wait_task(1).await; let expected_response = json!({ - "message": "`_geo` 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.\n1:5 _geo = Glass", + "message": "`_geo` 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` coordinates.\n1:13 _geo = Glass", "code": "invalid_search_filter", "type": "invalid_request", "link": "https://docs.meilisearch.com/errors#invalid_search_filter" @@ -697,7 +697,7 @@ async fn filter_reserved_geo_attribute_string() { index.wait_task(1).await; let expected_response = json!({ - "message": "`_geo` 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.\n1:5 _geo = Glass", + "message": "`_geo` 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` coordinates.\n1:13 _geo = Glass", "code": "invalid_search_filter", "type": "invalid_request", "link": "https://docs.meilisearch.com/errors#invalid_search_filter" @@ -722,7 +722,7 @@ async fn filter_reserved_attribute_array() { index.wait_task(1).await; let expected_response = json!({ - "message": "`_geoDistance` is a reserved keyword and thus can't be used as a filter expression.\n1:13 _geoDistance = Glass", + "message": "`_geoDistance` 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` coordinates.\n1:21 _geoDistance = Glass", "code": "invalid_search_filter", "type": "invalid_request", "link": "https://docs.meilisearch.com/errors#invalid_search_filter" @@ -747,7 +747,7 @@ async fn filter_reserved_attribute_string() { index.wait_task(1).await; let expected_response = json!({ - "message": "`_geoDistance` is a reserved keyword and thus can't be used as a filter expression.\n1:13 _geoDistance = Glass", + "message": "`_geoDistance` 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` coordinates.\n1:21 _geoDistance = Glass", "code": "invalid_search_filter", "type": "invalid_request", "link": "https://docs.meilisearch.com/errors#invalid_search_filter" @@ -760,6 +760,56 @@ async fn filter_reserved_attribute_string() { .await; } +#[actix_rt::test] +async fn filter_reserved_geo_point_array() { + let server = Server::new().await; + let index = server.index("test"); + + index.update_settings(json!({"filterableAttributes": ["title"]})).await; + + let documents = DOCUMENTS.clone(); + index.add_documents(documents, None).await; + index.wait_task(1).await; + + let expected_response = json!({ + "message": "`_geoPoint` 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` coordinates.\n1:18 _geoPoint = Glass", + "code": "invalid_search_filter", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid_search_filter" + }); + index + .search(json!({"filter": ["_geoPoint = Glass"]}), |response, code| { + assert_eq!(response, expected_response); + assert_eq!(code, 400); + }) + .await; +} + +#[actix_rt::test] +async fn filter_reserved_geo_point_string() { + let server = Server::new().await; + let index = server.index("test"); + + index.update_settings(json!({"filterableAttributes": ["title"]})).await; + + let documents = DOCUMENTS.clone(); + index.add_documents(documents, None).await; + index.wait_task(1).await; + + let expected_response = json!({ + "message": "`_geoPoint` 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` coordinates.\n1:18 _geoPoint = Glass", + "code": "invalid_search_filter", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid_search_filter" + }); + index + .search(json!({"filter": "_geoPoint = Glass"}), |response, code| { + assert_eq!(response, expected_response); + assert_eq!(code, 400); + }) + .await; +} + #[actix_rt::test] async fn sort_geo_reserved_attribute() { let server = Server::new().await; diff --git a/milli/src/search/facet/filter.rs b/milli/src/search/facet/filter.rs index a4ac53950..32515fcde 100644 --- a/milli/src/search/facet/filter.rs +++ b/milli/src/search/facet/filter.rs @@ -54,8 +54,6 @@ impl Display for BadGeoError { enum FilterError<'a> { AttributeNotFilterable { attribute: &'a str, filterable_fields: HashSet }, ParseGeoError(BadGeoError), - ReservedGeo(&'a str), - Reserved(&'a str), TooDeep, } impl<'a> std::error::Error for FilterError<'a> {} @@ -96,12 +94,6 @@ 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.", - keyword - ), Self::ParseGeoError(error) => write!(f, "{}", error), } } @@ -332,23 +324,10 @@ impl<'a> Filter<'a> { Ok(RoaringBitmap::new()) } } else { - match fid.value() { - attribute @ "_geo" => { - Err(fid.as_external_error(FilterError::ReservedGeo(attribute)))? - } - attribute if attribute.starts_with("_geoPoint(") => { - Err(fid.as_external_error(FilterError::ReservedGeo("_geoPoint")))? - } - attribute @ "_geoDistance" => { - Err(fid.as_external_error(FilterError::Reserved(attribute)))? - } - attribute => { - Err(fid.as_external_error(FilterError::AttributeNotFilterable { - attribute, - filterable_fields: filterable_fields.clone(), - }))? - } - } + Err(fid.as_external_error(FilterError::AttributeNotFilterable { + attribute: fid.value(), + filterable_fields: filterable_fields.clone(), + }))? } } FilterCondition::Or(subfilters) => {