Merge branch 'better-sqlite-errors'

Improved reporting of errors triggered by sqlite3.  In particular, all
error messages are now tagged with their corresponding database filename
and, if they are API-level errors, the name of the sqlite3 function that
caused them.

Fixes #141.

NEWS

@@ -52,6 +52,11 @@ STILL UNDER DEVELOPMENT; NOT RELEASED YET.
   GDB 6.1.1 (circa 2004) does not have the -ex flag so we need to
   generate a temporary GDB script and feed it to GDB with -x instead.
 
+* Issue 141: Improved reporting of errors triggered by sqlite3.  In
+  particular, all error messages are now tagged with their corresponding
+  database filename and, if they are API-level errors, the name of the
+  sqlite3 function that caused them.
+
 * Issue 144: Improved documentation on the support for custom properties
   in the test metadata.
 

integration/cmd_test_test.sh

@@ -656,6 +656,22 @@ EOF
 }
 
 
+utils_test_case results_file__reuse
+results_file__reuse_body() {
+    utils_install_timestamp_wrapper
+
+    cat >Kyuafile <<EOF
+syntax(2)
+atf_test_program{name="simple_all_pass", test_suite="integration"}
+EOF
+    utils_cp_helper simple_all_pass .
+    atf_check -s exit:0 -o ignore -e empty kyua test -r results.db
+
+    atf_check -s exit:2 -o empty -e match:"results.db already exists" \
+        kyua test --results-file="results.db"
+}
+
+
 utils_test_case build_root_flag
 build_root_flag_body() {
     utils_install_timestamp_wrapper
@@ -1018,6 +1034,7 @@ atf_init_test_cases() {
     atf_add_test_case store_contents
     atf_add_test_case results_file__ok
     atf_add_test_case results_file__fail
+    atf_add_test_case results_file__reuse
 
     atf_add_test_case build_root_flag
 

store/write_backend.cpp

@@ -138,30 +138,8 @@ struct store::write_backend::impl : utils::noncopyable {
     /// Constructor.
     ///
     /// \param database_ The SQLite database instance.
-    /// \param metadata_ The metadata for the loaded database.  This must match
-    ///     the schema version we implement in this module; otherwise, a
-    ///     migration is necessary.
-    ///
-    /// \throw integrity_error If the schema in the database is too modern,
-    ///     which might indicate some form of corruption or an old binary.
-    /// \throw old_schema_error If the schema in the database is older than our
-    ///     currently-implemented version and needs an upgrade.  The caller can
-    ///     use migrate_schema() to fix this problem.
-    impl(sqlite::database& database_, const metadata& metadata_) :
-        database(database_)
+    impl(sqlite::database& database_) : database(database_)
     {
-        const int database_version = metadata_.schema_version();
-
-        if (database_version == detail::current_schema_version) {
-            // OK.
-        } else if (database_version < detail::current_schema_version) {
-            throw old_schema_error(database_version);
-        } else if (database_version > detail::current_schema_version) {
-            throw integrity_error(
-                F("Database at schema version %s, which is newer than the "
-                  "supported version %s")
-                % database_version % detail::current_schema_version);
-        }
     }
 };
 
@@ -194,10 +172,11 @@ store::write_backend::open_rw(const fs::path& file)
 {
     sqlite::database db = detail::open_and_setup(
         file, sqlite::open_readwrite | sqlite::open_create);
-    if (empty_database(db))
-        return write_backend(new impl(db, detail::initialize(db)));
-    else
-        return write_backend(new impl(db, metadata::fetch_latest(db)));
+    if (!empty_database(db))
+        throw error(F("%s already exists and is not empty; cannot open "
+                      "for write") % file);
+    detail::initialize(db);
+    return write_backend(new impl(db));
 }
 
 

store/write_backend_test.cpp

@@ -121,55 +121,53 @@ ATF_TEST_CASE_BODY(detail__schema_file__overriden)
 }
 
 
-ATF_TEST_CASE(write_backend__open_rw__ok);
-ATF_TEST_CASE_HEAD(write_backend__open_rw__ok)
+ATF_TEST_CASE(write_backend__open_rw__ok_if_empty);
+ATF_TEST_CASE_HEAD(write_backend__open_rw__ok_if_empty)
 {
     logging::set_inmemory();
     set_md_var("require.files", store::detail::schema_file().c_str());
 }
-ATF_TEST_CASE_BODY(write_backend__open_rw__ok)
+ATF_TEST_CASE_BODY(write_backend__open_rw__ok_if_empty)
 {
     {
         sqlite::database db = sqlite::database::open(
             fs::path("test.db"), sqlite::open_readwrite | sqlite::open_create);
-        store::detail::initialize(db);
     }
     store::write_backend backend = store::write_backend::open_rw(
         fs::path("test.db"));
     backend.database().exec("SELECT * FROM metadata");
 }
 
 
-ATF_TEST_CASE(write_backend__open_rw__create_missing);
-ATF_TEST_CASE_HEAD(write_backend__open_rw__create_missing)
+ATF_TEST_CASE(write_backend__open_rw__error_if_not_empty);
+ATF_TEST_CASE_HEAD(write_backend__open_rw__error_if_not_empty)
 {
     logging::set_inmemory();
     set_md_var("require.files", store::detail::schema_file().c_str());
 }
-ATF_TEST_CASE_BODY(write_backend__open_rw__create_missing)
+ATF_TEST_CASE_BODY(write_backend__open_rw__error_if_not_empty)
 {
-    store::write_backend backend = store::write_backend::open_rw(
-        fs::path("test.db"));
-    backend.database().exec("SELECT * FROM metadata");
+    {
+        sqlite::database db = sqlite::database::open(
+            fs::path("test.db"), sqlite::open_readwrite | sqlite::open_create);
+        store::detail::initialize(db);
+    }
+    ATF_REQUIRE_THROW_RE(store::error, "test.db already exists",
+                         store::write_backend::open_rw(fs::path("test.db")));
 }
 
 
-ATF_TEST_CASE(write_backend__open_rw__integrity_error);
-ATF_TEST_CASE_HEAD(write_backend__open_rw__integrity_error)
+ATF_TEST_CASE(write_backend__open_rw__create_missing);
+ATF_TEST_CASE_HEAD(write_backend__open_rw__create_missing)
 {
     logging::set_inmemory();
     set_md_var("require.files", store::detail::schema_file().c_str());
 }
-ATF_TEST_CASE_BODY(write_backend__open_rw__integrity_error)
+ATF_TEST_CASE_BODY(write_backend__open_rw__create_missing)
 {
-    {
-        sqlite::database db = sqlite::database::open(
-            fs::path("test.db"), sqlite::open_readwrite | sqlite::open_create);
-        store::detail::initialize(db);
-        db.exec("DELETE FROM metadata");
-    }
-    ATF_REQUIRE_THROW_RE(store::integrity_error, "metadata.*empty",
-                         store::write_backend::open_rw(fs::path("test.db")));
+    store::write_backend backend = store::write_backend::open_rw(
+        fs::path("test.db"));
+    backend.database().exec("SELECT * FROM metadata");
 }
 
 
@@ -199,8 +197,8 @@ ATF_INIT_TEST_CASES(tcs)
     ATF_ADD_TEST_CASE(tcs, detail__schema_file__builtin);
     ATF_ADD_TEST_CASE(tcs, detail__schema_file__overriden);
 
-    ATF_ADD_TEST_CASE(tcs, write_backend__open_rw__ok);
+    ATF_ADD_TEST_CASE(tcs, write_backend__open_rw__ok_if_empty);
+    ATF_ADD_TEST_CASE(tcs, write_backend__open_rw__error_if_not_empty);
     ATF_ADD_TEST_CASE(tcs, write_backend__open_rw__create_missing);
-    ATF_ADD_TEST_CASE(tcs, write_backend__open_rw__integrity_error);
     ATF_ADD_TEST_CASE(tcs, write_backend__close);
 }

