Add EnforcementPolicy in ParserConfig to enable configurable parsing strictness

include/sdf/Error.hh

@@ -65,6 +65,10 @@ namespace sdf
     /// \brief This error indicates that an SDF attribute is deprecated.
     ATTRIBUTE_DEPRECATED,
 
+    /// \brief Indicates an attribute was included that is not part of the sdf
+    /// spec
+    ATTRIBUTE_INCORRECT_TYPE,
+
     /// \brief Indicates that a required SDF element is missing.
     ELEMENT_MISSING,
 
@@ -134,6 +138,9 @@ namespace sdf
 
     /// \brief The specified placement frame is invalid
     MODEL_PLACEMENT_FRAME_INVALID,
+
+    /// \brief The provided version has been deprecated or it is pre-versioning
+    VERSION_DEPRECATED,
   };
 
   class SDFORMAT_VISIBLE Error

include/sdf/ParserConfig.hh

@@ -34,6 +34,19 @@ namespace sdf
 {
 inline namespace SDF_VERSION_NAMESPACE
 {
+/// \brief Policy to describe how to treat certain conditions when parsing
+enum class EnforcementPolicy
+{
+  /// \brief Policy is to treat condition as an error and fail parsing
+  ERR,
+
+  /// \brief Treat condition as a warning and issue to user
+  WARN,
+
+  /// \brief Ignore condition in favor of best effort parsing
+  LOG,
+};
+
 // Forward declare private data class.
 class ParserConfigPrivate;
 
@@ -114,6 +127,22 @@ class SDFORMAT_VISIBLE ParserConfig
   /// \sa sdf::findFile() for the order of search operations
   public: void AddURIPath(const std::string &_uri, const std::string &_path);
 
+  /// \brief Set the warning enforcment policy.
+  /// \param[in] _policy policy enum value to set
+  public: void SetWarningsPolicy(EnforcementPolicy _policy);
+
+  /// \brief Get the current warning enforcement policy
+  /// \return The warning enforcement policy enum value
+  public: EnforcementPolicy WarningsPolicy() const;
+
+  /// \brief Set the policy for unrecogonized elements without an xmlns
+  /// \param[in] _policy The unrecognized elements enforcement policy
+  public: void SetUnrecognizedElementsPolicy(EnforcementPolicy _policy);
+
+  /// \brief Get the current unrecognized elements policy
+  /// \return The unrecognized elements policy enum value
+  public: EnforcementPolicy UnrecognizedElementsPolicy() const;
+
   /// \brief Private data pointer.
   IGN_UTILS_IMPL_PTR(dataPtr)
 };

src/ParserConfig.cc

@@ -26,6 +26,16 @@ class sdf::ParserConfig::Implementation
 {
   public: ParserConfig::SchemeToPathMap uriPathMap;
   public: std::function<std::string(const std::string &)> findFileCB;
+
+  /// \brief Indicates how warnings and errors are tolerated.
+  /// Default is for warnings to be streamed via sdfwarn
+  public: EnforcementPolicy warningsPolicy = EnforcementPolicy::WARN;
+
+  /// \brief Policy indicating how unrecognized elements without an xmlns are
+  /// treated.
+  /// Default is to ignore them for compatibility with legacy behavior
+  public: EnforcementPolicy unrecognizedElementsPolicy =
+    EnforcementPolicy::LOG;
 };
 
 
@@ -83,3 +93,23 @@ void ParserConfig::AddURIPath(const std::string &_uri, const std::string &_path)
     }
   }
 }
+
+void ParserConfig::SetWarningsPolicy(EnforcementPolicy policy)
+{
+  this->dataPtr->warningsPolicy = policy;
+}
+
+EnforcementPolicy ParserConfig::WarningsPolicy() const
+{
+  return this->dataPtr->warningsPolicy;
+}
+
+void ParserConfig::SetUnrecognizedElementsPolicy(EnforcementPolicy _policy)
+{
+  this->dataPtr->unrecognizedElementsPolicy = _policy;
+}
+
+EnforcementPolicy ParserConfig::UnrecognizedElementsPolicy() const
+{
+  return this->dataPtr->unrecognizedElementsPolicy;
+}

src/Utils.cc

@@ -89,5 +89,27 @@ bool isValidFrameReference(const std::string &_name)
 {
   return "__root__" != _name;
 }
+
+void enforceConfigurablePolicyCondition(
+  const sdf::EnforcementPolicy _policy,
+  const std::string &_message,
+  const ErrorCode _error,
+  sdf::Errors &_errors)
+{
+  switch (_policy)
+  {
+    case EnforcementPolicy::ERR:
+      _errors.push_back({_error, _message});
+      break;
+    case EnforcementPolicy::WARN:
+      sdfwarn << _message;
+      break;
+    case EnforcementPolicy::LOG:
+      sdfdbg << _message;
+      break;
+    default:
+      throw std::runtime_error("Unhandled warning policy enum value");
+  }
+}
 }
 }

