Fix incorrect registration of behavior trees containing faulty XML (BehaviorTree#438)

* fix incorrect registration of faulty trees

* format

* simplify XML validation

* fix possible out-of-range exception in tests

* Add tests

* reduce scale of diffs

* fix comment

* add more test cases

Co-authored-by: Davide Faconti <[email protected]>

Author: schornakj

src/xml_parsing.cpp

@@ -199,6 +199,21 @@ void XMLParser::Pimpl::loadDocImpl(tinyxml2::XMLDocument* doc, bool add_includes
     current_path = previous_path;
   }
 
+  // Collect the names of all nodes registered with the behavior tree factory
+  std::unordered_map<std::string, BT::NodeType> registered_nodes;
+  for (const auto& it : factory.manifests())
+  {
+    registered_nodes.insert({it.first, it.second.type});
+  }
+
+  XMLPrinter printer;
+  doc->Print(&printer);
+  auto xml_text = std::string(printer.CStr(), size_t(printer.CStrSize() - 1));
+
+  // Verify the validity of the XML before adding any behavior trees to the parser's list of registered trees
+  VerifyXML(xml_text, registered_nodes);
+
+  // Register each BehaviorTree within the XML
   for (auto bt_node = xml_root->FirstChildElement("BehaviorTree"); bt_node != nullptr;
        bt_node = bt_node->NextSiblingElement("BehaviorTree"))
   {
@@ -211,24 +226,9 @@ void XMLParser::Pimpl::loadDocImpl(tinyxml2::XMLDocument* doc, bool add_includes
     {
       tree_name = "BehaviorTree_" + std::to_string(suffix_count++);
     }
-    tree_roots.insert({tree_name, bt_node});
-  }
-
-  std::unordered_map<std::string, BT::NodeType> registered_nodes;
-  XMLPrinter printer;
-  doc->Print(&printer);
-  auto xml_text = std::string(printer.CStr(), size_t(printer.CStrSize() - 1));
 
-  for (const auto& it : factory.manifests())
-  {
-    registered_nodes.insert({it.first, it.second.type});
-  }
-  for (const auto& it : tree_roots)
-  {
-    registered_nodes.insert({it.first, NodeType::SUBTREE});
+    tree_roots.insert({tree_name, bt_node});
   }
-
-  VerifyXML(xml_text, registered_nodes);
 }
 
 void VerifyXML(const std::string& xml_text,
@@ -390,6 +390,14 @@ void VerifyXML(const std::string& xml_text,
                                        "attribute [ID]");
       }
     }
+    else if (StrEqual(name, "BehaviorTree"))
+    {
+      if (children_count != 1)
+      {
+        ThrowError(node->GetLineNum(), "The node <BehaviorTree> must have exactly 1 "
+                                       "child");
+      }
+    }
     else
     {
       // search in the factory and the list of subtrees
@@ -420,26 +428,10 @@ void VerifyXML(const std::string& xml_text,
     }
   };
 
-  std::vector<std::string> tree_names;
-  int tree_count = 0;
-
   for (auto bt_root = xml_root->FirstChildElement("BehaviorTree"); bt_root != nullptr;
        bt_root = bt_root->NextSiblingElement("BehaviorTree"))
   {
-    tree_count++;
-    if (bt_root->Attribute("ID"))
-    {
-      tree_names.emplace_back(bt_root->Attribute("ID"));
-    }
-    if (ChildrenCount(bt_root) != 1)
-    {
-      ThrowError(bt_root->GetLineNum(), "The node <BehaviorTree> must have exactly "
-                                        "1 child");
-    }
-    else
-    {
-      recursiveStep(bt_root->FirstChildElement());
-    }
+    recursiveStep(bt_root);
   }
 }
 

tests/gtest_factory.cpp

@@ -109,6 +109,7 @@ TEST(BehaviorTreeFactory, XMLParsingOrder)
     XMLParser parser(factory);
     parser.loadFromText(xml_text_subtree);
     auto trees = parser.registeredBehaviorTrees();
+    ASSERT_EQ(trees.size(), 2);
     ASSERT_EQ(trees[0], "CrossDoorSubtree");
     ASSERT_EQ(trees[1], "MainTree");
   }
@@ -117,6 +118,7 @@ TEST(BehaviorTreeFactory, XMLParsingOrder)
     parser.loadFromText(xml_text_subtree_part1);
     parser.loadFromText(xml_text_subtree_part2);
     auto trees = parser.registeredBehaviorTrees();
+    ASSERT_EQ(trees.size(), 2);
     ASSERT_EQ(trees[0], "CrossDoorSubtree");
     ASSERT_EQ(trees[1], "MainTree");
   }
