diff --git a/lib/api/include/irods/delay_rule_unlock.h b/lib/api/include/irods/delay_rule_unlock.h index e6d264ae9c..60b349c636 100644 --- a/lib/api/include/irods/delay_rule_unlock.h +++ b/lib/api/include/irods/delay_rule_unlock.h @@ -12,12 +12,14 @@ struct RcComm; /// \since 5.0.0 typedef struct DelayRuleUnlockInput { - /// The catalog ID of the delay rule to unlock. + /// The list of delay rule IDs to unlock. /// - /// Must be a non-empty string. + /// The following requirements must be satisfied: + /// - It must be non-empty + /// - It must be a JSON list /// /// \since 5.0.0 - char rule_id[32]; // NOLINT(cppcoreguidelines-avoid-c-arrays, modernize-avoid-c-arrays) + char* rule_ids; // NOLINT(cppcoreguidelines-avoid-c-arrays, modernize-avoid-c-arrays) /// The set of key-value pair strings used to influence the behavior of the API /// operation. @@ -27,27 +29,28 @@ typedef struct DelayRuleUnlockInput } delayRuleUnlockInp_t; // NOLINTNEXTLINE(cppcoreguidelines-macro-usage) -#define DelayRuleUnlockInput_PI "str rule_id[32]; struct KeyValPair_PI;" +#define DelayRuleUnlockInput_PI "str *rule_ids; struct KeyValPair_PI;" #ifdef __cplusplus extern "C" { #endif -/// Removes the delay server lock information from a delay rule if it exists. +/// Atomically removes the delay server lock information from one or more delay rules. /// /// Requires \p rodsadmin level privileges. /// -/// On success, the following information will be removed from the target delay rule's catalog -/// entry: +/// On success, the following information will be removed from the delay rule catalog entries: /// - The host identifying the delay server /// - The PID of the delay server /// - The time the entry was locked /// +/// On failure, all catalog updates are rolled back and an error code is returned to the client. +/// /// \param[in] _comm A pointer to a RcComm. /// \param[in] _input A pointer to a DelayRuleUnlockInput. /// /// \return An integer. -/// \retval 0 On success. +/// \retval 0 On success. /// \retval <0 On failure. /// /// \b Example @@ -58,7 +61,9 @@ extern "C" { /// struct DelayRuleUnlockInput input; /// memset(&input, 0, sizeof(struct DelayRuleUnlockInput)); /// -/// strncpy(input.rule_id, "12345", sizeof(DelayRuleUnlockInput::rule_id) - 1); +/// // Don't forget to deallocate the memory pointed to by "input.rule_ids" +/// // following the completion of the API operation. +/// input.rule_ids = strdup("[\"12345\", \"67890\"]"); /// /// const int ec = rc_delay_rule_unlock(comm, &input); /// diff --git a/lib/core/src/rcMisc.cpp b/lib/core/src/rcMisc.cpp index cac9f78e36..abc403c9f2 100644 --- a/lib/core/src/rcMisc.cpp +++ b/lib/core/src/rcMisc.cpp @@ -1836,6 +1836,7 @@ void clearDelayRuleUnlockInput(void* _p) auto* q = static_cast(_p); + free_pointer(q->rule_ids); clearKeyVal(&q->cond_input); std::memset(q, 0, sizeof(DelayRuleUnlockInput)); diff --git a/plugins/database/src/db_plugin.cpp b/plugins/database/src/db_plugin.cpp index 8be043fb00..bfb27342be 100644 --- a/plugins/database/src/db_plugin.cpp +++ b/plugins/database/src/db_plugin.cpp @@ -15936,32 +15936,74 @@ auto db_delay_rule_lock(irods::plugin_context& _ctx, } } // db_delay_rule_lock -auto db_delay_rule_unlock(irods::plugin_context& _ctx, const char* _rule_id) -> irods::error +auto db_delay_rule_unlock(irods::plugin_context& _ctx, const char* _rule_ids) -> irods::error { if (const auto ret = _ctx.valid(); !ret.ok()) { return PASS(ret); } - if (!_rule_id) { - log_db::error("{}: Rule ID cannot be a null pointer.", __func__); - return ERROR(SYS_INTERNAL_NULL_INPUT_ERR, "Rule ID cannot be a null pointer."); + if (!_rule_ids) { + log_db::error("{}: Rule ID list cannot be a null pointer.", __func__); + return ERROR(SYS_INTERNAL_NULL_INPUT_ERR, "Rule ID list cannot be a null pointer."); } try { + const auto rule_ids = nlohmann::json::parse(_rule_ids); +#define IRODS_DB_IMPL_A +#ifdef IRODS_DB_IMPL_A + const auto rule_ids_vec = rule_ids.get>(); + auto [db_instance, db_conn] = irods::experimental::catalog::new_database_connection(); nanodbc::statement stmt{db_conn}; nanodbc::prepare(stmt, "update R_RULE_EXEC set lock_host = '', lock_host_pid = '', lock_ts = '' where rule_exec_id = ?"); + stmt.bind_strings(0, rule_ids_vec); + // This will throw an exception and rollback all updates on failure. + nanodbc::transact(stmt, rule_ids_vec.size()); - stmt.bind(0, _rule_id); + // It would be amazing to return CODE(result.affected_rows()), but we cannot + // because the ODBC drivers can produce different results. For this reason, the only + // thing we can do is return success or an error. + return SUCCESS(); +#elif defined(IRODS_DB_IMPL_B) + auto [db_instance, db_conn] = irods::experimental::catalog::new_database_connection(); - if (const auto result = nanodbc::execute(stmt); result.affected_rows() != 1) { - auto msg = fmt::format("{}: Failed to unlock delay rule [rule_id={}].", __func__, _rule_id); - log_db::error(msg); - return ERROR(CAT_NO_ROWS_UPDATED, std::move(msg)); + nanodbc::statement stmt{db_conn}; + nanodbc::prepare(stmt, "update R_RULE_EXEC set lock_host = '', lock_host_pid = '', lock_ts = '' where rule_exec_id = ?"); + + std::size_t unlock_count = 0; + + for (auto&& rule_id : rule_ids) { + stmt.bind(0, rule_id.get_ref().c_str()); + + if (const auto result = nanodbc::execute(stmt); result.affected_rows() == 1) { + ++unlock_count; + } } - return SUCCESS(); + return CODE(unlock_count); +#else + std::vector placeholders; + placeholders.reserve(rule_ids.size()); + std::for_each(std::begin(rule_ids), std::end(rule_ids), [&placeholders](auto) { placeholders.emplace_back("?"); }); + + const auto sql = fmt::format( + "update R_RULE_EXEC set lock_host = '', lock_host_pid = '', lock_ts = '' where rule_exec_id in ({})", + fmt::join(placeholders, ", ")); + + auto [db_instance, db_conn] = irods::experimental::catalog::new_database_connection(); + + nanodbc::statement stmt{db_conn}; + nanodbc::prepare(stmt, sql); + + for (decltype(rule_ids)::size_type i = 0; i < rule_ids.size(); ++i) { + stmt.bind(i, rule_ids.at(i).get_ref().c_str()); + } + + const auto result = nanodbc::execute(stmt); + + return CODE(result.affected_rows()); +#endif } catch (const irods::exception& e) { log_db::error("{}: {}", __func__, e.client_display_what()); diff --git a/server/api/include/irods/rs_delay_rule_unlock.hpp b/server/api/include/irods/rs_delay_rule_unlock.hpp index 0df9f9a2bf..c2a359910f 100644 --- a/server/api/include/irods/rs_delay_rule_unlock.hpp +++ b/server/api/include/irods/rs_delay_rule_unlock.hpp @@ -7,21 +7,22 @@ struct RsComm; -/// Removes the delay server lock information from a delay rule if it exists. +/// Atomically removes the delay server lock information from one or more delay rules. /// /// Requires \p rodsadmin level privileges. /// -/// On success, the following information will be removed from the target delay rule's catalog -/// entry: +/// On success, the following information will be removed from the delay rule catalog entries: /// - The host identifying the delay server /// - The PID of the delay server /// - The time the entry was locked /// +/// On failure, all catalog updates are rolled back and an error code is returned to the client. +/// /// \param[in] _comm A pointer to a RsComm. /// \param[in] _input A pointer to a DelayRuleUnlockInput. /// /// \return An integer. -/// \retval 0 On success. +/// \retval 0 On success. /// \retval <0 On failure. /// /// \b Example @@ -32,7 +33,9 @@ struct RsComm; /// struct DelayRuleUnlockInput input; /// memset(&input, 0, sizeof(struct DelayRuleUnlockInput)); /// -/// strncpy(input.rule_id, "12345", sizeof(DelayRuleUnlockInput::rule_id) - 1); +/// // Don't forget to deallocate the memory pointed to by "input.rule_ids" +/// // following the completion of the API operation. +/// input.rule_ids = strdup("[\"12345\", \"67890\"]"); /// /// const int ec = rs_delay_rule_unlock(comm, &input); /// diff --git a/server/api/src/rs_delay_rule_unlock.cpp b/server/api/src/rs_delay_rule_unlock.cpp index dd53bd0608..7d1aad8709 100644 --- a/server/api/src/rs_delay_rule_unlock.cpp +++ b/server/api/src/rs_delay_rule_unlock.cpp @@ -16,8 +16,8 @@ auto rs_delay_rule_unlock(RsComm* _comm, DelayRuleUnlockInput* _input) -> int return SYS_INTERNAL_NULL_INPUT_ERR; } - if (!is_non_empty_string(_input->rule_id, sizeof(DelayRuleUnlockInput::rule_id))) { - log_api::error("{}: Rule ID must be a non-empty string.", __func__); + if (!_input->rule_ids) { + log_api::error("{}: Rule ID must be a non-empty JSON string.", __func__); return SYS_INVALID_INPUT_PARAM; } @@ -41,7 +41,7 @@ auto rs_delay_rule_unlock(RsComm* _comm, DelayRuleUnlockInput* _input) -> int // try { - const auto ec = chl_delay_rule_unlock(*_comm, _input->rule_id); + const auto ec = chl_delay_rule_unlock(*_comm, _input->rule_ids); if (ec < 0) { log_api::error("{}: chl_delay_rule_unlock failed with error code [{}].", __func__, ec); diff --git a/server/icat/src/icatHighLevelRoutines.cpp b/server/icat/src/icatHighLevelRoutines.cpp index 996e3d4694..ad475c1840 100644 --- a/server/icat/src/icatHighLevelRoutines.cpp +++ b/server/icat/src/icatHighLevelRoutines.cpp @@ -4918,7 +4918,7 @@ auto chl_delay_rule_lock(RsComm& _comm, const char* _rule_id, const char* _lock_ return ret.code(); } // chl_delay_rule_lock -auto chl_delay_rule_unlock(RsComm& _comm, const char* _rule_id) -> int +auto chl_delay_rule_unlock(RsComm& _comm, const char* _rule_ids) -> int { irods::database_object_ptr db_obj_ptr; if (const auto ret = irods::database_factory(database_plugin_type, db_obj_ptr); !ret.ok()) { @@ -4937,7 +4937,7 @@ auto chl_delay_rule_unlock(RsComm& _comm, const char* _rule_id) -> int irods::first_class_object_ptr ptr = boost::dynamic_pointer_cast(db_obj_ptr); irods::database_ptr db = boost::dynamic_pointer_cast(db_plug_ptr); - const auto ret = db->call(&_comm, irods::DATABASE_OP_DELAY_RULE_UNLOCK, ptr, _rule_id); + const auto ret = db->call(&_comm, irods::DATABASE_OP_DELAY_RULE_UNLOCK, ptr, _rule_ids); // NOLINTNEXTLINE(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions) return ret.code(); diff --git a/unit_tests/src/test_delay_rule_locking_api.cpp b/unit_tests/src/test_delay_rule_locking_api.cpp index cad7bf9b01..68265db831 100644 --- a/unit_tests/src/test_delay_rule_locking_api.cpp +++ b/unit_tests/src/test_delay_rule_locking_api.cpp @@ -105,7 +105,8 @@ TEST_CASE("lock and unlock delay rule") // Unlock the delay rule. DelayRuleUnlockInput unlock_input{}; - rule_id.copy(unlock_input.rule_id, sizeof(DelayRuleUnlockInput::rule_id) - 1); + irods::at_scope_exit_unsafe clear_unlock_input{[&unlock_input] { clearDelayRuleUnlockInput(&unlock_input); }}; + unlock_input.rule_ids = strdup(nlohmann::json::array({rule_id}).dump().c_str()); REQUIRE(rc_delay_rule_unlock(static_cast(conn), &unlock_input) == 0); { @@ -136,6 +137,100 @@ TEST_CASE("lock and unlock delay rule") } } +// NOLINTNEXTLINE(readability-function-cognitive-complexity) +TEST_CASE("unlock multiple delay rules") +{ + load_client_api_plugins(); + + irods::experimental::client_connection conn; + + // Delete all existing delay rules to avoid issues with the tests. + REQUIRE(delete_all_delay_rules() == 0); + + // Delete the delay rule regardless of the test results. + irods::at_scope_exit_unsafe delete_delay_rules{[] { delete_all_delay_rules(); }}; + + // Schedule five delay rules for execution in the distant future. The delay rules + // MUST NOT be executed by the delay server. They are just placeholders to test the API. + constexpr const char* rule = R"___(writeLine("serverLog", "SHOULD NOT EXECUTE BEFORE TEST COMPLETES"))___"; + constexpr auto delay_rule_count = 5; + for (int i = 0; i < delay_rule_count; ++i) { + REQUIRE(create_delay_rule(rule) == 0); + } + + std::vector rule_ids; + { + DelayRuleLockInput lock_input{}; + boost::asio::ip::host_name().copy(lock_input.lock_host, sizeof(DelayRuleLockInput::lock_host) - 1); + lock_input.lock_host_pid = getpid(); + + // Capture the IDs of the delay rules and lock them. + for (auto&& row : irods::query{static_cast(conn), "select RULE_EXEC_ID where RULE_EXEC_LOCK_HOST = ''"}) { + rule_ids.push_back(row[0]); + + // Lock the delay rule. + row[0].copy(lock_input.rule_id, sizeof(DelayRuleLockInput::rule_id) - 1); + REQUIRE(rc_delay_rule_lock(static_cast(conn), &lock_input) == 0); + } + } + CHECK(rule_ids.size() == delay_rule_count); + + // Show the delay rules are now locked. + for (auto&& rule_id : rule_ids) { + const auto query_string = fmt::format("select RULE_EXEC_ID, RULE_EXEC_LOCK_HOST, RULE_EXEC_LOCK_HOST_PID, RULE_EXEC_LOCK_TIME where RULE_EXEC_ID = '{}'", rule_id); + irods::query query{static_cast(conn), query_string}; + REQUIRE_FALSE(query.empty()); + const auto row = query.front(); + CHECK(row[0] == rule_id); + CHECK(row[1] == boost::asio::ip::host_name()); + CHECK(row[2] == std::to_string(getpid())); + CHECK_FALSE(row[3].empty()); + } + + // Unlock the delay rules. + DelayRuleUnlockInput unlock_input{}; + irods::at_scope_exit_unsafe clear_unlock_input{[&unlock_input] { clearDelayRuleUnlockInput(&unlock_input); }}; + unlock_input.rule_ids = strdup(nlohmann::json(rule_ids).dump().c_str()); + REQUIRE(rc_delay_rule_unlock(static_cast(conn), &unlock_input) == 0); + + // Show the delay rules are now unlocked. + for (auto&& rule_id : rule_ids) { + const auto query_string = fmt::format("select RULE_EXEC_ID, RULE_EXEC_LOCK_HOST, RULE_EXEC_LOCK_HOST_PID, RULE_EXEC_LOCK_TIME where RULE_EXEC_ID = '{}'", rule_id); + irods::query query{static_cast(conn), query_string}; + REQUIRE_FALSE(query.empty()); + const auto row = query.front(); + CHECK(row[0] == rule_id); + CHECK(row[1].empty()); + CHECK(row[2].empty()); + CHECK(row[3].empty()); + } + + // Show that invoking the unlock API with the same IDs is safe and does not + // affect the delay rules. + CHECK(rc_delay_rule_unlock(static_cast(conn), &unlock_input) == 0); +} + +TEST_CASE("invalid delay rule ids") +{ + irods::experimental::client_connection conn; + + // Unlock a nonexistent delay rule. + DelayRuleUnlockInput unlock_input{}; + irods::at_scope_exit_unsafe clear_unlock_input{[&unlock_input] { clearDelayRuleUnlockInput(&unlock_input); }}; + + SECTION("an integer which does not identify a delay rule") + { + unlock_input.rule_ids = strdup(R"(["10"])"); + REQUIRE(rc_delay_rule_unlock(static_cast(conn), &unlock_input) == 0); + } + + SECTION("not an integer") + { + unlock_input.rule_ids = strdup(R"(["xyz"])"); + REQUIRE(rc_delay_rule_unlock(static_cast(conn), &unlock_input) == SYS_LIBRARY_ERROR); + } +} + auto delete_all_delay_rules() -> int { // NOLINTNEXTLINE(cert-env33-c)