Make exception for plugins when checking for name uniqueness (#721)

Adds an exception for the <plugin> element so that no warning is emitted when a plugin name matches the names of other entities.

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

include/sdf/Element.hh

@@ -381,6 +381,20 @@ namespace sdf
     /// names. Also return true if no elements of the specified type are found.
     public: bool HasUniqueChildNames(const std::string &_type = "") const;
 
+    /// \brief Checks whether any child elements of the specified element type,
+    /// except those listed in \p _ignoreElements, have identical name attribute
+    /// values and returns false if so.
+    /// \param[in] _type The type of Element to check. If empty, check names
+    /// of all child elements.
+    /// \param[in] _ignoreElements A list of child element types to ignore when
+    /// checking for uniqueness.
+    /// \return True if all child elements with name attributes of the
+    /// specified type have unique names, return false if there are duplicated
+    /// names. Also return true if no elements of the specified type are found.
+    public: bool HasUniqueChildNames(
+                const std::string &_type,
+                const std::vector<std::string> &_ignoreElements) const;
+
     /// \brief Count the number of child elements of the specified element type
     /// that have the same name attribute value.
     /// \param[in] _type The type of Element to check. If empty, count names
@@ -391,6 +405,20 @@ namespace sdf
     public: std::map<std::string, std::size_t>
             CountNamedElements(const std::string &_type = "") const;
 
+    /// \brief Count the number of child elements of the specified element type
+    /// that have the same name attribute value with the exception of elements
+    /// listed in \p _ignoreElements.
+    /// \param[in] _type The type of Element to check. If empty, count names
+    /// of all child elements.
+    /// \param[in] _ignoreElements A list of child element types to ignore when
+    /// checking for uniqueness.
+    /// \return Map from Element names to a count of how many times the name
+    /// occurs. The count should never be 0. If all 2nd values are 1, then
+    /// there are exclusively unique names.
+    public: std::map<std::string, std::size_t> CountNamedElements(
+                const std::string &_type,
+                const std::vector<std::string> &_ignoreElements) const;
+
     /// \brief Return a pointer to the child element with the provided name.
     ///
     /// A new child element, with the provided name, is added to this element
@@ -532,6 +560,11 @@ namespace sdf
     /// \return A pointer to the named element if found, nullptr otherwise.
     public: ElementPtr GetElementImpl(const std::string &_name) const;
 
+    /// \brief List of elements to which exceptions are made when checking for
+    /// name uniqueness.
+    /// \return List of element types that are allowed to have name collisions.
+    public: static std::vector<std::string> NameUniquenessExceptions();
+
     /// \brief Generate a string (XML) representation of this object.
     /// \param[in] _prefix arbitrary prefix to put on the string.
     /// \param[in] _includeDefaultElements flag to include default elements.

src/Element.cc

@@ -829,7 +829,15 @@ std::set<std::string> Element::GetElementTypeNames() const
 /////////////////////////////////////////////////
 bool Element::HasUniqueChildNames(const std::string &_type) const
 {
-  auto namedElementsCount = this->CountNamedElements(_type);
+  return this->HasUniqueChildNames(_type, {});
+}
+
+/////////////////////////////////////////////////
+bool Element::HasUniqueChildNames(
+    const std::string &_type,
+    const std::vector<std::string> &_ignoreElements) const
+{
+  auto namedElementsCount = this->CountNamedElements(_type, _ignoreElements);
   for (auto &iter : namedElementsCount)
   {
     if (iter.second > 1)
@@ -841,8 +849,16 @@ bool Element::HasUniqueChildNames(const std::string &_type) const
 }
 
 /////////////////////////////////////////////////
-std::map<std::string, std::size_t>
-Element::CountNamedElements(const std::string &_type) const
+std::map<std::string, std::size_t> Element::CountNamedElements(
+    const std::string &_type) const
+{
+  return this->CountNamedElements(_type, {});
+}
+
+/////////////////////////////////////////////////
+std::map<std::string, std::size_t> Element::CountNamedElements(
+    const std::string &_type,
+    const std::vector<std::string> &_ignoreElements) const
 {
   std::map<std::string, std::size_t> result;
 
@@ -858,7 +874,9 @@ Element::CountNamedElements(const std::string &_type) const
 
   while (elem)
   {
-    if (elem->HasAttribute("name"))
+    auto ignoreIt = std::find(_ignoreElements.begin(), _ignoreElements.end(),
+                              elem->GetName());
+    if (elem->HasAttribute("name") && ignoreIt == _ignoreElements.end())
     {
       // Get("name") returns attribute value if it exists before checking
       // for the value of a child element <name>, so it's safe to use
@@ -1192,3 +1210,10 @@ std::any Element::GetAny(const std::string &_key) const
   }
   return result;
 }
+
+//////////////////////////////////////////////////
+std::vector<std::string> Element::NameUniquenessExceptions()
+{
+  // We make exception for "plugin" when checking for name uniqueness.
+  return {"plugin"};
+}

src/Model.cc

@@ -33,6 +33,7 @@
 #include "FrameSemantics.hh"
 #include "ScopedGraph.hh"
 #include "Utils.hh"
+#include "sdf/parser.hh"
 
 using namespace sdf;
 
@@ -165,10 +166,15 @@ Errors Model::Load(sdf::ElementPtr _sdf, const ParserConfig &_config)
   // Load the pose. Ignore the return value since the model pose is optional.
   loadPose(_sdf, this->dataPtr->pose, this->dataPtr->poseRelativeTo);
 
-  if (!_sdf->HasUniqueChildNames())
+  for (const auto &[name, size] :
+       _sdf->CountNamedElements("", Element::NameUniquenessExceptions()))
   {
-    sdfwarn << "Non-unique names detected in XML children of model with name["
-            << this->Name() << "].\n";
+    if (size > 1)
+    {
+      sdfwarn << "Non-unique name[" << name << "] detected " << size
+              << " times in XML children of model with name[" << this->Name()
+              << "].\n";
+    }
   }
 
   // Set of implicit and explicit frame names in this model for tracking

src/World.cc

@@ -34,6 +34,7 @@
 #include "FrameSemantics.hh"
 #include "ScopedGraph.hh"
 #include "Utils.hh"
+#include "sdf/parser.hh"
 
 using namespace sdf;
 
@@ -197,10 +198,15 @@ Errors World::Load(sdf::ElementPtr _sdf, const ParserConfig &_config)
         sphericalCoordsErrors.end());
   }
 
-  if (!_sdf->HasUniqueChildNames())
+  for (const auto &[name, size] :
+       _sdf->CountNamedElements("", Element::NameUniquenessExceptions()))
   {
-    sdfwarn << "Non-unique names detected in XML children of world with name["
-            << this->Name() << "].\n";
+    if (size > 1)
+    {
+      sdfwarn << "Non-unique name[" << name << "] detected " << size
+              << " times in XML children of world with name[" << this->Name()
+              << "].\n";
+    }
   }
 
   // Set of implicit and explicit frame names in this model for tracking

src/ign_TEST.cc

@@ -114,6 +114,14 @@ TEST(check, IGN_UTILS_TEST_DISABLED_ON_WIN32(SDF))
     EXPECT_NE(output.find("Non-unique names"), std::string::npos)
       << output;
   }
