chore: Remove variants from Identifier and BindParameter types (#5642)

* chore: Remove variants from Identifier and BindParameter types This simplifies usage of these types. Display traits have been updated to properly quote and escape the output, when necessary. * chore: Fix docs
2022-09-15 16:52:31 +10:00 · 2022-09-15 16:52:31 +10:00 · e5d8f23fcd
parent 7c4c918636
commit e5d8f23fcd
6 changed files with 111 additions and 100 deletions
--- a/influxdb_influxql_parser/src/common.rs
+++ b/influxdb_influxql_parser/src/common.rs
@ -198,7 +198,7 @@ mod tests {
            MeasurementNameExpression {
                database: None,
                retention_policy: None,
-                name: Identifier::Unquoted("diskio".into()),
+                name: "diskio".into(),
            }
        );

@ -206,9 +206,9 @@ mod tests {
        assert_eq!(
            got,
            MeasurementNameExpression {
-                database: Some(Identifier::Unquoted("telegraf".into())),
-                retention_policy: Some(Identifier::Unquoted("autogen".into())),
-                name: Identifier::Unquoted("diskio".into()),
+                database: Some("telegraf".into()),
+                retention_policy: Some("autogen".into()),
+                name: "diskio".into(),
            }
        );

@ -216,9 +216,9 @@ mod tests {
        assert_eq!(
            got,
            MeasurementNameExpression {
-                database: Some(Identifier::Unquoted("telegraf".into())),
+                database: Some("telegraf".into()),
                retention_policy: None,
-                name: Identifier::Unquoted("diskio".into()),
+                name: "diskio".into(),
            }
        );
    }
--- a/influxdb_influxql_parser/src/expression.rs
+++ b/influxdb_influxql_parser/src/expression.rs
@ -200,7 +200,7 @@ fn parens(i: &str) -> ParseResult<&str, Expr> {
 fn call(i: &str) -> ParseResult<&str, Expr> {
    map(
        separated_pair(
-            unquoted_identifier,
+            map(unquoted_identifier, &str::to_string),
            multispace0,
            delimited(
                char('('),
@ -364,7 +364,7 @@ mod test {
    /// Constructs an [Expr::Identifier] expression.
    macro_rules! ident {
        ($EXPR: expr) => {
-            Expr::Identifier(crate::identifier::Identifier::Unquoted($EXPR.into()))
+            Expr::Identifier($EXPR.into())
        };
    }

@ -378,7 +378,7 @@ mod test {
    /// Constructs a [Expr::BindParameter] expression.
    macro_rules! param {
        ($EXPR: expr) => {
-            Expr::BindParameter(crate::parameter::BindParameter::Unquoted($EXPR.into()).into())
+            Expr::BindParameter(crate::parameter::BindParameter($EXPR.into()).into())
        };
    }

@ -603,7 +603,7 @@ mod test {
        // quoted identifier
        let (_, e) = conditional_expression(r#""foo" + 'bar'"#).unwrap();
        let got = format!("{}", e);
-        assert_eq!(got, r#""foo" + 'bar'"#);
+        assert_eq!(got, r#"foo + 'bar'"#);

        // Duration
        let (_, e) = conditional_expression("- 6h30m").unwrap();
--- a/influxdb_influxql_parser/src/identifier.rs
+++ b/influxdb_influxql_parser/src/identifier.rs
@ -16,7 +16,7 @@
 use crate::internal::ParseResult;
 use crate::keywords::sql_keyword;
 use crate::string::double_quoted_string;
-use crate::write_escaped;
+use crate::write_quoted_string;
 use nom::branch::alt;
 use nom::bytes::complete::tag;
 use nom::character::complete::{alpha1, alphanumeric1};
@ -27,42 +27,35 @@ use std::fmt;
 use std::fmt::{Display, Formatter, Write};

 /// Parse an unquoted InfluxQL identifier.
-pub fn unquoted_identifier(i: &str) -> ParseResult<&str, String> {
-    map(
-        preceded(
-            not(sql_keyword),
-            recognize(pair(
-                alt((alpha1, tag("_"))),
-                many0_count(alt((alphanumeric1, tag("_")))),
-            )),
-        ),
-        str::to_string,
+pub fn unquoted_identifier(i: &str) -> ParseResult<&str, &str> {
+    preceded(
+        not(sql_keyword),
+        recognize(pair(
+            alt((alpha1, tag("_"))),
+            many0_count(alt((alphanumeric1, tag("_")))),
+        )),
    )(i)
 }

-/// `Identifier` is a type that represents either a quoted ([`Identifier::Quoted`]) or unquoted ([`Identifier::Unquoted`])
-/// InfluxQL identifier.
+/// A type that represents an InfluxQL identifier.
 #[derive(Clone, Debug, Eq, Hash, PartialEq)]
-pub enum Identifier {
-    /// Contains an unquoted identifier
-    Unquoted(String),
+pub struct Identifier(pub String);

-    /// Contains an unescaped quoted identifier
-    Quoted(String),
+impl From<String> for Identifier {
+    fn from(s: String) -> Self {
+        Self(s)
+    }
+}
+
+impl From<&str> for Identifier {
+    fn from(s: &str) -> Self {
+        Self(s.to_string())
+    }
 }

 impl Display for Identifier {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
-        match self {
-            Self::Unquoted(s) => write!(f, "{}", s)?,
-            Self::Quoted(s) => {
-                f.write_char('"')?;
-                // escape characters per https://github.com/influxdata/influxql/blob/df51a45762be9c1b578f01718fa92d286a843fe9/scanner.go#L576-L583
-                write_escaped!(f, s, '\n' => "\\n", '\\' => "\\\\", '"' => "\\\"");
-                f.write_char('"')?;
-            }
-        };
-
+        write_quoted_string!(f, '"', self.0.as_str(), unquoted_identifier, '\n' => "\\n", '\\' => "\\\\", '"' => "\\\"");
        Ok(())
    }
 }
@ -71,8 +64,8 @@ impl Display for Identifier {
 pub fn identifier(i: &str) -> ParseResult<&str, Identifier> {
    // See: https://github.com/influxdata/influxql/blob/df51a45762be9c1b578f01718fa92d286a843fe9/scanner.go#L358-L362
    alt((
-        map(unquoted_identifier, Identifier::Unquoted),
-        map(double_quoted_string, Identifier::Quoted),
+        map(unquoted_identifier, Into::into),
+        map(double_quoted_string, Into::into),
    ))(i)
 }

@ -109,24 +102,21 @@ mod test {
    fn test_identifier() {
        // quoted
        let (_, got) = identifier("\"quick draw\"").unwrap();
-        assert!(matches!(got, Identifier::Quoted(s) if s == "quick draw"));
+        assert_eq!(got, "quick draw".into());

        // unquoted
        let (_, got) = identifier("quick_draw").unwrap();
-        assert!(matches!(got, Identifier::Unquoted(s) if s == "quick_draw"));
+        assert_eq!(got, "quick_draw".into());
    }

    #[test]
    fn test_identifier_display() {
-        // test quoted identifier properly escapes specific characters
-        let got = format!(
-            "{}",
-            Identifier::Quoted("quick\n\t\\\"'draw \u{1f47d}".to_string())
-        );
+        // Identifier properly escapes specific characters and quotes output
+        let got = format!("{}", Identifier("quick\n\t\\\"'draw \u{1f47d}".into()));
        assert_eq!(got, r#""quick\n	\\\"'draw 👽""#);

-        // test unquoted identifier
-        let got = format!("{}", Identifier::Unquoted("quick_draw".to_string()));
+        // Identifier displays unquoted output
+        let got = format!("{}", Identifier("quick_draw".into()));
        assert_eq!(got, "quick_draw");
    }
 }
--- a/influxdb_influxql_parser/src/parameter.rs
+++ b/influxdb_influxql_parser/src/parameter.rs
@ -11,7 +11,7 @@

 use crate::internal::ParseResult;
 use crate::string::double_quoted_string;
-use crate::write_escaped;
+use crate::write_quoted_string;
 use nom::branch::alt;
 use nom::bytes::complete::tag;
 use nom::character::complete::{alphanumeric1, char};
@ -22,36 +22,30 @@ use std::fmt;
 use std::fmt::{Display, Formatter, Write};

 /// Parse an unquoted InfluxQL bind parameter.
-fn unquoted_parameter(i: &str) -> ParseResult<&str, String> {
-    map(
-        recognize(many1_count(alt((alphanumeric1, tag("_"))))),
-        str::to_string,
-    )(i)
+fn unquoted_parameter(i: &str) -> ParseResult<&str, &str> {
+    recognize(many1_count(alt((alphanumeric1, tag("_")))))(i)
 }

-/// `BindParameter` is a type that represents either a quoted ([`BindParameter::Quoted`]) or unquoted ([`BindParameter::Unquoted`])
-/// InfluxQL bind parameter.
+/// A type that represents an InfluxQL bind parameter.
 #[derive(Clone, Debug, Eq, Hash, PartialEq)]
-pub enum BindParameter {
-    /// Contains an unquoted bind parameter
-    Unquoted(String),
+pub struct BindParameter(pub String);

-    /// Contains an unescaped quoted identifier
-    Quoted(String),
+impl From<String> for BindParameter {
+    fn from(s: String) -> Self {
+        Self(s)
+    }
+}
+
+impl From<&str> for BindParameter {
+    fn from(s: &str) -> Self {
+        Self(s.to_string())
+    }
 }

 impl Display for BindParameter {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
-        match self {
-            Self::Unquoted(s) => write!(f, "${}", s)?,
-            Self::Quoted(s) => {
-                f.write_str("$\"")?;
-                // escape characters per https://github.com/influxdata/influxql/blob/df51a45762be9c1b578f01718fa92d286a843fe9/scanner.go#L576-L583
-                write_escaped!(f, s, '\n' => "\\n", '\\' => "\\\\", '"' => "\\\"");
-                f.write_char('"')?;
-            }
-        };
-
+        f.write_char('$')?;
+        write_quoted_string!(f, '"', self.0.as_str(), unquoted_parameter, '\n' => "\\n", '\\' => "\\\\", '"' => "\\\"");
        Ok(())
    }
 }
@ -62,8 +56,8 @@ pub fn parameter(i: &str) -> ParseResult<&str, BindParameter> {
    preceded(
        char('$'),
        alt((
-            map(unquoted_parameter, BindParameter::Unquoted),
-            map(double_quoted_string, BindParameter::Quoted),
+            map(unquoted_parameter, Into::into),
+            map(double_quoted_string, Into::into),
        )),
    )(i)
 }
@ -76,23 +70,23 @@ mod test {
    fn test_parameter() {
        // all ascii
        let (_, got) = parameter("$cpu").unwrap();
-        assert_eq!(got, BindParameter::Unquoted("cpu".into()));
+        assert_eq!(got, "cpu".into());

        // digits
        let (_, got) = parameter("$01").unwrap();
-        assert_eq!(got, BindParameter::Unquoted("01".into()));
+        assert_eq!(got, "01".into());

        // all valid chars
        let (_, got) = parameter("$cpu_0").unwrap();
-        assert_eq!(got, BindParameter::Unquoted("cpu_0".into()));
+        assert_eq!(got, "cpu_0".into());

        // keyword
        let (_, got) = parameter("$from").unwrap();
-        assert_eq!(got, BindParameter::Unquoted("from".into()));
+        assert_eq!(got, "from".into());

        // quoted
        let (_, got) = parameter("$\"quick draw\"").unwrap();
-        assert!(matches!(got, BindParameter::Quoted(s) if s == "quick draw"));
+        assert_eq!(got, "quick draw".into());

        // ┌─────────────────────────────┐
        // │       Fallible tests        │
@ -104,12 +98,16 @@ mod test {

    #[test]
    fn test_bind_parameter_display() {
-        // test quoted identifier properly escapes specific characters
-        let got = format!("{}", BindParameter::Quoted("from".to_string()));
-        assert_eq!(got, r#"$"from""#);
+        // BindParameter displays quoted output
+        let got = format!("{}", BindParameter("from foo".into()));
+        assert_eq!(got, r#"$"from foo""#);

-        // test unquoted identifier
-        let got = format!("{}", BindParameter::Unquoted("quick_draw".to_string()));
+        // BindParameter displays quoted and escaped output
+        let got = format!("{}", BindParameter("from\nfoo".into()));
+        assert_eq!(got, r#"$"from\nfoo""#);
+
+        // BindParameter displays unquoted output
+        let got = format!("{}", BindParameter("quick_draw".into()));
        assert_eq!(got, "$quick_draw");
    }
 }
--- a/influxdb_influxql_parser/src/show_measurements.rs
+++ b/influxdb_influxql_parser/src/show_measurements.rs
@ -209,7 +209,7 @@ mod test {
        assert_eq!(
            got,
            ShowMeasurementsStatement {
-                on_expression: Some(OnExpression::Database(Identifier::Unquoted("foo".into()))),
+                on_expression: Some(OnExpression::Database("foo".into())),
                ..Default::default()
            },
        );
@ -221,12 +221,12 @@ mod test {
        assert_eq!(
            got,
            ShowMeasurementsStatement {
-                on_expression: Some(OnExpression::Database(Identifier::Unquoted("foo".into()))),
+                on_expression: Some(OnExpression::Database("foo".into())),
                measurement_expression: Some(MeasurementExpression::Equals(
                    MeasurementNameExpression {
                        database: None,
                        retention_policy: None,
-                        name: Identifier::Unquoted("bar".into()),
+                        name: "bar".into(),
                    }
                )),
                condition: Some(Expr::Literal(true.into())),
@ -245,7 +245,7 @@ mod test {
        assert_eq!(
            got,
            ShowMeasurementsStatement {
-                on_expression: Some(OnExpression::Database(Identifier::Unquoted("foo".into()))),
+                on_expression: Some(OnExpression::Database("foo".into())),
                measurement_expression: Some(MeasurementExpression::Regex(Regex("bar".into()))),
                condition: Some(Expr::Literal(true.into())),
                limit: None,
@ -272,7 +272,7 @@ mod test {
        let got = format!(
            "{}",
            ShowMeasurementsStatement {
-                on_expression: Some(OnExpression::Database(Identifier::Unquoted("foo".into()))),
+                on_expression: Some(OnExpression::Database("foo".into())),
                ..Default::default()
            }
        );
@ -282,8 +282,8 @@ mod test {
            "{}",
            ShowMeasurementsStatement {
                on_expression: Some(OnExpression::DatabaseRetentionPolicy(
-                    Identifier::Unquoted("foo".into()),
-                    Identifier::Unquoted("bar".into())
+                    "foo".into(),
+                    "bar".into()
                )),
                ..Default::default()
            }
@ -312,11 +312,12 @@ mod test {
    #[test]
    fn test_on_expression() {
        let (_, got) = on_expression("ON cpu").unwrap();
-        assert!(matches!(got, OnExpression::Database(Identifier::Unquoted(db)) if db == "cpu"));
+        assert_eq!(got, OnExpression::Database("cpu".into()));

        let (_, got) = on_expression("ON cpu.autogen").unwrap();
-        assert!(
-            matches!(got, OnExpression::DatabaseRetentionPolicy(Identifier::Unquoted(db), Identifier::Unquoted(rp)) if db == "cpu" && rp == "autogen")
+        assert_eq!(
+            got,
+            OnExpression::DatabaseRetentionPolicy("cpu".into(), "autogen".into())
        );

        let (_, got) = on_expression("ON *").unwrap();
@ -342,7 +343,7 @@ mod test {
            MeasurementExpression::Equals(MeasurementNameExpression {
                database: None,
                retention_policy: None,
-                name: Identifier::Unquoted("foo".into())
+                name: "foo".into()
            })
        );

--- a/influxdb_influxql_parser/src/string.rs
+++ b/influxdb_influxql_parser/src/string.rs
@ -17,20 +17,42 @@ use nom::sequence::{delimited, preceded};
 use nom::Parser;
 use std::fmt::{Display, Formatter, Write};

-/// Writes `s` to `f`, mapping any characters from => to their escaped equivalents.
+/// Writes `S` to `F`, mapping any characters `FROM` => `TO` their escaped equivalents.
 #[macro_export]
 macro_rules! write_escaped {
-    ($f: expr, $s: expr $(, $from:expr => $to:expr)+) => {
-        for c in $s.chars() {
+    ($F: expr, $STRING: expr $(, $FROM:expr => $TO:expr)+) => {
+        for c in $STRING.chars() {
            match c {
                $(
-                $from => $f.write_str($to)?,
+                $FROM => $F.write_str($TO)?,
                )+
-                _ => $f.write_char(c)?,
+                _ => $F.write_char(c)?,
            }
        }
    };
 }
+/// Writes `S` to `F`, optionally surrounding in `QUOTE`s, if FN(S) fails,
+/// and mapping any characters `FROM` => `TO` their escaped equivalents.
+#[macro_export]
+macro_rules! write_quoted_string {
+    ($F: expr, $QUOTE: literal, $STRING: expr, $FN: expr $(, $FROM:expr => $TO:expr)+) => {
+        if nom::sequence::terminated($FN, nom::combinator::eof)($STRING).is_ok() {
+            $F.write_str($STRING)?;
+        } else {
+            // must be escaped
+            $F.write_char($QUOTE)?;
+            for c in $STRING.chars() {
+                match c {
+                    $(
+                    $FROM => $F.write_str($TO)?,
+                    )+
+                    _ => $F.write_char(c)?,
+                }
+            }
+            $F.write_char($QUOTE)?;
+        }
+    };
+}

 /// A string fragment contains a fragment of a string being parsed: either
 /// a non-empty Literal (a series of non-escaped characters) or a single.