From cd381b74e035a6b3d403e670e6d782cdea49309f Mon Sep 17 00:00:00 2001 From: Maxime Jacob Date: Mon, 13 May 2024 21:22:39 -0400 Subject: [PATCH] Fix improperly escaped strings in stor insert (#12820) - fixes #12764 Replaced the custom logic with values_to_sql method that is already used in crate::database. This will ensure that handling of parameters is the same between sqlite and stor. --- crates/nu-command/src/database/mod.rs | 2 +- crates/nu-command/src/database/values/mod.rs | 2 +- crates/nu-command/src/stor/insert.rs | 300 ++++++++++++++----- 3 files changed, 228 insertions(+), 76 deletions(-) diff --git a/crates/nu-command/src/database/mod.rs b/crates/nu-command/src/database/mod.rs index 4fddf6638e..22b92a81d3 100644 --- a/crates/nu-command/src/database/mod.rs +++ b/crates/nu-command/src/database/mod.rs @@ -5,7 +5,7 @@ use commands::add_commands_decls; pub use values::{ convert_sqlite_row_to_nu_value, convert_sqlite_value_to_nu_value, open_connection_in_memory, - open_connection_in_memory_custom, SQLiteDatabase, MEMORY_DB, + open_connection_in_memory_custom, values_to_sql, SQLiteDatabase, MEMORY_DB, }; use nu_protocol::engine::StateWorkingSet; diff --git a/crates/nu-command/src/database/values/mod.rs b/crates/nu-command/src/database/values/mod.rs index 4442ec0783..b9bd3d5d4c 100644 --- a/crates/nu-command/src/database/values/mod.rs +++ b/crates/nu-command/src/database/values/mod.rs @@ -3,5 +3,5 @@ pub mod sqlite; pub use sqlite::{ convert_sqlite_row_to_nu_value, convert_sqlite_value_to_nu_value, open_connection_in_memory, - open_connection_in_memory_custom, SQLiteDatabase, MEMORY_DB, + open_connection_in_memory_custom, values_to_sql, SQLiteDatabase, MEMORY_DB, }; diff --git a/crates/nu-command/src/stor/insert.rs b/crates/nu-command/src/stor/insert.rs index 2aeb076d44..e0c0ad4d28 100644 --- a/crates/nu-command/src/stor/insert.rs +++ b/crates/nu-command/src/stor/insert.rs @@ -1,5 +1,6 @@ -use crate::database::{SQLiteDatabase, MEMORY_DB}; +use crate::database::{values_to_sql, SQLiteDatabase, MEMORY_DB}; use nu_engine::command_prelude::*; +use rusqlite::params_from_iter; #[derive(Clone)] pub struct StorInsert; @@ -57,86 +58,81 @@ impl Command for StorInsert { // let config = engine_state.get_config(); let db = Box::new(SQLiteDatabase::new(std::path::Path::new(MEMORY_DB), None)); - if table_name.is_none() { - return Err(ShellError::MissingParameter { - param_name: "requires at table name".into(), - span, - }); - } - let new_table_name = table_name.unwrap_or("table".into()); - if let Ok(conn) = db.open_connection() { - match columns { - Some(record) => { - let mut create_stmt = format!("INSERT INTO {} ( ", new_table_name); - let cols = record.columns(); - cols.for_each(|col| { - create_stmt.push_str(&format!("{}, ", col)); - }); - if create_stmt.ends_with(", ") { - create_stmt.pop(); - create_stmt.pop(); - } + process(table_name, span, &db, columns)?; - create_stmt.push_str(") VALUES ( "); - let vals = record.values(); - vals.for_each(|val| match val { - Value::Int { val, .. } => { - create_stmt.push_str(&format!("{}, ", val)); - } - Value::Float { val, .. } => { - create_stmt.push_str(&format!("{}, ", val)); - } - Value::String { val, .. } => { - create_stmt.push_str(&format!("'{}', ", val)); - } - Value::Date { val, .. } => { - create_stmt.push_str(&format!("'{}', ", val)); - } - Value::Bool { val, .. } => { - create_stmt.push_str(&format!("{}, ", val)); - } - _ => { - // return Err(ShellError::UnsupportedInput { - // msg: format!("{} is not a valid datepart, expected one of year, month, day, hour, minute, second, millisecond, microsecond, nanosecond", part.item), - // input: "value originates from here".to_string(), - // msg_span: span, - // input_span: val.span(), - // }); - } - }); - if create_stmt.ends_with(", ") { - create_stmt.pop(); - create_stmt.pop(); - } - - create_stmt.push(')'); - - // dbg!(&create_stmt); - - conn.execute(&create_stmt, []) - .map_err(|err| ShellError::GenericError { - error: "Failed to open SQLite connection in memory from insert".into(), - msg: err.to_string(), - span: Some(Span::test_data()), - help: None, - inner: vec![], - })?; - } - None => { - return Err(ShellError::MissingParameter { - param_name: "requires at least one column".into(), - span: call.head, - }); - } - }; - } - // dbg!(db.clone()); Ok(Value::custom(db, span).into_pipeline_data()) } } +fn process( + table_name: Option, + span: Span, + db: &SQLiteDatabase, + columns: Option, +) -> Result<(), ShellError> { + if table_name.is_none() { + return Err(ShellError::MissingParameter { + param_name: "requires at table name".into(), + span, + }); + } + let new_table_name = table_name.unwrap_or("table".into()); + if let Ok(conn) = db.open_connection() { + match columns { + Some(record) => { + let mut create_stmt = format!("INSERT INTO {} ( ", new_table_name); + let cols = record.columns(); + cols.for_each(|col| { + create_stmt.push_str(&format!("{}, ", col)); + }); + if create_stmt.ends_with(", ") { + create_stmt.pop(); + create_stmt.pop(); + } + + // Values are set as placeholders. + create_stmt.push_str(") VALUES ( "); + for (index, _) in record.columns().enumerate() { + create_stmt.push_str(&format!("?{}, ", index + 1)); + } + + if create_stmt.ends_with(", ") { + create_stmt.pop(); + create_stmt.pop(); + } + + create_stmt.push(')'); + + // dbg!(&create_stmt); + + // Get the params from the passed values + let params = values_to_sql(record.values().cloned())?; + + conn.execute(&create_stmt, params_from_iter(params)) + .map_err(|err| ShellError::GenericError { + error: "Failed to open SQLite connection in memory from insert".into(), + msg: err.to_string(), + span: Some(Span::test_data()), + help: None, + inner: vec![], + })?; + } + None => { + return Err(ShellError::MissingParameter { + param_name: "requires at least one column".into(), + span, + }); + } + }; + } + // dbg!(db.clone()); + Ok(()) +} + #[cfg(test)] mod test { + use chrono::DateTime; + use super::*; #[test] @@ -145,4 +141,160 @@ mod test { test_examples(StorInsert {}) } + + #[test] + fn test_process_with_simple_parameters() { + let db = Box::new(SQLiteDatabase::new(std::path::Path::new(MEMORY_DB), None)); + let create_stmt = "CREATE TABLE test_process_with_simple_parameters ( + int_column INTEGER, + real_column REAL, + str_column VARCHAR(255), + bool_column BOOLEAN, + date_column DATETIME DEFAULT(STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')) + )"; + + let conn = db + .open_connection() + .expect("Test was unable to open connection."); + conn.execute(create_stmt, []) + .expect("Failed to create table as part of test."); + let table_name = Some("test_process_with_simple_parameters".to_string()); + let span = Span::unknown(); + let mut columns = Record::new(); + columns.insert("int_column".to_string(), Value::test_int(42)); + columns.insert("real_column".to_string(), Value::test_float(3.1)); + columns.insert( + "str_column".to_string(), + Value::test_string("SimpleString".to_string()), + ); + columns.insert("bool_column".to_string(), Value::test_bool(true)); + columns.insert( + "date_column".to_string(), + Value::test_date( + DateTime::parse_from_str("2021-12-30 00:00:00 +0000", "%Y-%m-%d %H:%M:%S %z") + .expect("Date string should parse."), + ), + ); + + let result = process(table_name, span, &db, Some(columns)); + + assert!(result.is_ok()); + } + + #[test] + fn test_process_string_with_space() { + let db = Box::new(SQLiteDatabase::new(std::path::Path::new(MEMORY_DB), None)); + let create_stmt = "CREATE TABLE test_process_string_with_space ( + str_column VARCHAR(255) + )"; + + let conn = db + .open_connection() + .expect("Test was unable to open connection."); + conn.execute(create_stmt, []) + .expect("Failed to create table as part of test."); + let table_name = Some("test_process_string_with_space".to_string()); + let span = Span::unknown(); + let mut columns = Record::new(); + columns.insert( + "str_column".to_string(), + Value::test_string("String With Spaces".to_string()), + ); + + let result = process(table_name, span, &db, Some(columns)); + + assert!(result.is_ok()); + } + + #[test] + fn test_no_errors_when_string_too_long() { + let db = Box::new(SQLiteDatabase::new(std::path::Path::new(MEMORY_DB), None)); + let create_stmt = "CREATE TABLE test_errors_when_string_too_long ( + str_column VARCHAR(8) + )"; + + let conn = db + .open_connection() + .expect("Test was unable to open connection."); + conn.execute(create_stmt, []) + .expect("Failed to create table as part of test."); + let table_name = Some("test_errors_when_string_too_long".to_string()); + let span = Span::unknown(); + let mut columns = Record::new(); + columns.insert( + "str_column".to_string(), + Value::test_string("ThisIsALongString".to_string()), + ); + + let result = process(table_name, span, &db, Some(columns)); + // SQLite uses dynamic typing, making any length acceptable for a varchar column + assert!(result.is_ok()); + } + + #[test] + fn test_no_errors_when_param_is_wrong_type() { + let db = Box::new(SQLiteDatabase::new(std::path::Path::new(MEMORY_DB), None)); + let create_stmt = "CREATE TABLE test_errors_when_param_is_wrong_type ( + int_column INT + )"; + + let conn = db + .open_connection() + .expect("Test was unable to open connection."); + conn.execute(create_stmt, []) + .expect("Failed to create table as part of test."); + let table_name = Some("test_errors_when_param_is_wrong_type".to_string()); + let span = Span::unknown(); + let mut columns = Record::new(); + columns.insert( + "int_column".to_string(), + Value::test_string("ThisIsTheWrongType".to_string()), + ); + + let result = process(table_name, span, &db, Some(columns)); + // SQLite uses dynamic typing, making any type acceptable for a column + assert!(result.is_ok()); + } + + #[test] + fn test_errors_when_column_doesnt_exist() { + let db = Box::new(SQLiteDatabase::new(std::path::Path::new(MEMORY_DB), None)); + let create_stmt = "CREATE TABLE test_errors_when_column_doesnt_exist ( + int_column INT + )"; + + let conn = db + .open_connection() + .expect("Test was unable to open connection."); + conn.execute(create_stmt, []) + .expect("Failed to create table as part of test."); + let table_name = Some("test_errors_when_column_doesnt_exist".to_string()); + let span = Span::unknown(); + let mut columns = Record::new(); + columns.insert( + "not_a_column".to_string(), + Value::test_string("ThisIsALongString".to_string()), + ); + + let result = process(table_name, span, &db, Some(columns)); + + assert!(result.is_err()); + } + + #[test] + fn test_errors_when_table_doesnt_exist() { + let db = Box::new(SQLiteDatabase::new(std::path::Path::new(MEMORY_DB), None)); + + let table_name = Some("test_errors_when_table_doesnt_exist".to_string()); + let span = Span::unknown(); + let mut columns = Record::new(); + columns.insert( + "str_column".to_string(), + Value::test_string("ThisIsALongString".to_string()), + ); + + let result = process(table_name, span, &db, Some(columns)); + + assert!(result.is_err()); + } }