Remove robot not found error when parsing fails (#1290)

When a file fails to be parsed as SDF the parser.cc tries to parse it as URDF generating the following error: ``` Error: Could not find the 'robot' element in the xml file at line 109 in ./urdf_parser/src/model.cpp ``` This becomes confusing the file could just be a malformed SDF. This PR adds a check for the robot tag before attempting the parse in order to avoid the misleading error. It also adds an error in case it's not found indicating that the file could not be identified as SDF nor URDF. --------- Signed-off-by: Marco A. Gutierrez <[email protected]> Co-authored-by: Addisu Z. Taddese <[email protected]>
gazebosim · Jun 21, 2023 · b363e0e · b363e0e
1 parent a2eb081
commit b363e0e
Show file tree

Hide file tree

Showing 2 changed files with 129 additions and 25 deletions.
diff --git a/src/parser.cc b/src/parser.cc
@@ -768,25 +768,36 @@ bool readFileInternal(const std::string &_filename, const bool _convert,
     return false;
   }
 
-  // Suppress deprecation for sdf::URDF2SDF
-  if (readDoc(&xmlDoc, _sdf, filename, _convert, _config, _errors))
+  tinyxml2::XMLElement *sdfXml = xmlDoc.FirstChildElement("sdf");
+  if (sdfXml)
   {
-    return true;
+    // Suppress deprecation for sdf::URDF2SDF
+    return readDoc(&xmlDoc, _sdf, filename, _convert, _config, _errors);
   }
-  else if (URDF2SDF::IsURDF(filename))
+  else
   {
-    URDF2SDF u2g;
-    auto doc = makeSdfDoc();
-    u2g.InitModelFile(filename, _config, &doc);
-    if (sdf::readDoc(&doc, _sdf, filename, _convert, _config, _errors))
+    tinyxml2::XMLElement *robotXml = xmlDoc.FirstChildElement("robot");
+    if (robotXml)
     {
-      sdfdbg << "parse from urdf file [" << _filename << "].\n";
-      return true;
+      URDF2SDF u2g;
+      auto doc = makeSdfDoc();
+      u2g.InitModelFile(filename, _config, &doc);
+      if (sdf::readDoc(&doc, _sdf, filename, _convert, _config, _errors))
+      {
+        sdfdbg << "Converting URDF file [" << _filename << "] to SDFormat"
+               << " and parsing it.\n";
+        return true;
+      }
+      else
+      {
+        sdferr << "Failed to parse the URDF file after converting to"
+               << " SDFormat.\n";
+        return false;
+      }
     }
     else
     {
-      sdferr << "parse as old deprecated model file failed.\n";
-      return false;
+      sdferr << "XML does not seem to be an SDFormat or an URDF file.\n";
     }
   }
 
@@ -845,30 +856,40 @@ bool readStringInternal(const std::string &_xmlString, const bool _convert,
     sdferr << "Error parsing XML from string: " << xmlDoc.ErrorStr() << '\n';
     return false;
   }
-  if (readDoc(&xmlDoc, _sdf, std::string(kSdfStringSource), _convert, _config,
-              _errors))
+  tinyxml2::XMLElement *sdfXml = xmlDoc.FirstChildElement("sdf");
+  if (sdfXml)
   {
-    return true;
+    return readDoc(&xmlDoc, _sdf, std::string(kSdfStringSource), _convert,
+                   _config, _errors);
   }
   else
   {
-    URDF2SDF u2g;
-    auto doc = makeSdfDoc();
-    u2g.InitModelString(_xmlString, _config, &doc);
-
-    if (sdf::readDoc(&doc, _sdf, std::string(kUrdfStringSource), _convert,
-                    _config, _errors))
+    tinyxml2::XMLElement *robotXml = xmlDoc.FirstChildElement("robot");
+    if (robotXml)
     {
-      sdfdbg << "Parsing from urdf.\n";
-      return true;
+      URDF2SDF u2g;
+      auto doc = makeSdfDoc();
+      u2g.InitModelString(_xmlString, _config, &doc);
+
+      if (sdf::readDoc(&doc, _sdf, std::string(kUrdfStringSource), _convert,
+                      _config, _errors))
+      {
+        sdfdbg << "Converting URDF to SDFormat and parsing it.\n";
+        return true;
+      }
+      else
+      {
+        sdferr << "Failed to parse the URDF file after converting to"
+               << " SDFormat\n";
+        return false;
+      }
     }
     else
     {
-      sdferr << "parse as old deprecated model file failed.\n";
+      sdferr << "XML does not seem to be an SDFormat or an URDF string.\n";
       return false;
     }
   }
-
   return false;
 }
 

