Error when delimiter "::" found in element name in SDFormat 1.8 (#515)

* error when '::' found in elem name Signed-off-by: Jenn Nguyen <[email protected]> * check version using math::SemanticVersion Signed-off-by: Jenn Nguyen <[email protected]> * pass errors to readString in test Signed-off-by: Jenn Nguyen <[email protected]>
gazebosim · Mar 18, 2021 · 936a2e3 · 936a2e3
1 parent 1333f59
commit 936a2e3
Show file tree

Hide file tree

Showing 4 changed files with 93 additions and 0 deletions.
diff --git a/Migration.md b/Migration.md
@@ -327,6 +327,9 @@ but with improved human-readability..
 
 1. **heightmap.sdf**: sampling now defaults to 1 instead of 2.
 
+1. Element attribute names containing delimiter "::" no longer accepted
+    * [Issue 420](https://github.com/osrf/sdformat/issues/420)
+
 ### Deprecations
 
 1. **joint.sdf** `initial_position` element in `<joint><axis>` and `<joint><axis2>` is deprecated

diff --git a/include/sdf/parser.hh b/include/sdf/parser.hh
@@ -417,6 +417,16 @@ namespace sdf
   SDFORMAT_VISIBLE
   bool recursiveSiblingUniqueNames(sdf::ElementPtr _elem);
 
+  /// \brief Check that all sibling elements do not contain the delimiter
+  /// double colons '::' in element names, which is reserved for forming scopes
+  /// in SDFormat 1.8. This checks recursively and should check the files
+  /// exhaustively rather than terminating early when the first name
+  /// containing '::' is found.
+  /// \param[in] _elem SDF Element to check recursively.
+  /// \return True if all contained element names do not have the delimiter '::'
+  SDFORMAT_VISIBLE
+  bool recursiveSiblingNoDoubleColonInNames(sdf::ElementPtr _elem);
+
   /// \brief Check whether the element should be validated. If this returns
   /// false, validators such as the unique name and reserve name checkers should
   /// skip this element and its descendants.

diff --git a/src/parser.cc b/src/parser.cc
@@ -685,6 +685,18 @@ bool readDoc(tinyxml2::XMLDocument *_xmlDoc, SDFPtr _sdf,
           "Error reading element <" + _sdf->Root()->GetName() + ">"});
       return false;
     }
+
+    // delimiter '::' in element names not allowed in SDFormat >= 1.8
+    ignition::math::SemanticVersion sdfVersion(_sdf->Root()->OriginalVersion());
+    if (sdfVersion >= ignition::math::SemanticVersion(1, 8)
+        && !recursiveSiblingNoDoubleColonInNames(_sdf->Root()))
+    {
+      _errors.push_back({ErrorCode::RESERVED_NAME,
+          "Delimiter '::' found in attribute names of element <"
+          + _sdf->Root()->GetName() +
+          ">, which is not allowed in SDFormat >= 1.8"});
+      return false;
+    }
   }
   else
   {
@@ -755,6 +767,18 @@ bool readDoc(tinyxml2::XMLDocument *_xmlDoc, ElementPtr _sdf,
           "Unable to parse sdf element["+ _sdf->GetName() + "]"});
       return false;
     }
+
+    // delimiter '::' in element names not allowed in SDFormat >= 1.8
+    ignition::math::SemanticVersion sdfVersion(_sdf->OriginalVersion());
+    if (sdfVersion >= ignition::math::SemanticVersion(1, 8)
+        && !recursiveSiblingNoDoubleColonInNames(_sdf))
+    {
+      _errors.push_back({ErrorCode::RESERVED_NAME,
+          "Delimiter '::' found in attribute names of element <"
+          + _sdf->GetName() +
+          ">, which is not allowed in SDFormat >= 1.8"});
+      return false;
+    }
   }
   else
   {
@@ -1739,6 +1763,32 @@ bool recursiveSiblingUniqueNames(sdf::ElementPtr _elem)
   return result;
 }
 
+//////////////////////////////////////////////////
+bool recursiveSiblingNoDoubleColonInNames(sdf::ElementPtr _elem)
+{
+  if (!shouldValidateElement(_elem))
+    return true;
+
+  bool result = true;
+  if (_elem->HasAttribute("name")
+      && _elem->Get<std::string>("name").find("::") != std::string::npos)
+  {
+    std::cerr << "Error: Detected delimiter '::' in element name in\n"
+             << _elem->ToString("")
+             << std::endl;
+    result = false;
+  }
+
+  sdf::ElementPtr child = _elem->GetFirstElement();
+  while (child)
+  {
+    result = recursiveSiblingNoDoubleColonInNames(child) && result;
+    child = child->GetNextElement();
+  }
+
+  return result;
+}
+
 //////////////////////////////////////////////////
 bool checkFrameAttachedToGraph(const sdf::Root *_root)
 {

diff --git a/src/parser_TEST.cc b/src/parser_TEST.cc
@@ -353,6 +353,36 @@ TEST(Parser, PlacementFrameMissingPose)
   EXPECT_EQ(sdf::ErrorCode::MODEL_PLACEMENT_FRAME_INVALID, errors[0].Code());
 }
 
+/////////////////////////////////////////////////
+// Delimiter '::' in name should error in SDFormat 1.8 but not in 1.7
+TEST(Parser, DoubleColonNameAttrError)
+{
+  sdf::SDFPtr sdf = InitSDF();
+  std::ostringstream stream;
+  stream << "<?xml version=\"1.0\"?>"
+         << "<sdf version='1.8'>"
+         << "  <model name='test'>"
+         << "    <link name='A::B'/>"
+         << "  </model>"
+         << "</sdf>";
+
+  sdf::Errors errors;
+  EXPECT_FALSE(sdf::readString(stream.str(), sdf, errors));
+  ASSERT_EQ(errors.size(), 1u);
+  EXPECT_EQ(errors[0].Code(), sdf::ErrorCode::RESERVED_NAME);
+
+  sdf = InitSDF();
+  stream.str("");
+  stream << "<?xml version=\"1.0\"?>"
+         << "<sdf version='1.7'>"
+         << "  <model name='test::A'/>"
+         << "</sdf>";
+
+  errors.clear();
+  EXPECT_TRUE(sdf::readString(stream.str(), sdf, errors));
+  EXPECT_EQ(errors.size(), 0u);
+}
+
 /////////////////////////////////////////////////
 /// Fixture for setting up stream redirection
 class ValueConstraintsFixture : public ::testing::Test