utils/sqlite/c_gate.cpp

@@ -28,10 +28,14 @@
 
 #include "utils/sqlite/c_gate.hpp"
 
+#include "utils/fs/path.hpp"
+#include "utils/optional.ipp"
 #include "utils/sqlite/database.hpp"
 
 namespace sqlite = utils::sqlite;
 
+using utils::none;
+
 
 /// Creates a new gateway to an existing C++ SQLite database.
 ///
@@ -65,7 +69,7 @@ sqlite::database_c_gate::~database_c_gate(void)
 sqlite::database
 sqlite::database_c_gate::connect(::sqlite3* raw_database)
 {
-    return database(static_cast< void* >(raw_database), false);
+    return database(none, static_cast< void* >(raw_database), false);
 }
 
 

utils/sqlite/c_gate_test.cpp

@@ -31,6 +31,7 @@
 #include <atf-c++.hpp>
 
 #include "utils/fs/path.hpp"
+#include "utils/optional.ipp"
 #include "utils/sqlite/database.hpp"
 #include "utils/sqlite/test_utils.hpp"
 
@@ -68,8 +69,28 @@ ATF_TEST_CASE_BODY(c_database)
 }
 
 
+ATF_TEST_CASE(database__db_filename);
+ATF_TEST_CASE_HEAD(database__db_filename)
+{
+    set_md_var("descr", "The current implementation of db_filename() has no "
+               "means to access the filename of a database connected to a raw "
+               "sqlite3 object");
+}
+ATF_TEST_CASE_BODY(database__db_filename)
+{
+    ::sqlite3* raw_db;
+    ATF_REQUIRE_EQ(SQLITE_OK, ::sqlite3_open_v2(
+        "test.db", &raw_db, SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE, NULL));
+
+    sqlite::database database = sqlite::database_c_gate::connect(raw_db);
+    ATF_REQUIRE(!database.db_filename());
+    ::sqlite3_close(raw_db);
+}
+
+
 ATF_INIT_TEST_CASES(tcs)
 {
     ATF_ADD_TEST_CASE(tcs, c_database);
     ATF_ADD_TEST_CASE(tcs, connect);
+    ATF_ADD_TEST_CASE(tcs, database__db_filename);
 }

