Repository: denoland/deno
Pull request: fix(ext/node): descriptive sqlite error messages

Fixes https://github.com/denoland/deno/issues/28259

Changed files:
filename: ext/node/ops/sqlite/mod.rs
patch: @@ -17,12 +17,12 @@ pub enum SqliteError {
   #[error(transparent)]
   SqliteError(#[from] rusqlite::Error),
   #[class(generic)]
+  #[error("{0}")]
+  SqliteSysError(String),
+  #[class(generic)]
   #[error("Database is already in use")]
   InUse,
   #[class(generic)]
-  #[error("Failed to step statement")]
-  FailedStep,
-  #[class(generic)]
   #[error("Failed to bind parameter. {0}")]
   FailedBind(&'static str),
   #[class(generic)]

filename: ext/node/ops/sqlite/statement.rs
patch: @@ -85,12 +85,12 @@ impl GarbageCollected for StatementSync {}
 
 impl StatementSync {
   // Clear the prepared statement back to its initial state.
-  fn reset(&self) {
+  fn reset(&self) -> Result<(), SqliteError> {
     // SAFETY: `self.inner` is a valid pointer to a sqlite3_stmt
     // as it lives as long as the StatementSync instance.
-    unsafe {
-      ffi::sqlite3_reset(self.inner);
-    }
+    let r = unsafe { ffi::sqlite3_reset(self.inner) };
+
+    self.check_error_code(r)
   }
 
   // Evaluate the prepared statement.
@@ -104,7 +104,7 @@ impl StatementSync {
         return Ok(true);
       }
       if r != ffi::SQLITE_ROW {
-        return Err(SqliteError::FailedStep);
+        self.check_error_code(r)?;
       }
     }
 
@@ -217,14 +217,12 @@ impl StatementSync {
     index: i32,
   ) -> Result<(), SqliteError> {
     let raw = self.inner;
-    if value.is_number() {
+    let r = if value.is_number() {
       let value = value.number_value(scope).unwrap();
 
       // SAFETY: `self.inner` is a valid pointer to a sqlite3_stmt
       // as it lives as long as the StatementSync instance.
-      unsafe {
-        ffi::sqlite3_bind_double(raw, index, value);
-      }
+      unsafe { ffi::sqlite3_bind_double(raw, index, value) }
     } else if value.is_string() {
       let value = value.to_rust_string_lossy(scope);
 
@@ -239,14 +237,12 @@ impl StatementSync {
           value.as_ptr() as *const _,
           value.len() as i32,
           ffi::SQLITE_TRANSIENT(),
-        );
+        )
       }
     } else if value.is_null() {
       // SAFETY: `self.inner` is a valid pointer to a sqlite3_stmt
       // as it lives as long as the StatementSync instance.
-      unsafe {
-        ffi::sqlite3_bind_null(raw, index);
-      }
+      unsafe { ffi::sqlite3_bind_null(raw, index) }
     } else if value.is_array_buffer_view() {
       let value: v8::Local<v8::ArrayBufferView> = value.try_into().unwrap();
       let data = value.data();
@@ -263,7 +259,7 @@ impl StatementSync {
           data,
           size as i32,
           ffi::SQLITE_TRANSIENT(),
-        );
+        )
       }
     } else if value.is_big_int() {
       let value: v8::Local<v8::BigInt> = value.try_into().unwrap();
@@ -276,11 +272,30 @@ impl StatementSync {
 
       // SAFETY: `self.inner` is a valid pointer to a sqlite3_stmt
       // as it lives as long as the StatementSync instance.
-      unsafe {
-        ffi::sqlite3_bind_int64(raw, index, as_int);
-      }
+      unsafe { ffi::sqlite3_bind_int64(raw, index, as_int) }
     } else {
       return Err(SqliteError::FailedBind("Unsupported type"));
+    };
+
+    self.check_error_code(r)
+  }
+
+  fn check_error_code(&self, r: i32) -> Result<(), SqliteError> {
+    if r != ffi::SQLITE_OK {
+      let db = self.db.borrow();
+      let db = db.as_ref().ok_or(SqliteError::InUse)?;
+
+      // SAFETY: lifetime of the connection is guaranteed by reference
+      // counting.
+      let err_str = unsafe { ffi::sqlite3_errmsg(db.handle()) };
+
+      if !err_str.is_null() {
+        // SAFETY: `err_str` is a valid pointer to a null-terminated string.
+        let err_str = unsafe { std::ffi::CStr::from_ptr(err_str) }
+          .to_string_lossy()
+          .into_owned();
+        return Err(SqliteError::SqliteSysError(err_str));
+      }
     }
 
     Ok(())
@@ -397,7 +412,7 @@ impl StatementSync {
     scope: &mut v8::HandleScope<'a>,
     #[varargs] params: Option<&v8::FunctionCallbackArguments>,
   ) -> Result<v8::Local<'a, v8::Value>, SqliteError> {
-    self.reset();
+    self.reset()?;
 
     self.bind_params(scope, params)?;
 
@@ -425,7 +440,7 @@ impl StatementSync {
     self.bind_params(scope, params)?;
     self.step()?;
 
-    self.reset();
+    self.reset()?;
 
     Ok(RunStatementResult {
       last_insert_rowid: db.last_insert_rowid(),
@@ -449,7 +464,7 @@ impl StatementSync {
       arr.push(result.into());
     }
 
-    self.reset();
+    self.reset()?;
 
     let arr = v8::Array::new_with_elements(scope, &arr);
     Ok(arr)
@@ -477,7 +492,7 @@ impl StatementSync {
       VALUE = "value",
     }
 
-    self.reset();
+    self.reset()?;
 
     self.bind_params(scope, params)?;
 
@@ -507,7 +522,7 @@ impl StatementSync {
       }
 
       let Ok(Some(row)) = statement.read_row(scope) else {
-        statement.reset();
+        let _ = statement.reset();
         statement.is_iter_finished = true;
 
         let values = &[
@@ -537,7 +552,7 @@ impl StatementSync {
       let statement = unsafe { &mut *(context.value() as *mut StatementSync) };
 
       statement.is_iter_finished = true;
-      statement.reset();
+      let _ = statement.reset();
 
       let names = &[
         DONE.v8_string(scope).unwrap().into(),

filename: tests/unit_node/sqlite_test.ts
patch: @@ -258,3 +258,17 @@ Deno.test("[node/sqlite] StatementSync for large integers", () => {
   assertEquals(result, { "2147483648": 2147483648, __proto__: null });
   db.close();
 });
+
+Deno.test("[node/sqlite] error message", () => {
+  const db = new DatabaseSync(":memory:");
+  db.exec("CREATE TABLE foo (a text, b text NOT NULL, c text)");
+
+  assertThrows(
+    () => {
+      db.prepare("INSERT INTO foo(a, b, c) VALUES (NULL, NULL, NULL)")
+        .run();
+    },
+    Error,
+    "NOT NULL constraint failed: foo.b",
+  );
+});