From 734fb6ce2a7d875491794d1e7d0605a1ef1ed033 Mon Sep 17 00:00:00 2001 From: John Ralls Date: Sat, 10 Sep 2022 16:18:32 -0700 Subject: [PATCH] [price-quotes] Switch error handling to exceptions. Allows for cleaner code with less state, less coupling of the GncQuotes class, and better transfer of error messages to client code. Also translates some error messages for presentation to users. --- gnucash/gnome-utils/dialog-transfer.cpp | 21 +++-- gnucash/gnome/dialog-price-edit-db.cpp | 20 ++-- gnucash/gnucash-commands.cpp | 29 ++---- gnucash/gnucash.cpp | 18 ++-- libgnucash/app-utils/CMakeLists.txt | 1 + libgnucash/app-utils/gnc-quotes.cpp | 111 +++++++++++++---------- libgnucash/app-utils/gnc-quotes.hpp | 8 +- libgnucash/app-utils/test/CMakeLists.txt | 1 + 8 files changed, 110 insertions(+), 99 deletions(-) diff --git a/gnucash/gnome-utils/dialog-transfer.cpp b/gnucash/gnome-utils/dialog-transfer.cpp index 9a818d5587..803c7ae9da 100644 --- a/gnucash/gnome-utils/dialog-transfer.cpp +++ b/gnucash/gnome-utils/dialog-transfer.cpp @@ -1785,18 +1785,19 @@ gnc_xfer_dialog_fetch (GtkButton *button, XferDialog *xferData) ENTER(" "); - GncQuotes quotes; - if (quotes.cmd_result() != 0) + try { - if (!quotes.error_msg().empty()) - PWARN ("%s", quotes.error_msg().c_str()); - LEAVE("quote retrieval failed"); - return; + GncQuotes quotes; + gnc_set_busy_cursor(nullptr, TRUE); + quotes.fetch(xferData->book); + gnc_unset_busy_cursor(nullptr); + } + catch (const GncQuoteException& err) + { + gnc_unset_busy_cursor(nullptr); + PERR("Price retrieval failed: %s", err.what()); + gnc_error_dialog(GTK_WINDOW(xferData->dialog), _("Price retrieval failed: %s"), err.what()); } - - gnc_set_busy_cursor (nullptr, TRUE); - quotes.fetch (xferData->book); - gnc_unset_busy_cursor (nullptr); /*the results should be in the price db now, but don't crash if not. */ PriceReq pr; diff --git a/gnucash/gnome/dialog-price-edit-db.cpp b/gnucash/gnome/dialog-price-edit-db.cpp index bec4fdb31e..81265dc34d 100644 --- a/gnucash/gnome/dialog-price-edit-db.cpp +++ b/gnucash/gnome/dialog-price-edit-db.cpp @@ -559,19 +559,19 @@ gnc_prices_dialog_get_quotes_clicked (GtkWidget *widget, gpointer data) auto pdb_dialog = static_cast (data); ENTER(" "); - GncQuotes quotes; - if (quotes.cmd_result() != 0) + try { + GncQuotes quotes; + gnc_set_busy_cursor (NULL, TRUE); + quotes.fetch (pdb_dialog->book); + gnc_unset_busy_cursor (NULL); + } + catch (const GncQuoteException& err) { - if (!quotes.error_msg().empty()) - PWARN ("%s", quotes.error_msg().c_str()); - LEAVE("quote retrieval failed"); - return; + gnc_unset_busy_cursor(nullptr); + PERR("Price retrieval failed: %s", err.what()); + gnc_error_dialog(GTK_WINDOW(pdb_dialog), _("Price retrieval failed: %s"), err.what()); } - gnc_set_busy_cursor (NULL, TRUE); - quotes.fetch (pdb_dialog->book); - gnc_unset_busy_cursor (NULL); - /* Without this, the summary bar on the accounts tab * won't reflect the new prices (bug #522095). */ gnc_gui_refresh_all (); diff --git a/gnucash/gnucash-commands.cpp b/gnucash/gnucash-commands.cpp index 145cd009c8..6eb6275179 100644 --- a/gnucash/gnucash-commands.cpp +++ b/gnucash/gnucash-commands.cpp @@ -303,9 +303,9 @@ int Gnucash::quotes_info (void) { gnc_prefs_init (); - GncQuotes quotes; - if (quotes.cmd_result() == 0) + try { + GncQuotes quotes; std::cout << bl::format (bl::translate ("Found Finance::Quote version {1}.")) % quotes.version() << "\n"; std::cout << bl::translate ("Finance::Quote sources: "); for (auto source : quotes.sources()) @@ -313,12 +313,9 @@ Gnucash::quotes_info (void) std::cout << std::endl; return 0; } - else + catch (const GncQuoteException& err) { - std::cerr << bl::translate ("Finance::Quote isn't " - "installed properly.") << "\n"; - std::cerr << bl::translate ("Error message:") << "\n"; - std::cerr << quotes.error_msg() << std::endl; + std::cout << err.what() << std::endl; return 1; } } @@ -341,32 +338,24 @@ Gnucash::add_quotes (const bo_str& uri) if (qof_session_get_error(session) != ERR_BACKEND_NO_ERR) cleanup_and_exit_with_failure (session); - GncQuotes quotes; - if (quotes.cmd_result() == 0) + try { + GncQuotes quotes; std::cout << bl::format (bl::translate ("Found Finance::Quote version {1}.")) % quotes.version() << std::endl; auto quote_sources = quotes.sources_as_glist(); gnc_quote_source_set_fq_installed (quotes.version().c_str(), quote_sources); g_list_free_full (quote_sources, g_free); + quotes.fetch(qof_session_get_book(session)); } - else + catch (const GncQuoteException& err) { - std::cerr << bl::translate ("No quotes retrieved. Finance::Quote isn't " - "installed properly.") << "\n"; - std::cerr << bl::translate ("Error message:") << std::endl; - std::cerr << quotes.error_msg() << std::endl; + std::cerr << err.what() << std::endl; } - quotes.fetch (qof_session_get_book(session)); - qof_session_save(session, NULL); if (qof_session_get_error(session) != ERR_BACKEND_NO_ERR) cleanup_and_exit_with_failure (session); qof_session_destroy(session); - - if (quotes.cmd_result() != 0) - std::cerr << bl::format (bl::translate ("Failed to add quotes to {1}.")) % *uri << "\n"; - qof_event_resume(); return 0; } diff --git a/gnucash/gnucash.cpp b/gnucash/gnucash.cpp index fe7513f1af..77ba65e68d 100644 --- a/gnucash/gnucash.cpp +++ b/gnucash/gnucash.cpp @@ -174,25 +174,25 @@ scm_run_gnucash (void *data, [[maybe_unused]] int argc, [[maybe_unused]] char ** gnc_hook_add_dangler(HOOK_UI_SHUTDOWN, (GFunc)gnc_file_quit, NULL, NULL); /* Install Price Quote Sources */ - auto msg = bl::translate ("Checking Finance::Quote...").str(gnc_get_boost_locale()); - GncQuotes quotes; - if (quotes.cmd_result() == 0) + try { - msg = (bl::format (bl::translate("Found Finance::Quote version {1}.")) % quotes.version()).str(gnc_get_boost_locale()); + auto msg = bl::translate ("Checking Finance::Quote...").str(gnc_get_boost_locale()); + GncQuotes quotes; + msg = (bl::format (bl::translate("Found Finance::Quote version {1}.")) % quotes.version()).str(gnc_get_boost_locale()); auto quote_sources = quotes.sources_as_glist(); gnc_quote_source_set_fq_installed (quotes.version().c_str(), quote_sources); g_list_free (quote_sources); + gnc_update_splash_screen (msg.c_str(), GNC_SPLASH_PERCENTAGE_UNKNOWN); } - else + catch (const GncQuoteException& err) { - msg = bl::translate("Unable to load Finance::Quote.").str(gnc_get_boost_locale()); + auto msg = bl::translate("Unable to load Finance::Quote.").str(gnc_get_boost_locale()); PINFO ("Attempt to load Finance::Quote returned this error message:\n"); - PINFO ("%s", quotes.error_msg().c_str()); + PINFO ("%s", err.what()); + gnc_update_splash_screen (msg.c_str(), GNC_SPLASH_PERCENTAGE_UNKNOWN); } - gnc_update_splash_screen (msg.c_str(), GNC_SPLASH_PERCENTAGE_UNKNOWN); - gnc_hook_run(HOOK_STARTUP, NULL); if (!user_file_spec->nofile && (fn = get_file_to_load (user_file_spec->file_to_load)) && *fn ) diff --git a/libgnucash/app-utils/CMakeLists.txt b/libgnucash/app-utils/CMakeLists.txt index 3192c5d582..ac9179994f 100644 --- a/libgnucash/app-utils/CMakeLists.txt +++ b/libgnucash/app-utils/CMakeLists.txt @@ -50,6 +50,7 @@ set(app_utils_ALL_LIBRARIES gnc-engine ${Boost_FILESYSTEM_LIBRARY} ${Boost_PROPERTY_TREE_LIBRARY} + ${Boost_LOCALE_LIBRARY} ${GIO_LDFLAGS} ${LIBXML2_LDFLAGS} ${LIBXSLT_LDFLAGS} diff --git a/libgnucash/app-utils/gnc-quotes.cpp b/libgnucash/app-utils/gnc-quotes.cpp index 6d595edf13..d65a89294c 100644 --- a/libgnucash/app-utils/gnc-quotes.cpp +++ b/libgnucash/app-utils/gnc-quotes.cpp @@ -23,6 +23,7 @@ #include #include +#include #include #include #include @@ -34,6 +35,7 @@ #include #include #include +#include #include #include #include "gnc-commodity.hpp" @@ -53,6 +55,7 @@ extern "C" { static const QofLogModule log_module = "gnc.price-quotes"; +namespace bl = boost::locale; namespace bp = boost::process; namespace bfs = boost::filesystem; namespace bpt = boost::property_tree; @@ -60,6 +63,11 @@ namespace bio = boost::iostreams; using QuoteResult = std::tuple; +struct GncQuoteSourceError : public std::runtime_error +{ + GncQuoteSourceError(const std::string& err) : std::runtime_error(err) {} +}; + CommVec gnc_quotes_get_quotable_commodities(const gnc_commodity_table * table); @@ -85,8 +93,6 @@ public: void fetch (CommVec& commodities); void fetch (gnc_commodity *comm); - int cmd_result() const noexcept { return m_cmd_result; } - const std::string& error_msg() noexcept { return m_error_msg; } const std::string& version() noexcept { return m_version.empty() ? not_found : m_version; } const QuoteSources& sources() noexcept { return m_sources; } GList* sources_as_glist (); @@ -101,8 +107,6 @@ private: CommVec m_comm_vec; std::string m_version; QuoteSources m_sources; - int m_cmd_result; - std::string m_error_msg; std::string m_fq_answer; QofBook *m_book; gnc_commodity *m_dflt_curr; @@ -138,21 +142,25 @@ m_version{}, m_sources{} auto [rv, sources, errors] = run_cmd(args, empty_string); if (rv) { - PERR("Failed to initialize Finance::Quote %s", errors.front().c_str()); - return; + std::string err{bl::translate("Failed to initialize Finance::Quote: ")}; + for (auto err_line : errors) + err += err_line.empty() ? "" : err_line + "\n"; + throw(GncQuoteSourceError(err)); } if (!errors.empty()) { - for(const auto& err : errors) - PERR("Finance::Quote check returned error %s", err.empty() ? "" : err.c_str()); - return; + std::string err{bl::translate("Finance::Quote check returned error ")}; + for(const auto& err_line : errors) + err += err.empty() ? "" : err_line + "\n"; + throw(GncQuoteSourceError(err)); } static const boost::regex version_fmt{"[0-9]\\.[0-9][0-9]"}; auto version{sources.front()}; if (version.empty() || !boost::regex_match(version, version_fmt)) { - PERR("Invalid Finance::Quote Version %s", version.empty() ? "" : version.c_str()); - return; + std::string err{bl::translate("Invalid Finance::Quote Version ")}; + err += version.empty() ? "" : version; + throw(GncQuoteSourceError(err)); } m_ready = true; sources.erase(sources.begin()); @@ -221,7 +229,7 @@ GncFQQuoteSource::run_cmd (const StrVec& args, const std::string& json_string) c /* GncQuotes implementation */ GncQuotesImpl::GncQuotesImpl() : m_quotesource{new GncFQQuoteSource}, -m_version{}, m_sources{}, m_cmd_result{}, m_error_msg{}, m_book{qof_session_get_book(gnc_get_current_session())}, +m_version{}, m_sources{}, m_book{qof_session_get_book(gnc_get_current_session())}, m_dflt_curr{gnc_default_currency()} { if (!m_quotesource->usable()) @@ -230,7 +238,7 @@ m_dflt_curr{gnc_default_currency()} } GncQuotesImpl::GncQuotesImpl(QofBook* book) : m_quotesource{new GncFQQuoteSource}, -m_version{}, m_sources{}, m_cmd_result{}, m_error_msg{}, m_book{book}, +m_version{}, m_sources{}, m_book{book}, m_dflt_curr{gnc_default_currency()} { if (!m_quotesource->usable()) @@ -240,8 +248,7 @@ m_dflt_curr{gnc_default_currency()} GncQuotesImpl::GncQuotesImpl(QofBook* book, std::unique_ptr quote_source) : m_quotesource{std::move(quote_source)}, -m_version{}, m_sources{}, m_cmd_result{}, m_error_msg{}, m_book{book}, -m_dflt_curr{gnc_default_currency()} +m_version{}, m_sources{}, m_book{book}, m_dflt_curr{gnc_default_currency()} { if (!m_quotesource->usable()) return; @@ -262,12 +269,7 @@ void GncQuotesImpl::fetch (QofBook *book) { if (!book) - { - m_cmd_result = 1; - m_error_msg = _("No book set"); - m_error_msg += "\n"; - return; - } + throw (GncQuoteException(bl::translate("GncQuotes::Fetch called with no book."))); auto commodities = gnc_quotes_get_quotable_commodities ( gnc_commodity_table_get_table (book)); fetch (commodities); @@ -290,8 +292,7 @@ GncQuotesImpl::fetch (CommVec& commodities) m_book = qof_instance_get_book (m_comm_vec[0]); query_fq (); - if (m_cmd_result == 0) - parse_quotes (); + parse_quotes (); } static const std::vector @@ -336,14 +337,19 @@ GncQuotesImpl::query_fq (void) auto json_str{comm_vec_to_json_string()}; auto [rv, quotes, errors] = m_quotesource->get_quotes(json_str); m_fq_answer.clear(); - m_cmd_result = rv; + if (rv == 0) + { for (auto line : quotes) m_fq_answer.append(line + "\n"); + } else - for (auto line : errors) - m_error_msg.append(line + "\n"); - + { + std::string err_str; + for (auto line: errors) + err_str.append(line + "\n"); + throw(GncQuoteException(err_str)); + } // for (auto line : quotes) // PINFO("Output line retrieved from wrapper:\n%s", line.c_str()); // @@ -546,24 +552,35 @@ GncQuotesImpl::parse_quotes (void) { bpt::ptree pt; std::istringstream ss {m_fq_answer}; + const char* what = nullptr; try { bpt::read_json (ss, pt); } catch (bpt::json_parser_error &e) { - m_cmd_result = -1; - m_error_msg = m_error_msg + - _("Failed to parse result returned by Finance::Quote.") + "\n" + - _("Error message:") + "\n" + - e.what() + "\n"; - return; + what = e.what(); + } + catch (const std::runtime_error& e) + { + what = e.what(); + } + catch (const std::logic_error& e) + { + what = e.what(); } catch (...) { - m_cmd_result = -1; - m_error_msg = m_error_msg + - _("Failed to parse result returned by Finance::Quote.") + "\n"; - return; + std::string error_msg{_("Failed to parse result returned by Finance::Quote.")}; + throw(GncQuoteException(error_msg)); + } + if (what) + { + std::string error_msg{_("Failed to parse result returned by Finance::Quote.")}; + error_msg += "\n"; + error_msg += _("Error message:"); + error_msg += "\n"; + error_msg += what; + throw(GncQuoteException(error_msg)); } auto pricedb{gnc_pricedb_get_db(m_book)}; @@ -672,9 +689,17 @@ gnc_quotes_get_quotable_commodities (const gnc_commodity_table * table) // Constructor - checks for presence of Finance::Quote and import version and quote sources GncQuotes::GncQuotes () { - m_impl = std::make_unique (); + try + { + m_impl = std::make_unique(); + } + catch (const GncQuoteSourceError& err) + { + throw(GncQuoteException(err.what())); + } } + void GncQuotes::fetch (QofBook *book) { @@ -691,16 +716,6 @@ void GncQuotes::fetch (gnc_commodity *comm) m_impl->fetch (comm); } -const int GncQuotes::cmd_result() noexcept -{ - return m_impl->cmd_result (); -} - -const std::string& GncQuotes::error_msg() noexcept -{ - return m_impl->error_msg (); -} - const std::string& GncQuotes::version() noexcept { return m_impl->version (); diff --git a/libgnucash/app-utils/gnc-quotes.hpp b/libgnucash/app-utils/gnc-quotes.hpp index 3c2ec0f5ce..e2968958a8 100644 --- a/libgnucash/app-utils/gnc-quotes.hpp +++ b/libgnucash/app-utils/gnc-quotes.hpp @@ -27,6 +27,7 @@ #include #include // For CommVec alias #include +#include extern "C" { #include @@ -36,6 +37,11 @@ using StrVec = std::vector ; using QuoteSources = StrVec; using CmdOutput = std::pair ; +struct GncQuoteException : public std::runtime_error +{ + GncQuoteException(const std::string& msg) : std::runtime_error(msg) {} +}; + const std::string not_found = std::string ("Not Found"); class GncQuotesImpl; @@ -54,8 +60,6 @@ public: // Fetch quote for the commodity if it has a quote source set void fetch (gnc_commodity *comm); - const int cmd_result() noexcept; - const std::string& error_msg() noexcept; const std::string& version() noexcept; const QuoteSources& sources() noexcept; GList* sources_as_glist (); diff --git a/libgnucash/app-utils/test/CMakeLists.txt b/libgnucash/app-utils/test/CMakeLists.txt index 799e655ad8..8616bb5f18 100644 --- a/libgnucash/app-utils/test/CMakeLists.txt +++ b/libgnucash/app-utils/test/CMakeLists.txt @@ -44,6 +44,7 @@ set(test_gnc_quotes_LIBS gnc-engine gtest ${Boost_FILESYSTEM_LIBRARY} + ${Boost_LOCALE_LIBRARY} ${Boost_PROPERTY_TREE_LIBRARY} ${Boost_SYSTEM_LIBRARY} )