[sql] Complain about statement mutations after stepping has started.

sql/statement.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sql/statement.h"
 
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "third_party/sqlite/sqlite3.h"
 
 namespace sql {
 
 // This empty constructor initializes our reference with an empty one so that
 // we don't have to NULL-check the ref_ to see if the statement is valid: we
 // only have to check the ref's validity bit.
 Statement::Statement()
     : ref_(new Connection::StatementRef(NULL, NULL, false)),
+      stepped_(false),
       succeeded_(false) {
 }
 
 Statement::Statement(scoped_refptr<Connection::StatementRef> ref)
     : ref_(ref),
+      stepped_(false),
       succeeded_(false) {
 }
 
 Statement::~Statement() {
   // Free the resources associated with this statement. We assume there's only
   // one statement active for a given sqlite3_stmt at any time, so this won't
   // mess with anything.
   Reset(true);
 }
 
 void Statement::Assign(scoped_refptr<Connection::StatementRef> ref) {
   Reset(true);
   ref_ = ref;
 }
 
 void Statement::Clear() {
   Assign(new Connection::StatementRef(NULL, NULL, false));
   succeeded_ = false;
 }
 
 bool Statement::CheckValid() const {
   // Allow operations to fail silently if a statement was invalidated
   // because the database was closed by an error handler.
   DLOG_IF(FATAL, !ref_->was_valid())
       << "Cannot call mutating statements on an invalid statement.";
   return is_valid();
 }
 
 bool Statement::Run() {
+  DCHECK(!stepped_);
   ref_->AssertIOAllowed();
   if (!CheckValid())
     return false;
 
+  stepped_ = true;
   return CheckError(sqlite3_step(ref_->stmt())) == SQLITE_DONE;
 }
 
 bool Statement::Step() {
   ref_->AssertIOAllowed();
   if (!CheckValid())
     return false;
 
+  stepped_ = true;
   return CheckError(sqlite3_step(ref_->stmt())) == SQLITE_ROW;
 }
 
 void Statement::Reset(bool clear_bound_vars) {
   ref_->AssertIOAllowed();
   if (is_valid()) {
     // We don't call CheckError() here because sqlite3_reset() returns
     // the last error that Step() caused thereby generating a second
     // spurious error callback.
     if (clear_bound_vars)
       sqlite3_clear_bindings(ref_->stmt());
     sqlite3_reset(ref_->stmt());
   }
 
   succeeded_ = false;
+  stepped_ = false;
 }
 
 bool Statement::Succeeded() const {

[Scott Hess - ex-Googler, 2013/10/24 19:51:37]

Almost tempted to put:
  DCHECK(stepped_);
here.

[Greg Billock, 2013/10/24 21:49:50]

Hmmmm. I hear you. Maybe a follow-up try run with that set would shake loose a
bug or two...

[Scott Hess - ex-Googler, 2013/10/25 17:50:36]

Fails SQLStatementTest.Run :-).

Thinking more about it, I'm no longer convinced about this notion.  Succeeded()
is documented as working this way.  I could kind of see a case where your
control-flow is such that it has a few cases, some of which optionally fire a
Step(), and then at the end it asks whether the statement has made any progress.

[I think that the is_valid() test may not be necessary, here.  succeeded_ can
never get to true if the statement isn't valid.]

   if (!is_valid())
     return false;
 
   return succeeded_;
 }
 
 bool Statement::BindNull(int col) {
+  DCHECK(!stepped_);
   if (!is_valid())
     return false;
 
   return CheckOk(sqlite3_bind_null(ref_->stmt(), col + 1));
 }
 
 bool Statement::BindBool(int col, bool val) {
+  DCHECK(!stepped_);
   return BindInt(col, val ? 1 : 0);
 }
 
 bool Statement::BindInt(int col, int val) {
   if (!is_valid())
     return false;
 
   return CheckOk(sqlite3_bind_int(ref_->stmt(), col + 1, val));
 }
 
 bool Statement::BindInt64(int col, int64 val) {
+  DCHECK(!stepped_);
   if (!is_valid())
     return false;
 
   return CheckOk(sqlite3_bind_int64(ref_->stmt(), col + 1, val));
 }
 
 bool Statement::BindDouble(int col, double val) {
+  DCHECK(!stepped_);
   if (!is_valid())
     return false;
 
   return CheckOk(sqlite3_bind_double(ref_->stmt(), col + 1, val));
 }
 
 bool Statement::BindCString(int col, const char* val) {
+  DCHECK(!stepped_);
   if (!is_valid())
     return false;
 
   return CheckOk(
       sqlite3_bind_text(ref_->stmt(), col + 1, val, -1, SQLITE_TRANSIENT));
 }
 
 bool Statement::BindString(int col, const std::string& val) {
+  DCHECK(!stepped_);
   if (!is_valid())
     return false;
 
   return CheckOk(sqlite3_bind_text(ref_->stmt(),
                                    col + 1,
                                    val.data(),
                                    val.size(),
                                    SQLITE_TRANSIENT));
 }
 
 bool Statement::BindString16(int col, const string16& value) {
   return BindString(col, UTF16ToUTF8(value));
 }
 
 bool Statement::BindBlob(int col, const void* val, int val_len) {
+  DCHECK(!stepped_);
   if (!is_valid())
     return false;
 
   return CheckOk(
       sqlite3_bind_blob(ref_->stmt(), col + 1, val, val_len, SQLITE_TRANSIENT));
 }
 
 int Statement::ColumnCount() const {
   if (!is_valid())
     return 0;
@@ skipping 152 matching lines @@
 
 int Statement::CheckError(int err) {
   // Please don't add DCHECKs here, OnSqliteError() already has them.
   succeeded_ = (err == SQLITE_OK || err == SQLITE_ROW || err == SQLITE_DONE);
   if (!succeeded_ && ref_.get() && ref_->connection())
     return ref_->connection()->OnSqliteError(err, this, NULL);
   return err;
 }
 
 }  // namespace sql