Initial --params implementation (#12249)

# Description
This PR adds a `--params` param to `query db`. This closes #11643.

You can't combine both named and positional parameters, I think this
might be a limitation with rusqlite itself. I tried using named
parameters with indices like `{ ':named': 123, '1': "positional" }` but
that always failed with a rusqlite error. On the flip side, the other
way around works: for something like `VALUES (:named, ?)`, you can treat
both as positional: `-p [hello 123]`.

This PR introduces some very gnarly code repetition in
`prepared_statement_to_nu_list`. I tried, I swear; the compiler wasn't
having any of it, it kept telling me to box my closures and then it said
that the reference lifetimes were incompatible in the match arms. I gave
up and put the mapping code in the match itself, but I'm still not
happy.

Another thing I'm unhappy about: I don't like how you have to put the
`:colon` in named parameters. I think nushell should insert it if it's
[missing](https://www.sqlite.org/lang_expr.html#parameters). But this is
the way [rusqlite
works](https://docs.rs/rusqlite/latest/rusqlite/trait.Params.html#example-named),
so for now, I'll let it be consistent. Just know that it's not really a
blocker, and it isn't a compatibility change to later make `{ colon: 123
}` work, without the quotes and `:`. This would require allocating and
turning our pretty little `&str` into a `String`, though

# User-Facing Changes
Less incentive to leave yourself open to SQL injection with statements
like `query db $"INSERT INTO x VALUES \($unsafe_user_input)"`.
Additionally, the `$""` syntax being annoying with parentheses plays in
our favor, making users even more likely to use ? with `--params`.

# Tests + Formatting
Hehe

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<Box<dyn rusqlite::ToSql>, 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<Item = Value>,
-) -> Result<Vec<Box<dyn rusqlite::ToSql>>, ShellError> {
-    values
-        .into_iter()
-        .map(value_to_sql)
-        .collect::<Result<Vec<_>, _>>()
-}
-
 // 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.

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<Example> {
-        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<PipelineData, ShellError> {
         let sql: Spanned<String> = 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])
+    }
+}

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<String>, call_span: Span) -> Result<Value, ShellError> {
+    pub fn query(
+        &self,
+        sql: &Spanned<String>,
+        params: NuSqlParams,
+        call_span: Span,
+    ) -> Result<Value, ShellError> {
         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<Connection, ShellE
 fn run_sql_query(
     conn: Connection,
     sql: &Spanned<String>,
+    params: NuSqlParams,
     ctrlc: Option<Arc<AtomicBool>>,
 ) -> Result<Value, SqliteError> {
     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<Box<dyn rusqlite::ToSql>, 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::<String>),
+
+        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<Item = Value>,
+) -> Result<Vec<Box<dyn rusqlite::ToSql>>, ShellError> {
+    values
+        .into_iter()
+        .map(value_to_sql)
+        .collect::<Result<Vec<_>, _>>()
+}
+
+pub enum NuSqlParams {
+    List(Vec<Box<dyn ToSql>>),
+    Named(Vec<(String, Box<dyn ToSql>)>),
+}
+
+impl Default for NuSqlParams {
+    fn default() -> Self {
+        NuSqlParams::List(Vec::new())
+    }
+}
+
+pub fn nu_value_to_params(value: Value) -> Result<NuSqlParams, ShellError> {
+    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<Arc<AtomicBool>>,
 ) -> Result<Value, SqliteError> {
+    // 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<Arc<AtomicBool>>,
 ) -> Result<Value, SqliteError> {
@@ -455,27 +554,68 @@ fn prepared_statement_to_nu_list(
         .map(String::from)
         .collect::<Vec<String>>();
 
-    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();
+
+            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![];
 
-    // 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);
+                }
+            }
 
-    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));
+            row_values
         }
+        NuSqlParams::Named(pairs) => {
+            let refs: Vec<_> = pairs
+                .iter()
+                .map(|(column, value)| (column.as_str(), &**value))
+                .collect();
+
+            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);
+                }
+            }
 
-        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);
     }