From b09a8f1b915911631e94925cf0420b569a6db577 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Lecrenier?= Date: Wed, 17 Aug 2022 16:06:29 +0200 Subject: [PATCH] Filters: add explicit error message when using a keyword as value --- filter-parser/src/error.rs | 29 +++++++++++++++++------- filter-parser/src/lib.rs | 45 ++++++++++++++++++++++++-------------- filter-parser/src/value.rs | 14 ++++++++++-- 3 files changed, 62 insertions(+), 26 deletions(-) diff --git a/filter-parser/src/error.rs b/filter-parser/src/error.rs index 0d2959126..ce9470ff8 100644 --- a/filter-parser/src/error.rs +++ b/filter-parser/src/error.rs @@ -48,6 +48,12 @@ pub struct Error<'a> { kind: ErrorKind<'a>, } +#[derive(Debug)] +pub enum ExpectedValueKind { + ReservedKeyword, + Other, +} + #[derive(Debug)] pub enum ErrorKind<'a> { ReservedGeo(&'a str), @@ -55,11 +61,11 @@ pub enum ErrorKind<'a> { MisusedGeo, InvalidPrimary, ExpectedEof, - ExpectedValue, + ExpectedValue(ExpectedValueKind), MalformedValue, InOpeningBracket, InClosingBracket, - InExpectedValue, + InExpectedValue(ExpectedValueKind), ReservedKeyword(String), MissingClosingDelimiter(char), Char(char), @@ -118,18 +124,22 @@ impl<'a> Display for Error<'a> { let escaped_input = input.escape_debug(); match &self.kind { - ErrorKind::ExpectedValue if input.trim().is_empty() => { + ErrorKind::ExpectedValue(_) if input.trim().is_empty() => { writeln!(f, "Was expecting a value but instead got nothing.")? } + ErrorKind::ExpectedValue(ExpectedValueKind::ReservedKeyword) => { + writeln!(f, "Was expecting a value but instead got `{escaped_input}`, which is a reserved keyword. To use `{escaped_input}` as a field name or a value, surround it by quotes.")? + } + ErrorKind::ExpectedValue(ExpectedValueKind::Other) => { + writeln!(f, "Was expecting a value but instead got `{}`.", escaped_input)? + } ErrorKind::MalformedValue => { writeln!(f, "Malformed value: `{}`.", escaped_input)? } ErrorKind::MissingClosingDelimiter(c) => { writeln!(f, "Expression `{}` is missing the following closing delimiter: `{}`.", escaped_input, c)? } - ErrorKind::ExpectedValue => { - writeln!(f, "Was expecting a value but instead got `{}`.", escaped_input)? - } + ErrorKind::InvalidPrimary if input.trim().is_empty() => { writeln!(f, "Was expecting an operation `=`, `!=`, `>=`, `>`, `<=`, `<`, `TO`, `EXISTS`, `NOT EXISTS`, or `_geoRadius` but instead got nothing.")? } @@ -157,8 +167,11 @@ impl<'a> Display for Error<'a> { ErrorKind::InClosingBracket => { writeln!(f, "Expected matching `]` after the list of field names given to `IN[`")? } - ErrorKind::InExpectedValue => { - writeln!(f, "Expected only comma-separated field names inside `IN[..]` but instead found `{escaped_input}`")? + ErrorKind::InExpectedValue(ExpectedValueKind::ReservedKeyword) => { + writeln!(f, "Expected only comma-separated field names inside `IN[..]` but instead found `{escaped_input}`, which is a keyword. To use `{escaped_input}` as a field name or a value, surround it by quotes.")? + } + ErrorKind::InExpectedValue(ExpectedValueKind::Other) => { + writeln!(f, "Expected only comma-separated field names inside `IN[..]` but instead found `{escaped_input}`.")? } ErrorKind::Char(c) => { panic!("Tried to display a char error with `{}`", c) diff --git a/filter-parser/src/lib.rs b/filter-parser/src/lib.rs index 31ca2919a..d0c2d8531 100644 --- a/filter-parser/src/lib.rs +++ b/filter-parser/src/lib.rs @@ -48,7 +48,7 @@ use std::str::FromStr; pub use condition::{parse_condition, parse_to, Condition}; use condition::{parse_exists, parse_not_exists}; -use error::{cut_with_err, NomErrorExt}; +use error::{cut_with_err, ExpectedValueKind, NomErrorExt}; pub use error::{Error, ErrorKind}; use nom::branch::alt; use nom::bytes::complete::tag; @@ -166,11 +166,6 @@ fn ws<'a, O>(inner: impl FnMut(Span<'a>) -> IResult) -> impl FnMut(Span<'a>) /// value_list = (value ("," value)* ","?)? fn parse_value_list<'a>(input: Span<'a>) -> IResult>> { - // TODO: here, I should return a failure with a clear explanation whenever possible - // for example: - // * expected the name of a field, but got `AND` - // * expected closing square bracket, but got `AND` - let (input, first_value) = opt(parse_value)(input)?; if let Some(first_value) = first_value { let value_list_el_parser = preceded(ws(tag(",")), parse_value); @@ -203,7 +198,14 @@ fn parse_in(input: Span) -> IResult { if eof::<_, ()>(input).is_ok() { Error::new_from_kind(input, ErrorKind::InClosingBracket) } else { - Error::new_from_kind(input, ErrorKind::InExpectedValue) + let expected_value_kind = match parse_value(input) { + Err(nom::Err::Error(e)) => match e.kind() { + ErrorKind::ReservedKeyword(_) => ExpectedValueKind::ReservedKeyword, + _ => ExpectedValueKind::Other, + }, + _ => ExpectedValueKind::Other, + }; + Error::new_from_kind(input, ErrorKind::InExpectedValue(expected_value_kind)) } })(input)?; @@ -319,6 +321,21 @@ fn parse_geo_point(input: Span) -> IResult { Err(nom::Err::Failure(Error::new_from_kind(input, ErrorKind::ReservedGeo("_geoPoint")))) } +fn parse_error_reserved_keyword(input: Span) -> IResult { + match parse_condition(input) { + Ok(result) => Ok(result), + Err(nom::Err::Error(inner) | nom::Err::Failure(inner)) => match inner.kind() { + ErrorKind::ExpectedValue(ExpectedValueKind::ReservedKeyword) => { + return Err(nom::Err::Failure(inner)); + } + _ => return Err(nom::Err::Error(inner)), + }, + Err(e) => { + return Err(e); + } + } +} + /// primary = (WS* "(" WS* expression WS* ")" WS*) | geoRadius | condition | exists | not_exists | to fn parse_primary(input: Span) -> IResult { alt(( @@ -339,6 +356,7 @@ fn parse_primary(input: Span) -> IResult { 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_point, + parse_error_reserved_keyword, ))(input) // if the inner parsers did not match enough information to return an accurate error .map_err(|e| e.map_err(|_| Error::new_from_kind(input, ErrorKind::InvalidPrimary))) @@ -376,13 +394,6 @@ pub mod tests { let test_case = [ // simple test - ( - "x = AND", - Fc::Not(Box::new(Fc::Not(Box::new(Fc::In { - fid: rtok("NOT NOT", "colour"), - els: vec![] - })))) - ), ( "colour IN[]", Fc::In { @@ -756,8 +767,8 @@ pub mod tests { ("channel = ", "Was expecting a value but instead got nothing."), ("channel = 🐻", "Was expecting a value but instead got `🐻`."), ("channel = 🐻 AND followers < 100", "Was expecting a value but instead got `🐻`."), - ("OR", "Was expecting an operation `=`, `!=`, `>=`, `>`, `<=`, `<`, `TO`, `EXISTS`, `NOT EXISTS`, or `_geoRadius` at `OR`."), - ("AND", "Was expecting an operation `=`, `!=`, `>=`, `>`, `<=`, `<`, `TO`, `EXISTS`, `NOT EXISTS`, or `_geoRadius` at `AND`."), + ("'OR'", "Was expecting an operation `=`, `!=`, `>=`, `>`, `<=`, `<`, `TO`, `EXISTS`, `NOT EXISTS`, or `_geoRadius` at `\\'OR\\'`."), + ("OR", "Was expecting a value but instead got `OR`, which is a reserved keyword. To use `OR` as a field name or a value, surround it by quotes."), ("channel Ponce", "Was expecting an operation `=`, `!=`, `>=`, `>`, `<=`, `<`, `TO`, `EXISTS`, `NOT EXISTS`, or `_geoRadius` at `channel Ponce`."), ("channel = Ponce OR", "Found unexpected characters at the end of the filter: `OR`. You probably forgot an `OR` or an `AND` rule."), ("_geoRadius", "The `_geoRadius` filter expects three arguments: `_geoRadius(latitude, longitude, radius)`."), @@ -778,6 +789,8 @@ pub mod tests { ("colour IN [blue, green, AND]", "Expected only comma-separated field names inside `IN[..]` but instead found `AND]`"), ("colour IN [blue, green", "Expected matching `]` after the list of field names given to `IN[`"), ("colour IN ['blue, green", "Expression `\\'blue, green` is missing the following closing delimiter: `'`."), + ("x = EXISTS", "Was expecting a value but instead got `EXISTS`, which is a reserved keyword. To use `EXISTS` as a field name or a value, surround it by quotes."), + ("AND = 8", "Was expecting a value but instead got `AND`, which is a reserved keyword. To use `AND` as a field name or a value, surround it by quotes."), ]; for (input, expected) in test_case { diff --git a/filter-parser/src/value.rs b/filter-parser/src/value.rs index 8a7e8f586..bfa3c2730 100644 --- a/filter-parser/src/value.rs +++ b/filter-parser/src/value.rs @@ -5,7 +5,7 @@ use nom::combinator::cut; use nom::sequence::{delimited, terminated}; use nom::{InputIter, InputLength, InputTake, Slice}; -use crate::error::NomErrorExt; +use crate::error::{ExpectedValueKind, NomErrorExt}; use crate::{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 (`\`). @@ -103,7 +103,17 @@ pub fn parse_value<'a>(input: Span<'a>) -> IResult> { )(input) // if we found nothing in the alt it means the user specified something that was not recognized as a value .map_err(|e: nom::Err| { - e.map_err(|_| Error::new_from_kind(error_word(input).unwrap().1, ErrorKind::ExpectedValue)) + e.map_err(|error| { + let expected_value_kind = if matches!(error.kind(), ErrorKind::ReservedKeyword(_)) { + ExpectedValueKind::ReservedKeyword + } else { + ExpectedValueKind::Other + }; + Error::new_from_kind( + error_word(input).unwrap().1, + ErrorKind::ExpectedValue(expected_value_kind), + ) + }) }) .map_err(|e| { e.map_fail(|failure| {