From 439f9cce3df547a6f0bbb6c4210bc5b03d94dba8 Mon Sep 17 00:00:00 2001 From: "Scott E. Graves" Date: Sat, 28 Sep 2024 17:28:47 -0500 Subject: [PATCH] [bug] Rename file is broken for files that are existing #19 --- .../include/file_manager/file_manager.hpp | 6 +- .../librepertory/include/types/repertory.hpp | 3 +- .../src/file_manager/file_manager.cpp | 32 ++--- .../librepertory/src/types/repertory.cpp | 15 +- .../src/file_manager_upload_test.cpp | 129 +++++++++--------- 5 files changed, 87 insertions(+), 98 deletions(-) diff --git a/repertory/librepertory/include/file_manager/file_manager.hpp b/repertory/librepertory/include/file_manager/file_manager.hpp index fac90ff3..38f23994 100644 --- a/repertory/librepertory/include/file_manager/file_manager.hpp +++ b/repertory/librepertory/include/file_manager/file_manager.hpp @@ -409,10 +409,10 @@ public: i_provider &provider_; private: - bool cancelled_ = false; - api_error error_ = api_error::success; + bool cancelled_{false}; + api_error error_{api_error::success}; std::unique_ptr thread_; - stop_type stop_requested_ = false; + stop_type stop_requested_{false}; private: void upload_thread(); diff --git a/repertory/librepertory/include/types/repertory.hpp b/repertory/librepertory/include/types/repertory.hpp index 8ec7f1a3..5b859349 100644 --- a/repertory/librepertory/include/types/repertory.hpp +++ b/repertory/librepertory/include/types/repertory.hpp @@ -86,7 +86,6 @@ enum class api_error { out_of_memory, permission_denied, upload_failed, - upload_stopped, xattr_buffer_small, xattr_exists, xattr_not_found, @@ -94,7 +93,7 @@ enum class api_error { ERROR_COUNT }; -[[nodiscard]] auto api_error_from_string(std::string_view s) -> api_error; +[[nodiscard]] auto api_error_from_string(std::string_view str) -> api_error; [[nodiscard]] auto api_error_to_string(const api_error &error) -> const std::string &; diff --git a/repertory/librepertory/src/file_manager/file_manager.cpp b/repertory/librepertory/src/file_manager/file_manager.cpp index 84089223..18252679 100644 --- a/repertory/librepertory/src/file_manager/file_manager.cpp +++ b/repertory/librepertory/src/file_manager/file_manager.cpp @@ -416,8 +416,6 @@ auto file_manager::handle_file_rename(const std::string &from_api_path, return ret; } - swap_renamed_items(from_api_path, to_api_path); - if (should_upload) { queue_upload(to_api_path, source_path, false); } @@ -603,16 +601,22 @@ void file_manager::remove_upload(const std::string &api_path, bool no_lock) { lock = std::make_unique(upload_mtx_); } - auto result = db::db_select{*db_.get(), upload_active_table} + auto result = db::db_select{*db_.get(), upload_table} .delete_query() .where("api_path") .equals(api_path) .go(); - auto file_iter = upload_lookup_.find(api_path); - if (file_iter != upload_lookup_.end()) { - file_iter->second->cancel(); + result = db::db_select{*db_.get(), upload_active_table} + .delete_query() + .where("api_path") + .equals(api_path) + .go(); + + if (upload_lookup_.find(api_path) != upload_lookup_.end()) { + auto ptr = std::move(upload_lookup_.at(api_path)); upload_lookup_.erase(api_path); + ptr->cancel(); } if (result.ok()) { @@ -974,8 +978,7 @@ void file_manager::upload_completed(const file_upload_completed &evt) { if (not utils::string::to_bool(evt.get_cancelled().get())) { auto err = api_error_from_string(evt.get_result().get()); - switch (err) { - case api_error::success: { + if (err == api_error::success) { auto result = db::db_select{*db_.get(), upload_active_table} .delete_query() .where("api_path") @@ -987,16 +990,7 @@ void file_manager::upload_completed(const file_upload_completed &evt) { evt.get_source().get(), "failed to remove from to upload_active table"); } - } break; - - case api_error::upload_stopped: { - event_system::instance().raise(evt.get_api_path(), - evt.get_source(), err); - queue_upload(evt.get_api_path(), evt.get_source(), true); - upload_notify_.wait_for(upload_lock, 5s); - } break; - - default: { + } else { bool exists{}; auto res = provider_.is_file(evt.get_api_path(), exists); if ((res == api_error::success && not exists) || @@ -1012,7 +1006,6 @@ void file_manager::upload_completed(const file_upload_completed &evt) { evt.get_source(), err); queue_upload(evt.get_api_path(), evt.get_source(), true); upload_notify_.wait_for(upload_lock, 5s); - } break; } upload_lookup_.erase(evt.get_api_path()); @@ -1051,7 +1044,6 @@ void file_manager::upload_handler() { switch (res) { case api_error::item_not_found: { should_wait = false; - // event_system::instance().raise(api_path, source_path); remove_upload(api_path, true); diff --git a/repertory/librepertory/src/types/repertory.cpp b/repertory/librepertory/src/types/repertory.cpp index b63ddcb3..df5332f6 100644 --- a/repertory/librepertory/src/types/repertory.cpp +++ b/repertory/librepertory/src/types/repertory.cpp @@ -91,24 +91,23 @@ static const std::unordered_map LOOKUP = { {api_error::out_of_memory, "out_of_memory"}, {api_error::permission_denied, "permission_denied"}, {api_error::upload_failed, "upload_failed"}, - {api_error::upload_stopped, "upload_stopped"}, {api_error::xattr_buffer_small, "xattr_buffer_small"}, {api_error::xattr_exists, "xattr_exists"}, {api_error::xattr_not_found, "xattr_not_found"}, {api_error::xattr_too_big, "xattr_too_big"}, }; -auto api_error_from_string(std::string_view s) -> api_error { +auto api_error_from_string(std::string_view str) -> api_error { if (LOOKUP.size() != static_cast(api_error::ERROR_COUNT)) { throw startup_exception("undefined api_error strings"); } - const auto it = - std::find_if(LOOKUP.begin(), LOOKUP.end(), - [&s](const std::pair &kv) -> bool { - return kv.second == s; - }); - return it == LOOKUP.end() ? api_error::error : it->first; + const auto iter = std::find_if( + LOOKUP.begin(), LOOKUP.end(), + [&str](const std::pair &item) -> bool { + return item.second == str; + }); + return iter == LOOKUP.end() ? api_error::error : iter->first; } auto api_error_to_string(const api_error &error) -> const std::string & { diff --git a/repertory/repertory_test/src/file_manager_upload_test.cpp b/repertory/repertory_test/src/file_manager_upload_test.cpp index b55af6ec..f52d81f6 100644 --- a/repertory/repertory_test/src/file_manager_upload_test.cpp +++ b/repertory/repertory_test/src/file_manager_upload_test.cpp @@ -23,49 +23,47 @@ #include "file_manager/file_manager.hpp" #include "mocks/mock_provider.hpp" -#include "mocks/mock_upload_manager.hpp" #include "utils/event_capture.hpp" -#include "utils/path.hpp" namespace repertory { -static constexpr const std::size_t test_chunk_size = 1024u; +static constexpr const std::size_t test_chunk_size{1024U}; TEST(upload, can_upload_a_valid_file) { - console_consumer c; + console_consumer con; event_system::instance().start(); const auto source_path = test::generate_test_file_name("upload_test"); - mock_provider mp; + mock_provider mock_prov; - EXPECT_CALL(mp, is_direct_only()).WillRepeatedly(Return(false)); + EXPECT_CALL(mock_prov, is_direct_only()).WillRepeatedly(Return(false)); filesystem_item fsi; fsi.api_path = "/test.txt"; - fsi.size = test_chunk_size * 4u; + fsi.size = test_chunk_size * 4U; fsi.source_path = source_path; - event_consumer ec("file_upload_completed", [&fsi](const event &e) { - const auto &ee = dynamic_cast(e); + event_consumer evt_com("file_upload_completed", [&fsi](const event &evt) { + const auto &comp_evt = dynamic_cast(evt); EXPECT_STREQ(fsi.api_path.c_str(), - ee.get_api_path().get().c_str()); + comp_evt.get_api_path().get().c_str()); EXPECT_STREQ(fsi.source_path.c_str(), - ee.get_source().get().c_str()); - EXPECT_STREQ("success", ee.get_result().get().c_str()); - EXPECT_STREQ("0", ee.get_cancelled().get().c_str()); + comp_evt.get_source().get().c_str()); + EXPECT_STREQ("success", comp_evt.get_result().get().c_str()); + EXPECT_STREQ("0", comp_evt.get_cancelled().get().c_str()); }); - EXPECT_CALL(mp, upload_file(fsi.api_path, fsi.source_path, _)) - .WillOnce([&fsi](const std::string &, const std::string &, - stop_type &stop_requested) -> api_error { + EXPECT_CALL(mock_prov, upload_file(fsi.api_path, fsi.source_path, _)) + .WillOnce([](const std::string &, const std::string &, + stop_type &stop_requested) -> api_error { EXPECT_FALSE(stop_requested); return api_error::success; }); - file_manager::upload upload(fsi, mp); + file_manager::upload upload(fsi, mock_prov); - event_capture e({"file_upload_completed"}); - e.wait_for_empty(); + event_capture evt_cap({"file_upload_completed"}); + evt_cap.wait_for_empty(); EXPECT_EQ(api_error::success, upload.get_api_error()); EXPECT_FALSE(upload.is_cancelled()); @@ -74,109 +72,110 @@ TEST(upload, can_upload_a_valid_file) { } TEST(upload, can_cancel_upload) { - console_consumer c; + console_consumer con; event_system::instance().start(); const auto source_path = test::generate_test_file_name("upload_test"); - mock_provider mp; + mock_provider mock_provider; - EXPECT_CALL(mp, is_direct_only()).WillRepeatedly(Return(false)); + EXPECT_CALL(mock_provider, is_direct_only()).WillRepeatedly(Return(false)); filesystem_item fsi; fsi.api_path = "/test.txt"; - fsi.size = test_chunk_size * 4u; + fsi.size = test_chunk_size * 4U; fsi.source_path = source_path; - event_consumer ec("file_upload_completed", [&fsi](const event &e) { - const auto &ee = dynamic_cast(e); + event_consumer evt_con("file_upload_completed", [&fsi](const event &evt) { + const auto &comp_evt = dynamic_cast(evt); EXPECT_STREQ(fsi.api_path.c_str(), - ee.get_api_path().get().c_str()); + comp_evt.get_api_path().get().c_str()); EXPECT_STREQ(fsi.source_path.c_str(), - ee.get_source().get().c_str()); - EXPECT_STREQ("upload_stopped", ee.get_result().get().c_str()); - EXPECT_STREQ("1", ee.get_cancelled().get().c_str()); + comp_evt.get_source().get().c_str()); + EXPECT_STREQ("comm_error", + comp_evt.get_result().get().c_str()); + EXPECT_STREQ("1", comp_evt.get_cancelled().get().c_str()); }); std::mutex mtx; - std::condition_variable cv; + std::condition_variable notify; - EXPECT_CALL(mp, upload_file(fsi.api_path, fsi.source_path, _)) - .WillOnce([&cv, &fsi, &mtx](const std::string &, const std::string &, - stop_type &stop_requested) -> api_error { + EXPECT_CALL(mock_provider, upload_file(fsi.api_path, fsi.source_path, _)) + .WillOnce([¬ify, &mtx](const std::string &, const std::string &, + stop_type &stop_requested) -> api_error { EXPECT_FALSE(stop_requested); - unique_mutex_lock l(mtx); - cv.notify_one(); - l.unlock(); + unique_mutex_lock lock(mtx); + notify.notify_one(); + lock.unlock(); - l.lock(); - cv.wait(l); - l.unlock(); + lock.lock(); + notify.wait(lock); + lock.unlock(); EXPECT_TRUE(stop_requested); - return api_error::upload_stopped; + return api_error::comm_error; }); - unique_mutex_lock l(mtx); - file_manager::upload upload(fsi, mp); - cv.wait(l); + unique_mutex_lock lock(mtx); + file_manager::upload upload(fsi, mock_provider); + notify.wait(lock); upload.cancel(); - cv.notify_one(); - l.unlock(); + notify.notify_one(); + lock.unlock(); - event_capture e({"file_upload_completed"}); - e.wait_for_empty(); + event_capture evt_cap({"file_upload_completed"}); + evt_cap.wait_for_empty(); - EXPECT_EQ(api_error::upload_stopped, upload.get_api_error()); + EXPECT_EQ(api_error::comm_error, upload.get_api_error()); EXPECT_TRUE(upload.is_cancelled()); event_system::instance().stop(); } TEST(upload, can_stop_upload) { - console_consumer c; + console_consumer con; event_system::instance().start(); const auto source_path = test::generate_test_file_name("upload_test"); - mock_provider mp; + mock_provider mock_provider; - EXPECT_CALL(mp, is_direct_only()).WillRepeatedly(Return(false)); + EXPECT_CALL(mock_provider, is_direct_only()).WillRepeatedly(Return(false)); filesystem_item fsi; fsi.api_path = "/test.txt"; - fsi.size = test_chunk_size * 4u; + fsi.size = test_chunk_size * 4U; fsi.source_path = source_path; - event_consumer ec("file_upload_completed", [&fsi](const event &e) { - const auto &ee = dynamic_cast(e); + event_consumer evt_con("file_upload_completed", [&fsi](const event &evt) { + const auto &evt_com = dynamic_cast(evt); EXPECT_STREQ(fsi.api_path.c_str(), - ee.get_api_path().get().c_str()); + evt_com.get_api_path().get().c_str()); EXPECT_STREQ(fsi.source_path.c_str(), - ee.get_source().get().c_str()); - EXPECT_STREQ("upload_stopped", ee.get_result().get().c_str()); - EXPECT_STREQ("0", ee.get_cancelled().get().c_str()); + evt_com.get_source().get().c_str()); + EXPECT_STREQ("comm_error", evt_com.get_result().get().c_str()); + EXPECT_STREQ("0", evt_com.get_cancelled().get().c_str()); }); - EXPECT_CALL(mp, upload_file(fsi.api_path, fsi.source_path, _)) - .WillOnce([&fsi](const std::string &, const std::string &, - stop_type &stop_requested) -> api_error { + EXPECT_CALL(mock_provider, upload_file(fsi.api_path, fsi.source_path, _)) + .WillOnce([](const std::string &, const std::string &, + stop_type &stop_requested) -> api_error { std::this_thread::sleep_for(3s); EXPECT_TRUE(stop_requested); - return api_error::upload_stopped; + return api_error::comm_error; }); - event_capture e({"file_upload_completed"}); + event_capture evt_cap({"file_upload_completed"}); - { file_manager::upload upload(fsi, mp); } + { file_manager::upload upload(fsi, mock_provider); } - e.wait_for_empty(); + evt_cap.wait_for_empty(); event_system::instance().stop(); }