Enforce minimum/maximum values specified in SDFormat description file…

…s (Retry PR) (gazebosim#303) Currently, some SDFormat description files contain min and max values, but the parser does not enforce those values in any way. This PR adds a validation step in the parser that checks if scalar values are within the allowed range. Note that this PR addresses only element values, not attributes. Signed-off-by: Steve Peters <[email protected]>
scpeters · Jun 30, 2020 · d4f1c86 · d4f1c86
1 parent 55029bb
commit d4f1c86
Show file tree

Hide file tree

Showing 11 changed files with 404 additions and 23 deletions.
diff --git a/Changelog.md b/Changelog.md
@@ -16,6 +16,9 @@
 
 ### libsdformat 10.0.0 (202X-XX-XX)
 
+1. Enforce minimum/maximum values specified in SDFormat description files
+    * [Pull request 303](https://github.com/osrf/sdformat/pull/303)
+
 1. Make parsing of values syntactically more strict with bad values generating an error
     * [Pull request 244](https://github.com/osrf/sdformat/pull/244)
 

diff --git a/Migration.md b/Migration.md
@@ -12,6 +12,24 @@ forward programmatically.
 This document aims to contain similar information to those files
 but with improved human-readability..
 
+## SDFormat 9.x to 10.0
+
+### Modifications
+
+1. + Minimum/maximum values specified in SDFormat description files are now enforced
+   + [Pull request 303](https://github.com/osrf/sdformat/pull/303)
+
+### Additions
+
+1. **sdf/Element.hh**
+    + void AddValue(const std::string &, const std::string &, bool, const std::string &, const std::string &, const std::string &)
+
+1. **sdf/Param.hh**
+    + Param(const std::string &, const std::string &e, const std::string &, bool, const std::string &, const std::string &, const std::string &)
+    + std::optional<std::string> GetMinValueAsString() const;
+    + std::optional<std::string> GetMaxValueAsString() const;
+    + bool ValidateValue() const;
+
 ## SDFormat 8.x to 9.0
 
 ### Additions

diff --git a/include/sdf/Element.hh b/include/sdf/Element.hh
@@ -174,15 +174,30 @@ namespace sdf
                               const std::string &_description="");
 
     /// \brief Add a value to this Element.
-    /// \param[in] _type Type of data the attribute will hold.
-    /// \param[in] _defaultValue Default value for the attribute.
+    /// \param[in] _type Type of data the parameter will hold.
+    /// \param[in] _defaultValue Default value for the parameter.
     /// \param[in] _required Requirement string. \as Element::SetRequired.
-    /// \param[in] _description A text description of the attribute.
+    /// \param[in] _description A text description of the parameter.
     /// \throws sdf::AssertionInternalError if an invalid type is given.
     public: void AddValue(const std::string &_type,
                           const std::string &_defaultValue, bool _required,
                           const std::string &_description="");
 
+    /// \brief Add a value to this Element. This override allows passing min and
+    /// max values of the parameter.
+    /// \param[in] _type Type of data the parameter will hold.
+    /// \param[in] _defaultValue Default value for the parameter.
+    /// \param[in] _required Requirement string. \as Element::SetRequired.
+    /// \param[in] _minValue Minimum allowed value for the parameter.
+    /// \param[in] _maxValue Maximum allowed value for the parameter.
+    /// \param[in] _description A text description of the parameter.
+    /// \throws sdf::AssertionInternalError if an invalid type is given.
+    public: void AddValue(const std::string &_type,
+                          const std::string &_defaultValue, bool _required,
+                          const std::string &_minValue,
+                          const std::string &_maxValue,
+                          const std::string &_description = "");
+
     /// \brief Get the param of an attribute.
     /// \param[in] _key the name of the attribute.
     /// \return The parameter attribute value. NULL if the key is invalid.

diff --git a/include/sdf/Param.hh b/include/sdf/Param.hh
@@ -24,6 +24,7 @@
 #include <cstdint>
 #include <functional>
 #include <memory>
+#include <optional>
 #include <sstream>
 #include <string>
 #include <typeinfo>
@@ -105,6 +106,41 @@ namespace sdf
                   const std::string &_default, bool _required,
                   const std::string &_description = "");
 
+    /// \brief Constructor with min and max values.
+    /// \param[in] _key Key for the parameter.
+    /// \param[in] _typeName String name for the value type (double,
+    /// int,...).
+    /// \param[in] _default Default value.
+    /// \param[in] _required True if the parameter is required to be set.
+    /// \param[in] _minValue Minimum allowed value for the parameter.
+    /// \param[in] _maxValue Maximum allowed value for the parameter.
+    /// \param[in] _description Description of the parameter.
+    /// \throws sdf::AssertionInternalError if an invalid type is given.
+    public: Param(const std::string &_key, const std::string &_typeName,
+                  const std::string &_default, bool _required,
+                  const std::string &_minValue, const std::string &_maxValue,
+                  const std::string &_description = "");
+
+    /// \brief Copy constructor
+    /// Note that the updateFunc member does not get copied
+    /// \param[in] _param Param to copy
+    public: Param(const Param &_param);
+
+    /// \brief Move constructor
+    /// \param[in] _param Param to move from
+    public: Param(Param &&_param) noexcept = default;
+
+    /// \brief Copy assignment operator
+    /// Note that the updateFunc member will not get copied
+    /// \param[in] _param The parameter to set values from.
+    /// \return *This
+    public: Param &operator=(const Param &_param);
+
+    /// \brief Move assignment operator
+    /// \param[in] _param Param to move from
+    /// \returns Reference to this
+    public: Param &operator=(Param &&_param) noexcept = default;
+
     /// \brief Destructor
     public: virtual ~Param();
 
@@ -116,6 +152,18 @@ namespace sdf
     /// \return String containing the default value of the parameter.
     public: std::string GetDefaultAsString() const;
 
+    /// \brief Get the minimum allowed value as a string
+    /// \return Returns a string containing the minimum allowed value of the
+    /// parameter if the minimum value is specified in the SDFormat description
+    /// of the parameter. nullopt otherwise.
+    public: std::optional<std::string> GetMinValueAsString() const;
+
+    /// \brief Get the maximum allowed value as a string
+    /// \return Returns a string containing the maximum allowed value of the
+    /// parameter if the maximum value is specified in the SDFormat description
+    /// of the parameter. nullopt otherwise.
+    public: std::optional<std::string> GetMaxValueAsString() const;
+
     /// \brief Set the parameter value from a string.
     /// \param[in] _value New value for the parameter in string form.
     public: bool SetFromString(const std::string &_value);
@@ -186,12 +234,6 @@ namespace sdf
     public: template<typename T>
             bool GetDefault(T &_value) const;
 
-    /// \brief Equal operator. Set's the value and default value from the
-    /// provided Param.
-    /// \param[in] _param The parameter to set values from.
-    /// \return *This
-    public: Param &operator=(const Param &_param);
-
     /// \brief Set the description of the parameter.
     /// \param[in] _desc New description for the parameter.
     public: void SetDescription(const std::string &_desc);
@@ -200,6 +242,10 @@ namespace sdf
     /// \return The description of the parameter.
     public: std::string GetDescription() const;
 
+    /// \brief Validate the value against minimum and maximum allowed values
+    /// \return True if the value is valid
+    public: bool ValidateValue() const;
+
     /// \brief Ostream operator. Outputs the parameter's value.
     /// \param[in] _out Output stream.
     /// \param[in] _p The parameter to output.
@@ -258,6 +304,12 @@ namespace sdf
 
     /// \brief This parameter's default value
     public: ParamVariant defaultValue;
+
+    /// \brief This parameter's minimum allowed value
+    public: std::optional<ParamVariant> minValue;
+
+    /// \brief This parameter's maximum allowed value
+    public: std::optional<ParamVariant> maxValue;
   };
 
   ///////////////////////////////////////////////

diff --git a/src/Element.cc b/src/Element.cc
@@ -121,6 +121,19 @@ void Element::AddValue(const std::string &_type,
       _type, _defaultValue, _required, _description);
 }
 