src/Utils.hh

@@ -23,6 +23,7 @@
 #include <vector>
 #include "sdf/Error.hh"
 #include "sdf/Element.hh"
+#include "sdf/ParserConfig.hh"
 #include "sdf/Types.hh"
 
 namespace sdf
@@ -69,6 +70,20 @@ namespace sdf
   /// value.
   double infiniteIfNegative(double _value);
 
+  /// \brief Handle a condition which can be treated as an error, warning or
+  /// ignored entirely.
+  /// Based on the policy, this will either add it to an errors vector, stream
+  /// it to sdfwarn, or sdfdbg.
+  /// \param[in] _policy The enforcement policy to follow
+  /// \param[in] _message The message to add for this warning
+  /// \param[in] _error An error code to use if the policy is ERR
+  /// \param[out] _errors The errors to append to if the policy is ERR
+  void enforceConfigurablePolicyCondition(
+    const sdf::EnforcementPolicy _policy,
+    const std::string &_message,
+    const ErrorCode _error,
+    sdf::Errors &_errors);
+
   /// \brief Load all objects of a specific sdf element type. No error
   /// is returned if an element is not present. This function assumes that
   /// an element has a "name" attribute that must be unique.
@@ -205,6 +220,6 @@ namespace sdf
       return &_opt.value();
     return nullptr;
   }
-  }
+}
 }
 #endif

src/parser.cc

@@ -736,7 +736,8 @@ bool readDoc(tinyxml2::XMLDocument *_xmlDoc, ElementPtr _sdf,
     if (_convert
         && strcmp(sdfNode->Attribute("version"), SDF::Version().c_str()) != 0)
     {
-      sdfwarn << "Converting a deprecated SDF source[" << _source << "].\n";
+      sdfdbg << "Converting a deprecated SDF source[" << _source << "].\n";
+
       Converter::Convert(_xmlDoc, SDF::Version());
     }
 
@@ -895,7 +896,11 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
   // Check if the element pointer is deprecated.
   if (_sdf->GetRequired() == "-1")
   {
-    sdfwarn << "SDF Element[" + _sdf->GetName() + "] is deprecated\n";
+    std::stringstream ss;
+    ss << "SDF Element[" + _sdf->GetName() + "] is deprecated\n";
+    enforceConfigurablePolicyCondition(
+      _config.WarningsPolicy(), ss.str(),
+      ErrorCode::ELEMENT_DEPRECATED, _errors);
   }
 
   if (!_xml)
@@ -992,9 +997,13 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
 
     if (i == _sdf->GetAttributeCount())
     {
-      sdfwarn << "XML Attribute[" << attribute->Name()
+      std::stringstream ss;
+      ss << "XML Attribute[" << attribute->Name()
               << "] in element[" << _xml->Value()
-              << "] not defined in SDF, ignoring.\n";
+              << "] not defined in SDF.\n";
+      enforceConfigurablePolicyCondition(
+        _config.WarningsPolicy(), ss.str(),
+        ErrorCode::ATTRIBUTE_INCORRECT_TYPE, _errors);
     }
 
     attribute = attribute->Next();
@@ -1110,10 +1119,14 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
             }
             else
             {
-              sdfwarn << "Found other top level element <" << elementType
-                      << "> in addition to <" << topLevelElem->GetName()
-                      << "> in include file. This is unsupported and in future "
-                      << "versions of libsdformat will become an error";
+              std::stringstream ss;
+              ss << "Found other top level element <" << elementType
+                 << "> in addition to <" << topLevelElem->GetName()
+                 << "> in include file. This is unsupported and in future "
+                 << "versions of libsdformat will become an error";
+              enforceConfigurablePolicyCondition(
+                _config.WarningsPolicy(), ss.str(),
+                ErrorCode::ELEMENT_INCORRECT_TYPE, _errors);
             }
           }
         }
@@ -1130,9 +1143,13 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
         // Check for more than one of the discovered top-level element type
         if (nullptr != topLevelElem->GetNextElement(topLevelElementType))
         {
-          sdfwarn << "Found more than one of " << topLevelElem->GetName()
-                  << " for <include>. This is unsupported and in future "
-                  << "versions of libsdformat will become an error";
+          std::stringstream ss;
+          ss << "Found more than one of " << topLevelElem->GetName()
+             << " for <include>. This is unsupported and in future "
+             << "versions of libsdformat will become an error";
+          enforceConfigurablePolicyCondition(
+            _config.WarningsPolicy(), ss.str(),
+            ErrorCode::ELEMENT_INCORRECT_TYPE, _errors);
         }
 
         bool isModel = topLevelElementType == "model";