@@ -125,6 +127,7 @@ TEST(BehaviorTreeFactory, XMLParsingOrder)
     parser.loadFromText(xml_text_subtree_part2);
     parser.loadFromText(xml_text_subtree_part1);
     auto trees = parser.registeredBehaviorTrees();
+    ASSERT_EQ(trees.size(), 2);
     ASSERT_EQ(trees[0], "CrossDoorSubtree");
     ASSERT_EQ(trees[1], "MainTree");
   }
@@ -217,7 +220,12 @@ TEST(BehaviorTreeFactory, Issue7)
   BehaviorTreeFactory factory;
   XMLParser parser(factory);
 
+  // We expect that an incorrectly-constructed behavior tree will fail to load
   EXPECT_THROW(parser.loadFromText(xml_text_issue), RuntimeError);
+
+  // We expect that no behavior trees will be registered after we unsuccessfully attempt to load a single tree
+  auto trees = parser.registeredBehaviorTrees();
+  EXPECT_TRUE( trees.empty() );
 }
 
 // clang-format off
@@ -408,6 +416,87 @@ TEST(BehaviorTreeFactory, DecoratorWithTwoChildrenThrows)
   ASSERT_THROW(factory.createTreeFromText(xml_text), BehaviorTreeException);
 }
 
+
+TEST(BehaviorTreeFactory, RegisterValidAndInvalidTrees)
+{
+const std::string xml_text_ok = R"(
+<root>
+    <BehaviorTree ID="ValidTree">
+      <Sequence name="door_open_sequence">
+        <Action ID="AlwaysSuccess" />
+      </Sequence>
+    </BehaviorTree>
+</root> )";
+
+const std::string xml_text_invalid = R"(
+<root>
+    <BehaviorTree ID="InvalidTreeWithNoChildren">
+    </BehaviorTree>
+</root> )";
+
+    BehaviorTreeFactory factory;
+    XMLParser parser(factory);
+
+    // GIVEN that a valid tree has been loaded
+    ASSERT_NO_THROW(parser.loadFromText(xml_text_ok));
+
+    // WHEN we attempt to load an invalid tree
+    ASSERT_THROW(parser.loadFromText(xml_text_invalid), RuntimeError);
+
+    // THEN the valid tree is still registered
+    auto trees = parser.registeredBehaviorTrees();
+    ASSERT_EQ(trees.size(), 1);
+    EXPECT_EQ(trees[0], "ValidTree");
+}
+
+TEST(BehaviorTreeFactory, RegisterInvalidXMLBadActionNodeThrows)
+{
+  // GIVEN an invalid tree
+  // This tree contains invalid XML because the action node is missing a trailing `/`.
+  // A valid line would read: <Action ID="AlwaysSuccess" />
+const std::string xml_text_invalid = R"(
+<root>
+    <BehaviorTree ID="InvalidTreeWithBadChild">
+      <Sequence name="seq">
+        <Action ID="AlwaysSuccess" >
+      </Sequence>
+    </BehaviorTree>
+</root> )";
+
+    BehaviorTreeFactory factory;
+    XMLParser parser(factory);
+
+    // WHEN we attempt to load an invalid tree
+    // THEN a RuntimeError exception is thrown
+    EXPECT_THROW(parser.loadFromText(xml_text_invalid), RuntimeError);
+
+    // THEN no tree is registered
+    auto trees = parser.registeredBehaviorTrees();
+    EXPECT_TRUE(trees.empty());
+}
+
+TEST(BehaviorTreeFactory, RegisterInvalidXMLNoRootThrows)
+{
+  // GIVEN an invalid tree
+  // This tree contains invalid XML because it does not have a root node
+const std::string xml_text_invalid = R"(
+    <BehaviorTree ID="InvalidTreeNoRoot">
+      <Sequence name="seq">
+        <Action ID="AlwaysSuccess" />
+      </Sequence>
+    </BehaviorTree> )";
+
+    BehaviorTreeFactory factory;
+    XMLParser parser(factory);
+
+    // WHEN we attempt to load an invalid tree
+    // THEN a RuntimeError exception is thrown
+    EXPECT_THROW(parser.loadFromText(xml_text_invalid), RuntimeError);
+
+    // THEN no tree is registered
+    auto trees = parser.registeredBehaviorTrees();
+    EXPECT_TRUE(trees.empty());
+
 TEST(BehaviorTreeFactory, ParserClearRegisteredBehaviorTrees)
 {
   const std::string tree_xml = R"(