+/////////////////////////////////////////////////
+void Element::AddValue(const std::string &_type,
+                       const std::string &_defaultValue,
+                       bool _required,
+                       const std::string &_minValue,
+                       const std::string &_maxValue,
+                       const std::string &_description)
+{
+  this->dataPtr->value =
+      std::make_shared<Param>(this->dataPtr->name, _type, _defaultValue,
+                              _required, _minValue, _maxValue, _description);
+}
+
 /////////////////////////////////////////////////
 ParamPtr Element::CreateParam(const std::string &_key,
                               const std::string &_type,
@@ -251,6 +264,17 @@ void Element::PrintDescription(const std::string &_prefix) const
               << "'"
               << " default ='" << this->dataPtr->value->GetDefaultAsString()
               << "'";
+    auto minValue = this->dataPtr->value->GetMinValueAsString();
+    if (minValue.has_value())
+    {
+      std::cout << " min ='" << *minValue << "'";
+    }
+
+    auto maxValue = this->dataPtr->value->GetMaxValueAsString();
+    if (maxValue.has_value())
+    {
+      std::cout << " max ='" << *maxValue << "'";
+    }
   }
 
   std::cout << ">\n";

diff --git a/src/Param.cc b/src/Param.cc
@@ -72,6 +72,42 @@ Param::Param(const std::string &_key, const std::string &_typeName,
   this->dataPtr->defaultValue = this->dataPtr->value;
 }
 
