Backport #721 to sdf9: Make exception for plugins when checking for n…

…ame uniqueness (#733)

* Backport test utilities from sdf10
* Backport Console redirect functionality
* Make exception for plugins when checking for name uniqueness (#721)
* Replace unavailable error printing API

Signed-off-by: Addisu Z. Taddese <[email protected]>

include/sdf/Console.hh

@@ -92,6 +92,16 @@ namespace sdf
                           const std::string &_file,
                           unsigned int _line, int _color);
 
+      /// \brief Set the stream object.
+      /// \param[in] _stream Pointer to an output stream. This can be
+      /// useful for redirecting the output, for example, to a std::stringstream
+      /// for testing.
+      public: void SetStream(std::ostream *_stream);
+
+      /// \brief Get the current stream object.
+      /// \return Pointer to current stream object.
+      public: std::ostream *GetStream();
+
       /// \brief The ostream to log to; can be NULL/nullptr.
       private: std::ostream *stream;
     };
@@ -128,6 +138,18 @@ namespace sdf
                                const std::string &file,
                                unsigned int line);
 
+    /// \brief Get the current message stream object. This can be
+    /// useful for redirecting the output, for example, to a std::stringstream
+    /// for testing.
+    /// \return Mutable reference to current message stream object.
+    public: ConsoleStream &GetMsgStream();
+
+    /// \brief Get the current log stream object. This can be
+    /// useful for redirecting the output, for example, to a std::stringstream
+    /// for testing.
+    /// \return Mutable reference to current log stream object.
+    public: ConsoleStream &GetLogStream();
+
     /// \internal
     /// \brief Pointer to private data.
     private: std::unique_ptr<ConsolePrivate> dataPtr;

include/sdf/Element.hh

@@ -350,6 +350,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
@@ -360,6 +374,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
@@ -467,6 +495,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/Console.cc

@@ -105,6 +105,18 @@ void Console::SetQuiet(bool _quiet)
   g_quiet = _quiet;
 }
 
+//////////////////////////////////////////////////
+sdf::Console::ConsoleStream &Console::GetMsgStream()
+{
+  return this->dataPtr->msgStream;
+}
+
+//////////////////////////////////////////////////
+sdf::Console::ConsoleStream &Console::GetLogStream()
+{
+  return this->dataPtr->logStream;
+}
+
 //////////////////////////////////////////////////
 Console::ConsoleStream &Console::ColorMsg(const std::string &lbl,
                                           const std::string &file,
@@ -157,3 +169,15 @@ void Console::ConsoleStream::Prefix(const std::string &_lbl,
       _file.substr(index , _file.size() - index)<< ":" << _line << "] ";
   }
 }
+
+//////////////////////////////////////////////////
+void Console::ConsoleStream::SetStream(std::ostream *_stream)
+{
+  this->stream = _stream;
+}
+
+//////////////////////////////////////////////////
+std::ostream *Console::ConsoleStream::GetStream()
+{
+  return this->stream;
+}

src/Element.cc

@@ -764,7 +764,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)
@@ -776,8 +784,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;
 
@@ -793,7 +809,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
@@ -1101,3 +1119,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

@@ -28,6 +28,7 @@
 #include "sdf/Types.hh"
 #include "FrameSemantics.hh"
 #include "Utils.hh"
+#include "sdf/parser.hh"
 
 using namespace sdf;
 
@@ -207,10 +208,15 @@ Errors Model::Load(ElementPtr _sdf)
   // 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

@@ -28,6 +28,7 @@
 #include "sdf/World.hh"
 #include "FrameSemantics.hh"
 #include "Utils.hh"
+#include "sdf/parser.hh"
 
 using namespace sdf;
 
@@ -255,10 +256,15 @@ Errors World::Load(sdf::ElementPtr _sdf)
     _sdf->Get<ignition::math::Vector3d>("magnetic_field",
         this->dataPtr->magneticField).first;
 
-  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

@@ -112,6 +112,14 @@ TEST(check, SDF)
     EXPECT_NE(output.find("Error: 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.
@@ -149,6 +157,15 @@ TEST(check, 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

@@ -1677,7 +1677,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)
@@ -374,3 +375,59 @@ TEST(DOMRoot, LoadNestedCanonicalLink)
   EXPECT_EQ("deep::deeper::deepest::deepest_link", body);
 }
 
+/////////////////////////////////////////////////
+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());
+    for (const auto &err : errors)
+    {
+      std::cout << err << std::endl;
+    }
+
+    // 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());
+    for (const auto &err : errors)
+    {
+      std::cout << err << std::endl;
+    }
+    EXPECT_TRUE(buffer.str().empty()) << buffer.str();
+  }
+}