From f9a4ab144a8e7f4d7da8edf1feb4200f23b19710 Mon Sep 17 00:00:00 2001 From: Kory Draughn Date: Sat, 16 Mar 2024 11:33:58 -0400 Subject: [PATCH] [#7443] Make copy_data_object() honor copy_options::none correctly. This commit makes it so that use of copy_options::none does not result in an exception being thrown. A unit test is included to make sure all values of the copy_options enumeration work as intended. --- .../include/irods/filesystem/copy_options.hpp | 20 +++--- lib/filesystem/src/filesystem.cpp | 10 +-- unit_tests/src/filesystem/test_filesystem.cpp | 71 +++++++++++++++++++ 3 files changed, 85 insertions(+), 16 deletions(-) diff --git a/lib/filesystem/include/irods/filesystem/copy_options.hpp b/lib/filesystem/include/irods/filesystem/copy_options.hpp index 048cdfef23..1ce6299b11 100644 --- a/lib/filesystem/include/irods/filesystem/copy_options.hpp +++ b/lib/filesystem/include/irods/filesystem/copy_options.hpp @@ -6,15 +6,17 @@ namespace irods::experimental::filesystem { - enum class copy_options - { - none = 0, - skip_existing = 1 << 0, - overwrite_existing = 1 << 1, - update_existing = 1 << 2, - recursive = 1 << 3, - collections_only = 1 << 4, - in_recursive_copy = 1 << 5 + enum class copy_options : std::uint32_t + { + // clang-format off + none = 0U, + skip_existing = 1U, + overwrite_existing = 2U, + update_existing = 4U, + recursive = 8U, + collections_only = 16U, + in_recursive_copy = 32U + // clang-format on }; inline auto operator|=(copy_options& _lhs, copy_options _rhs) noexcept -> copy_options& diff --git a/lib/filesystem/src/filesystem.cpp b/lib/filesystem/src/filesystem.cpp index 67bd6103e4..caeacc68b8 100644 --- a/lib/filesystem/src/filesystem.cpp +++ b/lib/filesystem/src/filesystem.cpp @@ -574,10 +574,6 @@ namespace irods::experimental::filesystem::NAMESPACE_IMPL throw filesystem_error{"path does not point to a data object", _to, make_error_code(INVALID_OBJECT_TYPE)}; } - if (copy_options::none == _options) { - throw filesystem_error{"copy options not set", make_error_code(SYS_INVALID_INPUT_PARAM)}; - } - if (copy_options::skip_existing == _options) { return false; } @@ -585,17 +581,17 @@ namespace irods::experimental::filesystem::NAMESPACE_IMPL if (copy_options::overwrite_existing == _options) { addKeyVal(&input.destDataObjInp.condInput, FORCE_FLAG_KW, ""); } - - if (copy_options::update_existing == _options) { + else if (copy_options::update_existing == _options) { if (last_write_time(_comm, _from) <= last_write_time(_comm, _to)) { return false; } + + addKeyVal(&input.destDataObjInp.condInput, FORCE_FLAG_KW, ""); } } std::strncpy(input.srcDataObjInp.objPath, _from.c_str(), std::strlen(_from.c_str())); std::strncpy(input.destDataObjInp.objPath, _to.c_str(), std::strlen(_to.c_str())); - addKeyVal(&input.destDataObjInp.condInput, DEST_RESC_NAME_KW, ""); if (const auto ec = rxDataObjCopy(&_comm, &input); ec < 0) { throw filesystem_error{"cannot copy data object", _from, _to, make_error_code(ec)}; diff --git a/unit_tests/src/filesystem/test_filesystem.cpp b/unit_tests/src/filesystem/test_filesystem.cpp index 2c6725214e..7f51801189 100644 --- a/unit_tests/src/filesystem/test_filesystem.cpp +++ b/unit_tests/src/filesystem/test_filesystem.cpp @@ -66,6 +66,7 @@ TEST_CASE("filesystem") namespace fs = irods::experimental::filesystem; // clang-format off + using idstream = irods::experimental::io::idstream; using odstream = irods::experimental::io::odstream; using default_transport = irods::experimental::io::client::default_transport; // clang-format on @@ -769,4 +770,74 @@ TEST_CASE("filesystem") CHECK(err.code().message() == "USER_FILE_DOES_NOT_EXIST"); CHECK(err.what() == msg + ": USER_FILE_DOES_NOT_EXIST"); } + + SECTION("copy_data_object") + { + constexpr std::string_view contents = "testing copy_data_object"; + const auto data_object = sandbox / "data_object.txt"; + + // Create a non-empty data object. + { + default_transport tp{conn}; + odstream{tp, data_object} << contents; + } + + REQUIRE(fs::client::is_data_object(conn, data_object)); + + // Create a copy of the data object using copy_options::none. + // The assertion that follows specifically proves #7443 is resolved. + const auto new_data_object = sandbox / "data_object.txt.copy"; + REQUIRE(fs::client::copy_data_object(conn, data_object, new_data_object, fs::copy_options::none)); + + // Show that attempting to copy over an existing data object using copy_options::none + // will result in an exception being thrown. + const auto matcher = Catch::Matchers::Message("cannot copy data object: OVERWRITE_WITHOUT_FORCE_FLAG"); + CHECK_THROWS_MATCHES(fs::client::copy_data_object(conn, data_object, new_data_object, fs::copy_options::none), + fs::filesystem_error, + matcher); + + // Show that the results are the same when the default copy_options are passed. + // The default is copy_options::none. + CHECK_THROWS_MATCHES( + fs::client::copy_data_object(conn, data_object, new_data_object), fs::filesystem_error, matcher); + + // Show the new data object contains the data. + { + default_transport tp{conn}; + idstream in{tp, new_data_object}; + + std::string line; + CHECK(std::getline(in, line)); + CHECK(contents == line); + } + + // Capture the mtime of the new data object. + // This will be used to assert correctness of copy_options::skip_existing. + auto old_mtime = fs::client::last_write_time(conn, new_data_object); + + // Guarantee a difference in the mtime is noticeable if a change occurs. + using namespace std::chrono_literals; + std::this_thread::sleep_for(2s); + + // Show copy_options::skip_existing works as intended. + // That is, if the target data object already exists, do nothing. + CHECK_FALSE(fs::client::copy_data_object(conn, data_object, new_data_object, fs::copy_options::skip_existing)); + CHECK(old_mtime == fs::client::last_write_time(conn, new_data_object)); + + // Show that copy_options::overwrite_existing works as intended. + // That is, overwrite the contents of the target data object. + // This will result in the mtime of the target data object being updated. + CHECK(fs::client::copy_data_object(conn, data_object, new_data_object, fs::copy_options::overwrite_existing)); + CHECK(old_mtime < fs::client::last_write_time(conn, new_data_object)); + + // Capture the mtime of the very first data object. + // This will be used to assert correctness of copy_options::update_existing. + old_mtime = fs::client::last_write_time(conn, data_object); + + // Show that copy_options::update_existing works as intended. + // That is, only overwrite the contents of the target data object if the source + // data object is newer than the target data object, as defined by last_write_time(). + CHECK(fs::client::copy_data_object(conn, new_data_object, data_object, fs::copy_options::update_existing)); + CHECK(old_mtime < fs::client::last_write_time(conn, data_object)); + } }