apply type checks in Blackboard::set when the key is remapped (#1232)

Behavior change: writes through a SubTree remapping now follow the same type rules as local writes. An unconvertible string written into a strongly typed parent entry throws instead of being stored as a std::string, and a remapped write locks the type of a weakly typed (AnyTypeAllowed) parent entry, as a local write already did.

Author: aysha-afrah26

include/behaviortree_cpp/blackboard.h

@@ -286,15 +286,16 @@ inline void Blackboard::set(const std::string& key, const T& value)
     rootBlackboard()->set(key.substr(1, key.size() - 1), value);
     return;
   }
-  std::shared_lock storage_lock(storage_mutex_);
-
-  // check local storage
-  auto it = storage_.find(key);
-  if(it == storage_.end())
+  // getEntry() follows the remapping to the parent blackboard, exactly like
+  // the readers do. A key that resolves to an existing entry (local or
+  // remapped) must go through the type check below: createEntryImpl() would
+  // follow the same remapping and return the pre-existing entry, and writing
+  // to it blindly bypasses the type lock and the string conversion.
+  auto entry_ptr = getEntry(key);
+  if(!entry_ptr)
   {
     // create a new entry
     Any new_value(value);
-    storage_lock.unlock();
     std::shared_ptr<Blackboard::Entry> entry;
     // if a new generic port is created with a string, it's type should be AnyTypeAllowed
     if constexpr(std::is_same_v<std::string, T>)
@@ -319,10 +320,6 @@ inline void Blackboard::set(const std::string& key, const T& value)
   {
     // this is not the first time we set this entry, we need to check
     // if the type is the same or not.
-    // Copy shared_ptr to prevent use-after-free if another thread
-    // calls unset() while we hold the reference (BUG-2 fix).
-    auto entry_ptr = it->second;
-    storage_lock.unlock();
     Entry& entry = *entry_ptr;
 
     std::scoped_lock scoped_lock(entry.entry_mutex);

tests/gtest_blackboard.cpp

@@ -1115,3 +1115,34 @@ TEST(BlackboardTest, GetLockedPortContentWithDefault_Issue942)
   // The value should be accessible from the blackboard
   ASSERT_EQ(tree.rootBlackboard()->get<int>("value"), 42);
 }
+
+TEST(BlackboardTest, SetThroughSubtreeRemappingChecksType)
+{
+  // set() must apply the same type check and string conversion when the key
+  // is remapped to the parent blackboard, like <SubTree local="{value}"> does.
+  auto parent_bb = Blackboard::create();
+  parent_bb->set("value", 42);  // strongly typed as int
+
+  auto child_bb = Blackboard::create(parent_bb);
+  child_bb->addSubtreeRemapping("local", "value");
+
+  // a string that can't be converted to int is rejected, as it is on the parent
+  ASSERT_ANY_THROW(parent_bb->set("value", std::string("garbage")));
+  ASSERT_ANY_THROW(child_bb->set("local", std::string("garbage")));
+
+  // the entry must keep both its declared type and its stored type
+  const auto entry = parent_bb->getEntry("value");
+  ASSERT_EQ(entry->info.type(), typeid(int));
+  ASSERT_EQ(entry->value.type(), typeid(int));
+
+  // a convertible string is parsed to the declared type, not stored as string
+  child_bb->set("local", std::string("99"));
+  ASSERT_EQ(entry->value.type(), typeid(int));
+  ASSERT_EQ(parent_bb->get<int>("value"), 99);
+
+  // a safe numeric conversion is accepted, as it is on the parent
+  parent_bb->set("small", static_cast<uint8_t>(1));
+  child_bb->addSubtreeRemapping("local_small", "small");
+  ASSERT_NO_THROW(child_bb->set("local_small", 100));
+  ASSERT_EQ(parent_bb->get<uint8_t>("small"), 100);
+}