Fix insert_resource corrupting sub_resources on rejected insert

Development/cmake/NmosCppTest.cmake

@@ -61,6 +61,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_temporal_redundancy_test.cpp
     nmos/test/sdp_utils_test.cpp

Development/nmos/resources.cpp

@@ -50,15 +50,13 @@ namespace nmos
 
         // all types (other than nodes, and subscriptions) must* be a sub-resource of an existing resource
         // (*assuming not out-of-order insertion by the allow_invalid_resources setting)
-        auto super_resource = find_resource(resources, get_super_resource(resource));
-        if (super_resource != resources.end())
-        {
-            // this isn't modifying the visible data of the super_resouce, so no resource events need to be generated
-            resources.modify(super_resource, [&](nmos::resource& super_resource)
-            {
-                super_resource.sub_resources.insert(resource.id);
-            });
-        }
+        // evaluate the super-resource id/type before insertion (get_super_resource may throw if
+        // referential-integrity fields are missing), but only update sub_resources after a successful
+        // insert or replace — otherwise a rejected insert whose id matches an existing resource
+        // (e.g. a device reusing a node id) would leave a self-reference and later recurse unboundedly
+        // in set_resource_health / erase_resource
+        // see https://github.com/sony/nmos-cpp/issues/403
+        const auto super_id_type = get_super_resource(resource);
 
         auto result = resources.insert(std::move(resource));
         // replacement of a deleted or expired resource is also allowed
@@ -72,6 +70,17 @@ namespace nmos
         if (result.second)
         {
             auto& inserted = *result.first;
+
+            auto super_resource = find_resource(resources, super_id_type);
+            if (super_resource != resources.end())
+            {
+                // this isn't modifying the visible data of the super_resouce, so no resource events need to be generated
+                resources.modify(super_resource, [&](nmos::resource& super_resource)
+                {
+                    super_resource.sub_resources.insert(inserted.id);
+                });
+            }
+
             insert_resource_events(resources, inserted.version, inserted.downgrade_version, inserted.type, web::json::value::null(), inserted.data);
 
             // set the initial health of this resource from the super-resource (if applicable)

Development/nmos/test/resources_test.cpp

@@ -0,0 +1,74 @@
+// 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;
+
+        return{ nmos::is04_versions::v1_3, nmos::types::node, value_of({
+            { U("id"), id }
+        }), false };
+    }
+
+    nmos::resource make_test_device(const nmos::id& id, const nmos::id& node_id)
+    {
+        using web::json::value_of;
+
+        return{ nmos::is04_versions::v1_3, nmos::types::device, value_of({
+            { U("id"), id },
+            { U("node_id"), node_id }
+        }), false };
+    }
+}
+
+////////////////////////////////////////////////////////////////////////////////////////////
+// A resource id is unique in the model, so a device that reuses an existing node id cannot
+// be inserted. Previously, insert_resource still recorded that id in the node's sub_resources
+// before the insert was rejected, leaving a self-reference that made set_resource_health /
+// erase_resource recurse without bound.
+// See https://github.com/sony/nmos-cpp/issues/403
+BST_TEST_CASE(testInsertResourceRejectsDuplicateIdWithoutCorruptingSubResources)
+{
+    const nmos::id id{ U("11111111-1111-1111-1111-111111111111") };
+
+    nmos::resources resources;
+    BST_REQUIRE(nmos::insert_resource(resources, make_test_node(id)).second);
+
+    // same id as the node, and node_id also equal to that id (the mistake from issue #403)
+    BST_REQUIRE(!nmos::insert_resource(resources, make_test_device(id, id)).second);
+
+    BST_REQUIRE_EQUAL(1u, resources.size());
+    const auto node = nmos::find_resource(resources, { id, nmos::types::node });
+    BST_REQUIRE(resources.end() != node);
+    BST_REQUIRE(node->sub_resources.empty());
+
+    // would previously stack-overflow / crash
+    nmos::set_resource_health(resources, id);
+    BST_REQUIRE_EQUAL(1u, nmos::erase_resource(resources, id));
+}
+
+////////////////////////////////////////////////////////////////////////////////////////////
+BST_TEST_CASE(testInsertResourceRecordsSubResources)
+{
+    const nmos::id node_id{ U("22222222-2222-2222-2222-222222222222") };
+    const nmos::id device_id{ U("33333333-3333-3333-3333-333333333333") };
+
+    nmos::resources resources;
+    BST_REQUIRE(nmos::insert_resource(resources, make_test_node(node_id)).second);
+    BST_REQUIRE(nmos::insert_resource(resources, make_test_device(device_id, node_id)).second);
+
+    const auto node = nmos::find_resource(resources, { node_id, nmos::types::node });
+    BST_REQUIRE(resources.end() != node);
+    BST_REQUIRE_EQUAL(1u, node->sub_resources.count(device_id));
+
+    const auto device = nmos::find_resource(resources, { device_id, nmos::types::device });
+    BST_REQUIRE(resources.end() != device);
+    BST_REQUIRE_EQUAL(node->health.load(), device->health.load());
+
+    BST_REQUIRE_EQUAL(2u, nmos::erase_resource(resources, node_id));
+}