fix: appender crash (#296)

* test: add test for appender errors

* fix: handle errors from duckdb_appender_flush

* fix: pass double ptr to result_from_duckdb_appender

this ensures that the pointer is correctly cleared when errors occur

Author: Mause

src/appender/arrow.rs

@@ -39,7 +39,7 @@ impl Appender<'_> {
         record_batch_to_duckdb_data_chunk(&record_batch, &mut data_chunk).map_err(|_op| Error::AppendError)?;
 
         let rc = unsafe { duckdb_append_data_chunk(self.app, data_chunk.get_ptr()) };
-        result_from_duckdb_appender(rc, self.app)
+        result_from_duckdb_appender(rc, &mut self.app)
     }
 }
 

src/appender/mod.rs

@@ -67,7 +67,7 @@ impl Appender<'_> {
         params.__bind_in(self)?;
         // NOTE: we only check end_row return value
         let rc = unsafe { ffi::duckdb_appender_end_row(self.app) };
-        result_from_duckdb_appender(rc, self.app)
+        result_from_duckdb_appender(rc, &mut self.app)
     }
 
     #[inline]
@@ -142,17 +142,18 @@ impl Appender<'_> {
 
     /// Flush data into DB
     #[inline]
-    pub fn flush(&mut self) {
+    pub fn flush(&mut self) -> Result<()> {
         unsafe {
-            ffi::duckdb_appender_flush(self.app);
+            let res = ffi::duckdb_appender_flush(self.app);
+            result_from_duckdb_appender(res, &mut self.app)
         }
     }
 }
 
 impl Drop for Appender<'_> {
     fn drop(&mut self) {
         if !self.app.is_null() {
-            self.flush();
+            let _ = self.flush(); // can't safely handle failures here
             unsafe {
                 ffi::duckdb_appender_close(self.app);
                 ffi::duckdb_appender_destroy(&mut self.app);
@@ -253,4 +254,24 @@ mod test {
         assert_eq!(val, (d.as_micros() as i32,));
         Ok(())
     }
+
+    #[test]
+    fn test_appender_error() -> Result<(), crate::Error> {
+        let conn = Connection::open_in_memory()?;
+        conn.execute(
+            r"CREATE TABLE foo (
+            foobar TEXT,
+            foobar_split TEXT[] AS (split(trim(foobar), ','))
+            );",
+            [],
+        )?;
+        let mut appender = conn.appender("foo")?;
+        match appender.append_row(["foo"]) {
+            Err(crate::Error::DuckDBFailure(.., Some(msg))) => {
+                assert_eq!(msg, "Call to EndRow before all rows have been appended to!")
+            }
+            _ => panic!("expected error"),
+        }
+        Ok(())
+    }
 }

src/error.rs

@@ -225,17 +225,17 @@ fn error_from_duckdb_code(code: ffi::duckdb_state, message: Option<String>) -> R
 
 #[cold]
 #[inline]
-pub fn result_from_duckdb_appender(code: ffi::duckdb_state, mut appender: ffi::duckdb_appender) -> Result<()> {
+pub fn result_from_duckdb_appender(code: ffi::duckdb_state, appender: *mut ffi::duckdb_appender) -> Result<()> {
     if code == ffi::DuckDBSuccess {
         return Ok(());
     }
     unsafe {
-        let message = if appender.is_null() {
+        let message = if (*appender).is_null() {
             Some("appender is null".to_string())
         } else {
-            let c_err = ffi::duckdb_appender_error(appender);
+            let c_err = ffi::duckdb_appender_error(*appender);
             let message = Some(CStr::from_ptr(c_err).to_string_lossy().to_string());
-            ffi::duckdb_appender_destroy(&mut appender);
+            ffi::duckdb_appender_destroy(appender);
             message
         };
         error_from_duckdb_code(code, message)

src/inner_connection.rs

@@ -102,7 +102,7 @@ impl InnerConnection {
                 &mut c_app,
             )
         };
-        result_from_duckdb_appender(r, c_app)?;
+        result_from_duckdb_appender(r, &mut c_app)?;
         Ok(Appender::new(conn, c_app))
     }