feat: enhance DataView support for parameter binding and fix RETURNING clause metadata handling

Author: mceachen

CLAUDE.md

@@ -297,6 +297,35 @@ Aggregate functions presented unique challenges due to N-API constraints in SQLi
 
 For detailed implementation history and lessons learned, see: `doc/archive/aggregate-function-implementation-summary.md`
 
+### N-API ArrayBufferView Type Checking
+
+**CRITICAL**: N-API's `IsBuffer()` returns `true` for ALL ArrayBufferView types, not just Node.js Buffer.
+
+This is because the underlying N-API implementation uses `IsArrayBufferView()`:
+- `Buffer` → IsBuffer: ✓, IsTypedArray: ✓, IsDataView: ✗
+- `Uint8Array` → IsBuffer: ✓, IsTypedArray: ✓, IsDataView: ✗
+- `DataView` → IsBuffer: ✓, IsTypedArray: ✗, IsDataView: ✓
+
+**The Problem**: If you check `IsBuffer()` before `IsDataView()`, DataView objects get caught by the Buffer check, but `Napi::Buffer<uint8_t>::As()` returns garbage (length=0, data=null) for DataView.
+
+**The Solution**: Always check `IsDataView()` BEFORE `IsBuffer()` in type-checking chains:
+
+```cpp
+// CORRECT ORDER:
+if (param.IsDataView()) {
+  // Handle DataView specifically
+} else if (param.IsBuffer()) {
+  // Handles Buffer and TypedArray (both work with Buffer cast)
+}
+
+// WRONG ORDER (DataView gets mishandled):
+if (param.IsBuffer()) {
+  // DataView matches here but Buffer::As() fails!
+} else if (param.IsDataView()) {
+  // Never reached for DataView
+}
+```
+
 ### Async Cleanup Anti-Patterns
 
 **IMPORTANT**: The following approaches are NOT valid solutions for async cleanup issues:

src/sqlite_impl.cpp

@@ -1400,9 +1400,15 @@ Napi::Value StatementSync::Run(const Napi::CallbackInfo &info) {
       return env.Undefined();
     }
 
-    int result = sqlite3_step(statement_);
+    // Execute the statement
+    sqlite3_step(statement_);
+    // Reset immediately after step to ensure sqlite3_changes() returns
+    // correct value. This fixes an issue where RETURNING queries would
+    // report changes: 0 on the first call.
+    // See: https://github.com/nodejs/node/issues/57344
+    int result = sqlite3_reset(statement_);
 
