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

@@ -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;

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,
 };

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<String>,
+    span: Span,
+    db: &SQLiteDatabase,
+    columns: Option<Record>,
+) -> 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());
+    }
 }