Check that included file only contains one of actor/light/model

Signed-off-by: Stephen Brawner <[email protected]>

sdf/1.8/model.sdf

@@ -38,7 +38,7 @@
   <include filename="gripper.sdf" required="*"/>
 
   <element name="include" required="*">
-    <description>Include resources from a URI. This can be used to nest models.</description>
+    <description>Include resources from a URI. This can be used to nest models. Included resources can only contain one 'model', 'light' or 'actor' element.</description>
     <element name="uri" type="string" default="__default__" required="1">
       <description>URI to a resource, such as a model</description>
     </element>

sdf/1.8/world.sdf

@@ -22,7 +22,7 @@
   </element>
 
   <element name="include" required="*">
-    <description>Include resources from a URI</description>
+    <description>Include resources from a URI. Included resources can only contain one 'model', 'light' or 'actor' element.</description>
     <element name="uri" type="string" default="__default__" required="1">
       <description>URI to a resource, such as a model</description>
     </element>

src/parser.cc

@@ -1001,30 +1001,47 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf, Errors &_errors)
         }
 
         sdf::ElementPtr topLevelElem;
-        bool isModel{false};
-        bool isActor{false};
-        if (includeSDF->Root()->HasElement("model"))
+        int countTopLevelElements = 0;
+        for (const auto & elementType : {"model", "actor", "light"})
         {
-          topLevelElem = includeSDF->Root()->GetElement("model");
-          isModel = true;
+          if (includeSDF->Root()->HasElement(elementType))
+          {
+            countTopLevelElements++;
+            topLevelElem = includeSDF->Root()->GetElement(elementType);
+          }
         }
-        else if (includeSDF->Root()->HasElement("actor"))
+
+        if (nullptr == topLevelElem)
         {
-          topLevelElem = includeSDF->Root()->GetElement("actor");
-          isActor = true;
+          _errors.push_back({ErrorCode::ELEMENT_INVALID,
+              "Failed to find top level <model> / <actor> / <light> for "
+              "<include>\n"});
+          continue;
         }
-        else if (includeSDF->Root()->HasElement("light"))
+
+        // Check if more than one type of top-level element
+        if (countTopLevelElements > 1)
         {
-          topLevelElem = includeSDF->Root()->GetElement("light");
+          _errors.push_back({ErrorCode::ELEMENT_INVALID,
+              "Found more than one of <model> / <actor> / <light> for "
+              "<include>\n"});
+          continue;
         }
-        else
+
+        const auto topLevelElementType = topLevelElem->GetName();
+
+        // Check for more than one of the discovered top-level element type
+        if (nullptr != topLevelElem->GetNextElement(topLevelElementType))
         {
-          _errors.push_back({ErrorCode::ELEMENT_MISSING,
-              "Failed to find top level <model> / <actor> / <light> for "
+          _errors.push_back({ErrorCode::ELEMENT_INVALID,
+              "Found more than one of " + topLevelElem->GetName() + " for "
               "<include>\n"});
           continue;
         }
 
+        bool isModel = topLevelElementType == "model";
+        bool isActor = topLevelElementType == "actor";
+
         if (elemXml->FirstChildElement("name"))
         {
           topLevelElem->GetAttribute("name")->SetFromString(

test/integration/CMakeLists.txt

@@ -27,6 +27,7 @@ set(tests
   model_dom.cc
   model_versions.cc
   nested_model.cc
+  nested_multiple_elements_error.cc
   parser_error_detection.cc
   plugin_attribute.cc
   plugin_bool.cc
@@ -69,5 +70,3 @@ if (UNIX AND NOT APPLE)
   add_test(NAME INTEGRATION_versioned_symbols
     COMMAND bash ${CMAKE_CURRENT_BINARY_DIR}/all_symbols_have_version.bash ${CMAKE_BINARY_DIR}/src/libsdformat${SDF_MAJOR_VERSION}.so)
 endif()
-
-

test/integration/nested_multiple_elements_error.cc

@@ -0,0 +1,110 @@
+/*
+ * Copyright 2020 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 <iostream>
+#include <string>
+#include <gtest/gtest.h>
+
+#include "sdf/Actor.hh"
+#include "sdf/Collision.hh"
+#include "sdf/Filesystem.hh"
+#include "sdf/Geometry.hh"
+#include "sdf/Light.hh"
+#include "sdf/Link.hh"
+#include "sdf/Mesh.hh"
+#include "sdf/Model.hh"
+#include "sdf/parser.hh"
+#include "sdf/Root.hh"
+#include "sdf/SDFImpl.hh"
+#include "sdf/Visual.hh"
+#include "sdf/World.hh"
+#include "test_config.h"
+
+const auto g_testPath = sdf::filesystem::append(PROJECT_SOURCE_PATH, "test");
+const auto g_modelsPath =
+    sdf::filesystem::append(g_testPath, "integration", "model");
+
+/////////////////////////////////////////////////
+std::string findFileCb(const std::string &_input)
+{
+  return sdf::filesystem::append(g_testPath, "integration", "model", _input);
+}
+
+//////////////////////////////////////////////////
+TEST(IncludesTest, nested_multiple_models_error)
+{
+  sdf::setFindCallback(findFileCb);
+
+  const auto worldFile =
+    sdf::filesystem::append(g_modelsPath, "nested_multiple_models_error");
+
+  sdf::Root root;
+  sdf::Errors errors = root.Load(worldFile);
+  ASSERT_FALSE(errors.empty());
+  EXPECT_EQ(
+    std::string("Found more than one of model for <include>\n"),
+    errors[0].Message());
+}
+
+//////////////////////////////////////////////////
+TEST(IncludesTest, nested_multiple_actors_error)
+{
+  sdf::setFindCallback(findFileCb);
+
+  const auto worldFile =
+    sdf::filesystem::append(g_modelsPath, "nested_multiple_actors_error");
+
+  sdf::Root root;
+  sdf::Errors errors = root.Load(worldFile);
+  ASSERT_FALSE(errors.empty());
+  EXPECT_EQ(
+    std::string("Found more than one of actor for <include>\n"),
+    errors[0].Message());
+}
+
+//////////////////////////////////////////////////
+TEST(IncludesTest, nested_multiple_lights_error)
+{
+  sdf::setFindCallback(findFileCb);
+
+  const auto worldFile =
+    sdf::filesystem::append(g_modelsPath, "nested_multiple_lights_error");
+
+  sdf::Root root;
+  sdf::Errors errors = root.Load(worldFile);
+  ASSERT_FALSE(errors.empty());
+  EXPECT_EQ(
+    std::string("Found more than one of light for <include>\n"),
+    errors[0].Message());
+}
+
+//////////////////////////////////////////////////
+TEST(IncludesTest, nested_multiple_elements_error)
+{
+  sdf::setFindCallback(findFileCb);
+
+  const auto worldFile =
+    sdf::filesystem::append(g_modelsPath, "nested_multiple_elements_error");
+
+  sdf::Root root;
+  sdf::Errors errors = root.Load(worldFile);
+  ASSERT_FALSE(errors.empty());
+  EXPECT_EQ(
+    std::string(
+      "Found more than one of <model> / <actor> / <light> for <include>\n"),
+    errors[0].Message());
+}