Prevent creation of spurious <plugin> elements when saving worlds (#1192

)

Spurious <plugin> elements were created when saving worlds because sdf::Element::GetElement was used to check if a <plugin> child element exists. However this function always creates a new element if the element does not exist instead of returning a nullptr. The solution is to use sdf::Element::FindElement instead.

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

src/LevelManager.cc

@@ -155,7 +155,7 @@ void LevelManager::ReadLevelPerformerInfo()
 
   sdf::ElementPtr pluginElem;
   // Get the ignition::gazebo plugin element
-  for (auto plugin = worldElem->GetElement("plugin"); plugin;
+  for (auto plugin = worldElem->FindElement("plugin"); plugin;
        plugin = plugin->GetNextElement("plugin"))
   {
     if (plugin->Get<std::string>("name") == kPluginName)

src/SimulationRunner.cc

@@ -959,16 +959,15 @@ void SimulationRunner::LoadLoggingPlugins(const ServerConfig &_config)
 void SimulationRunner::LoadPlugins(const Entity _entity,
     const sdf::ElementPtr &_sdf)
 {
-  sdf::ElementPtr pluginElem = _sdf->GetElement("plugin");
+  sdf::ElementPtr pluginElem = _sdf->FindElement("plugin");
   while (pluginElem)
   {
     auto filename = pluginElem->Get<std::string>("filename");
     auto name = pluginElem->Get<std::string>("name");
     // No error message for the 'else' case of the following 'if' statement
     // because SDF create a default <plugin> element even if it's not
     // specified. An error message would result in spamming
-    // the console. \todo(nkoenig) Fix SDF should so that elements are not
-    // automatically added.
+    // the console.
     if (filename != "__default__" && name != "__default__")
     {
       this->LoadPlugin(_entity, filename, name, pluginElem);

src/SimulationRunner_TEST.cc

@@ -1439,6 +1439,52 @@ TEST_P(SimulationRunnerTest, GenerateWorldSdf)
   EXPECT_EQ(3u, world->ModelCount());
 }
 
+/////////////////////////////////////////////////
+/// Helper function to recursively check for plugins with filename and name
+/// attributes set to "__default__"
+testing::AssertionResult checkForSpuriousPlugins(sdf::ElementPtr _elem)
+{
+  auto plugin = _elem->FindElement("plugin");
+  if (nullptr != plugin &&
+      plugin->Get<std::string>("filename") == "__default__" &&
+      plugin->Get<std::string>("name") == "__default__")
+  {
+    return testing::AssertionFailure() << _elem->ToString("");
+  }
+  for (auto child = _elem->GetFirstElement(); child;
+       child = child->GetNextElement())
+  {
+    auto result = checkForSpuriousPlugins(child);
+    if (!result)
+      return result;
+  }
+  return testing::AssertionSuccess();
+}
+
+/////////////////////////////////////////////////
+TEST_P(SimulationRunnerTest, GeneratedSdfHasNoSpuriousPlugins)
+{
+  // Load SDF file
+  sdf::Root root;
+  root.Load(common::joinPaths(PROJECT_SOURCE_PATH,
+      "test", "worlds", "shapes.sdf"));
+
+  ASSERT_EQ(1u, root.WorldCount());
+
+  // Create simulation runner
+  auto systemLoader = std::make_shared<SystemLoader>();
+  SimulationRunner runner(root.WorldByIndex(0), systemLoader);
+
+  msgs::SdfGeneratorConfig req;
+  msgs::StringMsg genWorldSdf;
+  EXPECT_TRUE(runner.GenerateWorldSdf(req, genWorldSdf));
+  EXPECT_FALSE(genWorldSdf.data().empty());
+
+  sdf::Root newRoot;
+  newRoot.LoadSdfString(genWorldSdf.data());
+  EXPECT_TRUE(checkForSpuriousPlugins(newRoot.Element()));
+}
+
 // Run multiple times. We want to make sure that static globals don't cause
 // problems.
 INSTANTIATE_TEST_SUITE_P(ServerRepeat, SimulationRunnerTest,