From 4462b984e9f23db5c9b1ff4f2cc840aa68ffcf9c Mon Sep 17 00:00:00 2001 From: Julio Merino Date: Fri, 3 Jul 2015 23:03:26 -0400 Subject: [PATCH 1/5] Clarify error when the results file already exists If we invoke "kyua test" on a results file that already exists, we should clearly tell that to the user. Improve the error condition in this case. --- integration/cmd_test_test.sh | 17 ++++++++++++++ store/write_backend.cpp | 33 +++++---------------------- store/write_backend_test.cpp | 44 +++++++++++++++++------------------- 3 files changed, 44 insertions(+), 50 deletions(-) diff --git a/integration/cmd_test_test.sh b/integration/cmd_test_test.sh index c47735f6..dab4a9af 100644 --- a/integration/cmd_test_test.sh +++ b/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 < 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)); } diff --git a/store/write_backend_test.cpp b/store/write_backend_test.cpp index c635ef85..a1052154 100644 --- a/store/write_backend_test.cpp +++ b/store/write_backend_test.cpp @@ -121,18 +121,17 @@ 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")); @@ -140,36 +139,35 @@ ATF_TEST_CASE_BODY(write_backend__open_rw__ok) } -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); } From 8003c86d1b73e4fd0c87a569d50277966d136556 Mon Sep 17 00:00:00 2001 From: Julio Merino Date: Sun, 5 Jul 2015 23:38:59 -0400 Subject: [PATCH 2/5] Add the db_filename method to sqlite::database Keep track of the filename the sqlite::database object was connected to so that the user of the class can query it at all times. This will be necessary to improve error reporting on sqlite::error exceptions. --- utils/sqlite/c_gate_test.cpp | 21 +++++++++++++++++++ utils/sqlite/database.cpp | 33 ++++++++++++++++++++++++++++++ utils/sqlite/database.hpp | 3 +++ utils/sqlite/database_test.cpp | 37 ++++++++++++++++++++++++++++++++++ 4 files changed, 94 insertions(+) diff --git a/utils/sqlite/c_gate_test.cpp b/utils/sqlite/c_gate_test.cpp index 9c9bea12..8afcaf1b 100644 --- a/utils/sqlite/c_gate_test.cpp +++ b/utils/sqlite/c_gate_test.cpp @@ -31,6 +31,7 @@ #include #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", "Ensure that the value of db_filename cached by the " + "sqlite::database class works after a connect() call"); +} +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_EQ(utils::make_optional(fs::path("test.db").to_absolute()), + 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); } diff --git a/utils/sqlite/database.cpp b/utils/sqlite/database.cpp index ed1904a2..32792ff6 100644 --- a/utils/sqlite/database.cpp +++ b/utils/sqlite/database.cpp @@ -32,22 +32,36 @@ extern "C" { #include } +#include #include #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 by sqlite3_db_filename after construction. + /// + /// We need to hold a copy of this value ourselves because we want to be + /// able to identify the database connection even after close() has been + /// called, but sqlite3_db_filename returns NULL after the connection is + /// closed. + optional< fs::path > db_filename; + /// The SQLite 3 internal database. ::sqlite3* db; @@ -63,6 +77,11 @@ struct utils::sqlite::database::impl : utils::noncopyable { db(db_), owned(owned_) { + const char* raw_db_filename = ::sqlite3_db_filename(db, "main"); + if (raw_db_filename == NULL || std::strlen(raw_db_filename) == 0) + db_filename = none; + else + db_filename = utils::make_optional(fs::path(raw_db_filename)); } /// Destructor. @@ -234,6 +253,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 diff --git a/utils/sqlite/database.hpp b/utils/sqlite/database.hpp index 2f96c975..75bff024 100644 --- a/utils/sqlite/database.hpp +++ b/utils/sqlite/database.hpp @@ -44,6 +44,7 @@ extern "C" { #include #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" @@ -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); diff --git a/utils/sqlite/database_test.cpp b/utils/sqlite/database_test.cpp index 12ee994a..bbf39fc4 100644 --- a/utils/sqlite/database_test.cpp +++ b/utils/sqlite/database_test.cpp @@ -32,6 +32,7 @@ #include "utils/fs/operations.hpp" #include "utils/fs/path.hpp" +#include "utils/optional.ipp" #include "utils/sqlite/statement.ipp" #include "utils/sqlite/test_utils.hpp" #include "utils/sqlite/transaction.hpp" @@ -39,6 +40,8 @@ namespace fs = utils::fs; namespace sqlite = utils::sqlite; +using utils::optional; + ATF_TEST_CASE_WITHOUT_HEAD(in_memory); ATF_TEST_CASE_BODY(in_memory) @@ -151,6 +154,36 @@ ATF_TEST_CASE_BODY(copy) } +ATF_TEST_CASE_WITHOUT_HEAD(db_filename__in_memory); +ATF_TEST_CASE_BODY(db_filename__in_memory) +{ + const sqlite::database db = sqlite::database::in_memory(); + ATF_REQUIRE(!db.db_filename()); +} + + +ATF_TEST_CASE_WITHOUT_HEAD(db_filename__file); +ATF_TEST_CASE_BODY(db_filename__file) +{ + const sqlite::database db = sqlite::database::open(fs::path("test.db"), + sqlite::open_readwrite | sqlite::open_create); + ATF_REQUIRE(db.db_filename()); + ATF_REQUIRE_EQ(fs::path("test.db").to_absolute(), db.db_filename().get()); +} + + +ATF_TEST_CASE_WITHOUT_HEAD(db_filename__ok_after_close); +ATF_TEST_CASE_BODY(db_filename__ok_after_close) +{ + sqlite::database db = sqlite::database::open(fs::path("test.db"), + sqlite::open_readwrite | sqlite::open_create); + const optional< fs::path > db_filename = db.db_filename(); + ATF_REQUIRE(db_filename); + db.close(); + ATF_REQUIRE_EQ(db_filename, db.db_filename()); +} + + ATF_TEST_CASE_WITHOUT_HEAD(exec__ok); ATF_TEST_CASE_BODY(exec__ok) { @@ -229,6 +262,10 @@ ATF_INIT_TEST_CASES(tcs) ATF_ADD_TEST_CASE(tcs, copy); + ATF_ADD_TEST_CASE(tcs, db_filename__in_memory); + ATF_ADD_TEST_CASE(tcs, db_filename__file); + ATF_ADD_TEST_CASE(tcs, db_filename__ok_after_close); + ATF_ADD_TEST_CASE(tcs, exec__ok); ATF_ADD_TEST_CASE(tcs, exec__fail); From 9df5d271807ef44050f4b6adb0e2d66cfd6a9d12 Mon Sep 17 00:00:00 2001 From: Julio Merino Date: Tue, 7 Jul 2015 15:59:31 -0400 Subject: [PATCH 3/5] Tag all sqlite errors with the db name sqlite errors are quite ambiguous on their own (e.g. "I/O error") so it is helpful to always relate their instances to the database that triggered them. --- utils/sqlite/exceptions.cpp | 66 ++++++++++++++++++++++++++++---- utils/sqlite/exceptions.hpp | 16 ++++++-- utils/sqlite/exceptions_test.cpp | 66 ++++++++++++++++++++++++-------- utils/sqlite/statement.cpp | 21 ++++++---- 4 files changed, 135 insertions(+), 34 deletions(-) diff --git a/utils/sqlite/exceptions.cpp b/utils/sqlite/exceptions.cpp index 11cf2a75..79443bc0 100644 --- a/utils/sqlite/exceptions.cpp +++ b/utils/sqlite/exceptions.cpp @@ -32,17 +32,51 @@ extern "C" { #include } +#include + #include "utils/format/macros.hpp" +#include "utils/fs/path.hpp" +#include "utils/optional.ipp" #include "utils/sqlite/c_gate.hpp" +#include "utils/sqlite/database.hpp" +namespace fs = utils::fs; namespace sqlite = utils::sqlite; +using utils::optional; + + +namespace { + + +/// Formats the database filename returned by sqlite for user consumption. +/// +/// \param db_filename An optional database filename. +/// +/// \return A string describing the filename. +static std::string +format_db_filename(const optional< fs::path >& db_filename) +{ + if (db_filename) + return db_filename.get().str(); + else + return "in-memory"; +} + + +} // anonymous namespace + /// Constructs a new error with a plain-text message. /// +/// \param db_filename_ Database filename as returned by database::db_filename() +/// for error reporting purposes. /// \param message The plain-text error message. -sqlite::error::error(const std::string& message) : - std::runtime_error(message) +sqlite::error::error(const optional< fs::path >& db_filename_, + const std::string& message) : + std::runtime_error(F("%s (sqlite db: %s)") % message % + format_db_filename(db_filename_)), + _db_filename(db_filename_) { } @@ -53,13 +87,26 @@ sqlite::error::~error(void) throw() } +/// Returns the path to the database that raised this error. +/// +/// \return A database filename as returned by database::db_filename(). +const optional< fs::path >& +sqlite::error::db_filename(void) const +{ + return _db_filename; +} + + /// Constructs a new error. /// +/// \param db_filename_ Database filename as returned by database::db_filename() +/// for error reporting purposes. /// \param api_function_ The name of the API function that caused the error. /// \param message_ The plain-text error message provided by SQLite. -sqlite::api_error::api_error(const std::string& api_function_, +sqlite::api_error::api_error(const optional< fs::path >& db_filename_, + const std::string& api_function_, const std::string& message_) : - error(message_), + error(db_filename_, message_), _api_function(api_function_) { } @@ -83,7 +130,8 @@ sqlite::api_error::from_database(database& database_, const std::string& api_function_) { ::sqlite3* c_db = database_c_gate(database_).c_database(); - return api_error(api_function_, ::sqlite3_errmsg(c_db)); + return api_error(database_.db_filename(), api_function_, + ::sqlite3_errmsg(c_db)); } @@ -99,9 +147,13 @@ sqlite::api_error::api_function(void) const /// Constructs a new error. /// +/// \param db_filename_ Database filename as returned by database::db_filename() +/// for error reporting purposes. /// \param name_ The name of the unknown column. -sqlite::invalid_column_error::invalid_column_error(const std::string& name_) : - error(F("Unknown column '%s'") % name_), +sqlite::invalid_column_error::invalid_column_error( + const optional< fs::path >& db_filename_, + const std::string& name_) : + error(db_filename_, F("Unknown column '%s'") % name_), _column_name(name_) { } diff --git a/utils/sqlite/exceptions.hpp b/utils/sqlite/exceptions.hpp index 6324ec50..a9450fce 100644 --- a/utils/sqlite/exceptions.hpp +++ b/utils/sqlite/exceptions.hpp @@ -35,6 +35,8 @@ #include #include +#include "utils/fs/path_fwd.hpp" +#include "utils/optional.hpp" #include "utils/sqlite/database_fwd.hpp" namespace utils { @@ -43,9 +45,15 @@ namespace sqlite { /// Base exception for sqlite errors. class error : public std::runtime_error { + /// Path to the database that raised this error. + utils::optional< utils::fs::path > _db_filename; + public: - explicit error(const std::string&); + explicit error(const utils::optional< utils::fs::path >&, + const std::string&); virtual ~error(void) throw(); + + const utils::optional< utils::fs::path >& db_filename(void) const; }; @@ -55,7 +63,8 @@ class api_error : public error { std::string _api_function; public: - explicit api_error(const std::string&, const std::string&); + explicit api_error(const utils::optional< utils::fs::path >&, + const std::string&, const std::string&); virtual ~api_error(void) throw(); static api_error from_database(database&, const std::string&); @@ -70,7 +79,8 @@ class invalid_column_error : public error { std::string _column_name; public: - explicit invalid_column_error(const std::string&); + explicit invalid_column_error(const utils::optional< utils::fs::path >&, + const std::string&); virtual ~invalid_column_error(void) throw(); const std::string& column_name(void) const; diff --git a/utils/sqlite/exceptions_test.cpp b/utils/sqlite/exceptions_test.cpp index 8d8515c1..74b45d07 100644 --- a/utils/sqlite/exceptions_test.cpp +++ b/utils/sqlite/exceptions_test.cpp @@ -28,29 +28,53 @@ #include "utils/sqlite/exceptions.hpp" +extern "C" { +#include +} + #include +#include #include +#include "utils/fs/path.hpp" +#include "utils/optional.ipp" #include "utils/sqlite/c_gate.hpp" #include "utils/sqlite/database.hpp" +namespace fs = utils::fs; namespace sqlite = utils::sqlite; +using utils::none; + + +ATF_TEST_CASE_WITHOUT_HEAD(error__no_filename); +ATF_TEST_CASE_BODY(error__no_filename) +{ + const sqlite::database db = sqlite::database::in_memory(); + const sqlite::error e(db.db_filename(), "Some text"); + ATF_REQUIRE_EQ("Some text (sqlite db: in-memory)", std::string(e.what())); + ATF_REQUIRE_EQ(db.db_filename(), e.db_filename()); +} -ATF_TEST_CASE_WITHOUT_HEAD(error); -ATF_TEST_CASE_BODY(error) + +ATF_TEST_CASE_WITHOUT_HEAD(error__with_filename); +ATF_TEST_CASE_BODY(error__with_filename) { - const sqlite::error e("Some text"); - ATF_REQUIRE(std::strcmp("Some text", e.what()) == 0); + const sqlite::database db = sqlite::database::open( + fs::path("test.db"), sqlite::open_readwrite | sqlite::open_create); + const sqlite::error e(db.db_filename(), "Some text"); + ATF_REQUIRE_MATCH("Some text \\(sqlite db: .*/test.db\\)", + std::string(e.what())); + ATF_REQUIRE_EQ(db.db_filename(), e.db_filename()); } ATF_TEST_CASE_WITHOUT_HEAD(api_error__explicit); ATF_TEST_CASE_BODY(api_error__explicit) { - const sqlite::api_error e("some_function", "Some text"); - ATF_REQUIRE(std::strcmp("Some text", e.what()) == 0); + const sqlite::api_error e(none, "some_function", "Some text"); + ATF_REQUIRE_EQ("Some text (sqlite db: in-memory)", std::string(e.what())); ATF_REQUIRE_EQ("some_function", e.api_function()); } @@ -58,32 +82,40 @@ ATF_TEST_CASE_BODY(api_error__explicit) ATF_TEST_CASE_WITHOUT_HEAD(api_error__from_database); ATF_TEST_CASE_BODY(api_error__from_database) { - ::sqlite3* raw_db; - ATF_REQUIRE_EQ(SQLITE_CANTOPEN, ::sqlite3_open_v2("missing.db", &raw_db, - SQLITE_OPEN_READONLY, NULL)); + sqlite::database db = sqlite::database::open( + fs::path("test.db"), sqlite::open_readwrite | sqlite::open_create); + + // Use the raw sqlite3 API to cause an error. Our C++ wrappers catch all + // errors and reraise them as exceptions, but here we want to handle the raw + // error directly for testing purposes. + sqlite::database_c_gate gate(db); + ::sqlite3_stmt* dummy_stmt; + const char* query = "ABCDE INVALID QUERY"; + (void)::sqlite3_prepare_v2(gate.c_database(), query, std::strlen(query), + &dummy_stmt, NULL); - sqlite::database gate = sqlite::database_c_gate::connect(raw_db); const sqlite::api_error e = sqlite::api_error::from_database( - gate, "real_function"); - ATF_REQUIRE(std::strcmp("unable to open database file", e.what()) == 0); + db, "real_function"); + ATF_REQUIRE_MATCH(".*ABCDE.*\\(sqlite db: .*/test.db\\)", + std::string(e.what())); ATF_REQUIRE_EQ("real_function", e.api_function()); - - ::sqlite3_close(raw_db); } ATF_TEST_CASE_WITHOUT_HEAD(invalid_column_error); ATF_TEST_CASE_BODY(invalid_column_error) { - const sqlite::invalid_column_error e("some_name"); - ATF_REQUIRE(std::strcmp("Unknown column 'some_name'", e.what()) == 0); + const sqlite::invalid_column_error e(none, "some_name"); + ATF_REQUIRE_EQ("Unknown column 'some_name' (sqlite db: in-memory)", + std::string(e.what())); ATF_REQUIRE_EQ("some_name", e.column_name()); } ATF_INIT_TEST_CASES(tcs) { - ATF_ADD_TEST_CASE(tcs, error); + ATF_ADD_TEST_CASE(tcs, error__no_filename); + ATF_ADD_TEST_CASE(tcs, error__with_filename); ATF_ADD_TEST_CASE(tcs, api_error__explicit); ATF_ADD_TEST_CASE(tcs, api_error__from_database); diff --git a/utils/sqlite/statement.cpp b/utils/sqlite/statement.cpp index 00e036f9..aa6430e7 100644 --- a/utils/sqlite/statement.cpp +++ b/utils/sqlite/statement.cpp @@ -40,6 +40,7 @@ extern "C" { #include "utils/noncopyable.hpp" #include "utils/sanity.hpp" #include "utils/sqlite/c_gate.hpp" +#include "utils/sqlite/database.hpp" #include "utils/sqlite/exceptions.hpp" namespace sqlite = utils::sqlite; @@ -261,7 +262,7 @@ sqlite::statement::column_id(const char* name) const std::map< std::string, int >::const_iterator iter = cache.find(name); if (iter == cache.end()) - throw invalid_column_error(name); + throw invalid_column_error(_pimpl->db.db_filename(), name); else return (*iter).second; } @@ -371,7 +372,8 @@ sqlite::statement::safe_column_blob(const char* name) { const int column = column_id(name); if (column_type(column) != sqlite::type_blob) - throw sqlite::error(F("Column '%s' is not a blob") % name); + throw sqlite::error(_pimpl->db.db_filename(), + F("Column '%s' is not a blob") % name); return column_blob(column); } @@ -389,7 +391,8 @@ sqlite::statement::safe_column_double(const char* name) { const int column = column_id(name); if (column_type(column) != sqlite::type_float) - throw sqlite::error(F("Column '%s' is not a float") % name); + throw sqlite::error(_pimpl->db.db_filename(), + F("Column '%s' is not a float") % name); return column_double(column); } @@ -407,7 +410,8 @@ sqlite::statement::safe_column_int(const char* name) { const int column = column_id(name); if (column_type(column) != sqlite::type_integer) - throw sqlite::error(F("Column '%s' is not an integer") % name); + throw sqlite::error(_pimpl->db.db_filename(), + F("Column '%s' is not an integer") % name); return column_int(column); } @@ -425,7 +429,8 @@ sqlite::statement::safe_column_int64(const char* name) { const int column = column_id(name); if (column_type(column) != sqlite::type_integer) - throw sqlite::error(F("Column '%s' is not an integer") % name); + throw sqlite::error(_pimpl->db.db_filename(), + F("Column '%s' is not an integer") % name); return column_int64(column); } @@ -443,7 +448,8 @@ sqlite::statement::safe_column_text(const char* name) { const int column = column_id(name); if (column_type(column) != sqlite::type_text) - throw sqlite::error(F("Column '%s' is not a string") % name); + throw sqlite::error(_pimpl->db.db_filename(), + F("Column '%s' is not a string") % name); return column_text(column); } @@ -462,7 +468,8 @@ sqlite::statement::safe_column_bytes(const char* name) const int column = column_id(name); if (column_type(column) != sqlite::type_blob && column_type(column) != sqlite::type_text) - throw sqlite::error(F("Column '%s' is not a blob or a string") % name); + throw sqlite::error(_pimpl->db.db_filename(), + F("Column '%s' is not a blob or a string") % name); return column_bytes(column); } From a5b7ab2f787a8ce591aab3b02d8a1c0b10ba09d2 Mon Sep 17 00:00:00 2001 From: Julio Merino Date: Tue, 7 Jul 2015 16:07:43 -0400 Subject: [PATCH 4/5] Add the API function that failed to the message The sqlite::api_error class took the name of the API function that caused the error as a parameter but did not add such information to the error message. I probably assumed that the message returned by the sqlite API would be descriptive enough, but isn't. Add the details now. --- utils/sqlite/exceptions.cpp | 2 +- utils/sqlite/exceptions_test.cpp | 9 ++++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/utils/sqlite/exceptions.cpp b/utils/sqlite/exceptions.cpp index 79443bc0..222299b4 100644 --- a/utils/sqlite/exceptions.cpp +++ b/utils/sqlite/exceptions.cpp @@ -106,7 +106,7 @@ sqlite::error::db_filename(void) const sqlite::api_error::api_error(const optional< fs::path >& db_filename_, const std::string& api_function_, const std::string& message_) : - error(db_filename_, message_), + error(db_filename_, F("%s (sqlite op: %s)") % message_ % api_function_), _api_function(api_function_) { } diff --git a/utils/sqlite/exceptions_test.cpp b/utils/sqlite/exceptions_test.cpp index 74b45d07..27d62c54 100644 --- a/utils/sqlite/exceptions_test.cpp +++ b/utils/sqlite/exceptions_test.cpp @@ -74,7 +74,9 @@ ATF_TEST_CASE_WITHOUT_HEAD(api_error__explicit); ATF_TEST_CASE_BODY(api_error__explicit) { const sqlite::api_error e(none, "some_function", "Some text"); - ATF_REQUIRE_EQ("Some text (sqlite db: in-memory)", std::string(e.what())); + ATF_REQUIRE_EQ( + "Some text (sqlite op: some_function) (sqlite db: in-memory)", + std::string(e.what())); ATF_REQUIRE_EQ("some_function", e.api_function()); } @@ -96,8 +98,9 @@ ATF_TEST_CASE_BODY(api_error__from_database) const sqlite::api_error e = sqlite::api_error::from_database( db, "real_function"); - ATF_REQUIRE_MATCH(".*ABCDE.*\\(sqlite db: .*/test.db\\)", - std::string(e.what())); + ATF_REQUIRE_MATCH( + ".*ABCDE.*\\(sqlite op: real_function\\) \\(sqlite db: .*/test.db\\)", + std::string(e.what())); ATF_REQUIRE_EQ("real_function", e.api_function()); } From f84f0c548067c410641b1783358826e6ba26118d Mon Sep 17 00:00:00 2001 From: Julio Merino Date: Tue, 7 Jul 2015 17:00:31 -0400 Subject: [PATCH 5/5] Avoid using sqlite3_db_filename Ubuntu 12.04 LTS ships with sqlite 3.7.9, which does not include the sqlite3_db_filename function (which first appeared in 3.7.10). As we need to support this release for Travis CI builds, stop using this non-existent function and instead handle the database filename on our own. This is unfortunate because there are a couple of shortcomings. First, we cannot determine the name of temporary databases and, second, we cannot determine the name of databases reopened via the c_gate. None of these are used in final Kyua builds so this is not a big deal. --- utils/sqlite/c_gate.cpp | 6 ++++- utils/sqlite/c_gate_test.cpp | 8 +++---- utils/sqlite/database.cpp | 41 ++++++++++++++++---------------- utils/sqlite/database.hpp | 2 +- utils/sqlite/database_test.cpp | 11 ++++++++- utils/sqlite/exceptions.cpp | 2 +- utils/sqlite/exceptions_test.cpp | 14 ++++++----- 7 files changed, 50 insertions(+), 34 deletions(-) diff --git a/utils/sqlite/c_gate.cpp b/utils/sqlite/c_gate.cpp index 909f05b2..e89ac533 100644 --- a/utils/sqlite/c_gate.cpp +++ b/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); } diff --git a/utils/sqlite/c_gate_test.cpp b/utils/sqlite/c_gate_test.cpp index 8afcaf1b..edf46f76 100644 --- a/utils/sqlite/c_gate_test.cpp +++ b/utils/sqlite/c_gate_test.cpp @@ -72,8 +72,9 @@ ATF_TEST_CASE_BODY(c_database) ATF_TEST_CASE(database__db_filename); ATF_TEST_CASE_HEAD(database__db_filename) { - set_md_var("descr", "Ensure that the value of db_filename cached by the " - "sqlite::database class works after a connect() call"); + 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) { @@ -82,8 +83,7 @@ ATF_TEST_CASE_BODY(database__db_filename) "test.db", &raw_db, SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE, NULL)); sqlite::database database = sqlite::database_c_gate::connect(raw_db); - ATF_REQUIRE_EQ(utils::make_optional(fs::path("test.db").to_absolute()), - database.db_filename()); + ATF_REQUIRE(!database.db_filename()); ::sqlite3_close(raw_db); } diff --git a/utils/sqlite/database.cpp b/utils/sqlite/database.cpp index 32792ff6..41935c3b 100644 --- a/utils/sqlite/database.cpp +++ b/utils/sqlite/database.cpp @@ -54,12 +54,7 @@ using utils::optional; /// Internal implementation for sqlite::database. struct utils::sqlite::database::impl : utils::noncopyable { - /// Path to the database as seen by sqlite3_db_filename after construction. - /// - /// We need to hold a copy of this value ourselves because we want to be - /// able to identify the database connection even after close() has been - /// called, but sqlite3_db_filename returns NULL after the connection is - /// closed. + /// Path to the database as seen at construction time. optional< fs::path > db_filename; /// The SQLite 3 internal database. @@ -70,18 +65,17 @@ 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_) { - const char* raw_db_filename = ::sqlite3_db_filename(db, "main"); - if (raw_db_filename == NULL || std::strlen(raw_db_filename) == 0) - db_filename = none; - else - db_filename = utils::make_optional(fs::path(raw_db_filename)); } /// Destructor. @@ -117,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"); } @@ -147,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_)) { } @@ -174,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); } @@ -210,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); } @@ -223,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); } diff --git a/utils/sqlite/database.hpp b/utils/sqlite/database.hpp index 75bff024..84228b57 100644 --- a/utils/sqlite/database.hpp +++ b/utils/sqlite/database.hpp @@ -83,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: diff --git a/utils/sqlite/database_test.cpp b/utils/sqlite/database_test.cpp index bbf39fc4..70f057b9 100644 --- a/utils/sqlite/database_test.cpp +++ b/utils/sqlite/database_test.cpp @@ -168,7 +168,15 @@ ATF_TEST_CASE_BODY(db_filename__file) const sqlite::database db = sqlite::database::open(fs::path("test.db"), sqlite::open_readwrite | sqlite::open_create); ATF_REQUIRE(db.db_filename()); - ATF_REQUIRE_EQ(fs::path("test.db").to_absolute(), db.db_filename().get()); + ATF_REQUIRE_EQ(fs::path("test.db"), db.db_filename().get()); +} + + +ATF_TEST_CASE_WITHOUT_HEAD(db_filename__temporary); +ATF_TEST_CASE_BODY(db_filename__temporary) +{ + const sqlite::database db = sqlite::database::temporary(); + ATF_REQUIRE(!db.db_filename()); } @@ -264,6 +272,7 @@ ATF_INIT_TEST_CASES(tcs) ATF_ADD_TEST_CASE(tcs, db_filename__in_memory); ATF_ADD_TEST_CASE(tcs, db_filename__file); + ATF_ADD_TEST_CASE(tcs, db_filename__temporary); ATF_ADD_TEST_CASE(tcs, db_filename__ok_after_close); ATF_ADD_TEST_CASE(tcs, exec__ok); diff --git a/utils/sqlite/exceptions.cpp b/utils/sqlite/exceptions.cpp index 222299b4..cc2d42ca 100644 --- a/utils/sqlite/exceptions.cpp +++ b/utils/sqlite/exceptions.cpp @@ -60,7 +60,7 @@ format_db_filename(const optional< fs::path >& db_filename) if (db_filename) return db_filename.get().str(); else - return "in-memory"; + return "in-memory or temporary"; } diff --git a/utils/sqlite/exceptions_test.cpp b/utils/sqlite/exceptions_test.cpp index 27d62c54..d9e81038 100644 --- a/utils/sqlite/exceptions_test.cpp +++ b/utils/sqlite/exceptions_test.cpp @@ -53,7 +53,8 @@ ATF_TEST_CASE_BODY(error__no_filename) { const sqlite::database db = sqlite::database::in_memory(); const sqlite::error e(db.db_filename(), "Some text"); - ATF_REQUIRE_EQ("Some text (sqlite db: in-memory)", std::string(e.what())); + ATF_REQUIRE_EQ("Some text (sqlite db: in-memory or temporary)", + std::string(e.what())); ATF_REQUIRE_EQ(db.db_filename(), e.db_filename()); } @@ -64,8 +65,7 @@ ATF_TEST_CASE_BODY(error__with_filename) const sqlite::database db = sqlite::database::open( fs::path("test.db"), sqlite::open_readwrite | sqlite::open_create); const sqlite::error e(db.db_filename(), "Some text"); - ATF_REQUIRE_MATCH("Some text \\(sqlite db: .*/test.db\\)", - std::string(e.what())); + ATF_REQUIRE_EQ("Some text (sqlite db: test.db)", std::string(e.what())); ATF_REQUIRE_EQ(db.db_filename(), e.db_filename()); } @@ -75,7 +75,8 @@ ATF_TEST_CASE_BODY(api_error__explicit) { const sqlite::api_error e(none, "some_function", "Some text"); ATF_REQUIRE_EQ( - "Some text (sqlite op: some_function) (sqlite db: in-memory)", + "Some text (sqlite op: some_function) " + "(sqlite db: in-memory or temporary)", std::string(e.what())); ATF_REQUIRE_EQ("some_function", e.api_function()); } @@ -99,7 +100,7 @@ ATF_TEST_CASE_BODY(api_error__from_database) const sqlite::api_error e = sqlite::api_error::from_database( db, "real_function"); ATF_REQUIRE_MATCH( - ".*ABCDE.*\\(sqlite op: real_function\\) \\(sqlite db: .*/test.db\\)", + ".*ABCDE.*\\(sqlite op: real_function\\) \\(sqlite db: test.db\\)", std::string(e.what())); ATF_REQUIRE_EQ("real_function", e.api_function()); } @@ -109,7 +110,8 @@ ATF_TEST_CASE_WITHOUT_HEAD(invalid_column_error); ATF_TEST_CASE_BODY(invalid_column_error) { const sqlite::invalid_column_error e(none, "some_name"); - ATF_REQUIRE_EQ("Unknown column 'some_name' (sqlite db: in-memory)", + ATF_REQUIRE_EQ("Unknown column 'some_name' " + "(sqlite db: in-memory or temporary)", std::string(e.what())); ATF_REQUIRE_EQ("some_name", e.column_name()); }