-    if (result != SQLITE_DONE && result != SQLITE_ROW) {
+    if (result != SQLITE_OK) {
       std::string error = sqlite3_errmsg(database_->connection());
       ThrowEnhancedSqliteErrorWithDB(env, database_, database_->connection(),
                                      result, error);
@@ -1943,7 +1949,27 @@ void StatementSync::BindSingleParameter(int param_index, Napi::Value param) {
     } else if (param.IsBoolean()) {
       sqlite3_bind_int(statement_, param_index,
                        param.As<Napi::Boolean>().Value() ? 1 : 0);
+    } else if (param.IsDataView()) {
+      // IMPORTANT: Check DataView BEFORE IsBuffer() because N-API's IsBuffer()
+      // returns true for ALL ArrayBufferViews (including DataView), but
+      // Buffer::As() doesn't work correctly for DataView (returns length=0).
+      Napi::DataView dataView = param.As<Napi::DataView>();
+      Napi::ArrayBuffer arrayBuffer = dataView.ArrayBuffer();
+      size_t byteOffset = dataView.ByteOffset();
+      size_t byteLength = dataView.ByteLength();
+
+      if (arrayBuffer.Data() != nullptr && byteLength > 0) {
+        const uint8_t *data =
+            static_cast<const uint8_t *>(arrayBuffer.Data()) + byteOffset;
+        sqlite3_bind_blob(statement_, param_index, data,
+                          static_cast<int>(byteLength), SQLITE_TRANSIENT);
+      } else {
+        sqlite3_bind_null(statement_, param_index);
+      }
     } else if (param.IsBuffer()) {
+      // Handles Buffer and TypedArray (both are ArrayBufferViews that work
+      // correctly with Buffer cast - Buffer::Data() handles byte offsets
+      // internally)
       Napi::Buffer<uint8_t> buffer = param.As<Napi::Buffer<uint8_t>>();
       sqlite3_bind_blob(statement_, param_index, buffer.Data(),
                         static_cast<int>(buffer.Length()), SQLITE_TRANSIENT);
@@ -1960,99 +1986,13 @@ void StatementSync::BindSingleParameter(int param_index, Napi::Value param) {
       } else {
         sqlite3_bind_null(statement_, param_index);
       }
-    } else if (param.IsTypedArray() || param.IsDataView()) {
-      // Handle TypedArray and DataView as binary data (both are ArrayBufferView
-      // types)
-      if (param.IsTypedArray()) {
-        // Handle TypedArray using native methods
-        Napi::TypedArray typedArray = param.As<Napi::TypedArray>();
-        Napi::ArrayBuffer arrayBuffer = typedArray.ArrayBuffer();
-
-        if (!arrayBuffer.IsUndefined() && arrayBuffer.Data() != nullptr) {
-          const uint8_t *data =
-              static_cast<const uint8_t *>(arrayBuffer.Data()) +
-              typedArray.ByteOffset();
-          sqlite3_bind_blob(statement_, param_index, data,
-                            static_cast<int>(typedArray.ByteLength()),
-                            SQLITE_TRANSIENT);
-        } else {
-          sqlite3_bind_null(statement_, param_index);
-        }
-      } else {
-        // Handle DataView using Buffer.from(dataView.buffer, offset, length)
-        // conversion
-        try {
-          Napi::Env env = param.Env();
-          Napi::Object dataViewObj = param.As<Napi::Object>();
-
-          // Get DataView properties via JavaScript
-          Napi::Value bufferValue = dataViewObj.Get("buffer");
-          Napi::Value byteOffsetValue = dataViewObj.Get("byteOffset");
-          Napi::Value byteLengthValue = dataViewObj.Get("byteLength");
-
-          if (bufferValue.IsArrayBuffer() && byteOffsetValue.IsNumber() &&
-              byteLengthValue.IsNumber()) {
-            // Create Buffer from underlying ArrayBuffer with proper offset and
-            // length
-            Napi::Function bufferFrom = env.Global()
-                                            .Get("Buffer")
-                                            .As<Napi::Object>()
-                                            .Get("from")
-                                            .As<Napi::Function>();
-            Napi::Value buffer = bufferFrom.Call(
-                {bufferValue, byteOffsetValue, byteLengthValue});
-
-            if (buffer.IsBuffer()) {
-              Napi::Buffer<uint8_t> buf = buffer.As<Napi::Buffer<uint8_t>>();
-              if (buf.Length() > 0) {
-                sqlite3_bind_blob(statement_, param_index, buf.Data(),
-                                  static_cast<int>(buf.Length()),
-                                  SQLITE_TRANSIENT);
-              } else {
-                sqlite3_bind_null(statement_, param_index);
-              }
-            } else {
-              sqlite3_bind_null(statement_, param_index);
-            }
-          } else {
-            sqlite3_bind_null(statement_, param_index);
-          }
-        } catch (const Napi::Error &e) {
-          // Re-throw Napi errors (preserves error message)
-          throw;
-        } catch (const std::exception &e) {
-          // If conversion fails, bind as NULL
-          sqlite3_bind_null(statement_, param_index);
-        }
-      }
     } else if (param.IsObject()) {
-      // Handle any other object that might be a DataView that wasn't caught
-      // This is a fallback for objects that aren't explicitly handled above
-      try {
-        // Try to see if this is a DataView by checking for DataView properties
-        Napi::Object obj = param.As<Napi::Object>();
-        Napi::Value constructorName =
-            obj.Get("constructor").As<Napi::Object>().Get("name");
-
-        if (constructorName.IsString() &&
-            constructorName.As<Napi::String>().Utf8Value() == "DataView") {
-          // TEMPORARY: Just bind DataView as NULL to test basic branching
-          sqlite3_bind_null(statement_, param_index);
-        } else {
-          // Objects and arrays should throw error like Node.js does
-          throw Napi::Error::New(
-              Env(), "Provided value cannot be bound to SQLite parameter " +
-                         std::to_string(param_index) + ".");
-        }
-      } catch (const Napi::Error &e) {
-        // Re-throw Napi errors (preserves error message)
-        throw;
-      } catch (const std::exception &e) {
-        // If any property access fails, treat as non-bindable object
-        throw Napi::Error::New(
-            Env(), "Provided value cannot be bound to SQLite parameter " +
-                       std::to_string(param_index) + ".");
-      }
+      // Objects and arrays cannot be bound to SQLite parameters (same as
+      // Node.js behavior). Note: DataView, Buffer, TypedArray, and ArrayBuffer
+      // are handled above and don't reach this branch.
+      throw Napi::Error::New(
+          Env(), "Provided value cannot be bound to SQLite parameter " +
+                     std::to_string(param_index) + ".");
     } else {
       // For any other type, throw error like Node.js does
       throw Napi::Error::New(

test/parameter-binding.test.ts

@@ -225,15 +225,13 @@ describe("Parameter Binding Tests", () => {
       });
     });
 
-    test.skip("DataView binding - Not Currently Supported", () => {
-      // NOTE: DataView parameter binding is not currently supported.
-      // Use Buffer instead for binary data binding.
+    test("DataView binding", () => {
       const stmt = db.prepare(
         "INSERT INTO test_params (value_blob) VALUES (?)",
       );
 
-      // Create a buffer with various data types
-      const buffer = new ArrayBuffer(20); // Increased size to accommodate all data
+      // Test case 1: Basic DataView with various data types
+      const buffer = new ArrayBuffer(20);
       const view = new DataView(buffer);
 
       // Write different values
@@ -243,7 +241,7 @@ describe("Parameter Binding Tests", () => {
       view.setUint16(4, 65535, true);
       view.setInt32(6, -2147483648, true);
       view.setUint32(10, 4294967295, true);
-      view.setFloat32(16, Math.PI, true); // Fixed offset to not overflow
+      view.setFloat32(16, Math.PI, true);
 
       const result = stmt.run(view);
       const row = db
@@ -263,6 +261,42 @@ describe("Parameter Binding Tests", () => {
       expect(resultView.getFloat32(16, true)).toBeCloseTo(Math.PI);
     });
 
+    test("DataView with offset and length", () => {
+      const stmt = db.prepare(
+        "INSERT INTO test_params (value_blob) VALUES (?)",
+      );
+
+      // Create a DataView that only uses part of the underlying buffer
+      const bytes = new Uint8Array([0x48, 0x65, 0x6c, 0x6c, 0x6f]); // "Hello"
+      const offsetView = new DataView(bytes.buffer, 1, 3); // "ell"
+
+      const result = stmt.run(offsetView);
+      const row = db
+        .prepare("SELECT value_blob FROM test_params WHERE id = ?")
+        .get(result.lastInsertRowid);
+
+      expect(row.value_blob).toBeInstanceOf(Uint8Array);
+      expect(row.value_blob.length).toBe(3);
+      expect(Array.from(row.value_blob)).toEqual([0x65, 0x6c, 0x6c]); // "ell"
+    });
+
+    test("Empty DataView", () => {
+      const stmt = db.prepare(
+        "INSERT INTO test_params (value_blob) VALUES (?)",
+      );
+
+      const emptyBuffer = new ArrayBuffer(0);
+      const emptyView = new DataView(emptyBuffer);
+
+      const result = stmt.run(emptyView);
+      const row = db
+        .prepare("SELECT value_blob FROM test_params WHERE id = ?")
+        .get(result.lastInsertRowid);
+
+      // Empty DataView should bind as NULL
+      expect(row.value_blob).toBeNull();
+    });
+
     test("multiple parameters of different types", () => {
       const stmt = db.prepare(`
         INSERT INTO test_params (value_null, value_int, value_real, value_text, value_blob, value_bigint) 
@@ -479,8 +513,7 @@ describe("Parameter Binding Tests", () => {
         { value: Buffer.from([1, 2, 3]), expectedType: "blob" },
         { value: new Uint8Array([4, 5, 6]), expectedType: "blob" },
         { value: new Float32Array([1.1, 2.2]), expectedType: "blob" },
-        // DataView not supported yet - N-API limitation
-        // { value: new DataView(new ArrayBuffer(4)), expectedType: "blob" },
+        { value: new DataView(new ArrayBuffer(4)), expectedType: "blob" },
       ];
 
       testCases.forEach(({ value, expectedType }) => {
@@ -491,4 +524,46 @@ describe("Parameter Binding Tests", () => {
       });
     });
   });
+
+  describe("RETURNING clause metadata", () => {
+    // Regression test for: https://github.com/nodejs/node/issues/57344
+    // This test ensures that run() returns correct metadata when using RETURNING.
+    // The bug was that sqlite3_changes() was called before sqlite3_reset(),
+    // causing incorrect change counts on the first call.
+    test("returns correct metadata when using RETURNING clause", () => {
+      db.exec(
+        "CREATE TABLE returning_test (key INTEGER PRIMARY KEY, val INTEGER NOT NULL)",
+      );
+      const stmt = db.prepare(
+        "INSERT INTO returning_test (key, val) VALUES ($k, $v) RETURNING key",
+      );
+
+      // First insert - this was returning changes: 0 before the fix
+      const result1 = stmt.run({ $k: 1, $v: 10 });
+      expect(result1).toEqual({ changes: 1, lastInsertRowid: 1 });
+
+      // Subsequent inserts should also work correctly
+      const result2 = stmt.run({ $k: 2, $v: 20 });
+      expect(result2).toEqual({ changes: 1, lastInsertRowid: 2 });
+
+      const result3 = stmt.run({ $k: 3, $v: 30 });
+      expect(result3).toEqual({ changes: 1, lastInsertRowid: 3 });
+    });
+
+    test("returns correct metadata when reusing statement with RETURNING", () => {
+      db.exec(
+        "CREATE TABLE returning_reuse_test (id INTEGER PRIMARY KEY, value TEXT)",
+      );
+      const stmt = db.prepare(
+        "INSERT INTO returning_reuse_test (value) VALUES (?) RETURNING id",
+      );
+
+      // Run multiple times with different values
+      for (let i = 1; i <= 5; i++) {
+        const result = stmt.run(`value${i}`);
+        expect(result.changes).toBe(1);
+        expect(result.lastInsertRowid).toBe(i);
+      }
+    });
+  });
 });