+  // Check an SDF world file is allowed to have duplicate plugin names
+  {
+    std::string path = pathBase +"/world_duplicate_plugins.sdf";
+
+    std::string output =
+      custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion());
+    EXPECT_EQ("Valid.\n", output) << output;
+  }
 
   // Check an SDF file with sibling elements of the same type (link)
   // that have duplicate names.
@@ -151,6 +159,15 @@ TEST(check, IGN_UTILS_TEST_DISABLED_ON_WIN32(SDF))
       << output;
   }
 
+  // Check an SDF model file is allowed to have duplicate plugin names
+  {
+    std::string path = pathBase +"/model_duplicate_plugins.sdf";
+
+    std::string output =
+      custom_exec_str(IgnCommand() + " sdf -k " + path + SdfVersion());
+    EXPECT_EQ("Valid.\n", output) << output;
+  }
+
   // Check an SDF file with sibling elements of the same type (collision)
   // that have duplicate names.
   {

src/parser.cc

@@ -2310,7 +2310,8 @@ bool recursiveSiblingUniqueNames(sdf::ElementPtr _elem)
   if (!shouldValidateElement(_elem))
     return true;
 
-  bool result = _elem->HasUniqueChildNames();
+  bool result =
+      _elem->HasUniqueChildNames("", Element::NameUniquenessExceptions());
   if (!result)
   {
     std::cerr << "Error: Non-unique names detected in "

test/integration/model_dom.cc

@@ -29,6 +29,7 @@
 #include "sdf/Types.hh"
 #include "sdf/World.hh"
 #include "test_config.h"
+#include "test_utils.hh"
 
 //////////////////////////////////////////////////
 TEST(DOMModel, NotAModel)
@@ -578,3 +579,52 @@ TEST(DOMRoot, LoadInvalidNestedModelWithoutLinks)
   // errors[5]
   // errors[6]
 }
+
+/////////////////////////////////////////////////
+TEST(DOMModel, LoadModelWithDuplicateChildNames)
+{
+  // Redirect sdfwarn output
+  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
+
+  {
+    buffer.str("");
+    const std::string testFile =
+      sdf::testing::TestFile("sdf", "model_link_joint_same_name.sdf");
+
+    // Load the SDF file
+    sdf::Root root;
+    auto errors = root.Load(testFile);
+    EXPECT_TRUE(errors.empty()) << errors;
+
+    // Check warning message
+    EXPECT_NE(
+        std::string::npos,
+        buffer.str().find("Non-unique name[attachment] detected 2 times in XML "
+                          "children of model with name[link_joint_same_name]"))
+        << buffer.str();
+  }
+
+  // Check that there's an exception for "plugin" elements
+  {
+    buffer.str("");
+    const std::string testFile =
+      sdf::testing::TestFile("sdf", "model_duplicate_plugins.sdf");
+
+    // Load the SDF file
+    sdf::Root root;
+    auto errors = root.Load(testFile);
+    EXPECT_TRUE(errors.empty()) << errors;
+    EXPECT_TRUE(buffer.str().empty()) << buffer.str();
+  }
+}

test/integration/world_dom.cc

@@ -26,6 +26,7 @@
 #include "sdf/Root.hh"
 #include "sdf/World.hh"
 #include "test_config.h"
+#include "test_utils.hh"
 
 //////////////////////////////////////////////////
 TEST(DOMWorld, NoName)
@@ -309,3 +310,51 @@ TEST(DOMWorld, NestedFrames)
       nullptr,
       world->FrameByName("top_level_model::nested_model::nested_model_frame"));
 }
