Skip to content

fix: mysql ON UPDATE not taking an expression #586

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

fix: mysql ON UPDATE not taking an expression #586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
653cd2b
fix: mysql ON UPDATE not taking an expression
Sibz Jul 16, 2022
e6b0c64
refactor: removed allocation in check fn
Sibz Dec 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/ast/ddl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,8 @@ pub enum ColumnOption {
/// - MySQL's `AUTO_INCREMENT` or SQLite's `AUTOINCREMENT`
/// - ...
DialectSpecific(Vec<Token>),
/// MySql specific ON UPDATE <expr>
OnUpdate(Expr),
CharacterSet(ObjectName),
Comment(String),
}
Expand Down Expand Up @@ -576,6 +578,7 @@ impl fmt::Display for ColumnOption {
DialectSpecific(val) => write!(f, "{}", display_separated(val, " ")),
CharacterSet(n) => write!(f, "CHARACTER SET {}", n),
Comment(v) => write!(f, "COMMENT '{}'", escape_single_quote_string(v)),
OnUpdate(expr) => write!(f, "ON UPDATE {}", expr),
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/keywords.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,7 @@ define_keywords!(
NOSUPERUSER,
NOT,
NOTHING,
NOW,
NOWAIT,
NTH_VALUE,
NTILE,
Expand Down
36 changes: 30 additions & 6 deletions src/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,9 +497,8 @@ impl<'a> Parser<'a> {
| Keyword::CURRENT_TIME
| Keyword::CURRENT_DATE
| Keyword::LOCALTIME
| Keyword::LOCALTIMESTAMP => {
self.parse_time_functions(ObjectName(vec![w.to_ident()]))
}
| Keyword::LOCALTIMESTAMP
| Keyword::NOW => self.parse_time_functions(ObjectName(vec![w.to_ident()])),
Keyword::CASE => self.parse_case_expr(),
Keyword::CAST => self.parse_cast_expr(),
Keyword::TRY_CAST => self.parse_try_cast_expr(),
Expand Down Expand Up @@ -3254,9 +3253,9 @@ impl<'a> Parser<'a> {
} else if self.parse_keywords(&[Keyword::ON, Keyword::UPDATE])
&& dialect_of!(self is MySqlDialect)
{
Ok(Some(ColumnOption::DialectSpecific(vec![
Token::make_keyword("ON UPDATE"),
])))
let expr = self.parse_expr()?;
Self::check_on_update_expr_is_valid(&expr)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend just accepting expr here

Ok(Some(ColumnOption::OnUpdate(expr)))
} else {
Ok(None)
}
Expand Down Expand Up @@ -6318,6 +6317,31 @@ impl<'a> Parser<'a> {
pub fn index(&self) -> usize {
self.index
}

fn check_on_update_expr_is_valid(expr: &Expr) -> Result<(), ParserError> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my preference is to leave this type of "semantic" check downstream -- so I would prefer that sqlparser accepts any expression in this place, and then downstream crates can enforce limits like the expr must be one of the specified time functions.

@AugustoFKL and I tried to explain this difference in the readme recently

https://github.com/sqlparser-rs/sqlparser-rs#extensible-sql-lexer-and-parser-for-rust

This crate provides only a syntax parser, and tries to avoid applying
any SQL semantics, and accepts queries that specific databases would
reject, even when using that Database's specific Dialect. For
example, CREATE TABLE(x int, x int) is accepted by this crate, even
though most SQL engines will reject this statement due to the repeated
column name x.

This crate avoids semantic analysis because it varies drastically
between dialects and implementations. If you want to do semantic
analysis, feel free to use this project as a base

const VALID_FN_NAMES: [&str; 4] = [
keywords::CURRENT_TIMESTAMP,
keywords::LOCALTIME,
keywords::LOCALTIMESTAMP,
keywords::NOW,
];

if let Expr::Function(f) = expr {
if let Some(fn_name_ident) = f.name.0.first() {
if VALID_FN_NAMES
.iter()
.any(|x| x.eq_ignore_ascii_case(&fn_name_ident.value.as_str()))
{
return Ok(());
}
}
}

Err(ParserError::ParserError(format!(
"Expected one of '{}' after ON UPDATE in column definition",
VALID_FN_NAMES.map(|x| x.to_string()).join("', '")
)))
}
}

impl Word {
Expand Down
48 changes: 44 additions & 4 deletions tests/sqlparser_mysql.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1031,7 +1031,7 @@ fn parse_kill() {

#[test]
fn parse_table_colum_option_on_update() {
let sql1 = "CREATE TABLE foo (`modification_time` DATETIME ON UPDATE)";
let sql1 = "CREATE TABLE foo (`modification_time` DATETIME ON UPDATE CURRENT_TIMESTAMP())";
match mysql().verified_stmt(sql1) {
Statement::CreateTable { name, columns, .. } => {
assert_eq!(name.to_string(), "foo");
Expand All @@ -1042,9 +1042,13 @@ fn parse_table_colum_option_on_update() {
collation: None,
options: vec![ColumnOptionDef {
name: None,
option: ColumnOption::DialectSpecific(vec![Token::make_keyword(
"ON UPDATE"
)]),
option: ColumnOption::OnUpdate(Expr::Function(Function {
name: ObjectName(vec!["CURRENT_TIMESTAMP".into()]),
args: vec![],
over: None,
distinct: false,
special: false,
})),
},],
}],
columns
Expand All @@ -1054,6 +1058,42 @@ fn parse_table_colum_option_on_update() {
}
}

#[test]
fn parse_table_colum_option_on_update_now_lowercase() {
let sql1 = "CREATE TABLE foo (`modification_time` DATETIME ON UPDATE now())";
match mysql().verified_stmt(sql1) {
Statement::CreateTable { name, columns, .. } => {
assert_eq!(name.to_string(), "foo");
assert_eq!(
vec![ColumnDef {
name: Ident::with_quote('`', "modification_time"),
data_type: DataType::Datetime(None),
collation: None,
options: vec![ColumnOptionDef {
name: None,
option: ColumnOption::OnUpdate(Expr::Function(Function {
name: ObjectName(vec!["now".into()]),
args: vec![],
over: None,
distinct: false,
special: false,
})),
},],
}],
columns
);
}
_ => unreachable!(),
}
}

#[test]
fn parse_table_colum_option_on_update_error() {
let sql1 = "CREATE TABLE foo (`modification_time` DATETIME ON UPDATE BOO())";
assert_eq!(mysql().parse_sql_statements(sql1).unwrap_err(),
sqlparser::parser::ParserError::ParserError("Expected one of 'CURRENT_TIMESTAMP', 'LOCALTIME', 'LOCALTIMESTAMP', 'NOW' after ON UPDATE in column definition".to_string()));
}

#[test]
fn parse_set_names() {
let stmt = mysql_and_generic().verified_stmt("SET NAMES utf8mb4");
Expand Down