Merged in name_collisions_fail_17 (pull request gazebosim#648)

Model::Load: fail fast if an sdf 1.7 file has name collisions

Approved-by: Eric Cousineau <eacousineau@gmail.com>
Approved-by: Addisu Z. Taddese <addisu@openrobotics.org>

Changelog.md

@@ -34,6 +34,9 @@
 1. Access the original parsed version of an SDF document with `Element::OriginalVersion`.
     * [Pull request 640](https://bitbucket.org/osrf/sdformat/pull-requests/640)
 
+1. Model::Load: fail fast if an sdf 1.7 file has name collisions.
+    * [Pull request 648](https://bitbucket.org/osrf/sdformat/pull-requests/648)
+
 ### SDFormat 9.0.0 (2019-12-10)
 
 1. Move recursiveSameTypeUniqueNames from ign.cc to parser.cc and make public.

src/Model.cc

@@ -19,6 +19,7 @@
 #include <unordered_set>
 #include <vector>
 #include <ignition/math/Pose3.hh>
+#include <ignition/math/SemanticVersion.hh>
 #include "sdf/Error.hh"
 #include "sdf/Frame.hh"
 #include "sdf/Joint.hh"
@@ -147,6 +148,7 @@ Errors Model::Load(ElementPtr _sdf)
   Errors errors;
 
   this->dataPtr->sdf = _sdf;
+  ignition::math::SemanticVersion sdfVersion(_sdf->OriginalVersion());
 
   // Check that the provided SDF element is a <model>
   // This is an error that cannot be recovered, so return an error.
@@ -246,17 +248,28 @@ Errors Model::Load(ElementPtr _sdf)
     std::string jointName = joint.Name();
     if (frameNames.count(jointName) > 0)
     {
-      jointName += "_joint";
-      int i = 0;
-      while (frameNames.count(jointName) > 0)
+      // This joint has a name collision
+      if (sdfVersion < ignition::math::SemanticVersion(1, 7))
       {
-        jointName = joint.Name() + "_joint" + std::to_string(i++);
+        // This came from an old file, so try to workaround by renaming joint
+        jointName += "_joint";
+        int i = 0;
+        while (frameNames.count(jointName) > 0)
+        {
+          jointName = joint.Name() + "_joint" + std::to_string(i++);
+        }
+        sdfwarn << "Joint with name [" << joint.Name() << "] "
+                << "in model with name [" << this->Name() << "] "
+                << "has a name collision, changing joint name to ["
+                << jointName << "].\n";
+        joint.SetName(jointName);
+      }
+      else
+      {
+        sdferr << "Joint with name [" << joint.Name() << "] "
+               << "in model with name [" << this->Name() << "] "
+               << "has a name collision. Please rename this joint.\n";
       }
-      sdfwarn << "Joint with name [" << joint.Name() << "] "
-              << "in model with name [" << this->Name() << "] "
-              << "has a name collision, changing joint name to ["
-              << jointName << "].\n";
-      joint.SetName(jointName);
     }
     frameNames.insert(jointName);
   }
@@ -272,17 +285,28 @@ Errors Model::Load(ElementPtr _sdf)
     std::string frameName = frame.Name();
     if (frameNames.count(frameName) > 0)
     {
-      frameName += "_frame";
-      int i = 0;
-      while (frameNames.count(frameName) > 0)
+      // This joint has a name collision
+      if (sdfVersion < ignition::math::SemanticVersion(1, 7))
+      {
+        // This came from an old file, so try to workaround by renaming frame
+        frameName += "_frame";
+        int i = 0;
+        while (frameNames.count(frameName) > 0)
+        {
+          frameName = frame.Name() + "_frame" + std::to_string(i++);
+        }
+        sdfwarn << "Frame with name [" << frame.Name() << "] "
+                << "in model with name [" << this->Name() << "] "
+                << "has a name collision, changing frame name to ["
+                << frameName << "].\n";
+        frame.SetName(frameName);
+      }
+      else
       {
-        frameName = frame.Name() + "_frame" + std::to_string(i++);
+        sdferr << "Frame with name [" << frame.Name() << "] "
+               << "in model with name [" << this->Name() << "] "
+               << "has a name collision. Please rename this frame.\n";
       }
-      sdfwarn << "Frame with name [" << frame.Name() << "] "
-              << "in model with name [" << this->Name() << "] "
-              << "has a name collision, changing frame name to ["
-              << frameName << "].\n";
-      frame.SetName(frameName);
     }
     frameNames.insert(frameName);
   }

test/integration/joint_dom.cc

@@ -25,7 +25,9 @@
 #include "sdf/Link.hh"
 #include "sdf/Model.hh"
 #include "sdf/Root.hh"
+#include "sdf/SDFImpl.hh"
 #include "sdf/Types.hh"
+#include "sdf/parser.hh"
 #include "test_config.h"
 
 //////////////////////////////////////////////////
@@ -373,7 +375,38 @@ TEST(DOMJoint, LoadInvalidChild)
 }
 
 /////////////////////////////////////////////////
-TEST(DOMJoint, LoadLinkJointSameName)
+TEST(DOMJoint, LoadLinkJointSameName17Invalid)
+{
+  const std::string testFile =
+    sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "sdf",
+        "model_link_joint_same_name.sdf");
+
+  // Read with sdf::readFile, which converts from 1.6 to latest
+  sdf::SDFPtr sdf(new sdf::SDF());
+  sdf::init(sdf);
+  sdf::readFile(testFile, sdf);
+
+  // Load the SDF file from the converted string and expect errors
+  sdf::Root root;
+  auto errors = root.LoadSdfString(sdf->Root()->ToString(""));
+  for (auto e : errors)
+    std::cout << e << std::endl;
+  EXPECT_FALSE(errors.empty());
+  EXPECT_EQ(2u, errors.size());
+  EXPECT_EQ(errors[0].Code(), sdf::ErrorCode::DUPLICATE_NAME);
+  EXPECT_NE(std::string::npos,
+    errors[0].Message().find(
+      "Joint with non-unique name [attachment] detected in model with name "
+      "[link_joint_same_name]."));
+  EXPECT_EQ(errors[1].Code(), sdf::ErrorCode::DUPLICATE_NAME);
+  EXPECT_NE(std::string::npos,
+    errors[1].Message().find(
+      "Joint with non-unique name [attachment] detected in model with name "
+      "[link_joint_same_name]."));
+}
+
+/////////////////////////////////////////////////
+TEST(DOMJoint, LoadLinkJointSameName16Valid)
 {
   const std::string testFile =
     sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "sdf",