From 3f984552593a703a899c59d955c81daf62f64218 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 6 Feb 2024 13:09:50 +0100 Subject: [PATCH] chore(spans): Upgrade sqlparser (#3057) The `sqlparser` lib has been on a fork since https://github.com/getsentry/relay/pull/2846. We can now go back to the official version because https://github.com/sqlparser-rs/sqlparser-rs/pull/1065 has been merged & released. --- CHANGELOG.md | 1 + Cargo.lock | 12 +- relay-event-normalization/Cargo.toml | 5 +- .../src/normalize/span/description/sql/mod.rs | 11 +- .../normalize/span/description/sql/parser.rs | 193 +++++++++++------- 5 files changed, 127 insertions(+), 95 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b0fb9bd064..f7e741ba54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ - Drop spans ending outside the valid timestamp range. ([#3013](https://github.com/getsentry/relay/pull/3013)) - Extract INP metrics from spans. ([#2969](https://github.com/getsentry/relay/pull/2969), [#3041](https://github.com/getsentry/relay/pull/3041)) - Add ability to rate limit metric buckets by namespace. ([#2941](https://github.com/getsentry/relay/pull/2941)) +- Upgrade sqlparser to 0.43.1.([#3057](https://github.com/getsentry/relay/pull/3057)) ## 24.1.1 diff --git a/Cargo.lock b/Cargo.lock index 019a27f83f..0766be6d56 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4841,8 +4841,9 @@ dependencies = [ [[package]] name = "sqlparser" -version = "0.37.0" -source = "git+https://github.com/getsentry/sqlparser-rs.git?rev=0bb7ec6ce661ecfc468f647fd1003b7839d37fa6#0bb7ec6ce661ecfc468f647fd1003b7839d37fa6" +version = "0.43.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f95c4bae5aba7cd30bd506f7140026ade63cff5afd778af8854026f9606bf5d4" dependencies = [ "log", "sqlparser_derive", @@ -4850,12 +4851,13 @@ dependencies = [ [[package]] name = "sqlparser_derive" -version = "0.1.1" -source = "git+https://github.com/getsentry/sqlparser-rs.git?rev=0bb7ec6ce661ecfc468f647fd1003b7839d37fa6#0bb7ec6ce661ecfc468f647fd1003b7839d37fa6" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "01b2e185515564f15375f593fb966b5718bc624ba77fe49fa4616ad619690554" dependencies = [ "proc-macro2", "quote", - "syn 1.0.109", + "syn 2.0.38", ] [[package]] diff --git a/relay-event-normalization/Cargo.toml b/relay-event-normalization/Cargo.toml index 8987ee9853..61a08b8aec 100644 --- a/relay-event-normalization/Cargo.toml +++ b/relay-event-normalization/Cargo.toml @@ -30,10 +30,7 @@ serde = { workspace = true } serde_json = { workspace = true } serde_urlencoded = "0.7.1" smallvec = { workspace = true } -# Use a fork of sqlparser until https://github.com/sqlparser-rs/sqlparser-rs/pull/1065 gets merged: -sqlparser = { git = "https://github.com/getsentry/sqlparser-rs.git", rev = "0bb7ec6ce661ecfc468f647fd1003b7839d37fa6", features = [ - "visitor", -] } +sqlparser = { version = "0.43.1", features = ["visitor"] } thiserror = { workspace = true } url = { workspace = true } uuid = { workspace = true } diff --git a/relay-event-normalization/src/normalize/span/description/sql/mod.rs b/relay-event-normalization/src/normalize/span/description/sql/mod.rs index edeed6434c..54d0684a43 100644 --- a/relay-event-normalization/src/normalize/span/description/sql/mod.rs +++ b/relay-event-normalization/src/normalize/span/description/sql/mod.rs @@ -383,7 +383,7 @@ mod tests { scrub_sql_test!( strip_prefixes_mysql_generic, r#"SELECT `table`.`foo`, count(*) from `table` WHERE sku = %s"#, - r#"SELECT foo, count(*) from table WHERE sku = %s"# + r#"SELECT foo, count(*) FROM table WHERE sku = %s"# ); scrub_sql_test_with_dialect!( @@ -581,7 +581,7 @@ mod tests { scrub_sql_test!( values_multi, "INSERT INTO a (b, c, d, e) VALuES (%s, %s, %s, %s), (%s, %s, %s, %s), (%s, %s, %s, %s) ON CONFLICT DO NOTHING", - "INSERT INTO a (..) VALUES (%s) ON CONFLICT DO NOTHING" + "INSERT INTO a (..) VALUES (%s) ON CONFLICT DO NOTHING" ); scrub_sql_test!( @@ -876,13 +876,10 @@ mod tests { ); scrub_sql_test_with_dialect!( - dont_fallback_to_regex, + replace_into, "mysql", - // sqlparser cannot parse REPLACE INTO. If we know that - // a query is MySQL, we should give up rather than try to scrub - // with regex r#"REPLACE INTO `foo` (`a`) VALUES ("abcd1234")"#, - "REPLACE INTO foo (a) VALUES (%s)" + "REPLACE INTO foo (..) VALUES (%s)" ); scrub_sql_test!( diff --git a/relay-event-normalization/src/normalize/span/description/sql/parser.rs b/relay-event-normalization/src/normalize/span/description/sql/parser.rs index 7ce7f8df13..05c7edd026 100644 --- a/relay-event-normalization/src/normalize/span/description/sql/parser.rs +++ b/relay-event-normalization/src/normalize/span/description/sql/parser.rs @@ -6,8 +6,8 @@ use itertools::Itertools; use once_cell::sync::Lazy; use regex::Regex; use sqlparser::ast::{ - AlterTableOperation, Assignment, BinaryOperator, CloseCursor, ColumnDef, Expr, Ident, - ObjectName, Query, Select, SelectItem, SetExpr, Statement, TableAlias, TableConstraint, + AlterTableOperation, Assignment, BinaryOperator, CloseCursor, ColumnDef, Expr, FunctionArg, + Ident, ObjectName, Query, Select, SelectItem, SetExpr, Statement, TableAlias, TableConstraint, TableFactor, UnaryOperator, Value, VisitMut, VisitorMut, }; use sqlparser::dialect::{Dialect, GenericDialect}; @@ -198,12 +198,41 @@ impl VisitorMut for NormalizeVisitor { Self::simplify_table_alias(alias); } TableFactor::Pivot { - table_alias, - pivot_alias, + value_column, + alias, + .. + } => { + Self::simplify_compound_identifier(value_column); + Self::simplify_table_alias(alias); + } + TableFactor::Function { + name, args, alias, .. + } => { + Self::simplify_compound_identifier(&mut name.0); + for arg in args { + if let FunctionArg::Named { name, .. } = arg { + Self::scrub_name(name); + } + } + Self::simplify_table_alias(alias); + } + TableFactor::JsonTable { columns, alias, .. } => { + for column in columns { + Self::scrub_name(&mut column.name); + } + Self::simplify_table_alias(alias); + } + TableFactor::Unpivot { + value, + name, + columns, + alias, .. } => { - Self::simplify_table_alias(table_alias); - Self::simplify_table_alias(pivot_alias); + Self::scrub_name(value); + Self::scrub_name(name); + Self::simplify_compound_identifier(columns); + Self::simplify_table_alias(alias); } } ControlFlow::Continue(()) @@ -311,8 +340,10 @@ impl VisitorMut for NormalizeVisitor { columns, source, .. } => { *columns = vec![Self::ellipsis()]; - if let SetExpr::Values(v) = &mut *source.body { - v.rows = vec![vec![Expr::Value(Self::placeholder())]] + if let Some(source) = source.as_mut() { + if let SetExpr::Values(v) = &mut *source.body { + v.rows = vec![vec![Expr::Value(Self::placeholder())]] + } } } // Simple lists of col = value assignments are collapsed to `..`. @@ -350,89 +381,93 @@ impl VisitorMut for NormalizeVisitor { Statement::Close { cursor: CloseCursor::Specific { name }, } => Self::erase_name(name), - Statement::AlterTable { name, operation } => { + Statement::AlterTable { + name, operations, .. + } => { Self::simplify_compound_identifier(&mut name.0); - match operation { - AlterTableOperation::AddConstraint(c) => match c { - TableConstraint::Unique { name, columns, .. } => { - if let Some(name) = name { - Self::scrub_name(name); + for operation in operations { + match operation { + AlterTableOperation::AddConstraint(c) => match c { + TableConstraint::Unique { name, columns, .. } => { + if let Some(name) = name { + Self::scrub_name(name); + } + for column in columns { + Self::scrub_name(column); + } } - for column in columns { - Self::scrub_name(column); + TableConstraint::ForeignKey { + name, + columns, + referred_columns, + .. + } => { + if let Some(name) = name { + Self::scrub_name(name); + } + for column in columns { + Self::scrub_name(column); + } + for column in referred_columns { + Self::scrub_name(column); + } } - } - TableConstraint::ForeignKey { - name, - columns, - referred_columns, - .. - } => { - if let Some(name) = name { - Self::scrub_name(name); + TableConstraint::Check { name, .. } => { + if let Some(name) = name { + Self::scrub_name(name); + } } - for column in columns { - Self::scrub_name(column); + TableConstraint::Index { name, columns, .. } => { + if let Some(name) = name { + Self::scrub_name(name); + } + for column in columns { + Self::scrub_name(column); + } } - for column in referred_columns { - Self::scrub_name(column); + TableConstraint::FulltextOrSpatial { + opt_index_name, + columns, + .. + } => { + if let Some(name) = opt_index_name { + Self::scrub_name(name); + } + for column in columns { + Self::scrub_name(column); + } } + }, + AlterTableOperation::AddColumn { column_def, .. } => { + let ColumnDef { name, .. } = column_def; + Self::scrub_name(name); } - TableConstraint::Check { name, .. } => { - if let Some(name) = name { - Self::scrub_name(name); - } + AlterTableOperation::DropConstraint { name, .. } => Self::scrub_name(name), + AlterTableOperation::DropColumn { column_name, .. } => { + Self::scrub_name(column_name) } - TableConstraint::Index { name, columns, .. } => { - if let Some(name) = name { - Self::scrub_name(name); - } - for column in columns { - Self::scrub_name(column); - } + AlterTableOperation::RenameColumn { + old_column_name, + new_column_name, + } => { + Self::scrub_name(old_column_name); + Self::scrub_name(new_column_name); } - TableConstraint::FulltextOrSpatial { - opt_index_name, - columns, - .. + AlterTableOperation::ChangeColumn { + old_name, new_name, .. } => { - if let Some(name) = opt_index_name { - Self::scrub_name(name); - } - for column in columns { - Self::scrub_name(column); - } + Self::scrub_name(old_name); + Self::scrub_name(new_name); } - }, - AlterTableOperation::AddColumn { column_def, .. } => { - let ColumnDef { name, .. } = column_def; - Self::scrub_name(name); - } - AlterTableOperation::DropConstraint { name, .. } => Self::scrub_name(name), - AlterTableOperation::DropColumn { column_name, .. } => { - Self::scrub_name(column_name) - } - AlterTableOperation::RenameColumn { - old_column_name, - new_column_name, - } => { - Self::scrub_name(old_column_name); - Self::scrub_name(new_column_name); - } - AlterTableOperation::ChangeColumn { - old_name, new_name, .. - } => { - Self::scrub_name(old_name); - Self::scrub_name(new_name); - } - AlterTableOperation::RenameConstraint { old_name, new_name } => { - Self::scrub_name(old_name); - Self::scrub_name(new_name); - } - AlterTableOperation::AlterColumn { column_name, .. } => { - Self::scrub_name(column_name); + AlterTableOperation::RenameConstraint { old_name, new_name } => { + Self::scrub_name(old_name); + Self::scrub_name(new_name); + } + AlterTableOperation::AlterColumn { column_name, .. } => { + Self::scrub_name(column_name); + } + _ => {} } - _ => {} } }