+
+
+/////////////////////////////////////////////////
+TEST(DOMWorld, LoadWorldWithDuplicateChildNames)
+{
+  // Redirect sdfwarn output
+  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
+
+  {
+    buffer.str("");
+    const std::string testFile =
+      sdf::testing::TestFile("sdf", "world_sibling_same_names.sdf");
+
+    // Load the SDF file
+    sdf::Root root;
+    auto errors = root.Load(testFile);
+    EXPECT_TRUE(errors.empty()) << errors;
+
+    // Check warning message
+    EXPECT_NE(std::string::npos,
+        buffer.str().find("Non-unique name[spot] detected 2 times in XML "
+          "children of world with name[default]"));
+  }
+
+  // Check that there's an exception for "plugin" elements
+  {
+    buffer.str("");
+    const std::string testFile =
+      sdf::testing::TestFile("sdf", "world_duplicate_plugins.sdf");
+
+    // Load the SDF file
+    sdf::Root root;
+    auto errors = root.Load(testFile);
+    EXPECT_TRUE(errors.empty()) << errors;
+    EXPECT_TRUE(buffer.str().empty()) << buffer.str();
+  }
+}

test/sdf/model_duplicate_plugins.sdf

@@ -0,0 +1,13 @@
+<?xml version="1.0" ?>
+<sdf version="1.7">
+  <model name="model_duplicate_plugins">
+    <link name="link1">
+      <sensor name="test_sensor" type="imu">
+        <plugin name="sensor_dup_name" filename="test/file/name"/>
+        <plugin name="sensor_dup_name" filename="test/file/name"/>
+      </sensor>
+    </link>
+    <plugin name="dup_name" filename="test/file/name"/>
+    <plugin name="dup_name" filename="test/file/name"/>
+  </model>
+</sdf>

test/sdf/world_duplicate_plugins.sdf

@@ -0,0 +1,16 @@
+<?xml version="1.0" ?>
+<sdf version="1.7">
+  <world name="world_duplicate_plugins">
+    <model name="test_model">
+      <link name="link1">
+        <sensor name="test_sensor" type="imu">
+          <plugin name="sensor_dup_name" filename="test/file/name"/>
+          <plugin name="sensor_dup_name" filename="test/file/name"/>
+        </sensor>
+      </link>
+    </model>
+
+    <plugin name="dup_name" filename="test/file/name"/>
+    <plugin name="dup_name" filename="test/file/name"/>
+  </world>
+</sdf>