[sql] Framework for allowing tests to handle errors.

sql/ throws FATAL whenever it sees inappropriate calls, which makes
production code to handle errors hard to test.  ScopedErrorIgnorer
provides a way for tests to signal that specific errors are expected
and will be handled.

As a first pass, code up some additional tests for some Raze() edge
cases, and modify things to pass those tests.

BUG=159490

Review URL: https://chromiumcodereview.appspot.com/16664005

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

6 files changed, 218 insertions, 12 deletions

diff --git a/sql/connection.cc b/sql/connection.cc
index 510d6a7..67cdf08 100644
--- a/sql/connection.cc
+++ b/sql/connection.cc
@@ -68,6 +68,28 @@ class ScopedWritableSchema {
 
 namespace sql {
 
+// static
+Connection::ErrorIgnorerCallback* Connection::current_ignorer_cb_ = NULL;
+
+// static
+bool Connection::ShouldIgnore(int error) {
+  if (!current_ignorer_cb_)
+    return false;
+  return current_ignorer_cb_->Run(error);
+}
+
+// static
+void Connection::SetErrorIgnorer(Connection::ErrorIgnorerCallback* cb) {
+  CHECK(current_ignorer_cb_ == NULL);
+  current_ignorer_cb_ = cb;
+}
+
+// static
+void Connection::ResetErrorIgnorer() {
+  CHECK(current_ignorer_cb_);
+  current_ignorer_cb_ = NULL;
+}
+
 bool StatementID::operator<(const StatementID& other) const {
   if (number_ != other.number_)
     return number_ < other.number_;
@@ -435,7 +457,8 @@ bool Connection::Execute(const char* sql) {
 
   // This needs to be a FATAL log because the error case of arriving here is
   // that there's a malformed SQL statement. This can arise in development if
-  // a change alters the schema but not all queries adjust.
+  // a change alters the schema but not all queries adjust.  This can happen
+  // in production if the schema is corrupted.
   if (error == SQLITE_ERROR)
     DLOG(FATAL) << "SQL Error in " << sql << ", " << GetErrorMessage();
   return error == SQLITE_OK;
@@ -660,11 +683,10 @@ bool Connection::OpenInternal(const std::string& file_name) {
   // assumptions about who might change things in the database.
   // http://crbug.com/56559
   if (exclusive_locking_) {
-    // TODO(shess): This should probably be a full CHECK().  Code
-    // which requests exclusive locking but doesn't get it is almost
-    // certain to be ill-tested.
-    if (!Execute("PRAGMA locking_mode=EXCLUSIVE"))
-      DLOG(FATAL) << "Could not set locking mode: " << GetErrorMessage();
+    // TODO(shess): This should probably be a failure.  Code which
+    // requests exclusive locking but doesn't get it is almost certain
+    // to be ill-tested.
+    ignore_result(Execute("PRAGMA locking_mode=EXCLUSIVE"));
   }
 
   // http://www.sqlite.org/pragma.html#pragma_journal_mode
@@ -689,19 +711,16 @@ bool Connection::OpenInternal(const std::string& file_name) {
     DCHECK_LE(page_size_, kSqliteMaxPageSize);
     const std::string sql =
         base::StringPrintf("PRAGMA page_size=%d", page_size_);
-    if (!ExecuteWithTimeout(sql.c_str(), kBusyTimeout))
-      DLOG(FATAL) << "Could not set page size: " << GetErrorMessage();
+    ignore_result(ExecuteWithTimeout(sql.c_str(), kBusyTimeout));
   }
 
   if (cache_size_ != 0) {
     const std::string sql =
         base::StringPrintf("PRAGMA cache_size=%d", cache_size_);
-    if (!ExecuteWithTimeout(sql.c_str(), kBusyTimeout))
-      DLOG(FATAL) << "Could not set cache size: " << GetErrorMessage();
+    ignore_result(ExecuteWithTimeout(sql.c_str(), kBusyTimeout));
   }
 
   if (!ExecuteWithTimeout("PRAGMA secure_delete=ON", kBusyTimeout)) {
-    DLOG(FATAL) << "Could not enable secure_delete: " << GetErrorMessage();
     Close();
     return false;
   }
@@ -761,7 +780,8 @@ int Connection::OnSqliteError(int err, sql::Statement *stmt) {
   }
 
   // The default handling is to assert on debug and to ignore on release.
-  DLOG(FATAL) << GetErrorMessage();
+  if (!ShouldIgnore(err))
+    DLOG(FATAL) << GetErrorMessage();
   return err;
 }
 
diff --git a/sql/connection.h b/sql/connection.h
index 8db794d..91139ce 100644
--- a/sql/connection.h
+++ b/sql/connection.h
@@ -335,6 +335,9 @@ class SQL_EXPORT Connection {
   const char* GetErrorMessage() const;
 
  private:
+  // Allow test-support code to set/reset error ignorer.
+  friend class ScopedErrorIgnorer;
+
   // Statement accesses StatementRef which we don't want to expose to everybody
   // (they should go through Statement).
   friend class Statement;
@@ -359,6 +362,14 @@ class SQL_EXPORT Connection {
   // Internal helper for DoesTableExist and DoesIndexExist.
   bool DoesTableOrIndexExist(const char* name, const char* type) const;
 
+  // Accessors for global error-ignorer, for injecting behavior during tests.
+  // See test/scoped_error_ignorer.h.
+  typedef base::Callback<bool(int)> ErrorIgnorerCallback;
+  static ErrorIgnorerCallback* current_ignorer_cb_;
+  static bool ShouldIgnore(int error);
+  static void SetErrorIgnorer(ErrorIgnorerCallback* ignorer);
+  static void ResetErrorIgnorer();
+
   // A StatementRef is a refcounted wrapper around a sqlite statement pointer.
   // Refcounting allows us to give these statements out to sql::Statement
   // objects while also optionally maintaining a cache of compiled statements
diff --git a/sql/connection_unittest.cc b/sql/connection_unittest.cc
index 2aaeb27..b43e83c 100644
--- a/sql/connection_unittest.cc
+++ b/sql/connection_unittest.cc
@@ -8,6 +8,7 @@
 #include "sql/connection.h"
 #include "sql/meta_table.h"
 #include "sql/statement.h"
+#include "sql/test/scoped_error_ignorer.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/sqlite/sqlite3.h"
 
@@ -136,6 +137,19 @@ TEST_F(SQLConnectionTest, Rollback) {
   EXPECT_TRUE(db().BeginTransaction());
 }
 
+// Test the scoped error ignorer by attempting to insert a duplicate
+// value into an index.
+TEST_F(SQLConnectionTest, ScopedIgnoreError) {
+  const char* kCreateSql = "CREATE TABLE foo (id INTEGER UNIQUE)";
+  ASSERT_TRUE(db().Execute(kCreateSql));
+  ASSERT_TRUE(db().Execute("INSERT INTO foo (id) VALUES (12)"));
+
+  sql::ScopedErrorIgnorer ignore_errors;
+  ignore_errors.IgnoreError(SQLITE_CONSTRAINT);
+  ASSERT_FALSE(db().Execute("INSERT INTO foo (id) VALUES (12)"));
+  ASSERT_TRUE(ignore_errors.CheckIgnoredErrors());
+}
+
 // Test that sql::Connection::Raze() results in a database without the
 // tables from the original database.
 TEST_F(SQLConnectionTest, Raze) {
diff --git a/sql/sql.gyp b/sql/sql.gyp
index 86cfbc9..944defd 100644
--- a/sql/sql.gyp
+++ b/sql/sql.gyp
@@ -14,6 +14,9 @@
         '../base/base.gyp:base',
         '../third_party/sqlite/sqlite.gyp:sqlite',
       ],
+      'export_dependent_settings': [
+        '../base/base.gyp:base',
+      ],
       'defines': [ 'SQL_IMPLEMENTATION' ],
       'sources': [
         'connection.cc',
@@ -28,14 +31,48 @@
         'transaction.cc',
         'transaction.h',
       ],
+      'include_dirs': [
+        '..',
+      ],
+      'direct_dependent_settings': {
+        'include_dirs': [
+          '..',
+        ],
+      },
       # TODO(jschuh): crbug.com/167187 fix size_t to int truncations.
       'msvs_disabled_warnings': [4267, ],
     },
     {
+      'target_name': 'test_support_sql',
+      'type': 'static_library',
+      'dependencies': [
+        'sql',
+        '../base/base.gyp:base',
+        '../testing/gtest.gyp:gtest',
+      ],
+      'export_dependent_settings': [
+        'sql',
+        '../base/base.gyp:base',
+      ],
+      'sources': [
+        'test/scoped_error_ignorer.cc',
+        'test/scoped_error_ignorer.h',
+      ],
+      'include_dirs': [
+        '..',
+      ],
+      'direct_dependent_settings': {
+        'include_dirs': [
+          '..',
+        ],
+      },
+    },
+    {
       'target_name': 'sql_unittests',
       'type': '<(gtest_target_type)',
       'dependencies': [
         'sql',
+        'test_support_sql',
         '../base/base.gyp:test_support_base',
         '../testing/gtest.gyp:gtest',
       ],
diff --git a/sql/test/scoped_error_ignorer.cc b/sql/test/scoped_error_ignorer.cc
new file mode 100644
index 0000000..c585229
--- /dev/null
+++ b/sql/test/scoped_error_ignorer.cc
@@ -0,0 +1,60 @@
+// Copyright 2013 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/test/scoped_error_ignorer.h"
+
+#include "base/bind.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace sql {
+
+ScopedErrorIgnorer::ScopedErrorIgnorer()
+    : checked_(false) {
+  callback_ =
+      base::Bind(&ScopedErrorIgnorer::ShouldIgnore, base::Unretained(this));
+  Connection::SetErrorIgnorer(&callback_);
+}
+
+ScopedErrorIgnorer::~ScopedErrorIgnorer() {
+  EXPECT_TRUE(checked_) << " Test must call CheckIgnoredErrors()";
+  Connection::ResetErrorIgnorer();
+}
+
+void ScopedErrorIgnorer::IgnoreError(int err) {
+  EXPECT_EQ(0u, ignore_errors_.count(err))
+      << " Error " << err << " is already ignored";
+  ignore_errors_.insert(err);
+}
+
+bool ScopedErrorIgnorer::CheckIgnoredErrors() {
+  checked_ = true;
+  return errors_ignored_ == ignore_errors_;
+}
+
+bool ScopedErrorIgnorer::ShouldIgnore(int err) {
+  // Look for extended code.
+  if (ignore_errors_.count(err) > 0) {
+    // Record that the error was seen and ignore it.
+    errors_ignored_.insert(err);
+    return true;
+  }
+
+  // Trim extended codes and check again.
+  int base_err = err & 0xff;
+  if (ignore_errors_.count(base_err) > 0) {
+    // Record that the error was seen and ignore it.
+    errors_ignored_.insert(base_err);
+    return true;
+  }
+
+  // Unexpected error.
+  ADD_FAILURE() << " Unexpected SQLite error " << err;
+
+  // TODO(shess): If it never makes sense to pass through an error
+  // under the test harness, then perhaps the ignore callback
+  // signature should be changed.
+  return true;
+}
+
+}  // namespace sql
diff --git a/sql/test/scoped_error_ignorer.h b/sql/test/scoped_error_ignorer.h
new file mode 100644
index 0000000..e777179
--- /dev/null
+++ b/sql/test/scoped_error_ignorer.h
@@ -0,0 +1,64 @@
+// Copyright 2013 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.
+
+#ifndef SQL_TEST_SCOPED_ERROR_IGNORER_H_
+#define SQL_TEST_SCOPED_ERROR_IGNORER_H_
+
+#include <set>
+
+#include "base/basictypes.h"
+#include "sql/connection.h"
+
+namespace sql {
+
+// sql::Connection and sql::Statement treat most SQLite errors as
+// fatal in debug mode.  The intention is to catch inappropriate uses
+// of SQL before the code is shipped to production.  This makes it
+// challenging to write tests for things like recovery from
+// corruption.  This scoper can be used to ignore selected errors
+// during a test.  Errors are ignored globally (on all connections).
+//
+// Since errors can be very context-dependent, the class is pedantic -
+// specific errors must be ignored, and every error ignored must be
+// seen.
+//
+// NOTE(shess): There are still fatal error cases this does not
+// address.  If your test is handling database errors and you're
+// hitting a case not handled, contact me.
+class ScopedErrorIgnorer {
+ public:
+  ScopedErrorIgnorer();
+  ~ScopedErrorIgnorer();
+
+  // Add an error to ignore.  Extended error codes can be ignored
+  // specifically, or the base code can ignore an entire group
+  // (SQLITE_IOERR_* versus SQLITE_IOERR).
+  void IgnoreError(int err);
+
+  // Allow containing test to check if the errors were encountered.
+  // Failure to call results in ADD_FAILURE() in destructor.
+  bool CheckIgnoredErrors();
+
+  // Record an error and check if it should be ignored.
+  bool ShouldIgnore(int err);
+
+ private:
+  // Storage for callback passed to Connection::SetErrorIgnorer().
+  Connection::ErrorIgnorerCallback callback_;
+
+  // Record whether CheckIgnoredErrors() has been called.
+  bool checked_;
+
+  // Errors to ignore.
+  std::set<int> ignore_errors_;
+
+  // Errors which have been ignored.
+  std::set<int> errors_ignored_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedErrorIgnorer);
+};
+
+}  // namespace sql
+
+#endif  // SQL_TEST_SCOPED_ERROR_IGNORER_H_