diff --git a/CHANGELOG.md b/CHANGELOG.md index 939466dd..5161633a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ ### Changes from v2.0.7-release * Added check version support to remote mounts +* Fixed directory item count bug on S3 provider * Fixed handling of `FALLOC_FL_KEEP_SIZE` on Linux * Fixed intermittent client hang on remote mount server disconnect * Implemented POSIX-compliant `unlink()` with FUSE `hard_remove` diff --git a/repertory/librepertory/src/drives/fuse/fuse_drive.cpp b/repertory/librepertory/src/drives/fuse/fuse_drive.cpp index 4ace5b1d..0455bb45 100644 --- a/repertory/librepertory/src/drives/fuse/fuse_drive.cpp +++ b/repertory/librepertory/src/drives/fuse/fuse_drive.cpp @@ -926,6 +926,9 @@ auto fuse_drive::rename_impl(std::string from_api_path, std::string to_api_path) } auto fuse_drive::rmdir_impl(std::string api_path) -> api_error { + REPERTORY_USES_FUNCTION_NAME(); + utils::error::handle_info(function_name, "called"); + auto res = check_parent_access(api_path, W_OK | X_OK); if (res != api_error::success) { return res; diff --git a/repertory/librepertory/src/file_manager/file_manager.cpp b/repertory/librepertory/src/file_manager/file_manager.cpp index 55376260..93b25505 100644 --- a/repertory/librepertory/src/file_manager/file_manager.cpp +++ b/repertory/librepertory/src/file_manager/file_manager.cpp @@ -696,6 +696,10 @@ void file_manager::queue_upload(const std::string &api_path, auto file_manager::remove_directory(const std::string &api_path) -> api_error { REPERTORY_USES_FUNCTION_NAME(); + if (api_path == "/") { + return api_error::permission_denied; + } + unique_recur_mutex_lock open_lock(open_file_mtx_); if (provider_.get_directory_item_count(api_path) != 0) { return api_error::directory_not_empty; diff --git a/repertory/librepertory/src/providers/s3/s3_provider.cpp b/repertory/librepertory/src/providers/s3/s3_provider.cpp index 5198578e..c601aff2 100644 --- a/repertory/librepertory/src/providers/s3/s3_provider.cpp +++ b/repertory/librepertory/src/providers/s3/s3_provider.cpp @@ -275,31 +275,34 @@ auto s3_provider::get_directory_item_count(const std::string &api_path) const try { const auto &cfg{get_s3_config()}; - auto is_encrypted{not cfg.encryption_token.empty()}; + const auto is_encrypted{!cfg.encryption_token.empty()}; std::string key; if (is_encrypted) { - auto res{get_item_meta(api_path, META_KEY, key)}; - if (res != api_error::success) { + if (get_item_meta(api_path, META_KEY, key) != api_error::success) { return 0U; } } - auto object_name{ - api_path == "/" - ? "" - : utils::path::create_api_path(is_encrypted ? key : api_path), - }; + auto object_name = + (api_path == "/") + ? std::string{} + : utils::path::create_api_path(is_encrypted ? key : api_path); - std::string response_data{}; - long response_code{}; - auto prefix{object_name.empty() ? object_name : object_name + "/"}; + auto prefix = object_name.empty() ? std::string{} : object_name + "/"; - auto grab_more{true}; + std::unordered_set seen_prefixes; + std::unordered_set seen_keys; + + bool grab_more{true}; std::string token{}; - std::uint64_t total_count{}; + std::uint64_t total_count{0}; + while (grab_more) { - if (not get_object_list(response_data, response_code, "/", "", token)) { + long response_code{}; + std::string response_data{}; + if (not get_object_list(response_data, response_code, "/", prefix, + token)) { return total_count; } @@ -312,8 +315,8 @@ auto s3_provider::get_directory_item_count(const std::string &api_path) const } pugi::xml_document doc; - auto res{doc.load_string(response_data.c_str())}; - if (res.status != pugi::xml_parse_status::status_ok) { + auto parsed = doc.load_string(response_data.c_str()); + if (parsed.status != pugi::xml_parse_status::status_ok) { return total_count; } @@ -328,15 +331,40 @@ auto s3_provider::get_directory_item_count(const std::string &api_path) const .as_string(); } - auto node_list{ - doc.select_nodes("/ListBucketResult/CommonPrefixes/Prefix"), - }; - total_count += node_list.size(); + for (auto const &node : + doc.select_nodes("/ListBucketResult/CommonPrefixes/Prefix")) { + std::string cur_prefix = node.node().text().as_string(); + if (not cur_prefix.empty() && not seen_prefixes.contains(cur_prefix)) { + seen_prefixes.insert(cur_prefix); + ++total_count; + } + } - node_list = doc.select_nodes("/ListBucketResult/Contents"); - total_count += node_list.size(); - if (not prefix.empty()) { - --total_count; + for (auto const &node : doc.select_nodes("/ListBucketResult/Contents")) { + std::string cur_key = node.node().child("Key").text().as_string(); + if (cur_key.empty()) { + continue; + } + + if (not prefix.empty() && // and + (cur_key == prefix || not cur_key.starts_with(prefix))) { + continue; + } + + if (cur_key.back() == '/') { + continue; + } + + if (seen_prefixes.contains(cur_key + "/")) { + continue; + } + + if (seen_keys.contains(cur_key)) { + continue; + } + + seen_keys.insert(cur_key); + ++total_count; } } @@ -356,7 +384,7 @@ auto s3_provider::get_directory_items_impl(const std::string &api_path, const auto &cfg{get_s3_config()}; auto is_encrypted{not cfg.encryption_token.empty()}; - const auto add_diretory_item = + const auto add_directory_item = [this, &is_encrypted, &list](const std::string &child_object_name, bool directory, auto node) -> api_error { auto res{api_error::success}; @@ -442,9 +470,13 @@ auto s3_provider::get_directory_items_impl(const std::string &api_path, auto grab_more{true}; std::string token{}; + std::unordered_set seen_prefixes; + std::unordered_set seen_keys; + auto api_path_prefix = utils::path::create_api_path(prefix); + while (grab_more) { - std::string response_data{}; long response_code{}; + std::string response_data{}; if (not get_object_list(response_data, response_code, "/", prefix, token)) { return api_error::comm_error; } @@ -476,31 +508,56 @@ auto s3_provider::get_directory_items_impl(const std::string &api_path, .as_string(); } - auto node_list{ - doc.select_nodes("/ListBucketResult/CommonPrefixes/Prefix"), - }; - for (const auto &node : node_list) { - auto child_object_name{ - utils::path::create_api_path( - utils::path::combine("/", {node.node().text().as_string()})), - }; - auto res{add_diretory_item(child_object_name, true, node.node())}; + for (const auto &node : + doc.select_nodes("/ListBucketResult/CommonPrefixes/Prefix")) { + std::string cur_prefix = node.node().text().as_string(); + if (cur_prefix.empty()) { + continue; + } + + if (not prefix.empty() && not cur_prefix.starts_with(prefix)) { + continue; + } + + if (not seen_prefixes.insert(cur_prefix).second) { + continue; + } + + auto child_object_name = + utils::path::create_api_path(utils::path::combine("/", {cur_prefix})); + auto res = add_directory_item(child_object_name, true, node.node()); if (res != api_error::success) { return res; } } - node_list = doc.select_nodes("/ListBucketResult/Contents"); - for (const auto &node : node_list) { - auto child_object_name{ - utils::path::create_api_path( - node.node().select_node("Key").node().text().as_string()), - }; - if (child_object_name == utils::path::create_api_path(prefix)) { + for (const auto &node : doc.select_nodes("/ListBucketResult/Contents")) { + std::string cur_key = node.node().child("Key").text().as_string(); + if (cur_key.empty()) { continue; } - auto res{add_diretory_item(child_object_name, false, node.node())}; + if (not prefix.empty() && // and + (cur_key == prefix || // or + not cur_key.starts_with(prefix) || // or + utils::path::create_api_path(cur_key) == api_path_prefix)) { + continue; + } + + if (cur_key.back() == '/') { + continue; + } + + if (seen_prefixes.contains(cur_key + "/")) { + continue; + } + + if (not seen_keys.insert(cur_key).second) { + continue; + } + + auto child_object_name = utils::path::create_api_path(cur_key); + auto res = add_directory_item(child_object_name, false, node.node()); if (res != api_error::success) { return res; } diff --git a/repertory/repertory_test/src/fuse_drive_directory_test.cpp b/repertory/repertory_test/src/fuse_drive_directory_test.cpp index 9c8bcb1d..7f87a726 100644 --- a/repertory/repertory_test/src/fuse_drive_directory_test.cpp +++ b/repertory/repertory_test/src/fuse_drive_directory_test.cpp @@ -123,15 +123,15 @@ TYPED_TEST(fuse_test, directory_rmdir_on_non_empty_directory_should_fail) { std::string dir_name{"non_empty"}; auto dir = this->create_directory_and_test(dir_name); - std::string fname{dir_name + "/child"}; - auto fpath = this->create_file_and_test(fname, 0644); - this->overwrite_text(fpath, "X"); + std::string name{dir_name + "/child"}; + auto file = this->create_file_and_test(name, 0644); + this->overwrite_text(file, "X"); errno = 0; EXPECT_EQ(-1, ::rmdir(dir.c_str())); EXPECT_EQ(ENOTEMPTY, errno); - this->unlink_file_and_test(fpath); + this->unlink_file_and_test(file); this->rmdir_and_test(dir); } } // namespace repertory