diff --git a/src/parser_TEST.cc b/src/parser_TEST.cc
@@ -835,6 +835,89 @@ TEST(Parser, ElementRemovedAfterDeprecation)
   EXPECT_EQ(errors[0].LineNumber().value(), 10);
 }
 
+/////////////////////////////////////////////////
+/// Check for non SDF non URDF error on a string
+TEST(Parser, ReadStringErrorNotSDForURDF)
+{
+  std::stringstream buffer;
+  sdf::testing::RedirectConsoleStream redir(
+      sdf::Console::Instance()->GetMsgStream(), &buffer);
+
+  #ifdef _WIN32
+    sdf::Console::Instance()->SetQuiet(false);
+    sdf::testing::ScopeExit revertSetQuiet(
+      []
+      {
+        sdf::Console::Instance()->SetQuiet(true);
+      });
+  #endif
+
+  sdf::SDFPtr sdf = InitSDF();
+  const std::string testString = R"(<?xml version="1.0" ?>
+    <foo>
+    </foo>)";
+  sdf::Errors errors;
+  EXPECT_FALSE(sdf::readString(testString, sdf, errors));
+  ASSERT_EQ(errors.size(), 0u);
+  EXPECT_NE(std::string::npos, buffer.str().find(
+      "XML does not seem to be an SDFormat or an URDF string."));
+}
+
+/////////////////////////////////////////////////
+/// Check for non SDF non URDF error on a file
+TEST(Parser, ReadFileErrorNotSDForURDF)
+{
+  std::stringstream buffer;
+  sdf::testing::RedirectConsoleStream redir(
+      sdf::Console::Instance()->GetMsgStream(), &buffer);
+
+  #ifdef _WIN32
+    sdf::Console::Instance()->SetQuiet(false);
+    sdf::testing::ScopeExit revertSetQuiet(
+      []
+      {
+        sdf::Console::Instance()->SetQuiet(true);
+      });
+  #endif
+
+  const auto path =
+      sdf::testing::TestFile("sdf", "box.dae");
+
+  sdf::Errors errors;
+  sdf::SDFPtr sdf = sdf::readFile(path, errors);
+  ASSERT_EQ(errors.size(), 0u);
+  EXPECT_NE(std::string::npos, buffer.str().find(
+      "XML does not seem to be an SDFormat or an URDF file."));
+}
+
+/////////////////////////////////////////////////
+/// Check that malformed SDF files do not throw confusing error
+TEST(Parser, ReadFileMalformedSDFNoRobotError)
+{
+  // Capture sdferr output
+  std::stringstream buffer;
+  auto old = std::cerr.rdbuf(buffer.rdbuf());
+
+#ifdef _WIN32
+  sdf::Console::Instance()->SetQuiet(false);
+#endif
+
+  const auto path =
+      sdf::testing::TestFile("sdf", "bad_syntax_pose.sdf");
+
+  sdf::Errors errors;
+  sdf::SDFPtr sdf = sdf::readFile(path, errors);
+  // Check the old error is not printed anymore
+  EXPECT_EQ(std::string::npos, buffer.str().find(
+      "Could not find the 'robot' element in the xml file"));
+
+  // Revert cerr rdbug so as to not interfere with other tests
+  std::cerr.rdbuf(old);
+#ifdef _WIN32
+  sdf::Console::Instance()->SetQuiet(true);
+#endif
+}
+
 /////////////////////////////////////////////////
 /// Main
 int main(int argc, char **argv)