diff --git a/crates/nu-command/src/database/commands/into_sqlite.rs b/crates/nu-command/src/database/commands/into_sqlite.rs index fb74753155..af093927c5 100644 --- a/crates/nu-command/src/database/commands/into_sqlite.rs +++ b/crates/nu-command/src/database/commands/into_sqlite.rs @@ -1,4 +1,4 @@ -use crate::database::values::sqlite::open_sqlite_db; +use crate::database::values::sqlite::{open_sqlite_db, values_to_sql}; use nu_engine::CallExt; use nu_protocol::ast::Call; @@ -315,42 +315,6 @@ fn insert_value( } } -// This is taken from to text local_into_string but tweaks it a bit so that certain formatting does not happen -fn value_to_sql(value: Value) -> Result, ShellError> { - Ok(match value { - Value::Bool { val, .. } => Box::new(val), - Value::Int { val, .. } => Box::new(val), - Value::Float { val, .. } => Box::new(val), - Value::Filesize { val, .. } => Box::new(val), - Value::Duration { val, .. } => Box::new(val), - Value::Date { val, .. } => Box::new(val), - Value::String { val, .. } => { - // don't store ansi escape sequences in the database - // escape single quotes - Box::new(nu_utils::strip_ansi_unlikely(&val).into_owned()) - } - Value::Binary { val, .. } => Box::new(val), - val => { - return Err(ShellError::OnlySupportsThisInputType { - exp_input_type: - "bool, int, float, filesize, duration, date, string, nothing, binary".into(), - wrong_type: val.get_type().to_string(), - dst_span: Span::unknown(), - src_span: val.span(), - }) - } - }) -} - -fn values_to_sql( - values: impl IntoIterator, -) -> Result>, ShellError> { - values - .into_iter() - .map(value_to_sql) - .collect::, _>>() -} - // Each value stored in an SQLite database (or manipulated by the database engine) has one of the following storage classes: // NULL. The value is a NULL value. // INTEGER. The value is a signed integer, stored in 0, 1, 2, 3, 4, 6, or 8 bytes depending on the magnitude of the value. diff --git a/crates/nu-command/src/database/commands/query_db.rs b/crates/nu-command/src/database/commands/query_db.rs index 8a6abc1403..99e5d0e4a8 100644 --- a/crates/nu-command/src/database/commands/query_db.rs +++ b/crates/nu-command/src/database/commands/query_db.rs @@ -2,10 +2,12 @@ use nu_engine::CallExt; use nu_protocol::{ ast::Call, engine::{Command, EngineState, Stack}, - Category, Example, IntoPipelineData, PipelineData, ShellError, Signature, Spanned, SyntaxShape, - Type, + record, Category, Example, IntoPipelineData, PipelineData, ShellError, Signature, Span, + Spanned, SyntaxShape, Type, Value, }; +use crate::database::values::sqlite::nu_value_to_params; + use super::super::SQLiteDatabase; #[derive(Clone)] @@ -24,6 +26,13 @@ impl Command for QueryDb { SyntaxShape::String, "SQL to execute against the database.", ) + .named( + "params", + // TODO: Use SyntaxShape::OneOf with Records and Lists, when Lists no longer break inside OneOf + SyntaxShape::Any, + "List of parameters for the SQL statement", + Some('p'), + ) .category(Category::Database) } @@ -32,11 +41,29 @@ impl Command for QueryDb { } fn examples(&self) -> Vec { - vec![Example { - description: "Execute SQL against a SQLite database", - example: r#"open foo.db | query db "SELECT * FROM Bar""#, - result: None, - }] + vec![ + Example { + description: "Execute SQL against a SQLite database", + example: r#"open foo.db | query db "SELECT * FROM Bar""#, + result: None, + }, + Example { + description: "Execute a SQL statement with parameters", + example: r#"stor create -t my_table -c { first: str, second: int } +stor open | query db "INSERT INTO my_table VALUES (?, ?)" -p [hello 123]"#, + result: None, + }, + Example { + description: "Execute a SQL statement with named parameters", + example: r#"stor create -t my_table -c { first: str, second: int } +stor insert -t my_table -d { first: 'hello', second: '123' } +stor open | query db "SELECT * FROM my_table WHERE second = :search_second" -p { search_second: 123 }"#, + result: Some(Value::test_list(vec![Value::test_record(record! { + "first" => Value::test_string("hello"), + "second" => Value::test_int(123) + })])), + }, + ] } fn search_terms(&self) -> Vec<&str> { @@ -51,9 +78,29 @@ impl Command for QueryDb { input: PipelineData, ) -> Result { let sql: Spanned = call.req(engine_state, stack, 0)?; + let params_value: Value = call + .get_flag(engine_state, stack, "params")? + .unwrap_or_else(|| Value::nothing(Span::unknown())); + + let params = nu_value_to_params(params_value)?; let db = SQLiteDatabase::try_from_pipeline(input, call.head)?; - db.query(&sql, call.head) + db.query(&sql, params, call.head) .map(IntoPipelineData::into_pipeline_data) } } + +#[cfg(test)] +mod test { + use crate::{StorCreate, StorInsert, StorOpen}; + + use super::*; + + #[ignore = "stor db does not persist changes between pipelines"] + #[test] + fn test_examples() { + use crate::test_examples_with_commands; + + test_examples_with_commands(QueryDb {}, &[&StorOpen, &StorCreate, &StorInsert]) + } +} diff --git a/crates/nu-command/src/database/values/sqlite.rs b/crates/nu-command/src/database/values/sqlite.rs index 63bed04a5c..3ebd79de64 100644 --- a/crates/nu-command/src/database/values/sqlite.rs +++ b/crates/nu-command/src/database/values/sqlite.rs @@ -5,6 +5,7 @@ use super::definitions::{ use nu_protocol::{CustomValue, PipelineData, Record, ShellError, Span, Spanned, Value}; use rusqlite::{ types::ValueRef, Connection, DatabaseName, Error as SqliteError, OpenFlags, Row, Statement, + ToSql, }; use serde::{Deserialize, Serialize}; use std::{ @@ -99,17 +100,23 @@ impl SQLiteDatabase { Value::custom_value(db, span) } - pub fn query(&self, sql: &Spanned, call_span: Span) -> Result { + pub fn query( + &self, + sql: &Spanned, + params: NuSqlParams, + call_span: Span, + ) -> Result { let conn = open_sqlite_db(&self.path, call_span)?; - let stream = - run_sql_query(conn, sql, self.ctrlc.clone()).map_err(|e| ShellError::GenericError { + let stream = run_sql_query(conn, sql, params, self.ctrlc.clone()).map_err(|e| { + ShellError::GenericError { error: "Failed to query SQLite database".into(), msg: e.to_string(), span: Some(sql.span), help: None, inner: vec![], - })?; + } + })?; Ok(stream) } @@ -428,10 +435,100 @@ pub fn open_sqlite_db(path: &Path, call_span: Span) -> Result, + params: NuSqlParams, ctrlc: Option>, ) -> Result { let stmt = conn.prepare(&sql.item)?; - prepared_statement_to_nu_list(stmt, sql.span, ctrlc) + + prepared_statement_to_nu_list(stmt, params, sql.span, ctrlc) +} + +// This is taken from to text local_into_string but tweaks it a bit so that certain formatting does not happen +pub fn value_to_sql(value: Value) -> Result, ShellError> { + Ok(match value { + Value::Bool { val, .. } => Box::new(val), + Value::Int { val, .. } => Box::new(val), + Value::Float { val, .. } => Box::new(val), + Value::Filesize { val, .. } => Box::new(val), + Value::Duration { val, .. } => Box::new(val), + Value::Date { val, .. } => Box::new(val), + Value::String { val, .. } => { + // don't store ansi escape sequences in the database + // escape single quotes + Box::new(nu_utils::strip_ansi_unlikely(&val).into_owned()) + } + Value::Binary { val, .. } => Box::new(val), + Value::Nothing { .. } => Box::new(None::), + + val => { + return Err(ShellError::OnlySupportsThisInputType { + exp_input_type: + "bool, int, float, filesize, duration, date, string, nothing, binary".into(), + wrong_type: val.get_type().to_string(), + dst_span: Span::unknown(), + src_span: val.span(), + }) + } + }) +} + +pub fn values_to_sql( + values: impl IntoIterator, +) -> Result>, ShellError> { + values + .into_iter() + .map(value_to_sql) + .collect::, _>>() +} + +pub enum NuSqlParams { + List(Vec>), + Named(Vec<(String, Box)>), +} + +impl Default for NuSqlParams { + fn default() -> Self { + NuSqlParams::List(Vec::new()) + } +} + +pub fn nu_value_to_params(value: Value) -> Result { + match value { + Value::Record { val, .. } => { + let mut params = Vec::with_capacity(val.len()); + + for (mut column, value) in val.into_iter() { + let sql_type_erased = value_to_sql(value)?; + + if !column.starts_with([':', '@', '$']) { + column.insert(0, ':'); + } + + params.push((column, sql_type_erased)); + } + + Ok(NuSqlParams::Named(params)) + } + Value::List { vals, .. } => { + let mut params = Vec::with_capacity(vals.len()); + + for value in vals.into_iter() { + let sql_type_erased = value_to_sql(value)?; + + params.push(sql_type_erased); + } + + Ok(NuSqlParams::List(params)) + } + + // We accept no parameters + Value::Nothing { .. } => Ok(NuSqlParams::default()), + + _ => Err(ShellError::TypeMismatch { + err_message: "Invalid parameters value: expected record or list".to_string(), + span: value.span(), + }), + } } fn read_single_table( @@ -440,12 +537,14 @@ fn read_single_table( call_span: Span, ctrlc: Option>, ) -> Result { + // TODO: Should use params here? let stmt = conn.prepare(&format!("SELECT * FROM [{table_name}]"))?; - prepared_statement_to_nu_list(stmt, call_span, ctrlc) + prepared_statement_to_nu_list(stmt, NuSqlParams::default(), call_span, ctrlc) } fn prepared_statement_to_nu_list( mut stmt: Statement, + params: NuSqlParams, call_span: Span, ctrlc: Option>, ) -> Result { @@ -455,27 +554,68 @@ fn prepared_statement_to_nu_list( .map(String::from) .collect::>(); - let row_results = stmt.query_map([], |row| { - Ok(convert_sqlite_row_to_nu_value( - row, - call_span, - &column_names, - )) - })?; + // I'm very sorry for this repetition + // I tried scoping the match arms to the query_map alone, but lifetime and closure reference escapes + // got heavily in the way + let row_values = match params { + NuSqlParams::List(params) => { + let refs: Vec<&dyn ToSql> = params.iter().map(|value| (&**value)).collect(); - // we collect all rows before returning them. Not ideal but it's hard/impossible to return a stream from a CustomValue - let mut row_values = vec![]; + let row_results = stmt.query_map(refs.as_slice(), |row| { + Ok(convert_sqlite_row_to_nu_value( + row, + call_span, + &column_names, + )) + })?; - for row_result in row_results { - if nu_utils::ctrl_c::was_pressed(&ctrlc) { - // return whatever we have so far, let the caller decide whether to use it - return Ok(Value::list(row_values, call_span)); + // we collect all rows before returning them. Not ideal but it's hard/impossible to return a stream from a CustomValue + let mut row_values = vec![]; + + for row_result in row_results { + if nu_utils::ctrl_c::was_pressed(&ctrlc) { + // return whatever we have so far, let the caller decide whether to use it + return Ok(Value::list(row_values, call_span)); + } + + if let Ok(row_value) = row_result { + row_values.push(row_value); + } + } + + row_values } + NuSqlParams::Named(pairs) => { + let refs: Vec<_> = pairs + .iter() + .map(|(column, value)| (column.as_str(), &**value)) + .collect(); - if let Ok(row_value) = row_result { - row_values.push(row_value); + let row_results = stmt.query_map(refs.as_slice(), |row| { + Ok(convert_sqlite_row_to_nu_value( + row, + call_span, + &column_names, + )) + })?; + + // we collect all rows before returning them. Not ideal but it's hard/impossible to return a stream from a CustomValue + let mut row_values = vec![]; + + for row_result in row_results { + if nu_utils::ctrl_c::was_pressed(&ctrlc) { + // return whatever we have so far, let the caller decide whether to use it + return Ok(Value::list(row_values, call_span)); + } + + if let Ok(row_value) = row_result { + row_values.push(row_value); + } + } + + row_values } - } + }; Ok(Value::list(row_values, call_span)) } @@ -493,8 +633,14 @@ fn read_entire_sqlite_db( for row in rows { let table_name: String = row?; + // TODO: Should use params here? let table_stmt = conn.prepare(&format!("select * from [{table_name}]"))?; - let rows = prepared_statement_to_nu_list(table_stmt, call_span, ctrlc.clone())?; + let rows = prepared_statement_to_nu_list( + table_stmt, + NuSqlParams::default(), + call_span, + ctrlc.clone(), + )?; tables.push(table_name, rows); } diff --git a/crates/nu-command/src/example_test.rs b/crates/nu-command/src/example_test.rs index a7644fac31..476113aa2d 100644 --- a/crates/nu-command/src/example_test.rs +++ b/crates/nu-command/src/example_test.rs @@ -2,8 +2,19 @@ use nu_protocol::engine::Command; #[cfg(test)] +/// Runs the test examples in the passed in command and check their signatures and return values. +/// +/// # Panics +/// If you get a ExternalNotSupported panic, you may be using a command +/// that's not in the default working set of the test harness. +/// You may want to use test_examples_with_commands and include any other dependencies. pub fn test_examples(cmd: impl Command + 'static) { - test_examples::test_examples(cmd); + test_examples::test_examples(cmd, &[]); +} + +#[cfg(test)] +pub fn test_examples_with_commands(cmd: impl Command + 'static, commands: &[&dyn Command]) { + test_examples::test_examples(cmd, commands); } #[cfg(test)] @@ -27,10 +38,10 @@ mod test_examples { }; use std::collections::HashSet; - pub fn test_examples(cmd: impl Command + 'static) { + pub fn test_examples(cmd: impl Command + 'static, commands: &[&dyn Command]) { let examples = cmd.examples(); let signature = cmd.signature(); - let mut engine_state = make_engine_state(cmd.clone_box()); + let mut engine_state = make_engine_state(cmd.clone_box(), commands); let cwd = std::env::current_dir().expect("Could not get current working directory."); @@ -40,11 +51,12 @@ mod test_examples { if example.result.is_none() { continue; } + witnessed_type_transformations.extend( check_example_input_and_output_types_match_command_signature( &example, &cwd, - &mut make_engine_state(cmd.clone_box()), + &mut make_engine_state(cmd.clone_box(), commands), &signature.input_output_types, signature.operates_on_cell_paths(), ), @@ -58,7 +70,7 @@ mod test_examples { ); } - fn make_engine_state(cmd: Box) -> Box { + fn make_engine_state(cmd: Box, commands: &[&dyn Command]) -> Box { let mut engine_state = Box::new(EngineState::new()); let delta = { @@ -106,6 +118,12 @@ mod test_examples { working_set.add_decl(Box::new(Update)); working_set.add_decl(Box::new(Values)); working_set.add_decl(Box::new(Wrap)); + + // Add any extra commands that the test harness needs + for command in commands { + working_set.add_decl(command.clone_box()); + } + // Adding the command that is being tested to the working set working_set.add_decl(cmd); diff --git a/crates/nu-command/src/lib.rs b/crates/nu-command/src/lib.rs index c7376cdd12..a854fe3507 100644 --- a/crates/nu-command/src/lib.rs +++ b/crates/nu-command/src/lib.rs @@ -37,7 +37,7 @@ pub use debug::*; pub use default_context::*; pub use env::*; #[cfg(test)] -pub use example_test::test_examples; +pub use example_test::{test_examples, test_examples_with_commands}; pub use experimental::*; pub use filesystem::*; pub use filters::*; diff --git a/crates/nu-command/src/stor/create.rs b/crates/nu-command/src/stor/create.rs index c487ebaafa..b8ba5e79c4 100644 --- a/crates/nu-command/src/stor/create.rs +++ b/crates/nu-command/src/stor/create.rs @@ -84,10 +84,7 @@ fn process( if let Ok(conn) = db.open_connection() { match columns { Some(record) => { - let mut create_stmt = format!( - "CREATE TABLE {} ( id INTEGER NOT NULL PRIMARY KEY, ", - new_table_name - ); + let mut create_stmt = format!("CREATE TABLE {} ( ", new_table_name); for (column_name, column_datatype) in record { match column_datatype.coerce_str()?.as_ref() { "int" => { diff --git a/crates/nu-command/tests/commands/database/mod.rs b/crates/nu-command/tests/commands/database/mod.rs index 168aeb6295..f13452c20d 100644 --- a/crates/nu-command/tests/commands/database/mod.rs +++ b/crates/nu-command/tests/commands/database/mod.rs @@ -1 +1,2 @@ mod into_sqlite; +mod query_db; diff --git a/crates/nu-command/tests/commands/database/query_db.rs b/crates/nu-command/tests/commands/database/query_db.rs new file mode 100644 index 0000000000..b74c11e37e --- /dev/null +++ b/crates/nu-command/tests/commands/database/query_db.rs @@ -0,0 +1,115 @@ +use nu_test_support::{nu, nu_repl_code, playground::Playground}; + +// Multiple nu! calls don't persist state, so we can't store it in a function +const DATABASE_INIT: &str = r#"stor open | query db "CREATE TABLE IF NOT EXISTS test_db ( + name TEXT, + age INTEGER, + height REAL, + serious BOOLEAN, + created_at DATETIME, + largest_file INTEGER, + time_slept INTEGER, + null_field TEXT, + data BLOB +)""#; + +#[test] +fn data_types() { + Playground::setup("empty", |_, _| { + let results = nu!(nu_repl_code(&[ + DATABASE_INIT, + // Add row with our data types + r#"stor open + | query db "INSERT INTO test_db VALUES ( + 'nimurod', + 20, + 6.0, + true, + date('2024-03-23T00:15:24-03:00'), + 72400000, + 1000000, + NULL, + x'68656c6c6f' + )" + "#, + // Query our table with the row we just added to get its nushell types + r#" + stor open | query db "SELECT * FROM test_db" | first | values | each { describe } | str join "-" + "# + ])); + + // Assert data types match. Booleans are mapped to "numeric" due to internal SQLite representations: + // https://www.sqlite.org/datatype3.html + // They are simply 1 or 0 in practice, but the column could contain any valid SQLite value + assert_eq!( + results.out, + "string-int-float-int-string-int-int-nothing-binary" + ); + }); +} + +#[test] +fn ordered_params() { + Playground::setup("empty", |_, _| { + let results = nu!(nu_repl_code(&[ + DATABASE_INIT, + // Add row with our data types + r#"(stor open + | query db "INSERT INTO test_db VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)" + -p [ 'nimurod', 20, 6.0, true, ('2024-03-23T00:15:24-03:00' | into datetime), 72.4mb, 1ms, null, ("hello" | into binary) ] + )"#, + // Query our nu values and types + r#" + let values = (stor open | query db "SELECT * FROM test_db" | first | values); + + ($values | str join '-') + "_" + ($values | each { describe } | str join '-') + "# + ])); + + assert_eq!( + results.out, + "nimurod-20-6-1-2024-03-23 00:15:24-03:00-72400000-1000000--[104, 101, 108, 108, 111]_\ + string-int-float-int-string-int-int-nothing-binary" + ); + }); +} + +#[test] +fn named_params() { + Playground::setup("empty", |_, _| { + let results = nu!(nu_repl_code(&[ + DATABASE_INIT, + // Add row with our data types. query db should support all possible named parameters + // @-prefixed, $-prefixed, and :-prefixed + // But :prefix is the "blessed" way to do it, and as such, the only one that's + // promoted to from a bare word `key: value` property in the record + // In practice, users should not use @param or $param + r#"(stor open + | query db "INSERT INTO test_db VALUES (:name, :age, @height, $serious, :created_at, :largest_file, :time_slept, :null_field, :data)" + -p { + name: 'nimurod', + ':age': 20, + '@height': 6.0, + '$serious': true, + created_at: ('2024-03-23T00:15:24-03:00' | into datetime), + largest_file: 72.4mb, + time_slept: 1ms, + null_field: null, + data: ("hello" | into binary) + } + )"#, + // Query our nu values and types + r#" + let values = (stor open | query db "SELECT * FROM test_db" | first | values); + + ($values | str join '-') + "_" + ($values | each { describe } | str join '-') + "# + ])); + + assert_eq!( + results.out, + "nimurod-20-6-1-2024-03-23 00:15:24-03:00-72400000-1000000--[104, 101, 108, 108, 111]_\ + string-int-float-int-string-int-int-nothing-binary" + ); + }); +}