Repository: diesel-rs/diesel (language: rust)

Issue:
Unexpected NotNullViolation with Sqlite and custom ToSql implementation
### Discussed in https://github.com/diesel-rs/diesel/discussions/3421

<div type='discussions-op-text'>

<sup>Originally posted by **PhotonQuantum** November 21, 2022</sup>
I have a custom rust type like this
```rust
#[derive(Debug, FromSqlRow, AsExpression)]
#[diesel(sql_type = Text)]
struct CustomWrapper(String);
```

... and its `ToSql` implementation

```rust
impl ToSql<Text, Sqlite> for CustomWrapper {
    fn to_sql<'b>(&'b self, out: &mut Output<'b, '_, Sqlite>) -> serialize::Result {
        // Change this to `out.set_value(&self.0);` works.
        out.set_value(self.0.to_string());
        Ok(serialize::IsNull::No)
    }
}
```

I expect it to work on a `TEXT NOT NULL` field. However, when inserting it into the table, I got `DatabaseError(NotNullViolation, "NOT NULL constraint failed: kvs.v")`. And if I feed set_value a borrowed str, it works as intended.

I read the doc and the implementation of [`SqliteBindValue`](http://docs.diesel.rs/2.0.x/diesel/sqlite/struct.SqliteBindValue.html) but still don't know what went wrong. Could you please help me with this?

The minimal reproduce repo is at https://github.com/PhotonQuantum/diesel-not-null-test.</div>


Discussion / hints:
This seems to be a bug in our bind logic. We handle owned and borrowed values in a different manner to avoid additional allocations whenever possible. The relevant code section is here:
https://github.com/diesel-rs/diesel/blob/abe221d7af8455d0dba3a90a2a8aad27e8833b9d/diesel/src/sqlite/connection/stmt.rs#L106-L127

The problem there is the `if` statement, which checks if the string is empty or not. For an empty string we just use a null pointer as argument to `sqlite3_bind_text`. Seems like I missed the relevant part of the sqlite documentation:

> The third argument is the value to bind to the parameter. If the third parameter to sqlite3_bind_text() or sqlite3_bind_text16() or sqlite3_bind_blob() is a NULL pointer then the fourth parameter is ignored and the end result is the same as sqlite3_bind_null().

To fix this we need to remove the special handling of empty strings there. I think it might be fine to just unconditionally execute the other branch, but I've not checked that yet. The same fix needs to be applied to the `InternalSqliteBindValue::Binary` branch a few lines above: https://github.com/diesel-rs/diesel/blob/abe221d7af8455d0dba3a90a2a8aad27e8833b9d/diesel/src/sqlite/connection/stmt.rs#L76

Also we likely want a test for this, as our CI executes most of the sqlite tests with asan enabled. This might catch additional issues here.

I'm happy to accept a PR for this, if someone want's to touch the corresponding unsafe code. I myself will likely not be able to work on a fix till next week or so.

Fix patch:
diff --git a/diesel/src/sqlite/connection/mod.rs b/diesel/src/sqlite/connection/mod.rs
index 4665ea032420..c56f289a6cbf 100644
--- a/diesel/src/sqlite/connection/mod.rs
+++ b/diesel/src/sqlite/connection/mod.rs
@@ -759,4 +759,65 @@ mod tests {
             .load::<String>(connection);
         assert_eq!(Ok(&[][..]), result.as_ref().map(|vec| vec.as_ref()));
     }
+
+    // regression test for https://github.com/diesel-rs/diesel/issues/3425
+    #[test]
+    fn test_correct_seralization_of_owned_strings() {
+        use crate::prelude::*;
+
+        #[derive(Debug, crate::expression::AsExpression)]
+        #[diesel(sql_type = diesel::sql_types::Text)]
+        struct CustomWrapper(String);
+
+        impl crate::serialize::ToSql<Text, Sqlite> for CustomWrapper {
+            fn to_sql<'b>(
+                &'b self,
+                out: &mut crate::serialize::Output<'b, '_, Sqlite>,
+            ) -> crate::serialize::Result {
+                out.set_value(self.0.to_string());
+                Ok(crate::serialize::IsNull::No)
+            }
+        }
+
+        let connection = &mut SqliteConnection::establish(":memory:").unwrap();
+
+        let res = crate::select(
+            CustomWrapper("".into())
+                .into_sql::<crate::sql_types::Text>()
+                .nullable(),
+        )
+        .get_result::<Option<String>>(connection)
+        .unwrap();
+        assert_eq!(res, Some(String::new()));
+    }
+
+    #[test]
+    fn test_correct_seralization_of_owned_bytes() {
+        use crate::prelude::*;
+
+        #[derive(Debug, crate::expression::AsExpression)]
+        #[diesel(sql_type = diesel::sql_types::Binary)]
+        struct CustomWrapper(Vec<u8>);
+
+        impl crate::serialize::ToSql<crate::sql_types::Binary, Sqlite> for CustomWrapper {
+            fn to_sql<'b>(
+                &'b self,
+                out: &mut crate::serialize::Output<'b, '_, Sqlite>,
+            ) -> crate::serialize::Result {
+                out.set_value(self.0.clone());
+                Ok(crate::serialize::IsNull::No)
+            }
+        }
+
+        let connection = &mut SqliteConnection::establish(":memory:").unwrap();
+
+        let res = crate::select(
+            CustomWrapper(Vec::new())
+                .into_sql::<crate::sql_types::Binary>()
+                .nullable(),
+        )
+        .get_result::<Option<Vec<u8>>>(connection)
+        .unwrap();
+        assert_eq!(res, Some(Vec::new()));
+    }
 }
diff --git a/diesel/src/sqlite/connection/stmt.rs b/diesel/src/sqlite/connection/stmt.rs
index 905d47d7bf4f..d39c5e26573e 100644
--- a/diesel/src/sqlite/connection/stmt.rs
+++ b/diesel/src/sqlite/connection/stmt.rs
@@ -78,14 +78,8 @@ impl Statement {
                 // We need a separate pointer here to pass it to sqlite
                 // as the returned pointer is a pointer to a dyn sized **slice**
                 // and not the pointer to the first element of the slice
-                let ptr;
-                ret_ptr = if len > 0 {
-                    ptr = bytes.as_mut_ptr();
-                    NonNull::new(Box::into_raw(bytes))
-                } else {
-                    ptr = std::ptr::null_mut();
-                    None
-                };
+                let ptr = bytes.as_mut_ptr();
+                ret_ptr = NonNull::new(Box::into_raw(bytes));
                 ffi::sqlite3_bind_blob(
                     self.inner_statement.as_ptr(),
                     bind_index,
@@ -109,14 +103,8 @@ impl Statement {
                 // We need a separate pointer here to pass it to sqlite
                 // as the returned pointer is a pointer to a dyn sized **slice**
                 // and not the pointer to the first element of the slice
-                let ptr;
-                ret_ptr = if len > 0 {
-                    ptr = bytes.as_mut_ptr();
-                    NonNull::new(Box::into_raw(bytes))
-                } else {
-                    ptr = std::ptr::null_mut();
-                    None
-                };
+                let ptr = bytes.as_mut_ptr();
+                ret_ptr = NonNull::new(Box::into_raw(bytes));
                 ffi::sqlite3_bind_text(
                     self.inner_statement.as_ptr(),
                     bind_index,


Test patch: