diff --git a/Development/cmake/NmosCppTest.cmake b/Development/cmake/NmosCppTest.cmake index 4a589c51..1ad6fdae 100644 --- a/Development/cmake/NmosCppTest.cmake +++ b/Development/cmake/NmosCppTest.cmake @@ -60,6 +60,7 @@ set(NMOS_CPP_TEST_NMOS_TEST_SOURCES nmos/test/node_interfaces_test.cpp nmos/test/paging_utils_test.cpp nmos/test/query_api_test.cpp + nmos/test/resources_test.cpp nmos/test/sdp_test_utils.cpp nmos/test/sdp_utils_test.cpp nmos/test/settings_test.cpp diff --git a/Development/nmos/resources.cpp b/Development/nmos/resources.cpp index a4c0b214..7a23dffd 100644 --- a/Development/nmos/resources.cpp +++ b/Development/nmos/resources.cpp @@ -131,47 +131,62 @@ namespace nmos return result; } - // erase the resource with the specified id from the specified resources (if present) - // and return the count of the number of resources erased (including sub-resources) - // resources may optionally be initially "erased" by setting data to null, and remain in this non-extant state until they are explicitly forgotten (or reinserted) - resources::size_type erase_resource(resources& resources, const id& id, bool forget_now) + namespace details { - // also erase all sub-resources of this resource, i.e. - // for a node, all devices with matching node_id - // for a device, all sources, senders and receivers with matching device_id - // for a sender, all flows with matching source_id - // it won't be a very deep recursion... - resources::size_type count = 0; - auto found = resources.find(id); - if (resources.end() != found && found->has_data()) + // as nmos::erase_resource, but with protection against cyclic sub-resource relationships + static resources::size_type erase_resource(nmos::resources& resources, const nmos::id& id, bool forget_now, std::set& erasing) { - for (auto& sub_resource : found->sub_resources) + // also erase all sub-resources of this resource, i.e. + // for a node, all devices with matching node_id + // for a device, all sources, senders and receivers with matching device_id + // for a sender, all flows with matching source_id + // it won't be a very deep recursion... + resources::size_type count = 0; + + // a resource that is directly or indirectly listed as a sub-resource of itself + // would otherwise cause unbounded recursion (see nmos-cpp issue #403) + if (!erasing.insert(id).second) return count; + + auto found = resources.find(id); + if (resources.end() != found && found->has_data()) { - count += erase_resource(resources, sub_resource, forget_now); - } + for (auto& sub_resource : found->sub_resources) + { + count += details::erase_resource(resources, sub_resource, forget_now, erasing); + } - const auto pre = found->data; + const auto pre = found->data; - auto resource_updated = nmos::strictly_increasing_update(resources); - resources.modify(found, [&resource_updated](resource& resource) - { - resource.data = web::json::value::null(); + auto resource_updated = nmos::strictly_increasing_update(resources); + resources.modify(found, [&resource_updated](resource& resource) + { + resource.data = web::json::value::null(); - // set the update timestamp when a resource is deleted - resource.updated = resource_updated; - }); + // set the update timestamp when a resource is deleted + resource.updated = resource_updated; + }); - auto& erased = *found; - insert_resource_events(resources, erased.version, erased.downgrade_version, erased.type, pre, erased.data); + auto& erased = *found; + insert_resource_events(resources, erased.version, erased.downgrade_version, erased.type, pre, erased.data); - if (forget_now) - { - resources.erase(found); - } + if (forget_now) + { + resources.erase(found); + } - ++count; + ++count; + } + return count; } - return count; + } + + // erase the resource with the specified id from the specified resources (if present) + // and return the count of the number of resources erased (including sub-resources) + // resources may optionally be initially "erased" by setting data to null, and remain in this non-extant state until they are explicitly forgotten (or reinserted) + resources::size_type erase_resource(resources& resources, const id& id, bool forget_now) + { + std::set erasing; + return details::erase_resource(resources, id, forget_now, erasing); } // forget all erased resources which expired *before* the specified time from the specified resources @@ -266,22 +281,36 @@ namespace nmos // find the resource with the specified id in the specified resources (if present) and // set the health of the resource and all of its sub-resources, to prevent them expiring // note, since health is mutable, no need for the resources parameter to be non-const - void set_resource_health(const resources& resources, const id& id, health health) + namespace details { - auto found = resources.find(id); - if (resources.end() != found && found->has_data()) + // as nmos::set_resource_health, but with protection against cyclic sub-resource relationships + static void set_resource_health(const nmos::resources& resources, const nmos::id& id, health health, std::set& setting) { - for (auto& sub_resource : found->sub_resources) + // a resource that is directly or indirectly listed as a sub-resource of itself + // would otherwise cause unbounded recursion (see nmos-cpp issue #403) + if (!setting.insert(id).second) return; + + auto found = resources.find(id); + if (resources.end() != found && found->has_data()) { - set_resource_health(resources, sub_resource, health); - } + for (auto& sub_resource : found->sub_resources) + { + details::set_resource_health(resources, sub_resource, health, setting); + } - // since health is mutable, no need for: - // resources.modify(found, [&health](nmos::resource& resource){ resource.health = health; }); - found->health = health; + // since health is mutable, no need for: + // resources.modify(found, [&health](nmos::resource& resource){ resource.health = health; }); + found->health = health; + } } } + void set_resource_health(const resources& resources, const id& id, health health) + { + std::set setting; + details::set_resource_health(resources, id, health, setting); + } + static inline std::pair no_resource() { return{}; } // get the super-resource id and type, according to the guidelines on referential integrity diff --git a/Development/nmos/test/resources_test.cpp b/Development/nmos/test/resources_test.cpp new file mode 100644 index 00000000..fa2b741d --- /dev/null +++ b/Development/nmos/test/resources_test.cpp @@ -0,0 +1,78 @@ +// The first "test" is of course whether the header compiles standalone +#include "nmos/resources.h" + +#include "bst/test/test.h" +#include "nmos/is04_versions.h" + +namespace +{ + nmos::resource make_test_node(const nmos::id& id) + { + using web::json::value_of; + + auto data = value_of({ + { U("id"), id } + }); + return{ nmos::is04_versions::v1_3, nmos::types::node, std::move(data), id, false }; + } + + nmos::resource make_test_device(const nmos::id& id, const nmos::id& node_id) + { + using web::json::value_of; + + auto data = value_of({ + { U("id"), id }, + { U("node_id"), node_id } + }); + return{ nmos::is04_versions::v1_3, nmos::types::device, std::move(data), id, false }; + } +} + +//////////////////////////////////////////////////////////////////////////////////////////// +BST_TEST_CASE(testEraseResourceWithCyclicSubResources) +{ + // a resource listed as a sub-resource of itself must not cause unbounded recursion + // see https://github.com/sony/nmos-cpp/issues/403 + { + nmos::resources resources; + auto self = make_test_node(U("self")); + self.sub_resources.insert(U("self")); + nmos::insert_resource(resources, std::move(self)); + + BST_REQUIRE_EQUAL(1u, nmos::erase_resource(resources, U("self"))); + BST_REQUIRE(resources.empty()); + } + + // resources indirectly listed as sub-resources of themselves must not either + { + nmos::resources resources; + auto a = make_test_node(U("a")); + a.sub_resources.insert(U("b")); + auto b = make_test_device(U("b"), U("a")); + b.sub_resources.insert(U("a")); + nmos::insert_resource(resources, std::move(a)); + nmos::insert_resource(resources, std::move(b)); + + BST_REQUIRE_EQUAL(2u, nmos::erase_resource(resources, U("a"))); + BST_REQUIRE(resources.empty()); + } +} + +//////////////////////////////////////////////////////////////////////////////////////////// +BST_TEST_CASE(testEraseResourceWithSubResources) +{ + // erasing a resource also erases its sub-resources, and only those + nmos::resources resources; + auto node = make_test_node(U("node")); + node.sub_resources.insert(U("device")); + auto device = make_test_device(U("device"), U("node")); + auto other = make_test_node(U("other")); + nmos::insert_resource(resources, std::move(node)); + nmos::insert_resource(resources, std::move(device)); + nmos::insert_resource(resources, std::move(other)); + + BST_REQUIRE_EQUAL(2u, nmos::erase_resource(resources, U("node"))); + BST_REQUIRE(resources.end() == resources.find(U("node"))); + BST_REQUIRE(resources.end() == resources.find(U("device"))); + BST_REQUIRE(resources.end() != resources.find(U("other"))); +}