From 6a1cb5eecd794d65ab0d15161b1628750b29acb6 Mon Sep 17 00:00:00 2001 From: John Ralls Date: Thu, 4 Jun 2020 13:05:55 -0700 Subject: [PATCH] Replace the three bool parameters to qof_session_begin to an enum. For clarity. In so doing found the backend behavior a bit inconsistent so it's modified to do what the enum values indicate. In the course of changing the various calls I found some implementation errors in the back end and corrected them. --- bindings/python/sqlite3test.c | 2 +- gnucash/gnome-utils/assistant-xml-encoding.c | 2 +- gnucash/gnome-utils/gnc-file.c | 25 +++--- gnucash/gnucash-commands.cpp | 4 +- gnucash/import-export/aqb/test/test-kvp.c | 4 +- libgnucash/backend/dbi/gnc-backend-dbi.cpp | 34 +++----- libgnucash/backend/dbi/gnc-backend-dbi.hpp | 2 +- .../backend/dbi/gnc-dbisqlconnection.cpp | 10 +-- .../backend/dbi/gnc-dbisqlconnection.hpp | 4 +- .../dbi/test/test-backend-dbi-basic.cpp | 22 +++--- .../sql/test/utest-gnc-backend-sql.cpp | 2 +- libgnucash/backend/xml/gnc-xml-backend.cpp | 42 ++++------ libgnucash/backend/xml/gnc-xml-backend.hpp | 2 +- .../backend/xml/test/test-load-xml2.cpp | 3 +- .../backend/xml/test/test-save-in-lang.cpp | 4 +- libgnucash/engine/qof-backend.hpp | 19 +---- libgnucash/engine/qofsession.cpp | 20 +++-- libgnucash/engine/qofsession.h | 77 ++++++++++++------- libgnucash/engine/qofsession.hpp | 2 +- libgnucash/engine/test/test-qofinstance.cpp | 4 +- .../engine/test/test-qofsession-old.cpp | 21 ++--- libgnucash/engine/test/test-qofsession.cpp | 24 +++--- libgnucash/engine/test/utest-Transaction.cpp | 2 +- 23 files changed, 155 insertions(+), 176 deletions(-) diff --git a/bindings/python/sqlite3test.c b/bindings/python/sqlite3test.c index a1e626952b..b8c41889f4 100644 --- a/bindings/python/sqlite3test.c +++ b/bindings/python/sqlite3test.c @@ -32,7 +32,7 @@ int main() gnc_engine_init(0, no_args); s = qof_session_new(NULL); - qof_session_begin(s, testurl, 0, 1, 0); + qof_session_begin(s, testurl, SESSION_NEW_STORE); qof_session_load(s, NULL); qof_session_save(s, NULL); qof_session_end(s); diff --git a/gnucash/gnome-utils/assistant-xml-encoding.c b/gnucash/gnome-utils/assistant-xml-encoding.c index d702226708..86b62fc259 100644 --- a/gnucash/gnome-utils/assistant-xml-encoding.c +++ b/gnucash/gnome-utils/assistant-xml-encoding.c @@ -1082,7 +1082,7 @@ gxi_parse_file (GncXmlImportData *data) gxi_session_destroy (data); session = qof_session_new (NULL); data->session = session; - qof_session_begin (session, data->filename, TRUE, FALSE, FALSE); + qof_session_begin (session, data->filename, SESSION_READ_ONLY); io_err = qof_session_get_error (session); if (io_err != ERR_BACKEND_NO_ERR) { diff --git a/gnucash/gnome-utils/gnc-file.c b/gnucash/gnome-utils/gnc-file.c index 33ea97f778..e601f9b893 100644 --- a/gnucash/gnome-utils/gnc-file.c +++ b/gnucash/gnome-utils/gnc-file.c @@ -790,7 +790,8 @@ RESTART: new_session = qof_session_new (qof_book_new()); // Begin the new session. If we are in read-only mode, ignore the locks. - qof_session_begin (new_session, newfile, is_readonly, FALSE, FALSE); + qof_session_begin (new_session, newfile, + is_readonly ? SESSION_READ_ONLY : SESSION_NORMAL_OPEN); io_err = qof_session_get_error (new_session); if (ERR_BACKEND_BAD_URL == io_err) @@ -872,11 +873,11 @@ RESTART: case RESPONSE_READONLY: is_readonly = TRUE; /* user told us to open readonly. We do ignore locks (just as before), but now also force the opening. */ - qof_session_begin (new_session, newfile, is_readonly, FALSE, TRUE); + qof_session_begin (new_session, newfile, SESSION_READ_ONLY); break; case RESPONSE_OPEN: /* user told us to ignore locks. So ignore them. */ - qof_session_begin (new_session, newfile, TRUE, FALSE, FALSE); + qof_session_begin (new_session, newfile, SESSION_BREAK_LOCK); break; default: /* Can't use the given file, so just create a new @@ -895,7 +896,7 @@ RESTART: /* user told us to create a new database. Do it. We * shouldn't have to worry about locking or clobbering, * it's supposed to be new. */ - qof_session_begin (new_session, newfile, FALSE, TRUE, FALSE); + qof_session_begin (new_session, newfile, SESSION_NEW_STORE); } } @@ -1286,7 +1287,7 @@ gnc_file_do_export(GtkWindow *parent, const char * filename) /* -- this session code is NOT identical in FileOpen and FileSaveAs -- */ new_session = qof_session_new (NULL); - qof_session_begin (new_session, newfile, FALSE, TRUE, FALSE); + qof_session_begin (new_session, newfile, SESSION_NEW_STORE); io_err = qof_session_get_error (new_session); /* If the file exists and would be clobbered, ask the user */ @@ -1305,7 +1306,7 @@ gnc_file_do_export(GtkWindow *parent, const char * filename) { return; } - qof_session_begin (new_session, newfile, FALSE, TRUE, TRUE); + qof_session_begin (new_session, newfile, SESSION_NEW_OVERWRITE); } /* if file appears to be locked, ask the user ... */ if (ERR_BACKEND_LOCKED == io_err || ERR_BACKEND_READONLY == io_err) @@ -1313,7 +1314,7 @@ gnc_file_do_export(GtkWindow *parent, const char * filename) if (!show_session_error (parent, io_err, newfile, GNC_FILE_DIALOG_EXPORT)) { /* user told us to ignore locks. So ignore them. */ - qof_session_begin (new_session, newfile, TRUE, FALSE, FALSE); + qof_session_begin (new_session, newfile, SESSION_BREAK_LOCK); } } @@ -1532,7 +1533,7 @@ gnc_file_do_save_as (GtkWindow *parent, const char* filename) save_in_progress++; new_session = qof_session_new (NULL); - qof_session_begin (new_session, newfile, FALSE, TRUE, FALSE); + qof_session_begin (new_session, newfile, SESSION_NEW_STORE); io_err = qof_session_get_error (new_session); @@ -1558,15 +1559,15 @@ gnc_file_do_save_as (GtkWindow *parent, const char* filename) save_in_progress--; return; } - qof_session_begin (new_session, newfile, FALSE, TRUE, TRUE); + qof_session_begin (new_session, newfile, SESSION_NEW_OVERWRITE); } /* if file appears to be locked, ask the user ... */ else if (ERR_BACKEND_LOCKED == io_err || ERR_BACKEND_READONLY == io_err) { if (!show_session_error (parent, io_err, newfile, GNC_FILE_DIALOG_SAVE)) { - /* user told us to ignore locks. So ignore them. */ - qof_session_begin (new_session, newfile, TRUE, FALSE, FALSE); + // User wants to replace the file. + qof_session_begin (new_session, newfile, SESSION_BREAK_LOCK); } } @@ -1578,7 +1579,7 @@ gnc_file_do_save_as (GtkWindow *parent, const char* filename) if (!show_session_error (parent, io_err, newfile, GNC_FILE_DIALOG_SAVE)) { /* user told us to create a new database. Do it. */ - qof_session_begin (new_session, newfile, FALSE, TRUE, FALSE); + qof_session_begin (new_session, newfile, SESSION_NEW_STORE); } } diff --git a/gnucash/gnucash-commands.cpp b/gnucash/gnucash-commands.cpp index 6ac7c3eeb0..d6aa71b575 100644 --- a/gnucash/gnucash-commands.cpp +++ b/gnucash/gnucash-commands.cpp @@ -95,7 +95,7 @@ scm_add_quotes(void *data, [[maybe_unused]] int argc, [[maybe_unused]] char **ar if (!session) scm_cleanup_and_exit_with_failure (session); - qof_session_begin(session, add_quotes_file->c_str(), FALSE, FALSE, FALSE); + qof_session_begin(session, add_quotes_file->c_str(), SESSION_NORMAL_OPEN); if (qof_session_get_error(session) != ERR_BACKEND_NO_ERR) scm_cleanup_and_exit_with_failure (session); @@ -175,7 +175,7 @@ scm_run_report (void *data, if (!session) scm_cleanup_and_exit_with_failure (session); - qof_session_begin (session, datafile, FALSE, FALSE, FALSE); + qof_session_begin (session, datafile, SESSION_NORMAL_OPEN); if (qof_session_get_error (session) != ERR_BACKEND_NO_ERR) scm_cleanup_and_exit_with_failure (session); diff --git a/gnucash/import-export/aqb/test/test-kvp.c b/gnucash/import-export/aqb/test/test-kvp.c index 62458e08bb..dce9fb614a 100644 --- a/gnucash/import-export/aqb/test/test-kvp.c +++ b/gnucash/import-export/aqb/test/test-kvp.c @@ -68,7 +68,7 @@ test_qofsession_aqb_kvp( void ) QofSession *new_session = qof_session_new (book); char *newfile = g_strdup_printf("file://%s", file1); - qof_session_begin (new_session, newfile, TRUE, FALSE, FALSE); + qof_session_begin (new_session, newfile, SESSION_READ_ONLY); io_err = qof_session_get_error (new_session); //printf("io_err1 = %d\n", io_err); g_assert(io_err != ERR_BACKEND_NO_HANDLER); // Do not have no handler @@ -97,7 +97,7 @@ test_qofsession_aqb_kvp( void ) QofSession *new_session = qof_session_new (book); char *newfile = g_strdup_printf("file://%s", file2); - qof_session_begin (new_session, newfile, TRUE, FALSE, FALSE); + qof_session_begin (new_session, newfile, SESSION_READ_ONLY); io_err = qof_session_get_error (new_session); //printf("io_err1 = %d\n", io_err); g_assert(io_err != ERR_BACKEND_NO_HANDLER); // Do not have no handler diff --git a/libgnucash/backend/dbi/gnc-backend-dbi.cpp b/libgnucash/backend/dbi/gnc-backend-dbi.cpp index d84bf39e22..25699e5ca8 100644 --- a/libgnucash/backend/dbi/gnc-backend-dbi.cpp +++ b/libgnucash/backend/dbi/gnc-backend-dbi.cpp @@ -377,8 +377,7 @@ error_handler (dbi_conn conn, void* user_data) template <> void GncDbiBackend::session_begin(QofSession* session, const char* new_uri, - bool ignore_lock, - bool create, bool force) + SessionOpenMode mode) { gboolean file_exists; PairVec options; @@ -395,6 +394,7 @@ GncDbiBackend::session_begin(QofSession* session, GFileTest ftest = static_cast ( G_FILE_TEST_IS_REGULAR | G_FILE_TEST_EXISTS) ; file_exists = g_file_test (filepath.c_str(), ftest); + bool create{mode == SESSION_NEW_STORE || mode == SESSION_NEW_OVERWRITE}; if (!create && !file_exists) { set_error (ERR_FILEIO_FILE_NOT_FOUND); @@ -407,12 +407,12 @@ GncDbiBackend::session_begin(QofSession* session, if (create && file_exists) { - if (force) + if (mode == SESSION_NEW_OVERWRITE) g_unlink (filepath.c_str()); else { set_error (ERR_BACKEND_STORE_EXISTS); - auto msg = "Might clobber, no force"; + auto msg = "Might clobber, mode not SESSION_NEW_OVERWRITE"; PWARN ("%s", msg); LEAVE("Error"); return; @@ -466,7 +466,7 @@ GncDbiBackend::session_begin(QofSession* session, try { connect(new GncDbiSqlConnection(DbType::DBI_SQLITE, - this, conn, ignore_lock)); + this, conn, mode)); } catch (std::runtime_error& err) { @@ -677,6 +677,7 @@ GncDbiBackend::session_begin (QofSession* session, const char* new_uri, } connect(nullptr); + bool create{mode == SESSION_NEW_STORE || mode == SESSION_NEW_OVERWRITE}; auto conn = conn_setup(options, uri); if (conn == nullptr) { @@ -696,26 +697,15 @@ GncDbiBackend::session_begin (QofSession* session, const char* new_uri, LEAVE("Error"); return; } - if (create && save_may_clobber_data(conn, - uri.quote_dbname(Type))) + bool create = (mode == SESSION_NEW_STORE || + mode == SESSION_NEW_OVERWRITE); + if (create && save_may_clobber_data(conn, uri.quote_dbname(Type))) { - if (force) + if (mode == SESSION_NEW_OVERWRITE) { - // Drop DB - const char *root_db; - if (Type == DbType::DBI_PGSQL) - { - root_db = "template1"; - } - else if (Type == DbType::DBI_MYSQL) - { - root_db = "mysql"; - } - else - { if (!drop_database(conn, uri)) return; - } + } else { set_error (ERR_BACKEND_STORE_EXISTS); @@ -782,7 +772,7 @@ GncDbiBackend::session_begin (QofSession* session, const char* new_uri, connect(nullptr); try { - connect(new GncDbiSqlConnection(Type, this, conn, ignore_lock)); + connect(new GncDbiSqlConnection(Type, this, conn, mode)); } catch (std::runtime_error& err) { diff --git a/libgnucash/backend/dbi/gnc-backend-dbi.hpp b/libgnucash/backend/dbi/gnc-backend-dbi.hpp index cdcfdf93b3..f02d590076 100644 --- a/libgnucash/backend/dbi/gnc-backend-dbi.hpp +++ b/libgnucash/backend/dbi/gnc-backend-dbi.hpp @@ -92,7 +92,7 @@ public: GncDbiBackend(GncSqlConnection *conn, QofBook* book) : GncSqlBackend(conn, book), m_exists{false} {} ~GncDbiBackend(); - void session_begin(QofSession*, const char*, bool, bool, bool) override; + void session_begin(QofSession*, const char*, SessionOpenMode) override; void session_end() override; void load(QofBook*, QofBackendLoadType) override; void safe_sync(QofBook*) override; diff --git a/libgnucash/backend/dbi/gnc-dbisqlconnection.cpp b/libgnucash/backend/dbi/gnc-dbisqlconnection.cpp index 2577fe6ad5..8414a904e5 100644 --- a/libgnucash/backend/dbi/gnc-dbisqlconnection.cpp +++ b/libgnucash/backend/dbi/gnc-dbisqlconnection.cpp @@ -81,7 +81,7 @@ GncDbiSqlStatement::add_where_cond(QofIdTypeConst type_name, } GncDbiSqlConnection::GncDbiSqlConnection (DbType type, QofBackend* qbe, - dbi_conn conn, bool ignore_lock) : + dbi_conn conn, SessionOpenMode mode) : m_qbe{qbe}, m_conn{conn}, m_provider{type == DbType::DBI_SQLITE ? make_dbi_provider() : @@ -91,7 +91,7 @@ GncDbiSqlConnection::GncDbiSqlConnection (DbType type, QofBackend* qbe, m_conn_ok{true}, m_last_error{ERR_BACKEND_NO_ERR}, m_error_repeat{0}, m_retry{false}, m_sql_savepoint{0} { - if (!lock_database(ignore_lock)) + if (mode != SESSION_READ_ONLY && !lock_database(mode == SESSION_BREAK_LOCK)) throw std::runtime_error("Failed to lock database!"); if (!check_and_rollback_failed_save()) { @@ -101,7 +101,7 @@ GncDbiSqlConnection::GncDbiSqlConnection (DbType type, QofBackend* qbe, } bool -GncDbiSqlConnection::lock_database (bool ignore_lock) +GncDbiSqlConnection::lock_database (bool break_lock) { const char *errstr; /* Protect everything with a single transaction to prevent races */ @@ -127,7 +127,7 @@ GncDbiSqlConnection::lock_database (bool ignore_lock) } } - /* Check for an existing entry; delete it if ignore_lock is true, otherwise fail */ + /* Check for an existing entry; delete it if break_lock is true, otherwise fail */ char hostname[ GNC_HOST_NAME_MAX + 1 ]; auto result = dbi_conn_queryf (m_conn, "SELECT * FROM %s", lock_table.c_str()); @@ -135,7 +135,7 @@ GncDbiSqlConnection::lock_database (bool ignore_lock) { dbi_result_free (result); result = nullptr; - if (!ignore_lock) + if (!break_lock) { qof_backend_set_error (m_qbe, ERR_BACKEND_LOCKED); /* FIXME: After enhancing the qof_backend_error mechanism, report in the dialog what is the hostname of the machine holding the lock. */ diff --git a/libgnucash/backend/dbi/gnc-dbisqlconnection.hpp b/libgnucash/backend/dbi/gnc-dbisqlconnection.hpp index bb71a9636c..5ac6d9c626 100644 --- a/libgnucash/backend/dbi/gnc-dbisqlconnection.hpp +++ b/libgnucash/backend/dbi/gnc-dbisqlconnection.hpp @@ -42,7 +42,7 @@ class GncDbiSqlConnection : public GncSqlConnection { public: GncDbiSqlConnection (DbType type, QofBackend* qbe, dbi_conn conn, - bool ignore_lock); + SessionOpenMode mode); ~GncDbiSqlConnection() override; GncSqlResultPtr execute_select_statement (const GncSqlStatementPtr&) noexcept override; @@ -108,7 +108,7 @@ private: */ bool m_retry; unsigned int m_sql_savepoint; - bool lock_database(bool ignore_lock); + bool lock_database(bool break_lock); void unlock_database(); bool rename_table(const std::string& old_name, const std::string& new_name); bool drop_table(const std::string& table); diff --git a/libgnucash/backend/dbi/test/test-backend-dbi-basic.cpp b/libgnucash/backend/dbi/test/test-backend-dbi-basic.cpp index 994c5e8c42..3e5ed83ac9 100644 --- a/libgnucash/backend/dbi/test/test-backend-dbi-basic.cpp +++ b/libgnucash/backend/dbi/test/test-backend-dbi-basic.cpp @@ -97,8 +97,8 @@ setup (Fixture* fixture, gconstpointer pData) * prevents creating the lock file. Force the session to get * around that. */ - qof_session_begin (fixture->session, DBI_TEST_XML_FILENAME, TRUE, - FALSE, TRUE); + qof_session_begin (fixture->session, DBI_TEST_XML_FILENAME, + SESSION_BREAK_LOCK); g_assert_cmpint (qof_session_get_error (fixture->session), == , ERR_BACKEND_NO_ERR); qof_session_load (fixture->session, NULL); @@ -394,7 +394,7 @@ test_dbi_store_and_reload (Fixture* fixture, gconstpointer pData) // Save the session data auto book2{qof_book_new()}; auto session_2 = qof_session_new (book2); - qof_session_begin (session_2, url, FALSE, TRUE, TRUE); + qof_session_begin (session_2, url, SESSION_NEW_OVERWRITE); g_assert (session_2 != NULL); g_assert_cmpint (qof_session_get_error (session_2), == , ERR_BACKEND_NO_ERR); qof_session_swap_data (fixture->session, session_2); @@ -407,7 +407,7 @@ test_dbi_store_and_reload (Fixture* fixture, gconstpointer pData) auto book3{qof_book_new()}; auto session_3 = qof_session_new (book3); g_assert (session_3 != NULL); - qof_session_begin (session_3, url, TRUE, FALSE, FALSE); + qof_session_begin (session_3, url, SESSION_READ_ONLY); g_assert (session_3 != NULL); g_assert_cmpint (qof_session_get_error (session_3), == , ERR_BACKEND_NO_ERR); qof_session_load (session_3, NULL); @@ -447,7 +447,7 @@ test_dbi_safe_save (Fixture* fixture, gconstpointer pData) // Load the session data auto book1{qof_book_new()}; auto session_1 = qof_session_new (book1); - qof_session_begin (session_1, url, FALSE, TRUE, TRUE); + qof_session_begin (session_1, url, SESSION_NEW_OVERWRITE); if (session_1 && qof_session_get_error (session_1) != ERR_BACKEND_NO_ERR) { @@ -473,7 +473,7 @@ test_dbi_safe_save (Fixture* fixture, gconstpointer pData) /* Destroy the session and reload it */ session_2 = qof_session_new (qof_book_new()); - qof_session_begin (session_2, url, TRUE, FALSE, FALSE); + qof_session_begin (session_2, url, SESSION_READ_ONLY); if (session_2 && qof_session_get_error (session_2) != ERR_BACKEND_NO_ERR) { @@ -521,7 +521,7 @@ test_dbi_version_control (Fixture* fixture, gconstpointer pData) if (fixture->filename) url = fixture->filename; auto sess = qof_session_new (nullptr); - qof_session_begin (sess, url, FALSE, TRUE, TRUE); + qof_session_begin (sess, url, SESSION_NEW_OVERWRITE); if (sess && qof_session_get_error (sess) != ERR_BACKEND_NO_ERR) { g_warning ("Session Error: %d, %s", qof_session_get_error (sess), @@ -541,7 +541,7 @@ test_dbi_version_control (Fixture* fixture, gconstpointer pData) qof_session_end (sess); qof_session_destroy (sess); sess = qof_session_new (qof_book_new()); - qof_session_begin (sess, url, TRUE, FALSE, FALSE); + qof_session_begin (sess, url, SESSION_NORMAL_OPEN); qof_session_load (sess, NULL); err = qof_session_pop_error (sess); g_assert_cmpint (err, == , ERR_SQL_DB_TOO_OLD); @@ -554,7 +554,7 @@ test_dbi_version_control (Fixture* fixture, gconstpointer pData) qof_session_end (sess); qof_session_destroy (sess); sess = qof_session_new (qof_book_new()); - qof_session_begin (sess, url, TRUE, FALSE, FALSE); + qof_session_begin (sess, url, SESSION_NORMAL_OPEN); qof_session_load (sess, NULL); qof_session_ensure_all_data_loaded (sess); err = qof_session_pop_error (sess); @@ -583,14 +583,14 @@ test_dbi_business_store_and_reload (Fixture* fixture, gconstpointer pData) url = fixture->filename; // Save the session data auto session_2 = qof_session_new (qof_book_new()); - qof_session_begin (session_2, url, FALSE, TRUE, TRUE); + qof_session_begin (session_2, url, SESSION_NEW_OVERWRITE); qof_session_swap_data (fixture->session, session_2); qof_book_mark_session_dirty (qof_session_get_book (session_2)); qof_session_save (session_2, NULL); // Reload the session data auto session_3 = qof_session_new (qof_book_new()); - qof_session_begin (session_3, url, TRUE, FALSE, FALSE); + qof_session_begin (session_3, url, SESSION_READ_ONLY); qof_session_load (session_3, NULL); // Compare with the original data diff --git a/libgnucash/backend/sql/test/utest-gnc-backend-sql.cpp b/libgnucash/backend/sql/test/utest-gnc-backend-sql.cpp index 253bd1c6e3..b89cde9a7e 100644 --- a/libgnucash/backend/sql/test/utest-gnc-backend-sql.cpp +++ b/libgnucash/backend/sql/test/utest-gnc-backend-sql.cpp @@ -40,7 +40,7 @@ class GncMockSqlBackend : public GncSqlBackend public: GncMockSqlBackend(GncSqlConnection* conn, QofBook* book) : GncSqlBackend(conn, book) {} - void session_begin(QofSession*, const char*, bool, bool, bool) override {} + void session_begin(QofSession*, const char*, SessionOpenMode) override {} void session_end() override {} void safe_sync(QofBook* book) override { sync(book); } }; diff --git a/libgnucash/backend/xml/gnc-xml-backend.cpp b/libgnucash/backend/xml/gnc-xml-backend.cpp index 3984d4d8a3..28e9939ec5 100644 --- a/libgnucash/backend/xml/gnc-xml-backend.cpp +++ b/libgnucash/backend/xml/gnc-xml-backend.cpp @@ -107,7 +107,7 @@ GncXmlBackend::check_path (const char* fullpath, bool create) void GncXmlBackend::session_begin(QofSession* session, const char* new_uri, - bool ignore_lock, bool create, bool force) + SessionOpenMode mode) { /* Make sure the directory is there */ m_fullpath = gnc_uri_get_path (new_uri); @@ -118,14 +118,15 @@ GncXmlBackend::session_begin(QofSession* session, const char* new_uri, set_message("No path specified"); return; } - if (create && !force && save_may_clobber_data()) + if (mode == SESSION_NEW_STORE && save_may_clobber_data()) { set_error(ERR_BACKEND_STORE_EXISTS); PWARN ("Might clobber, no force"); return; } - if (!check_path(m_fullpath.c_str(), create)) + if (!check_path(m_fullpath.c_str(), + SESSION_NEW_STORE || mode == SESSION_NEW_OVERWRITE)) return; m_dirname = g_path_get_dirname (m_fullpath.c_str()); @@ -137,34 +138,19 @@ GncXmlBackend::session_begin(QofSession* session, const char* new_uri, xaccLogSetBaseName (m_fullpath.c_str()); PINFO ("logpath=%s", m_fullpath.empty() ? "(null)" : m_fullpath.c_str()); - /* And let's see if we can get a lock on it. */ + if (mode == SESSION_READ_ONLY) + return; // Read-only, don't care about locks. + + /* Set the lock file */ m_lockfile = m_fullpath + ".LCK"; - - if (!ignore_lock && !get_file_lock()) + auto locked = get_file_lock(); + if (mode == SESSION_BREAK_LOCK && !locked) { - // We should not ignore the lock, but couldn't get it. The - // be_get_file_lock() already set the appropriate backend_error in this - // case, so we just return here. - m_lockfile.clear(); - - if (force) - { - QofBackendError berror = get_error(); - if (berror == ERR_BACKEND_LOCKED || berror == ERR_BACKEND_READONLY) - { - // Even though we couldn't get the lock, we were told to force - // the opening. This is ok because the FORCE argument is - // changed only if the caller wants a read-only book. - } - else - { - // Unknown error. Push it again on the error stack. - set_error(berror); - return; - } - } + // Don't pass on locked or readonly errors. + QofBackendError berror = get_error(); + if (!(berror == ERR_BACKEND_LOCKED || berror == ERR_BACKEND_READONLY)) + set_error(berror); } - m_book = nullptr; } void diff --git a/libgnucash/backend/xml/gnc-xml-backend.hpp b/libgnucash/backend/xml/gnc-xml-backend.hpp index aeb04a3e00..362038452f 100644 --- a/libgnucash/backend/xml/gnc-xml-backend.hpp +++ b/libgnucash/backend/xml/gnc-xml-backend.hpp @@ -36,7 +36,7 @@ public: GncXmlBackend operator=(const GncXmlBackend&&) = delete; ~GncXmlBackend() = default; void session_begin(QofSession* session, const char* new_uri, - bool ignore_lock, bool create, bool force) override; + SessionOpenMode mode) override; void session_end() override; void load(QofBook* book, QofBackendLoadType loadType) override; /* The XML backend isn't able to do anything with individual instances. */ diff --git a/libgnucash/backend/xml/test/test-load-xml2.cpp b/libgnucash/backend/xml/test/test-load-xml2.cpp index 2daa106c22..f1e3cfdffc 100644 --- a/libgnucash/backend/xml/test/test-load-xml2.cpp +++ b/libgnucash/backend/xml/test/test-load-xml2.cpp @@ -97,7 +97,8 @@ test_load_file (const char* filename) ignore_lock = (g_strcmp0 (g_getenv ("SRCDIR"), ".") != 0); /* gnc_prefs_set_file_save_compressed(FALSE); */ - qof_session_begin (session, filename, ignore_lock, FALSE, TRUE); + qof_session_begin (session, filename, + ignore_lock ? SESSION_READ_ONLY : SESSION_NORMAL_OPEN); qof_session_load (session, NULL); auto book = qof_session_get_book (session); diff --git a/libgnucash/backend/xml/test/test-save-in-lang.cpp b/libgnucash/backend/xml/test/test-save-in-lang.cpp index e6a3919b64..8501a7c883 100644 --- a/libgnucash/backend/xml/test/test-save-in-lang.cpp +++ b/libgnucash/backend/xml/test/test-save-in-lang.cpp @@ -100,7 +100,7 @@ test_file (const char* filename) auto session = qof_session_new (nullptr); - qof_session_begin (session, filename, TRUE, FALSE, FALSE); + qof_session_begin (session, filename, SESSION_READ_ONLY); err = qof_session_pop_error (session); if (err) { @@ -121,7 +121,7 @@ test_file (const char* filename) auto new_session = qof_session_new (nullptr); - qof_session_begin (new_session, new_file, FALSE, FALSE, FALSE); + qof_session_begin (new_session, new_file, SESSION_NORMAL_OPEN); err = qof_session_pop_error (new_session); if (err) { diff --git a/libgnucash/engine/qof-backend.hpp b/libgnucash/engine/qof-backend.hpp index ee424937cc..f2e8985088 100644 --- a/libgnucash/engine/qof-backend.hpp +++ b/libgnucash/engine/qof-backend.hpp @@ -186,25 +186,10 @@ public: * Open the file or connect to the server. * @param session The QofSession that will control the backend. * @param new_uri The location of the data store that the backend will use. - * @param ignore_lock indicates whether the single-user lock on the backend - * should be cleared. The typical GUI sequence leading to this is: - * (1) GUI attempts to open the backend by calling this routine with - * ignore_lock false. - * (2) If backend error'ed BACKEND_LOCK, then GUI asks user what to do. - * (3) if user answers 'break & enter' then this routine is called again with - * ignore_lock true. - * @param create indicates whether this routine should create a new - * 'database', if it doesn't already exist. For example, for a file-backend, - * this would create the file, if it didn't already exist. For an SQL - * backend, this would create the database (the schema) if it didn't already - * exist. This flag is used to implement the 'SaveAs' GUI, where the user - * requests to save data to a new backend. - * - * @param force works with create to force creating a new database even if - * one already exists at the same URI. + * @param mode The session open mode. See qof_session_begin(). */ virtual void session_begin(QofSession *session, const char* new_uri, - bool ignore_lock, bool create, bool force) = 0; + SessionOpenMode mode) = 0; virtual void session_end() = 0; /** * Load the minimal set of application data needed for the application to be diff --git a/libgnucash/engine/qofsession.cpp b/libgnucash/engine/qofsession.cpp index 3371d84652..7e3ad94208 100644 --- a/libgnucash/engine/qofsession.cpp +++ b/libgnucash/engine/qofsession.cpp @@ -20,7 +20,7 @@ \********************************************************************/ /** - * @file qofsession.c + * @file qofsession.cpp * @brief Encapsulate a connection to a storage backend. * * HISTORY: @@ -250,11 +250,11 @@ QofSessionImpl::load (QofPercentageFunc percentage_func) noexcept } void -QofSessionImpl::begin (const char* new_uri, bool ignore_lock, - bool create, bool force) noexcept +QofSessionImpl::begin (const char* new_uri, SessionOpenMode mode) noexcept { - ENTER (" sess=%p ignore_lock=%d, book-id=%s", - this, ignore_lock, new_uri); + + + ENTER (" sess=%p mode=%d, URI=%s", this, mode, new_uri); clear_error (); /* Check to see if this session is already open */ if (m_uri.size ()) @@ -294,7 +294,7 @@ QofSessionImpl::begin (const char* new_uri, bool ignore_lock, destroy_backend (); /* Store the session URL */ m_uri = new_uri; - m_creating = create; + m_creating = mode == SESSION_NEW_STORE || mode == SESSION_NEW_OVERWRITE; if (filename) load_backend ("file"); else /* access method found, load appropriate backend */ @@ -314,8 +314,7 @@ QofSessionImpl::begin (const char* new_uri, bool ignore_lock, } /* If there's a begin method, call that. */ - m_backend->session_begin(this, m_uri.c_str(), - ignore_lock, create, force); + m_backend->session_begin(this, m_uri.c_str(), mode); PINFO ("Done running session_begin on backend"); QofBackendError const err {m_backend->get_error()}; auto msg (m_backend->get_message()); @@ -610,11 +609,10 @@ qof_session_get_backend (const QofSession *session) } void -qof_session_begin (QofSession *session, const char * new_uri, - gboolean ignore_lock, gboolean create, gboolean force) +qof_session_begin (QofSession *session, const char * uri, SessionOpenMode mode) { if (!session) return; - session->begin((new_uri ? new_uri : ""), ignore_lock, create, force); + session->begin(uri, mode); } void diff --git a/libgnucash/engine/qofsession.h b/libgnucash/engine/qofsession.h index 9611cc2a2e..5836d879c6 100644 --- a/libgnucash/engine/qofsession.h +++ b/libgnucash/engine/qofsession.h @@ -109,6 +109,27 @@ extern "C" #endif #define QOF_MOD_SESSION "qof.session" +/** + * Mode for opening sessions. + */ +/* This replaces three booleans that were passed in order: ignore_lock, create, + * and force. It's structured so that one can use it as a bit field with the + * values in the same order, i.e. ignore_lock = 1 << 2, create = 1 << 1, and + * force = 1. + */ +typedef enum +{ + SESSION_NORMAL_OPEN = 0, // All False + /** Open will fail if the URI doesn't exist or is locked. */ + SESSION_NEW_STORE = 2, // False, True, False (create) + /** Create a new store at the URI. It will fail if the store already exists and is found to contain data that would be overwritten. */ + SESSION_NEW_OVERWRITE = 3, // False, True, True (create | force) + /** Create a new store at the URI even if a store already exists there. */ + SESSION_READ_ONLY = 4, // True, False, False (ignore_lock) + /** Open the session read-only, ignoring any existing lock and not creating one if the URI isn't locked. */ + SESSION_BREAK_LOCK = 5 // True, False, True (ignore_lock | force) + /** Open the session, taking over any existing lock. */ +} SessionOpenMode; /* PROTOTYPES ******************************************************/ @@ -122,39 +143,41 @@ void qof_session_destroy (QofSession *session); * for 'Save As' type functionality. */ void qof_session_swap_data (QofSession *session_1, QofSession *session_2); -/** The qof_session_begin () method begins a new session. - * It takes as an argument the book id. The book id must be a string - * in the form of a URI/URL. The access method specified depends - * on the loaded backends. Paths may be relative or absolute. - * If the path is relative; that is, if the argument is "file://somefile.xml" - * then the current working directory is assumed. Customized backends can - * choose to search other, application-specific, directories as well. +/** Begins a new session. * - * The 'ignore_lock' argument, if set to TRUE, will cause this routine - * to ignore any global-datastore locks (e.g. file locks) that it finds. - * If set to FALSE, then file/database-global locks will be tested and - * obeyed. + * @param session Newly-allocated with qof_session_new. * - * If the datastore exists, can be reached (e.g over the net), - * connected to, opened and read, and a lock can be obtained then a - * lock will be obtained. Note that while multi-user datastores - * (e.g. the SQL backend) typically will have record-level locking - * and therefor should not need to get a global lock, qof works by - * having a local copy of the whole database and can't be trusted - * to handle multiple users writing data, so we lock the database - * anyway. + * @param uri must be a string in the form of a URI/URL. The access method + * specified depends on the loaded backends. Paths may be relative or + * absolute. If the path is relative, that is if the argument is + * "file://somefile.xml", then the current working directory is + * assumed. Customized backends can choose to search other + * application-specific directories or URI schemes as well. * - * If qof_session_begin is called with create == TRUE, then it will - * check for the existence of the file or database and return after - * posting a QOF_BACKEND_STORE_EXISTS error if it exists, unless - * force is also set to true. + * @param mode The SessionMode. * - * If an error occurs, it will be pushed onto the session error - * stack, and that is where it should be examined. + * ==== SessionMode ==== + * `SESSION_NORMAL`: Find an existing file or database at the provided uri and + * open it if it is unlocked. If it is locked post a QOF_BACKEND_LOCKED error. + * `SESSION_NEW_STORE`: Check for an existing file or database at the provided + * uri and if none is found, create it. If the file or database exists post a + * QOF_BACKED_STORE_EXISTS and return. + * `SESSION_READ_ONLY`: Find an existing file or database and open it without + * disturbing the lock if it exists or setting one if not. This will also set a + * flag on the book that will prevent many elements from being edited and will + * prevent the backend from saving any edits. + * `SESSION_OVERWRITE`: Create a new file or database at the provided uri, + * deleting any existing file or database. + * `SESSION_BREAK_LOCK1: Find an existing file or database, lock it, and open + * it. If there is already a lock replace it with a new one for this session. + * + * ==== Errors ==== + * This function signals failure by queuing errors. After it completes use + * qof_session_get_error() and test that the value is `ERROR_BACKEND_NONE` to + * determine that the session began successfully. */ void qof_session_begin (QofSession *session, const char * new_uri, - gboolean ignore_lock, gboolean create, - gboolean force); + SessionOpenMode mode); /** * The qof_session_load() method causes the QofBook to be made ready to diff --git a/libgnucash/engine/qofsession.hpp b/libgnucash/engine/qofsession.hpp index df4e02ab81..9029320c39 100644 --- a/libgnucash/engine/qofsession.hpp +++ b/libgnucash/engine/qofsession.hpp @@ -41,7 +41,7 @@ struct QofSessionImpl ~QofSessionImpl () noexcept; /** Begin this session. */ - void begin (const char* new_uri, bool ignore_lock, bool create, bool force) noexcept; + void begin (const char* new_uri, SessionOpenMode mode) noexcept; /** Swap books with another session */ void swap_books (QofSessionImpl &) noexcept; diff --git a/libgnucash/engine/test/test-qofinstance.cpp b/libgnucash/engine/test/test-qofinstance.cpp index 6e04b49bc1..301c7e2897 100644 --- a/libgnucash/engine/test/test-qofinstance.cpp +++ b/libgnucash/engine/test/test-qofinstance.cpp @@ -55,8 +55,8 @@ public: QofInstMockBackend() : m_qof_error{ERR_BACKEND_NO_ERR} { commit_test.m_be = this; } - void session_begin(QofSession* sess, const char* book_name, - bool ignore_lock, bool create, bool force) override {} + void session_begin(QofSession* sess, const char* uri, + SessionOpenMode mode) override {} void session_end() override {} void load(QofBook*, QofBackendLoadType) override {} void sync(QofBook* book) override {} diff --git a/libgnucash/engine/test/test-qofsession-old.cpp b/libgnucash/engine/test/test-qofsession-old.cpp index f10b64fa95..2282c0df50 100644 --- a/libgnucash/engine/test/test-qofsession-old.cpp +++ b/libgnucash/engine/test/test-qofsession-old.cpp @@ -287,7 +287,7 @@ static struct static void mock_session_begin (QofBackend *be, QofSession *session, const char *uri, - gboolean ignore_lock, gboolean create, gboolean force) + SessionOpenMode mode) { g_assert (be); g_assert (be == session_begin_struct.be); @@ -295,9 +295,7 @@ mock_session_begin (QofBackend *be, QofSession *session, const char *uri, g_assert (session == session_begin_struct.session); g_assert (uri); g_assert_cmpstr (uri, == , session_begin_struct.uri); - g_assert (ignore_lock); - g_assert (!create); - g_assert (force); + g_assert (mode == SESSION_BREAK_LOCK); if (session_begin_struct.produce_error) { qof_backend_set_error (be, ERR_BACKEND_DATA_CORRUPT); @@ -336,13 +334,10 @@ QofMockSessBackendProvider::create_backend (void) static void test_qof_session_begin (Fixture *fixture, gconstpointer pData) { - gboolean ignore_lock, create, force; QofBackend *be = NULL; /* setup */ - ignore_lock = TRUE; - create = FALSE; - force = TRUE; + SessionOpenMode mode{SESSION_BREAK_LOCK}; be = g_new0 (QofBackend, 1); g_assert (be); @@ -352,12 +347,12 @@ test_qof_session_begin (Fixture *fixture, gconstpointer pData) g_test_message ("Test when uri is set backend is not changed"); qof_book_set_backend (qof_session_get_book (fixture->session), be); p_qof_session_set_uri (fixture->session, "my book"); - qof_session_begin (fixture->session, "my book", ignore_lock, create, force); + qof_session_begin (fixture->session, "my book", mode); g_assert (qof_book_get_backend (qof_session_get_book (fixture->session)) == be); g_test_message ("Test when session uri is not set and uri passed is null backend is not changed"); p_qof_session_set_uri (fixture->session, NULL); - qof_session_begin (fixture->session, NULL, ignore_lock, create, force); + qof_session_begin (fixture->session, NULL, mode); g_assert (qof_book_get_backend (qof_session_get_book (fixture->session)) == be); g_test_message ("Test default access_method parsing"); @@ -365,13 +360,13 @@ test_qof_session_begin (Fixture *fixture, gconstpointer pData) * parse access_method as 'file' and try to find backend * as there is no backend registered error will be raised */ - qof_session_begin (fixture->session, "default_should_be_file", ignore_lock, create, force); + qof_session_begin (fixture->session, "default_should_be_file", mode); g_assert (qof_book_get_backend (qof_session_get_book (fixture->session)) == NULL); g_assert (!strlen (qof_session_get_url (fixture->session))); g_assert_cmpint (qof_session_get_error (fixture->session), == , ERR_BACKEND_NO_HANDLER); g_test_message ("Test access_method parsing"); - qof_session_begin (fixture->session, "postgres://localhost:8080", ignore_lock, create, force); + qof_session_begin (fixture->session, "postgres://localhost:8080", mode); g_assert (qof_book_get_backend (qof_session_get_book (fixture->session)) == NULL); g_assert (!strlen (qof_session_get_url (fixture->session))); g_assert_cmpint (qof_session_get_error (fixture->session), == , ERR_BACKEND_NO_HANDLER); @@ -386,7 +381,7 @@ test_qof_session_begin (Fixture *fixture, gconstpointer pData) "postgres")); qof_backend_register_provider (std::move(prov)); - qof_session_begin (fixture->session, "postgres://localhost:8080", ignore_lock, create, force); + qof_session_begin (fixture->session, "postgres://localhost:8080", mode); g_assert (qof_book_get_backend (qof_session_get_book (fixture->session))); g_assert (session_begin_struct.be == qof_book_get_backend (qof_session_get_book (fixture->session))); g_assert (session_begin_struct.backend_new_called == TRUE); diff --git a/libgnucash/engine/test/test-qofsession.cpp b/libgnucash/engine/test/test-qofsession.cpp index 0179f830df..72b701df87 100644 --- a/libgnucash/engine/test/test-qofsession.cpp +++ b/libgnucash/engine/test/test-qofsession.cpp @@ -44,7 +44,7 @@ public: QofSessionMockBackend(const QofSessionMockBackend&) = delete; QofSessionMockBackend(const QofSessionMockBackend&&) = delete; virtual ~QofSessionMockBackend() = default; - void session_begin(QofSession*, const char*, bool, bool, bool) {} + void session_begin(QofSession*, const char*, SessionOpenMode) {} void session_end() {} void load(QofBook*, QofBackendLoadType); void sync(QofBook*); @@ -106,9 +106,9 @@ TEST (QofSessionTest, swap_books) { qof_backend_register_provider (get_provider ()); QofSession s1(qof_book_new()); - s1.begin ("book1", false, false, false); + s1.begin ("book1", SESSION_NORMAL_OPEN); QofSession s2(qof_book_new()); - s2.begin ("book2", false, false, false); + s2.begin ("book2", SESSION_NORMAL_OPEN); QofBook * b1 {s1.get_book ()}; QofBook * b2 {s2.get_book ()}; ASSERT_NE (b1, b2); @@ -122,7 +122,7 @@ TEST (QofSessionTest, ensure_all_data_loaded) { qof_backend_register_provider (get_provider ()); QofSession s(qof_book_new()); - s.begin ("book1", false, false, false); + s.begin ("book1", SESSION_NORMAL_OPEN); data_loaded = false; s.ensure_all_data_loaded (); EXPECT_EQ (data_loaded, true); @@ -133,7 +133,7 @@ TEST (QofSessionTest, get_error) { qof_backend_register_provider (get_provider ()); QofSession s(qof_book_new()); - s.begin ("book1", false, false, false); + s.begin ("book1", SESSION_NORMAL_OPEN); s.ensure_all_data_loaded (); EXPECT_NE (s.get_error (), ERR_BACKEND_NO_ERR); //get_error should not clear the error. @@ -145,7 +145,7 @@ TEST (QofSessionTest, pop_error) { qof_backend_register_provider (get_provider ()); QofSession s(qof_book_new()); - s.begin ("book1", false, false, false); + s.begin ("book1", SESSION_NORMAL_OPEN); //We run the test first, and make sure there is an error condition. s.ensure_all_data_loaded (); EXPECT_NE (s.pop_error (), ERR_BACKEND_NO_ERR); @@ -157,7 +157,7 @@ TEST (QofSessionTest, clear_error) { qof_backend_register_provider (get_provider ()); QofSession s(qof_book_new()); - s.begin ("book1", false, false, false); + s.begin ("book1", SESSION_NORMAL_OPEN); //We run the test first, and make sure there is an error condition. s.ensure_all_data_loaded (); EXPECT_NE (s.get_error (), ERR_BACKEND_NO_ERR); @@ -176,7 +176,7 @@ TEST (QofSessionTest, load) */ qof_backend_register_provider (get_provider ()); QofSession s{qof_book_new()}; - s.begin ("book1", false, false, false); + s.begin ("book1", SESSION_NORMAL_OPEN); char *guidstr1 = guid_to_string(qof_instance_get_guid(s.get_book ())); s.load (nullptr); char *guidstr2 = guid_to_string(qof_instance_get_guid(s.get_book ())); @@ -206,7 +206,7 @@ TEST (QofSessionTest, save) { qof_backend_register_provider (get_provider ()); QofSession s(qof_book_new()); - s.begin ("book1", false, false, false); + s.begin ("book1", SESSION_NORMAL_OPEN); load_error = false; s.load (nullptr); qof_book_mark_session_dirty (s.get_book ()); @@ -221,7 +221,7 @@ TEST (QofSessionTest, safe_save) { qof_backend_register_provider (get_provider ()); QofSession s(qof_book_new()); - s.begin ("book1", false, false, false); + s.begin ("book1", SESSION_NORMAL_OPEN); s.safe_save (nullptr); EXPECT_EQ (safe_sync_called, true); qof_backend_unregister_all_providers (); @@ -233,11 +233,11 @@ TEST (QofSessionTest, export_session) qof_backend_register_provider (get_provider ()); auto b1 = qof_book_new(); QofSession s1(b1); - s1.begin ("book1", false, false, false); + s1.begin ("book1", SESSION_NORMAL_OPEN); qof_book_set_backend(b1, s1.get_backend()); auto b2 = qof_book_new(); QofSession s2(b2); - s2.begin ("book2", false, false, false); + s2.begin ("book2", SESSION_NORMAL_OPEN); qof_book_set_backend(b2, s2.get_backend()); s2.export_session (s1, nullptr); EXPECT_EQ (exported_book, b1); diff --git a/libgnucash/engine/test/utest-Transaction.cpp b/libgnucash/engine/test/utest-Transaction.cpp index 21b512282f..ac6effbe64 100644 --- a/libgnucash/engine/test/utest-Transaction.cpp +++ b/libgnucash/engine/test/utest-Transaction.cpp @@ -86,7 +86,7 @@ class TransMockBackend : public QofBackend public: TransMockBackend() : QofBackend(), m_last_call{"Constructor"}, m_result_err{ERR_BACKEND_NO_ERR} {} - void session_begin(QofSession*, const char*, bool, bool, bool) override { + void session_begin(QofSession*, const char*, SessionOpenMode) override { m_last_call = "session_begin"; } void session_end() override {