@@ -1259,10 +1276,16 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
       if (descCounter == _sdf->GetElementDescriptionCount()
             && std::strchr(elemXml->Value(), ':') == nullptr)
       {
-        sdfdbg << "XML Element[" << elemXml->Value()
-               << "], child of element[" << _xml->Value()
-               << "], not defined in SDF. Copying[" << elemXml->Value() << "] "
-               << "as children of [" << _xml->Value() << "].\n";
+        std::stringstream ss;
+        ss << "XML Element[" << elemXml->Value()
+           << "], child of element[" << _xml->Value()
+           << "], not defined in SDF. Copying[" << elemXml->Value() << "] "
+           << "as children of [" << _xml->Value() << "].\n";
+
+        enforceConfigurablePolicyCondition(
+          _config.UnrecognizedElementsPolicy(), ss.str(),
+          ErrorCode::ELEMENT_INCORRECT_TYPE, _errors);
+
         continue;
       }
     }

test/integration/unknown.cc

@@ -83,3 +83,55 @@ TEST(Unknown, CopyUnknownElement)
   EXPECT_EQ("my nested xml", nestedElem->Get<std::string>("attr"));
   EXPECT_EQ("AString", nestedElem->Get<std::string>("string"));
 }
+
+/////////////////////////////////////////////////
+/// Test that elements that aren't part of the spec are flagged with when
+/// UnrecognizedElementsPolicy is set to err
+TEST(UnrecognizedElements, UnrecognizedElementsWithWarningsPolicies)
+{
+  const std::string testFile =
+    sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "sdf",
+        "unrecognized_elements.sdf");
+
+  sdf::ParserConfig config;
+
+  {
+    config.SetUnrecognizedElementsPolicy(sdf::EnforcementPolicy::ERR);
+    sdf::Root root;
+    const auto errors = root.Load(testFile, config);
+    ASSERT_FALSE(errors.empty());
+    constexpr char expectedMessage[] =
+      "XML Element[not_a_link_element], child of element[link], not defined in"
+      " SDF. Copying[not_a_link_element] as children of [link].\n";
+    EXPECT_EQ(errors[0].Message(), expectedMessage);
+  }
+  {
+    config.SetUnrecognizedElementsPolicy(sdf::EnforcementPolicy::WARN);
+    sdf::Root root;
+    const auto errors = root.Load(testFile, config);
+    EXPECT_TRUE(errors.empty());
+  }
+  {
+    config.SetUnrecognizedElementsPolicy(sdf::EnforcementPolicy::LOG);
+    sdf::Root root;
+    const auto errors = root.Load(testFile, config);
+    EXPECT_TRUE(errors.empty());
+  }
+}
+
+/////////////////////////////////////////////////
+/// Test that elements that aren't part of the spec but have the namespace
+/// separator ":" don't cause errors even with EnforcementPolicy::ERR
+TEST(UnrecognizedElements, UnrecognizedElementsWithNamespaces)
+{
+  const std::string testFile =
+    sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "sdf",
+        "unrecognized_elements_with_namespace.sdf");
+
+  sdf::ParserConfig config;
+  config.SetUnrecognizedElementsPolicy(sdf::EnforcementPolicy::ERR);
+
+  sdf::Root root;
+  const auto errors = root.Load(testFile, config);
+  EXPECT_TRUE(errors.empty());
+}

test/sdf/unrecognized_elements.sdf

@@ -0,0 +1,11 @@
+<?xml version="1.0" ?>
+<sdf version="1.6">
+  <model name="joint_incorrect_tags" some_attribute="staheousth">
+    <link name="link">
+      <pose>0 0 1 0 0 0</pose>
+      <not_a_link_element>This should trigger a warning</not_a_link_element>
+    </link>
+    <not_a_model_element>This should also trigger a warning</not_a_model_element>
+  </model>
+  <not_an_sdf_element>Warnings time</not_an_sdf_element>
+</sdf>

test/sdf/unrecognized_elements_with_namespace.sdf

@@ -0,0 +1,19 @@
+<?xml version="1.0" ?>
+<!-- This world file is an example of minimum model SDF, with custom namespaced
+     elements -->
+<sdf version="1.8" xmlns:third_party_software="custom_ns_uri">
+  <model name="namespaced_tags" some_attribute="some value">
+    <link name="link">
+      <pose>0 0 1 0 0 0</pose>
+      <third_party_software:not_a_link_element>
+        This should be ignored because it's namespaced
+      </third_party_software:not_a_link_element>
+    </link>
+    <third_party_software:not_a_model_element>
+      This should also be ignored
+    </third_party_software:not_a_model_element>
+  </model>
+  <third_party_software:not_an_sdf_element>
+    Ignore this one tooo
+  </third_party_software:not_an_sdf_element>
+</sdf>