Add basic sqlite error handling to the new sqlite handlers

This part is just the plumbing of the error detection and notification.

BUG=none
TEST=none


Review URL: http://codereview.chromium.org/223003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@27268 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 120 insertions, 13 deletions

diff --git a/app/sql/connection.cc b/app/sql/connection.cc
index 53ab07d..886f781 100644
--- a/app/sql/connection.cc
+++ b/app/sql/connection.cc
@@ -313,4 +313,12 @@ void Connection::ClearCache() {
     (*i)->Close();
 }
 
+int Connection::OnSqliteError(int err, sql::Statement *stmt) {
+  if (error_delegate_.get())
+    return error_delegate_->OnError(err, this, stmt);
+  // The default handling is to assert on debug and to ignore on release.
+  NOTREACHED() << GetErrorMessage();
+  return err;
+}
+
 }  // namespace sql
diff --git a/app/sql/connection.h b/app/sql/connection.h
index cd9d213..904d614 100644
--- a/app/sql/connection.h
+++ b/app/sql/connection.h
@@ -66,6 +66,27 @@ class StatementID {
 
 #define SQL_FROM_HERE sql::StatementID(__FILE__, __LINE__)
 
+class Connection;
+
+// ErrorDelegate defines the interface to implement error handling and recovery
+// for sqlite operations. This allows the rest of the classes to return true or
+// false while the actual error code and causing statement are delivered using
+// the OnError() callback.
+// The tipical usage is to centralize the code designed to handle database
+// corruption, low-level IO errors or locking violations.
+class ErrorDelegate : public base::RefCounted<ErrorDelegate> {
+ public:
+  virtual ~ErrorDelegate() {}
+  // |error| is an sqlite result code as seen in sqlite\preprocessed\sqlite3.h
+  // |connection| is db connection where the error happened and |stmt| is
+  // our best guess at the statement that triggered the error. Do not store
+  // these pointers.
+  // If the error condition has been fixed an the original statement succesfuly
+  // re-tried then returning SQLITE_OK is appropiate; otherwise is recomended
+  // that you return the original |error| or the appropiae error code.
+  virtual int OnError(int error, Connection* connection, Statement* stmt) = 0;
+};
+
 class Connection {
  private:
   class StatementRef;  // Forward declaration, see real one below.
@@ -104,6 +125,13 @@ class Connection {
   // This must be called before Init() to have an effect.
   void set_exclusive_locking() { exclusive_locking_ = true; }
 
+  // Sets the object that will handle errors. Recomended that it should be set
+  // before calling Init(). If not set, the default is to ignore errors on
+  // release and assert on debug builds.
+  void set_error_delegate(ErrorDelegate* delegate) {
+    error_delegate_ = delegate;
+  }
+
   // Initialization ------------------------------------------------------------
 
   // Initializes the SQL connection for the given file, returning true if the
@@ -278,6 +306,10 @@ class Connection {
   // Frees all cached statements from statement_cache_.
   void ClearCache();
 
+  // Called by Statement objects when an sqlite function returns an error.
+  // The return value is the error code reflected back to client code.
+  int OnSqliteError(int err, Statement* stmt);
+
   // The actual sqlite database. Will be NULL before Init has been called or if
   // Init resulted in an error.
   sqlite3* db_;
@@ -308,6 +340,10 @@ class Connection {
   // a rollback instead of a commit.
   bool needs_rollback_;
 
+  // This object handles errors resulting from all forms of executing sqlite
+  // commands or statements. It can be null which means default handling.
+  scoped_refptr<ErrorDelegate> error_delegate_;
+
   DISALLOW_COPY_AND_ASSIGN(Connection);
 };
 
diff --git a/app/sql/statement.cc b/app/sql/statement.cc
index f625e86..3cadb6e 100644
--- a/app/sql/statement.cc
+++ b/app/sql/statement.cc
@@ -46,8 +46,13 @@ bool Statement::Step() {
 }
 
 void Statement::Reset() {
-  if (is_valid())
-    CheckError(sqlite3_reset(ref_->stmt()));
+  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.
+    sqlite3_clear_bindings(ref_->stmt());
+    sqlite3_reset(ref_->stmt());
+  }
   succeeded_ = false;
 }
 
@@ -60,7 +65,6 @@ bool Statement::Succeeded() const {
 bool Statement::BindNull(int col) {
   if (is_valid()) {
     int err = CheckError(sqlite3_bind_null(ref_->stmt(), col + 1));
-    DCHECK(err == SQLITE_OK) << ref_->connection()->GetErrorMessage();
     return err == SQLITE_OK;
   }
   return false;
@@ -69,7 +73,6 @@ bool Statement::BindNull(int col) {
 bool Statement::BindInt(int col, int val) {
   if (is_valid()) {
     int err = CheckError(sqlite3_bind_int(ref_->stmt(), col + 1, val));
-    DCHECK(err == SQLITE_OK) << ref_->connection()->GetErrorMessage();
     return err == SQLITE_OK;
   }
   return false;
@@ -78,7 +81,6 @@ bool Statement::BindInt(int col, int val) {
 bool Statement::BindInt64(int col, int64 val) {
   if (is_valid()) {
     int err = CheckError(sqlite3_bind_int64(ref_->stmt(), col + 1, val));
-    DCHECK(err == SQLITE_OK) << ref_->connection()->GetErrorMessage();
     return err == SQLITE_OK;
   }
   return false;
@@ -87,7 +89,6 @@ bool Statement::BindInt64(int col, int64 val) {
 bool Statement::BindDouble(int col, double val) {
   if (is_valid()) {
     int err = CheckError(sqlite3_bind_double(ref_->stmt(), col + 1, val));
-    DCHECK(err == SQLITE_OK) << ref_->connection()->GetErrorMessage();
     return err == SQLITE_OK;
   }
   return false;
@@ -97,7 +98,6 @@ bool Statement::BindCString(int col, const char* val) {
   if (is_valid()) {
     int err = CheckError(sqlite3_bind_text(ref_->stmt(), col + 1, val, -1,
                          SQLITE_TRANSIENT));
-    DCHECK(err == SQLITE_OK) << ref_->connection()->GetErrorMessage();
     return err == SQLITE_OK;
   }
   return false;
@@ -107,7 +107,6 @@ bool Statement::BindString(int col, const std::string& val) {
   if (is_valid()) {
     int err = CheckError(sqlite3_bind_text(ref_->stmt(), col + 1, val.data(),
                                            val.size(), SQLITE_TRANSIENT));
-    DCHECK(err == SQLITE_OK) << ref_->connection()->GetErrorMessage();
     return err == SQLITE_OK;
   }
   return false;
@@ -117,7 +116,6 @@ bool Statement::BindBlob(int col, const void* val, int val_len) {
   if (is_valid()) {
     int err = CheckError(sqlite3_bind_blob(ref_->stmt(), col + 1,
                          val, val_len, SQLITE_TRANSIENT));
-    DCHECK(err == SQLITE_OK) << ref_->connection()->GetErrorMessage();
     return err == SQLITE_OK;
   }
   return false;
@@ -208,10 +206,15 @@ void Statement::ColumnBlobAsVector(
   ColumnBlobAsVector(col, reinterpret_cast< std::vector<char>* >(val));
 }
 
+const char* Statement::GetSQLStatement() {
+  return sqlite3_sql(ref_->stmt());
+}
+
 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);
-
-  // TODO(brettw) enhance this to process the error.
+  if (!succeeded_ && is_valid())
+    return ref_->connection()->OnSqliteError(err, this);
   return err;
 }
 
diff --git a/app/sql/statement.h b/app/sql/statement.h
index a711ae5..6211d34 100644
--- a/app/sql/statement.h
+++ b/app/sql/statement.h
@@ -22,6 +22,10 @@ namespace sql {
 //   s.BindInt(0, a);
 //   if (s.Step())
 //     return s.ColumnString(0);
+//
+// Step() and Run() just return true to signal success. If you want to handle
+// specific errors such as database corruption, install an error handler in
+// in the connection object using set_error_delegate().
 class Statement {
  public:
   // Creates an uninitialized statement. The statement will be invalid until
@@ -110,6 +114,11 @@ class Statement {
   void ColumnBlobAsVector(int col, std::vector<char>* val) const;
   void ColumnBlobAsVector(int col, std::vector<unsigned char>* val) const;
 
+  // Diagnostics --------------------------------------------------------------
+
+  // Returns the original text of sql statement. Do not keep a pointer to it.
+  const char* GetSQLStatement();
+
  private:
   // This is intended to check for serious errors and report them to the
   // connection object. It takes a sqlite error code, and returns the same
diff --git a/app/sql/statement_unittest.cc b/app/sql/statement_unittest.cc
index 30a869c..5f04880 100644
--- a/app/sql/statement_unittest.cc
+++ b/app/sql/statement_unittest.cc
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <string>
+
 #include "app/sql/connection.h"
 #include "app/sql/statement.h"
 #include "base/file_path.h"
@@ -10,20 +12,51 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/sqlite/preprocessed/sqlite3.h"
 
+class StatementErrorHandler : public sql::ErrorDelegate {
+ public:
+  StatementErrorHandler() : error_(SQLITE_OK) {}
+
+  virtual int OnError(int error, sql::Connection* connection,
+                      sql::Statement* stmt) {
+    error_ = error;
+    const char* sql_txt = stmt->GetSQLStatement();
+    sql_text_ = sql_txt ? sql_txt : "no statement available";
+    return error;
+  }
+
+  int error() const { return error_; }
+
+  void reset_error() {
+    sql_text_.clear();
+    error_ = SQLITE_OK;
+  }
+
+  const char* sql_statement() const { return sql_text_.c_str(); }
+
+ private:
+  int error_;
+  std::string sql_text_;
+};
+
 class SQLStatementTest : public testing::Test {
  public:
-  SQLStatementTest() {}
+  SQLStatementTest() : error_handler_(new StatementErrorHandler) {}
 
   void SetUp() {
     ASSERT_TRUE(PathService::Get(base::DIR_TEMP, &path_));
     path_ = path_.AppendASCII("SQLStatementTest.db");
     file_util::Delete(path_, false);
     ASSERT_TRUE(db_.Init(path_));
+    // The |error_handler_| will be called if any sqlite statement operation
+    // returns an error code.
+    db_.set_error_delegate(error_handler_);
   }
 
   void TearDown() {
+    // If any error happened the original sql statement can be found in
+    // error_handler_->sql_statement().
+    EXPECT_EQ(SQLITE_OK, error_handler_->error());
     db_.Close();
-
     // If this fails something is going on with cleanup and later tests may
     // fail, so we want to identify problems right away.
     ASSERT_TRUE(file_util::Delete(path_, false));
@@ -31,9 +64,13 @@ class SQLStatementTest : public testing::Test {
 
   sql::Connection& db() { return db_; }
 
+  int sqlite_error() const { return error_handler_->error(); }
+  void reset_error() const { error_handler_->reset_error(); }
+
  private:
   FilePath path_;
   sql::Connection db_;
+  scoped_refptr<StatementErrorHandler> error_handler_;
 };
 
 TEST_F(SQLStatementTest, Assign) {
@@ -78,3 +115,17 @@ TEST_F(SQLStatementTest, Run) {
   EXPECT_FALSE(s.Step());
   EXPECT_TRUE(s.Succeeded());
 }
+
+TEST_F(SQLStatementTest, BasicErrorCallback) {
+  ASSERT_TRUE(db().Execute("CREATE TABLE foo (a INTEGER PRIMARY KEY, b)"));
+  EXPECT_EQ(SQLITE_OK, sqlite_error());
+  // Insert in the foo table the primary key. It is an error to insert
+  // something other than an number. This error causes the error callback
+  // handler to be called with SQLITE_MISMATCH as error code.
+  sql::Statement s(db().GetUniqueStatement("INSERT INTO foo (a) VALUES (?)"));
+  EXPECT_TRUE(s.is_valid());
+  s.BindCString(0, "bad bad");
+  EXPECT_FALSE(s.Run());
+  EXPECT_EQ(SQLITE_MISMATCH, sqlite_error());
+  reset_error();
+}