Check for SDF files containing sibling elements with the same name (Forward port of #3016)

Changelog.md

@@ -1,5 +1,13 @@
 ## Gazebo 11
 
+## Gazebo 11.6.0 (2021-06-xx)
+
+1. Enable output of gzerr for SDF sibling elements of any type with same name,
+   following the SDF 1.7 specification.
+   Environment variable GAZEBO11_BACKWARDS_COMPAT_WARNINGS_ERRORS can be set to
+   use the previous behaviour and do not report these problems.
+    * [Pull request #3017](https://github.com/osrf/gazebo/pull/3017)
+
 ## Gazebo 11.5.1 (2021-05-05)
 
 1. Avoid range-loop-construct in TopicManager

gazebo/Server.cc

@@ -30,9 +30,11 @@
 #include <boost/lexical_cast.hpp>
 #include <boost/program_options.hpp>
 
+#include <sdf/parser.hh>
 #include <sdf/sdf.hh>
 
 #include <ignition/math/Rand.hh>
+#include <ignition/math/SemanticVersion.hh>
 #include <ignition/common/Profiler.hh>
 #include <ignition/common/Filesystem.hh>
 #include <ignition/common/URI.hh>
@@ -77,6 +79,26 @@ namespace gazebo
 {
   struct ServerPrivate
   {
+    void InspectSDFElement(const sdf::ElementPtr _elem)
+    {
+      if (common::getEnv("GAZEBO11_BACKWARDS_COMPAT_WARNINGS_ERRORS"))
+        return;
+
+      // SDF 1.7 and above consider invalid sibling elements of ANY type with
+      // repeated names while SDF 1.6 and lower only elements of the SAME type
+      auto sdfVersion =
+        ignition::math::SemanticVersion(_elem->OriginalVersion());
+      bool result;
+      if (sdfVersion >= ignition::math::SemanticVersion(1, 7))
+        result = sdf::recursiveSiblingUniqueNames(_elem);
+      else
+        result = sdf::recursiveSameTypeUniqueNames(_elem);
+
+      if (not result)
+        gzerr << "SDF is not valid, see errors above. "
+              << "This can lead to an unexpected behaviour." << "\n";
+    }
+
     /// \brief Boolean used to stop the server.
     static bool stop;
 
@@ -496,6 +518,8 @@ bool Server::PreLoad()
 bool Server::LoadImpl(sdf::ElementPtr _elem,
                       const std::string &_physics)
 {
+  this->dataPtr->InspectSDFElement(_elem);
+
   // If a physics engine is specified,
   if (_physics.length())
   {

test/integration/CMakeLists.txt

@@ -95,6 +95,7 @@ set(tests
   rest_web.cc
   saving_and_loading.cc
   sdf.cc
+  sdf_errors.cc
   sensor.cc
   sdf_frame_semantics.cc
   server_fixture.cc

test/integration/sdf_errors.cc

@@ -0,0 +1,144 @@
+/*
+ * Copyright (C) 2021 Open Source Robotics Foundation
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+*/
+
+#include <gtest/gtest.h>
+
+#include "gazebo/common/CommonIface.hh"
+#include "gazebo/test/ServerFixture.hh"
+
+using namespace gazebo;
+
+/////////////////////////////////////////////////
+#ifdef _WIN32
+static int setenv(const char *envname, const char *envval, int overwrite)
+{
+  char *original = getenv(envname);
+  if (!original || !!overwrite)
+  {
+    std::string envstring = std::string(envname) + "=" + envval;
+    return _putenv(envstring.c_str());
+  }
+  return 0;
+}
+#endif
+
+class SDFLogsTest : public ServerFixture
+{
+  public: void SetUp()
+  {
+#ifndef _WIN32
+  const boost::filesystem::path home = common::getEnv("HOME");
+#else
+  const boost::filesystem::path home = common::getEnv("HOMEPATH");
+#endif
+    boost::filesystem::path log_path("/.gazebo/server-11345/default.log");
+    path = home / log_path;
+  }
+
+  public: void EXPECT_LOG_STRING(const std::string expected_text)
+  {
+    EXPECT_TRUE(log_string_search(expected_text)) <<
+      "The text '" + expected_text + "'" +
+      " was not found in the log. The test expects it to be there";
+  }
+
+  public: void EXPECT_NO_ERR_IN_LOG()
+  {
+    EXPECT_NO_LOG_STRING("[Err] ");
+  }
+
+  public: void EXPECT_NO_LOG_STRING(const std::string no_expected_text)
+  {
+    EXPECT_FALSE(log_string_search(no_expected_text)) <<
+      "The text '" + no_expected_text + "'" +
+      " was found in the log. The test does not expect it be there";
+  }
+
+  public: void EXPECT_SDF_ERR_IN_LOG()
+  {
+    EXPECT_LOG_STRING("[Err] ");
+    std::string sdfErrorString = "SDF is not valid";
+    EXPECT_LOG_STRING(sdfErrorString);
+  }
+
+  private: bool log_string_search(const std::string expected_text)
+  {
+    // Open the log file, and read back the string
+    std::ifstream ifs(path.string().c_str(), std::ios::in);
+    std::string loggedString;
+
+    while (!ifs.eof())
+    {
+      std::string line;
+      std::getline(ifs, line);
+      loggedString += line;
+    }
+
+    return loggedString.find(expected_text) != std::string::npos;
+  }
+
+  private: boost::filesystem::path path;
+};
+
+/////////////////////////////////////////////////
+TEST_F(SDFLogsTest, EmptyWorldNoErrors)
+{
+  Load("worlds/empty.world");
+  EXPECT_NO_ERR_IN_LOG();
+}
+
+/////////////////////////////////////////////////
+TEST_F(SDFLogsTest, DuplicateSiblingSameType16)
+{
+  Load("worlds/test_sdf16_err_sibling_same_type.world");
+  EXPECT_SDF_ERR_IN_LOG();
+}
+
+/////////////////////////////////////////////////
+TEST_F(SDFLogsTest, DuplicateSiblingSameTypeDisabled16)
+{
+  setenv("GAZEBO11_BACKWARDS_COMPAT_WARNINGS_ERRORS", "", 1);
+  Load("worlds/test_sdf16_err_sibling_same_type.world");
+  EXPECT_NO_ERR_IN_LOG();
+#ifdef _WIN32
+ _putenv("GAZEBO11_BACKWARDS_COMPAT_WARNINGS_ERRORS");
+#else
+  unsetenv("GAZEBO11_BACKWARDS_COMPAT_WARNINGS_ERRORS");
+#endif
+}
+
+/////////////////////////////////////////////////
+TEST_F(SDFLogsTest, DuplicateSiblingDifferentType16)
+{
+  Load("worlds/test_sdf16_err_sibling_different_type.world");
+  // 1.6 SDF does NOT enforce different names for different types
+  EXPECT_NO_ERR_IN_LOG();
+}
+
+TEST_F(SDFLogsTest, DuplicateSiblingDifferentType17)
+{
+  // 1.7+ SDF does enforce different names for different types
+  Load("worlds/test_sdf17_err_sibling_different_type.world");
+  EXPECT_SDF_ERR_IN_LOG();
+}
+
+/////////////////////////////////////////////////
+int main(int argc, char **argv)
+{
+  ::testing::InitGoogleTest(&argc, argv);
+  return RUN_ALL_TESTS();
+}

test/worlds/test_sdf16_err_sibling_different_type.world

@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<sdf version='1.6'>
+  <world name='default'>
+
+    <model name='wall'>
+      <static>1</static>
+      <pose frame=''>0 0 1 0 0 1</pose>
+      <link name='wall'>
+        <collision name='repeated_name'>
+          <pose frame=''>-2.5 0 0 0 0 0</pose>
+          <geometry>
+            <box>
+              <size>5 0.2 2</size>
+            </box>
+          </geometry>
+        </collision>
+         <collision name='collision_1'>
+          <pose frame=''>-2.5 0 0 0 0 0</pose>
+          <geometry>
+            <box>
+              <size>5 0.2 2</size>
+            </box>
+          </geometry>
+        </collision>
+         <collision name='collision_2'>
+          <pose frame=''>-2.5 0 0 0 0 0</pose>
+          <geometry>
+            <box>
+              <size>5 0.2 2</size>
+            </box>
+          </geometry>
+        </collision>
+        <visual name='repeated_name'>
+          <pose frame=''>-2.5 0 0 0 0 0</pose>
+          <geometry>
+            <box>
+              <size>5 0.2 2</size>
+            </box>
+          </geometry>
+        </visual>
+      </link>
+    </model>
+
+  </world>
+</sdf>

test/worlds/test_sdf16_err_sibling_same_type.world

@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<sdf version='1.6'>
+  <world name='default'>
+
+    <model name='wall'>
+      <static>1</static>
+      <pose frame=''>0 0 1 0 0 1</pose>
+      <link name='wall'>
+        <collision name='collision_1'>
+          <pose frame=''>-2.5 0 0 0 0 0</pose>
+          <geometry>
+            <box>
+              <size>5 0.2 2</size>
+            </box>
+          </geometry>
+        </collision>
+         <collision name='collision_1'>
+          <pose frame=''>-2.5 0 0 0 0 0</pose>
+          <geometry>
+            <box>
+              <size>5 0.2 2</size>
+            </box>
+          </geometry>
+        </collision>
+         <collision name='collision_1'>
+          <pose frame=''>-2.5 0 0 0 0 0</pose>
+          <geometry>
+            <box>
+              <size>5 0.2 2</size>
+            </box>
+          </geometry>
+        </collision>
+        <visual name='visual'>
+          <pose frame=''>-2.5 0 0 0 0 0</pose>
+          <geometry>
+            <box>
+              <size>5 0.2 2</size>
+            </box>
+          </geometry>
+        </visual>
+      </link>
+    </model>
+
+  </world>
+</sdf>

test/worlds/test_sdf17_err_sibling_different_type.world

@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<sdf version='1.7'>
+  <world name='default'>
+
+    <model name='wall'>
+      <static>1</static>
+      <pose frame=''>0 0 1 0 0 1</pose>
+      <link name='wall'>
+        <collision name='repeated_name'>
+          <pose frame=''>-2.5 0 0 0 0 0</pose>
+          <geometry>
+            <box>
+              <size>5 0.2 2</size>
+            </box>
+          </geometry>
+        </collision>
+         <collision name='collision_1'>
+          <pose frame=''>-2.5 0 0 0 0 0</pose>
+          <geometry>
+            <box>
+              <size>5 0.2 2</size>
+            </box>
+          </geometry>
+        </collision>
+         <collision name='collision_2'>
+          <pose frame=''>-2.5 0 0 0 0 0</pose>
+          <geometry>
+            <box>
+              <size>5 0.2 2</size>
+            </box>
+          </geometry>
+        </collision>
+        <visual name='repeated_name'>
+          <pose frame=''>-2.5 0 0 0 0 0</pose>
+          <geometry>
+            <box>
+              <size>5 0.2 2</size>
+            </box>
+          </geometry>
+        </visual>
+      </link>
+    </model>
+
+  </world>
+</sdf>