utils/sqlite/database.cpp

@@ -32,22 +32,31 @@ extern "C" {
 #include <sqlite3.h>
 }
 
+#include <cstring>
 #include <stdexcept>
 
 #include "utils/format/macros.hpp"
 #include "utils/fs/path.hpp"
 #include "utils/logging/macros.hpp"
 #include "utils/noncopyable.hpp"
+#include "utils/optional.ipp"
 #include "utils/sanity.hpp"
 #include "utils/sqlite/exceptions.hpp"
 #include "utils/sqlite/statement.ipp"
 #include "utils/sqlite/transaction.hpp"
 
+namespace fs = utils::fs;
 namespace sqlite = utils::sqlite;
 
+using utils::none;
+using utils::optional;
+
 
 /// Internal implementation for sqlite::database.
 struct utils::sqlite::database::impl : utils::noncopyable {
+    /// Path to the database as seen at construction time.
+    optional< fs::path > db_filename;
+
     /// The SQLite 3 internal database.
     ::sqlite3* db;
 
@@ -56,12 +65,16 @@ struct utils::sqlite::database::impl : utils::noncopyable {
 
     /// Constructor.
     ///
+    /// \param db_filename_ The path to the database as seen at construction
+    ///     time, if any, or none for in-memory databases.  We should use
+    ///     sqlite3_db_filename instead, but this function appeared in 3.7.10
+    ///     and Ubuntu 12.04 LTS (which we support for Travis CI builds as of
+    ///     2015-07-07) ships with 3.7.9.
     /// \param db_ The SQLite internal database.
     /// \param owned_ Whether this object owns the db_ object or not.  If it
     ///     does, the internal db_ will be released during destruction.
-    impl(::sqlite3* db_, const bool owned_) :
-        db(db_),
-        owned(owned_)
+    impl(optional< fs::path > db_filename_, ::sqlite3* db_, const bool owned_) :
+        db_filename(db_filename_), db(db_), owned(owned_)
     {
     }
 
@@ -98,7 +111,8 @@ struct utils::sqlite::database::impl : utils::noncopyable {
             if (db == NULL)
                 throw std::bad_alloc();
             else {
-                sqlite::database error_db(db, true);
+                sqlite::database error_db(utils::make_optional(fs::path(file)),
+                                          db, true);
                 throw sqlite::api_error::from_database(error_db,
                                                        "sqlite3_open_v2");
             }
@@ -128,10 +142,14 @@ struct utils::sqlite::database::impl : utils::noncopyable {
 /// SQLite session.  As soon as the object is destroyed, the session is
 /// terminated.
 ///
+/// \param db_filename_ The path to the database as seen at construction
+///     time, if any, or none for in-memory databases.
 /// \param db_ Raw pointer to the C SQLite 3 object.
 /// \param owned_ Whether this instance will own the pointer or not.
-sqlite::database::database(void* db_, const bool owned_) :
-    _pimpl(new impl(static_cast< ::sqlite3* >(db_), owned_))
+sqlite::database::database(
+    const utils::optional< utils::fs::path >& db_filename_, void* db_,
+    const bool owned_) :
+    _pimpl(new impl(db_filename_, static_cast< ::sqlite3* >(db_), owned_))
 {
 }
 
@@ -155,7 +173,8 @@ sqlite::database::~database(void)
 sqlite::database
 sqlite::database::in_memory(void)
 {
-    return database(impl::safe_open(":memory:", SQLITE_OPEN_READWRITE), true);
+    return database(none, impl::safe_open(":memory:", SQLITE_OPEN_READWRITE),
+                    true);
 }
 
 
@@ -191,7 +210,8 @@ sqlite::database::open(const fs::path& file, int open_flags)
     }
     PRE(open_flags == 0);
 
-    return database(impl::safe_open(file.c_str(), flags), true);
+    return database(utils::make_optional(file),
+                    impl::safe_open(file.c_str(), flags), true);
 }
 
 
@@ -204,7 +224,7 @@ sqlite::database::open(const fs::path& file, int open_flags)
 sqlite::database
 sqlite::database::temporary(void)
 {
-    return database(impl::safe_open("", SQLITE_OPEN_READWRITE), true);
+    return database(none, impl::safe_open("", SQLITE_OPEN_READWRITE), true);
 }
 
 
@@ -234,6 +254,20 @@ sqlite::database::close(void)
 }
 
 
+/// Returns the path to the connected database.
+///
+/// It is OK to call this function on a live database object, even after close()
+/// has been called.  The returned value is consistent at all times.
+///
+/// \return The path to the file that matches the connected database or none if
+/// the connection points to a transient database.
+const optional< fs::path >&
+sqlite::database::db_filename(void) const
+{
+    return _pimpl->db_filename;
+}
+
+
 /// Executes an arbitrary SQL string.
 ///
 /// As the documentation explains, this is unsafe.  The code should really be

utils/sqlite/database.hpp

@@ -44,6 +44,7 @@ extern "C" {
 #include <cstddef>
 
 #include "utils/fs/path_fwd.hpp"
+#include "utils/optional_fwd.hpp"
 #include "utils/shared_ptr.hpp"
 #include "utils/sqlite/c_gate_fwd.hpp"
 #include "utils/sqlite/statement_fwd.hpp"
@@ -82,7 +83,7 @@ class database {
     std::shared_ptr< impl > _pimpl;
 
     friend class database_c_gate;
-    database(void*, const bool);
+    database(const utils::optional< utils::fs::path >&, void*, const bool);
     void* raw_database(void);
 
 public:
@@ -93,6 +94,8 @@ class database {
     static database temporary(void);
     void close(void);
 
+    const utils::optional< utils::fs::path >& db_filename(void) const;
+
     void exec(const std::string&);
 
     transaction begin_transaction(void);