+//////////////////////////////////////////////////
+Param::Param(const std::string &_key, const std::string &_typeName,
+             const std::string &_default, bool _required,
+             const std::string &_minValue, const std::string &_maxValue,
+             const std::string &_description)
+    : Param(_key, _typeName, _default, _required, _description)
+{
+  auto valCopy = this->dataPtr->value;
+  if (!_minValue.empty())
+  {
+    SDF_ASSERT(
+        this->ValueFromString(_minValue),
+        std::string("Invalid [min] parameter in SDFormat description of [") +
+            _key + "]");
+    this->dataPtr->minValue = this->dataPtr->value;
+  }
+
+  if (!_maxValue.empty())
+  {
+    SDF_ASSERT(
+        this->ValueFromString(_maxValue),
+        std::string("Invalid [max] parameter in SDFormat description of [") +
+            _key + "]");
+    this->dataPtr->maxValue = this->dataPtr->value;
+  }
+
+  this->dataPtr->value = valCopy;
+}
+
+Param::Param(const Param &_param)
+    : dataPtr(std::make_unique<ParamPrivate>(*_param.dataPtr))
+{
+  // We don't want to copy the updateFunc
+  this->dataPtr->updateFunc = nullptr;
+}
+
 //////////////////////////////////////////////////
 Param::~Param()
 {
@@ -80,9 +116,11 @@ Param::~Param()
 /////////////////////////////////////////////////
 Param &Param::operator=(const Param &_param)
 {
-  this->dataPtr->value = _param.dataPtr->value;
-  this->dataPtr->defaultValue  = _param.dataPtr->defaultValue;
-  this->dataPtr->set  = _param.dataPtr->set;
+  auto updateFuncCopy = this->dataPtr->updateFunc;
+  *this = Param(_param);
+
+  // Restore the update func
+  this->dataPtr->updateFunc = updateFuncCopy;
   return *this;
 }
 
@@ -272,6 +310,32 @@ std::string Param::GetDefaultAsString() const
   return ss.str();
 }
 
+//////////////////////////////////////////////////
+std::optional<std::string> Param::GetMinValueAsString() const
+{
+  if (this->dataPtr->minValue.has_value())
+  {
+    StringStreamClassicLocale ss;
+
+    ss << ParamStreamer{ *this->dataPtr->minValue };
+    return ss.str();
+  }
+  return std::nullopt;
+}
+
+//////////////////////////////////////////////////
+std::optional<std::string> Param::GetMaxValueAsString() const
+{
+  if (this->dataPtr->maxValue.has_value())
+  {
+    StringStreamClassicLocale ss;
+
+    ss << ParamStreamer{ *this->dataPtr->maxValue };
+    return ss.str();
+  }
+  return std::nullopt;
+}
+
 //////////////////////////////////////////////////
 /// \brief Helper function for Param::ValueFromString
 /// \param[in] _input Input string.
@@ -478,11 +542,19 @@ bool Param::SetFromString(const std::string &_value)
     return true;
   }
 
+  auto oldValue = this->dataPtr->value;
   if (!this->ValueFromString(str))
   {
     return false;
   }
 
+  // Check if the value is permitted
+  if (!this->ValidateValue())
+  {
+    this->dataPtr->value = oldValue;
+    return false;
+  }
+
   this->dataPtr->set = true;
   return this->dataPtr->set;
 }
@@ -497,11 +569,7 @@ void Param::Reset()
 //////////////////////////////////////////////////
 ParamPtr Param::Clone() const
 {
-  ParamPtr clone(new Param(this->dataPtr->key, this->dataPtr->typeName,
-                            this->GetAsString(), this->dataPtr->required,
-                            this->dataPtr->description));
-  clone->dataPtr->set = this->dataPtr->set;
-  return clone;
+  return std::make_shared<Param>(*this);
 }
 
 //////////////////////////////////////////////////
@@ -539,3 +607,40 @@ bool Param::GetSet() const
 {
   return this->dataPtr->set;
 }
+
+/////////////////////////////////////////////////
+bool Param::ValidateValue() const
+{
+  return std::visit(
+      [this](const auto &_val) -> bool
+      {
+        using T = std::decay_t<decltype(_val)>;
+        // cppcheck-suppress syntaxError
+        if constexpr (std::is_scalar_v<T>)
+        {
+          if (this->dataPtr->minValue.has_value())
+          {
+            if (_val < std::get<T>(*this->dataPtr->minValue))
+            {
+              sdferr << "The value [" << _val
+                     << "] is less than the minimum allowed value of ["
+                     << *this->GetMinValueAsString() << "] for key ["
+                     << this->GetKey() << "]\n";
+              return false;
+            }
+          }
+          if (this->dataPtr->maxValue.has_value())
+          {
+            if (_val > std::get<T>(*this->dataPtr->maxValue))
+            {
+              sdferr << "The value [" << _val
+                     << "] is greater than the maximum allowed value of ["
+                     << *this->GetMaxValueAsString() << "] for key ["
+                     << this->GetKey() << "]\n";
+              return false;
+            }
+          }
+        }
+        return true;
+      }, this->dataPtr->value);
+}