diff --git a/include/behaviortree_cpp/bt_factory.h b/include/behaviortree_cpp/bt_factory.h index 0ba3c86bb..85e49e061 100644 --- a/include/behaviortree_cpp/bt_factory.h +++ b/include/behaviortree_cpp/bt_factory.h @@ -316,11 +316,20 @@ class BehaviorTreeFactory * * where "tree_id" come from the XML attribute * + * @throws RuntimeError if the XML registers a BehaviorTree ID that is + * already known to this factory. Call clearRegisteredBehaviorTrees() + * first to replace previous definitions intentionally. */ void registerBehaviorTreeFromFile(const std::filesystem::path& filename); - /// Same of registerBehaviorTreeFromFile, but passing the XML text, - /// instead of the filename. + /** + * @brief Same as registerBehaviorTreeFromFile, but passing the XML text + * instead of the filename. + * + * @throws RuntimeError if the XML registers a BehaviorTree ID that is + * already known to this factory. Call clearRegisteredBehaviorTrees() + * first to replace previous definitions intentionally. + */ void registerBehaviorTreeFromText(const std::string& xml_text); /// Returns the ID of the trees registered either with @@ -446,6 +455,10 @@ class BehaviorTreeFactory * @param text string containing the XML * @param blackboard blackboard of the root tree * @return the newly created tree + * + * @throws RuntimeError if the XML registers a BehaviorTree ID that is + * already known to this factory. Call clearRegisteredBehaviorTrees() + * first to replace previous definitions intentionally. */ [[nodiscard]] Tree createTreeFromText( const std::string& text, Blackboard::Ptr blackboard = Blackboard::create()); @@ -460,11 +473,25 @@ class BehaviorTreeFactory * @param file_path location of the file to load * @param blackboard blackboard of the root tree * @return the newly created tree + * + * @throws RuntimeError if the XML registers a BehaviorTree ID that is + * already known to this factory. Call clearRegisteredBehaviorTrees() + * first to replace previous definitions intentionally. */ [[nodiscard]] Tree createTreeFromFile(const std::filesystem::path& file_path, Blackboard::Ptr blackboard = Blackboard::create()); + /** + * @brief createTree instantiates a tree by ID from definitions already + * present in the factory's parser. It does not load or parse XML; + * duplicate-ID errors come from registerBehaviorTreeFrom* or + * createTreeFrom*, not from createTree(). + * + * @param tree_name ID of the tree to instantiate + * @param blackboard blackboard of the root tree + * @return the newly created tree + */ [[nodiscard]] Tree createTree(const std::string& tree_name, Blackboard::Ptr blackboard = Blackboard::create()); diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index b4a82e976..c276f824a 100644 --- a/src/xml_parsing.cpp +++ b/src/xml_parsing.cpp @@ -48,6 +48,7 @@ #include #include +#include #ifdef USING_ROS2 #include @@ -243,10 +244,12 @@ struct XMLParser::PImpl std::list > opened_documents; std::map tree_roots; + std::unordered_map tree_sources; const BehaviorTreeFactory* factory = nullptr; std::filesystem::path current_path; + std::string current_file = ""; std::map subtree_models; int suffix_count = 0; @@ -261,6 +264,8 @@ struct XMLParser::PImpl current_path = std::filesystem::current_path(); opened_documents.clear(); tree_roots.clear(); + tree_sources.clear(); + current_file = ""; } private: @@ -294,6 +299,7 @@ void XMLParser::loadFromFile(const std::filesystem::path& filepath, bool add_inc doc->LoadFile(filepath.string().c_str()); _p->current_path = std::filesystem::absolute(filepath.parent_path()); + _p->current_file = std::filesystem::absolute(filepath).string(); _p->loadDocImpl(doc, add_includes); } @@ -305,6 +311,8 @@ void XMLParser::loadFromText(const std::string& xml_text, bool add_includes) XMLDocument* doc = _p->opened_documents.back().get(); doc->Parse(xml_text.c_str(), xml_text.size()); + _p->current_file = ""; + _p->loadDocImpl(doc, add_includes); } @@ -418,13 +426,16 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) // change current path to the included file for handling additional relative paths const auto previous_path = current_path; + const std::string previous_file = current_file; current_path = std::filesystem::absolute(file_path.parent_path()); + current_file = std::filesystem::absolute(file_path).string(); next_doc->LoadFile(file_path.string().c_str()); loadDocImpl(next_doc, add_includes); // reset current path to the previous value current_path = previous_path; + current_file = previous_file; } // Collect the names of all nodes registered with the behavior tree factory @@ -457,7 +468,17 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) tree_name = "BehaviorTree_" + std::to_string(suffix_count++); } + const auto existing = tree_sources.find(tree_name); + if(existing != tree_sources.end()) + { + throw RuntimeError("Duplicate BehaviorTree ID '", tree_name, + "': first registered from '", existing->second, + "', re-registered from '", current_file, + "'. Call clearRegisteredBehaviorTrees() if you " + "intend to reload."); + } tree_roots[tree_name] = bt_node; + tree_sources[tree_name] = current_file; } } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index a009ee5ea..281062cc3 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -23,6 +23,7 @@ set(BT_TESTS gtest_blackboard.cpp gtest_coroutines.cpp gtest_decorator.cpp + gtest_duplicate_tree.cpp gtest_enums.cpp gtest_factory.cpp gtest_fallback.cpp diff --git a/tests/gtest_duplicate_tree.cpp b/tests/gtest_duplicate_tree.cpp new file mode 100644 index 000000000..5f7bb2f4b --- /dev/null +++ b/tests/gtest_duplicate_tree.cpp @@ -0,0 +1,218 @@ +/* Copyright (C) 2026 - duplicate BehaviorTree ID registration tests */ + +#include "behaviortree_cpp/bt_factory.h" + +#include +#include +#include +#include + +#include + +using namespace BT; + +namespace +{ +class TempSubdir +{ +public: + TempSubdir() + { + const auto stamp = std::chrono::steady_clock::now().time_since_epoch().count(); + path = std::filesystem::temp_directory_path() / + ("bt_duplicate_tree_" + std::to_string(stamp)); + std::filesystem::create_directories(path); + } + + ~TempSubdir() + { + std::error_code ec; + std::filesystem::remove_all(path, ec); + } + + std::filesystem::path path; + + TempSubdir(const TempSubdir&) = delete; + TempSubdir& operator=(const TempSubdir&) = delete; +}; + +void writeFile(const std::filesystem::path& p, const std::string& content) +{ + std::ofstream out(p.string()); + out << content; +} + +std::string minimalTreeXml(const std::string& tree_id) +{ + return std::string(R"( + + + + + + )"); +} + +} // namespace + +TEST(DuplicateBehaviorTreeId, DuplicateIdAcrossFiles) +{ + TempSubdir dir; + const auto path_a = dir.path / "a.xml"; + const auto path_b = dir.path / "b.xml"; + writeFile(path_a, minimalTreeXml("DupTree")); + writeFile(path_b, minimalTreeXml("DupTree")); + + const std::string abs_a = std::filesystem::absolute(path_a).string(); + const std::string abs_b = std::filesystem::absolute(path_b).string(); + + BehaviorTreeFactory factory; + ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path_a)); + + try + { + factory.registerBehaviorTreeFromFile(path_b); + FAIL() << "expected RuntimeError"; + } + catch(const RuntimeError& e) + { + const std::string msg = e.what(); + EXPECT_NE(msg.find(abs_a), std::string::npos) << msg; + EXPECT_NE(msg.find(abs_b), std::string::npos) << msg; + } +} + +TEST(DuplicateBehaviorTreeId, DuplicateIdWithinSameText) +{ + const std::string xml = R"( + + + + +)"; + + BehaviorTreeFactory factory; + try + { + factory.registerBehaviorTreeFromText(xml); + FAIL() << "expected RuntimeError"; + } + catch(const RuntimeError& e) + { + const std::string msg = e.what(); + EXPECT_NE(msg.find(""), std::string::npos) << msg; + } +} + +TEST(DuplicateBehaviorTreeId, DuplicateIdAcrossFileAndText) +{ + TempSubdir dir; + const auto path = dir.path / "one.xml"; + writeFile(path, minimalTreeXml("shared_id")); + + const std::string abs_path = std::filesystem::absolute(path).string(); + + BehaviorTreeFactory factory; + ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path)); + + const std::string xml = minimalTreeXml("shared_id"); + try + { + factory.registerBehaviorTreeFromText(xml); + FAIL() << "expected RuntimeError"; + } + catch(const RuntimeError& e) + { + const std::string msg = e.what(); + EXPECT_NE(msg.find(abs_path), std::string::npos) << msg; + EXPECT_NE(msg.find(""), std::string::npos) << msg; + } +} + +TEST(DuplicateBehaviorTreeId, DuplicateIdSamePathTwice) +{ + TempSubdir dir; + const auto path = dir.path / "reload.xml"; + writeFile(path, minimalTreeXml("same")); + + const std::string abs_path = std::filesystem::absolute(path).string(); + + BehaviorTreeFactory factory; + ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path)); + + try + { + factory.registerBehaviorTreeFromFile(path); + FAIL() << "expected RuntimeError"; + } + catch(const RuntimeError& e) + { + const std::string msg = e.what(); + EXPECT_NE(msg.find(abs_path), std::string::npos) << msg; + } +} + +TEST(DuplicateBehaviorTreeId, ClearAllowsReload) +{ + TempSubdir dir; + const auto path = dir.path / "clear.xml"; + writeFile(path, minimalTreeXml("cleared")); + + BehaviorTreeFactory factory; + ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path)); + factory.clearRegisteredBehaviorTrees(); + ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path)); + ASSERT_NO_THROW(static_cast(factory.createTree("cleared"))); +} + +TEST(DuplicateBehaviorTreeId, DifferentIdsCoexist) +{ + TempSubdir dir; + const auto path_a = dir.path / "tree_a.xml"; + const auto path_b = dir.path / "tree_b.xml"; + writeFile(path_a, minimalTreeXml("TreeA")); + writeFile(path_b, minimalTreeXml("TreeB")); + + BehaviorTreeFactory factory; + ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path_a)); + ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path_b)); + + const auto ids = factory.registeredBehaviorTrees(); + ASSERT_EQ(ids.size(), 2); + ASSERT_NO_THROW(static_cast(factory.createTree("TreeA"))); + ASSERT_NO_THROW(static_cast(factory.createTree("TreeB"))); +} + +TEST(DuplicateBehaviorTreeId, IncludeDuplicateThrows) +{ + TempSubdir dir; + const auto inner_path = dir.path / "inner.xml"; + const auto outer_path = dir.path / "outer.xml"; + + writeFile(inner_path, minimalTreeXml("foo")); + + const std::string outer_xml = R"( + + + + + + +)"; + writeFile(outer_path, outer_xml); + + const std::string inner_abs = std::filesystem::absolute(inner_path).string(); + + BehaviorTreeFactory factory; + try + { + factory.registerBehaviorTreeFromFile(std::filesystem::absolute(outer_path)); + FAIL() << "expected RuntimeError"; + } + catch(const RuntimeError& e) + { + const std::string msg = e.what(); + EXPECT_NE(msg.find(inner_abs), std::string::npos) << msg; + } +} diff --git a/tests/gtest_factory.cpp b/tests/gtest_factory.cpp index 01718cbce..a186a54a8 100644 --- a/tests/gtest_factory.cpp +++ b/tests/gtest_factory.cpp @@ -400,6 +400,7 @@ TEST(BehaviorTreeReload, ReloadSameTree) ASSERT_EQ(NodeStatus::SUCCESS, tree.tickWhileRunning()); } + factory.clearRegisteredBehaviorTrees(); factory.registerBehaviorTreeFromText(xmlB); { auto tree = factory.createTree("MainTree"); diff --git a/tests/gtest_parallel.cpp b/tests/gtest_parallel.cpp index 81279921a..3f0b88350 100644 --- a/tests/gtest_parallel.cpp +++ b/tests/gtest_parallel.cpp @@ -486,6 +486,8 @@ TEST(Parallel, ParallelAll) ASSERT_EQ(1, observer.getStatistics("third").success_count); } + factory.clearRegisteredBehaviorTrees(); + { const char* xml_text = R"( diff --git a/tests/gtest_subtree.cpp b/tests/gtest_subtree.cpp index cda5d68a8..78bf9e172 100644 --- a/tests/gtest_subtree.cpp +++ b/tests/gtest_subtree.cpp @@ -126,6 +126,8 @@ TEST(SubTree, BadRemapping) Tree tree_bad_in = factory.createTree("MainTree"); EXPECT_ANY_THROW(tree_bad_in.tickWhileRunning()); + factory.clearRegisteredBehaviorTrees(); + static const char* xml_text_bad_out = R"(