Allow different warning policies in ParserConfig

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

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

@@ -31,6 +31,19 @@ namespace sdf
 {
 inline namespace SDF_VERSION_NAMESPACE
 {
+
+enum class WarningsPolicy
+{
+  /// \brief Warnings are treated as errors and added to the cumulative errors
+  PEDANTIC,
+
+  /// \brief Warnings are streamed to sdfwarn
+  WARNED,
+
+  /// \brief Warnings are streamed to sdfdbg
+  IGNORED
+};
+
 // Forward declare private data class.
 class ParserConfigPrivate;
 
@@ -121,6 +134,14 @@ class SDFORMAT_VISIBLE ParserConfig
   /// \param[in] _path Colon separated set of paths.
   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(sdf::WarningsPolicy _policy);
+
+  /// \brief Get the current warning enforcement policy
+  /// \return The warning enforcement policy enum value
+  public: sdf::WarningsPolicy WarningsPolicy() const;
+
   /// \brief Private data pointer.
   private: ParserConfigPrivate *dataPtr;
 };

src/ParserConfig.cc

@@ -26,6 +26,11 @@ class sdf::ParserConfigPrivate
 {
   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: WarningsPolicy warningsPolicy =
+    WarningsPolicy::WARNED;
 };
 
 
@@ -115,3 +120,13 @@ void ParserConfig::AddURIPath(const std::string &_uri, const std::string &_path)
     }
   }
 }
+
+void ParserConfig::SetWarningsPolicy(sdf::WarningsPolicy policy)
+{
+  this->dataPtr->warningsPolicy = policy;
+}
+
+WarningsPolicy ParserConfig::WarningsPolicy() const
+{
+  return this->dataPtr->warningsPolicy;
+}

src/Utils.cc

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

src/Utils.hh

@@ -21,8 +21,10 @@
 #include <string>
 #include <optional>
 #include <vector>
+#include "sdf/Console.hh"
 #include "sdf/Error.hh"
 #include "sdf/Element.hh"
+#include "sdf/ParserConfig.hh"
 #include "sdf/Types.hh"
 
 namespace sdf
@@ -69,6 +71,20 @@ namespace sdf
   /// value.
   double infiniteIfNegative(double _value);
 
+
+  /// \brief Either stream the warning or add to _errors
+  /// Based on the ParserConfig warnings policy, this will either stream the
+  /// warnings to sdfwarn, sdfdbg or add the warnings to _errors.
+  /// \param[in] _policy The warnings 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 PEDANTIC
+  /// \param[in] _errors The errors to append to if the policy is PEDANTIC
+  void addRecoverableWarning(
+    const sdf::WarningsPolicy _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 +221,6 @@ namespace sdf
       return &_opt.value();
     return nullptr;
   }
-  }
+}
 }
 #endif

src/parser.cc

@@ -729,7 +729,12 @@ 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";
+      std::stringstream ss;
+      ss << "Converting a deprecated SDF source[" << _source << "].\n";
+      addRecoverableWarning(
+        _config.WarningsPolicy(), ss.str(),
+        ErrorCode::VERSION_DEPRECATED, _errors);
+
       Converter::Convert(_xmlDoc, SDF::Version());
     }
 
@@ -888,7 +893,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";
+    addRecoverableWarning(
+      _config.WarningsPolicy(), ss.str(),
+      ErrorCode::ELEMENT_DEPRECATED, _errors);
   }
 
   if (!_xml)
@@ -985,9 +994,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";
+      addRecoverableWarning(
+        _config.WarningsPolicy(), ss.str(),
+        ErrorCode::ATTRIBUTE_INCORRECT_TYPE, _errors);
     }
 
     attribute = attribute->Next();
@@ -1103,10 +1116,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";
+              addRecoverableWarning(
+                _config.WarningsPolicy(), ss.str(),
+                ErrorCode::ELEMENT_INCORRECT_TYPE, _errors);
             }
           }
         }
@@ -1123,9 +1140,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";
+          addRecoverableWarning(
+            _config.WarningsPolicy(), ss.str(),
+            ErrorCode::ELEMENT_INCORRECT_TYPE, _errors);
         }
 
         bool isModel = topLevelElementType == "model";
@@ -1252,10 +1273,15 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
       if (descCounter == _sdf->GetElementDescriptionCount()
             && std::strchr(elemXml->Value(), ':') == nullptr)
       {
-        sdfdbg << "XML Element[" << elemXml->Value()
+        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";
+
+        addRecoverableWarning(
+          _config.WarningsPolicy(), ss.str(),
+          ErrorCode::ELEMENT_INCORRECT_TYPE, _errors);
         continue;
       }
     }

test/integration/unknown.cc

@@ -83,3 +83,51 @@ 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 pedantic
+/// warnings
+TEST(UnrecognizedElements, UnrecognizedElementsWithWarningsPolicies)
+{
+  const std::string testFile =
+    sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "sdf",
+        "unrecognized_elements.sdf");
+
+  sdf::ParserConfig config;
+
+  {
+    config.SetWarningsPolicy(sdf::WarningsPolicy::PEDANTIC);
+    sdf::Root root;
+    const auto errors = root.Load(testFile, config);
+    EXPECT_FALSE(errors.empty());
+  }
+  {
+    config.SetWarningsPolicy(sdf::WarningsPolicy::WARNED);
+    sdf::Root root;
+    const auto errors = root.Load(testFile, config);
+    EXPECT_TRUE(errors.empty());
+  }
+  {
+    config.SetWarningsPolicy(sdf::WarningsPolicy::IGNORED);
+    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 pendatic
+TEST(UnrecognizedElements, UnrecognizedElementsWithNamespaces)
+{
+  const std::string testFile =
+    sdf::filesystem::append(PROJECT_SOURCE_PATH, "test", "sdf",
+        "unrecognized_elements_with_namespace.sdf");
+
+  sdf::ParserConfig config;
+  config.SetWarningsPolicy(sdf::WarningsPolicy::PEDANTIC);
+
+  sdf::Root root;
+  const auto errors = root.Load(testFile, config);
+  EXPECT_FALSE(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,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>
+      <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>