fix(ComponentsStorage): name the component type and check deletion of unknown ids

bindings/python/tests/model/test-py-brep.py

@@ -831,6 +831,20 @@ def test_registry(brep):
             raise ValueError("[Test] Wrong collection registry entry")
 
 
+def test_unknown_component_access():
+    brep = model.BRep()
+    unknown = basic.uuid()
+    try:
+        brep.surface(unknown)
+    except Exception as exception:
+        if f"Surface with id {unknown}" not in str(exception):
+            raise ValueError(
+                "[Test] Accessing an unknown Surface should name its type and id"
+            )
+    else:
+        raise ValueError("[Test] Accessing an unknown Surface should raise")
+
+
 def test_component_id_hash(brep, surface_uuids):
     surface = brep.surface(surface_uuids[0])
     same = model.ComponentID(
@@ -915,6 +929,7 @@ def test_component_id_hash(brep, surface_uuids):
     test_physical_properties(brep, physical_attribute_id)
     test_clone(brep)
     test_component_id_hash(brep, surface_uuids)
+    test_unknown_component_access()
     brep_copy = model.BRep()
     model.BRepBuilder(brep_copy).copy(brep)
     test_physical_properties(brep_copy, physical_attribute_id)

include/geode/model/mixin/core/detail/components_storage.hpp

@@ -162,7 +162,9 @@
 
         void delete_component( const uuid& component_id )
         {
-            components_.erase( components_.find( component_id ) );
+            const auto it = components_.find( component_id );
+            check_component_exists( it, component_id );
+            components_.erase( it );
         }
 
         /*!
@@ -211,9 +213,9 @@
             {
                 auto path = file.path();
                 auto filename = path.replace_extension( "" ).u8string();
                 if( filename.size() > 36 )
                 {
                     auto uuid = filename.substr( filename.size() - 36 );
                     mapping.emplace(
                         std::move( uuid ), file.path().u8string() );
                 }
@@ -241,7 +243,7 @@
                     } } } );
         }
 
         void repair_loaded_components( std::string_view filename )
         {
             bool has_wrong_component{ false };
             for( const auto& [component_id, component] : components_ )
@@ -267,7 +269,7 @@
                         component_id.string(), " from file: ", filename );
                     continue;
                 }
                 auto id = component->id();
                 if( id != component_id )
                 {
                     Logger::warning( "[ComponentsStorage::load_components] "
@@ -287,14 +289,22 @@
             components_ = std::move( repaired );
         }
 
-        [[nodiscard]] Component& find_component(
+        void check_component_exists(
+            typename ComponentsStore::const_iterator it,
             const uuid& component_id ) const
         {
-            const auto it = components_.find( component_id );
             OpenGeodeModelException::check_exception( it != components_.end(),
                 component_id, OpenGeodeException::TYPE::data,
-                "[ComponentsStorage::component] Component with id ",
+                "[ComponentsStorage] ",
+                Component::component_type_static().get(), " with id ",
                 component_id.string(), " does not exist" );
+        }
+
+        [[nodiscard]] Component& find_component(
+            const uuid& component_id ) const
+        {
+            const auto it = components_.find( component_id );
+            check_component_exists( it, component_id );
             auto& component = *it->second;
             OpenGeodeModelException::check_exception(
                 component.id() == component_id, component_id,

tests/model/test-brep.cpp

@@ -25,6 +25,7 @@
 
 #include <absl/container/flat_hash_map.h>
 #include <absl/strings/match.h>
+#include <absl/strings/str_cat.h>
 
 #include <geode/basic/assert.hpp>
 #include <geode/basic/attribute_manager.hpp>
@@ -65,11 +66,11 @@
 
 #include <geode/tests/common.hpp>
 
 std::array< geode::uuid, 6 > add_corners(
     const geode::BRep& model, geode::BRepBuilder& builder )
 {
     std::array< geode::uuid, 6 > uuids;
     for( const auto c : geode::Range{ 6 } )
     {
         uuids[c] = builder.add_corner();
         builder.set_corner_name(
@@ -1873,6 +1874,29 @@
         false, "Accessing an unknown component should throw" );
 }
 
+void test_remove_unknown_component()
+{
+    geode::BRep brep;
+    geode::BRepBuilder builder{ brep };
+    builder.add_surface();
+    const geode::uuid unknown_id;
+    bool thrown{ false };
+    try
+    {
+        builder.remove_surfaces( { unknown_id } );
+    }
+    catch( const geode::OpenGeodeException& exception )
+    {
+        thrown = absl::StrContains( exception.what(),
+            absl::StrCat( "Surface with id ", unknown_id.string() ) );
+    }
+    geode::OpenGeodeModelException::test( thrown,
+        "Removing an unknown Surface should throw an exception naming the "
+        "component type and id" );
+    geode::OpenGeodeModelException::test( brep.nb_surfaces() == 1,
+        "Removing an unknown Surface should not modify the model" );
+}
+
 void test_unknown_components( const geode::BRep& model )
 {
     test_unknown_component_access( [&model]( const geode::uuid& id ) {
@@ -2177,6 +2201,7 @@
         model, surface_uuids, surface_collection_uuids );
     test_block_collection_ranges( model, block_uuid, block_collection_uuid );
     test_unknown_components( model );
+    test_remove_unknown_component();
     test_is_closed();
     test_bounding_box();
     const geode::uuid physical_attribute_id;