From 1ffcca5eefdf739def3b80874f88e9a05368d2eb Mon Sep 17 00:00:00 2001 From: "Addisu Z. Taddese" Date: Wed, 29 Sep 2021 23:14:23 -0500 Subject: [PATCH] Make exception for plugins when checking for name uniqueness (#721) Adds an exception for the element so that no warning is emitted when a plugin name matches the names of other entities. Signed-off-by: Addisu Z. Taddese --- include/sdf/Element.hh | 33 ++++++++++++++++++ src/Element.cc | 33 +++++++++++++++--- src/Model.cc | 12 +++++-- src/World.cc | 12 +++++-- src/ign_TEST.cc | 17 ++++++++++ src/parser.cc | 3 +- test/integration/model_dom.cc | 50 ++++++++++++++++++++++++++++ test/integration/world_dom.cc | 49 +++++++++++++++++++++++++++ test/sdf/model_duplicate_plugins.sdf | 13 ++++++++ test/sdf/world_duplicate_plugins.sdf | 16 +++++++++ 10 files changed, 227 insertions(+), 11 deletions(-) create mode 100644 test/sdf/model_duplicate_plugins.sdf create mode 100644 test/sdf/world_duplicate_plugins.sdf diff --git a/include/sdf/Element.hh b/include/sdf/Element.hh index 088a31694..0a0d3fbb9 100644 --- a/include/sdf/Element.hh +++ b/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 &_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 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 CountNamedElements( + const std::string &_type, + const std::vector &_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 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. diff --git a/src/Element.cc b/src/Element.cc index 35b9fda73..091c7df24 100644 --- a/src/Element.cc +++ b/src/Element.cc @@ -829,7 +829,15 @@ std::set 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 &_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 -Element::CountNamedElements(const std::string &_type) const +std::map Element::CountNamedElements( + const std::string &_type) const +{ + return this->CountNamedElements(_type, {}); +} + +///////////////////////////////////////////////// +std::map Element::CountNamedElements( + const std::string &_type, + const std::vector &_ignoreElements) const { std::map 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 , so it's safe to use @@ -1192,3 +1210,10 @@ std::any Element::GetAny(const std::string &_key) const } return result; } + +////////////////////////////////////////////////// +std::vector Element::NameUniquenessExceptions() +{ + // We make exception for "plugin" when checking for name uniqueness. + return {"plugin"}; +} diff --git a/src/Model.cc b/src/Model.cc index 8fdd68181..791510f69 100644 --- a/src/Model.cc +++ b/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 diff --git a/src/World.cc b/src/World.cc index a6a9d77f9..b4849f3db 100644 --- a/src/World.cc +++ b/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 diff --git a/src/ign_TEST.cc b/src/ign_TEST.cc index c3e8adfac..f1d269ecf 100644 --- a/src/ign_TEST.cc +++ b/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. { diff --git a/src/parser.cc b/src/parser.cc index 76bfbc30b..5f29f93d9 100644 --- a/src/parser.cc +++ b/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 " diff --git a/test/integration/model_dom.cc b/test/integration/model_dom.cc index ff46d7ec6..69bc6cb89 100644 --- a/test/integration/model_dom.cc +++ b/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(); + } +} diff --git a/test/integration/world_dom.cc b/test/integration/world_dom.cc index 9ad37bf00..21310cc2c 100644 --- a/test/integration/world_dom.cc +++ b/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(); + } +} diff --git a/test/sdf/model_duplicate_plugins.sdf b/test/sdf/model_duplicate_plugins.sdf new file mode 100644 index 000000000..a3d2f38e8 --- /dev/null +++ b/test/sdf/model_duplicate_plugins.sdf @@ -0,0 +1,13 @@ + + + + + + + + + + + + + diff --git a/test/sdf/world_duplicate_plugins.sdf b/test/sdf/world_duplicate_plugins.sdf new file mode 100644 index 000000000..a5c28e7c3 --- /dev/null +++ b/test/sdf/world_duplicate_plugins.sdf @@ -0,0 +1,16 @@ + + + + + + + + + + + + + + + +