forget a rejected XML document in XMLParser (#1266)

Author: aysha-afrah26

src/xml_parsing.cpp

@@ -46,6 +46,7 @@
 #include "behaviortree_cpp/xml_parsing.h"
 
 #include <filesystem>
+#include <iterator>
 #include <map>
 #include <unordered_map>
 
@@ -245,6 +246,25 @@ struct XMLParser::PImpl
 
   void loadDocImpl(XMLDocument* doc, bool add_includes);
 
+  // Load the document just appended to opened_documents. A document that is
+  // rejected (XML error, invalid tree) registers no tree, so forget it again:
+  // it would otherwise remain the "first opened document" that
+  // instantiateTree() reads main_tree_to_execute from, and a document that
+  // did not parse has no root element at all.
+  void loadLastDocImpl(bool add_includes)
+  {
+    const auto doc_it = std::prev(opened_documents.end());
+    try
+    {
+      loadDocImpl(doc_it->get(), add_includes);
+    }
+    catch(...)
+    {
+      opened_documents.erase(doc_it);
+      throw;
+    }
+  }
+
   std::list<std::unique_ptr<XMLDocument> > opened_documents;
   std::map<std::string, const XMLElement*> tree_roots;
   std::unordered_map<std::string, std::string> tree_sources;
@@ -304,7 +324,7 @@ void XMLParser::loadFromFile(const std::filesystem::path& filepath, bool add_inc
   _p->current_path = std::filesystem::absolute(filepath.parent_path());
   _p->include_stack = { IncludeKey(std::filesystem::absolute(filepath)) };
 
-  _p->loadDocImpl(doc, add_includes);
+  _p->loadLastDocImpl(add_includes);
 }
 
 void XMLParser::loadFromText(const std::string& xml_text, bool add_includes)
@@ -315,7 +335,7 @@ void XMLParser::loadFromText(const std::string& xml_text, bool add_includes)
   doc->Parse(xml_text.c_str(), xml_text.size());
 
   _p->include_stack.clear();
-  _p->loadDocImpl(doc, add_includes);
+  _p->loadLastDocImpl(add_includes);
 }
 
 std::vector<std::string> XMLParser::registeredBehaviorTrees() const
@@ -448,7 +468,7 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes)
 
     include_stack.push_back(std::move(include_key));
     next_doc->LoadFile(file_path.string().c_str());
-    loadDocImpl(next_doc, add_includes);
+    loadLastDocImpl(add_includes);
     include_stack.pop_back();
 
     // reset current path to the previous value
@@ -745,6 +765,10 @@ Tree XMLParser::instantiateTree(const Blackboard::Ptr& root_blackboard,
   // or the one in the FIRST document opened
   if(main_tree_ID.empty())
   {
+    if(_p->opened_documents.empty())
+    {
+      throw RuntimeError("XMLParser::instantiateTree: no XML document was loaded");
+    }
     XMLElement* first_xml_root = _p->opened_documents.front()->RootElement();
 
     if(auto main_tree_attribute = first_xml_root->Attribute("main_tree_to_execute"))

tests/gtest_factory.cpp

@@ -864,6 +864,60 @@ TEST(BehaviorTreeFactory, MalformedXML_UnknownNodeType)
   EXPECT_THROW((void)factory.createTreeFromText(xml), RuntimeError);
 }
 
+TEST(BehaviorTreeFactory, RejectedDocumentIsForgotten)
+{
+  // A document that failed to load used to stay in the parser as the "first
+  // opened document", whose root element instantiateTree() reads
+  // main_tree_to_execute from. For a document that did not even parse, that
+  // root element is null.
+  const char* valid_xml = R"(
+  <root BTCPP_format="4">
+    <BehaviorTree ID="Main">
+      <AlwaysSuccess/>
+    </BehaviorTree>
+  </root>)";
+
+  BehaviorTreeFactory factory;
+  {
+    XMLParser parser(factory);
+    EXPECT_THROW(parser.loadFromText("this is not XML"), RuntimeError);
+    parser.loadFromText(valid_xml);
+    Tree tree = parser.instantiateTree(Blackboard::create());
+    EXPECT_EQ(tree.tickOnce(), NodeStatus::SUCCESS);
+  }
+  {
+    // the main_tree_to_execute of a rejected document must not be used either
+    const char* rejected_xml = R"(
+    <root BTCPP_format="4" main_tree_to_execute="Rejected">
+      <BehaviorTree ID="Rejected">
+        <NonExistentNodeType/>
+      </BehaviorTree>
+    </root>)";
+    XMLParser parser(factory);
+    EXPECT_THROW(parser.loadFromText(rejected_xml), RuntimeError);
+    parser.loadFromText(valid_xml);
+    Tree tree = parser.instantiateTree(Blackboard::create());
+    EXPECT_EQ(tree.tickOnce(), NodeStatus::SUCCESS);
+  }
+  {
+    // nothing loaded: an error, not an access to the front of an empty list
+    XMLParser parser(factory);
+    EXPECT_THROW((void)parser.instantiateTree(Blackboard::create()), RuntimeError);
+  }
+
+  // The factory keeps a single parser: a file rejected once poisoned every
+  // later creation that has to fall back to the first document. Here two
+  // trees and no main_tree_to_execute must be reported as such.
+  EXPECT_THROW(factory.registerBehaviorTreeFromText("<root><BehaviorTree>"),
+               RuntimeError);
+  const char* two_trees_xml = R"(
+  <root BTCPP_format="4">
+    <BehaviorTree ID="A"><AlwaysSuccess/></BehaviorTree>
+    <BehaviorTree ID="B"><AlwaysSuccess/></BehaviorTree>
+  </root>)";
+  EXPECT_THROW((void)factory.createTreeFromText(two_trees_xml), RuntimeError);
+}
+
 TEST(BehaviorTreeFactory, TooManyNodesForUID)
 {
   // UIDs are 16 bits wide. Past 65535 nodes the counter used to wrap around