Better behavior from statement.run(undefined)

Let's say you prepare a statement with no parameters, and .run() it to
completion. Now the 'status' field on the Statement is SQLITE_DONE.

Now let's say that you run it, accidentally calling
  statement.run(undefined, callback)
instead of leaving out the first argument or passing an empty array.

The Baton returned by Bind<RunBaton> will have a parameters array with a
single NULL.

Then Work_Run will invoke stmt->Bind with this one-element array, while
stmt->status is still SQLITE_DONE.

Before this patch, the for loop in stmt->Bind would execute once, not
calling any bind functions, but still execute the
"stmt->status != SQLITE_OK" check. Since status is SQLITE_DONE, this
will cause the stmt->Bind to return false and for Work_Run to skip the
actual call to sqlite3_step.

However, since stmt->status is *still* SQLITE_DONE, Work_AfterRun will
report success.

It is arguably a bug to pass undefined in this case anyway, but the
current behavior where run() reports success without actually executing
the method is confusing.  (Especially when things work as expected on
the FIRST call to run(), because the initial status value is SQLITE_OK.)

src/statement.cc

@@ -295,11 +295,11 @@ bool Statement::Bind(const Parameters & parameters) {
                     status = sqlite3_bind_null(_handle, pos);
                 } break;
             }
-        }
 
-        if (status != SQLITE_OK) {
-            message = std::string(sqlite3_errmsg(db->_handle));
-            return false;
+            if (status != SQLITE_OK) {
+                message = std::string(sqlite3_errmsg(db->_handle));
+                return false;
+            }
         }
     }
 

test/prepare.test.js

@@ -99,6 +99,57 @@ describe('prepare', function() {
         after(function(done) { db.close(done); });
     });
 
+    describe('inserting with accidental undefined', function() {
+        var db;
+        before(function(done) { db = new sqlite3.Database(':memory:', done); });
+
+        var inserted = 0;
+        var retrieved = 0;
+
+        it('should create the table', function(done) {
+            db.prepare("CREATE TABLE foo (num int)").run().finalize(done);
+        });
+
+        it('should insert two rows', function(done) {
+            db.prepare('INSERT INTO foo VALUES(4)').run(function(err) {
+                if (err) throw err;
+                inserted++;
+            }).run(undefined, function (err) {
+                // The second time we pass undefined as a parameter. This is
+                // a mistake, but it should either throw an error or be ignored,
+                // not silently fail to run the statement.
+                if (err) throw err;
+                inserted++;
+            }).finalize(function(err) {
+                if (err) throw err;
+                if (inserted == 2) done();
+            });
+        });
+
+        it('should retrieve the data', function(done) {
+            var stmt = db.prepare("SELECT num FROM foo", function(err) {
+                if (err) throw err;
+            });
+
+            for (var i = 0; i < 2; i++) (function(i) {
+                stmt.get(function(err, row) {
+                    if (err) throw err;
+                    assert(row);
+                    assert.equal(row.num, 4);
+                    retrieved++;
+                });
+            })(i);
+
+            stmt.finalize(done);
+        });
+
+        it('should have retrieved two rows', function() {
+            assert.equal(2, retrieved, "Didn't retrieve all rows");
+        });
+
+        after(function(done) { db.close(done); });
+    });
+
     describe('retrieving reset() function', function() {
         var db;
         before(function(done) { db = new sqlite3.Database('test/support/prepare.db', sqlite3.OPEN_READONLY, done); });