Skip to content

Commit

Permalink
Enforce minimum/maximum values specified in SDFormat description file…
Browse files Browse the repository at this point in the history
…s (Retry PR) (#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]>
  • Loading branch information
azeey authored and scpeters committed Jul 2, 2020
1 parent 55029bb commit 6780874
Show file tree
Hide file tree
Showing 11 changed files with 404 additions and 23 deletions.
3 changes: 3 additions & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
18 changes: 18 additions & 0 deletions Migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
21 changes: 18 additions & 3 deletions include/sdf/Element.hh
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
64 changes: 58 additions & 6 deletions include/sdf/Param.hh
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <cstdint>
#include <functional>
#include <memory>
#include <optional>
#include <sstream>
#include <string>
#include <typeinfo>
Expand Down Expand Up @@ -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();

Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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.
Expand Down Expand Up @@ -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;
};

///////////////////////////////////////////////
Expand Down
24 changes: 24 additions & 0 deletions src/Element.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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";
Expand Down
121 changes: 113 additions & 8 deletions src/Param.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand All @@ -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;
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
}
Expand All @@ -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);
}

//////////////////////////////////////////////////
Expand Down Expand Up @@ -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);
}
Loading

0 comments on commit 6780874

Please sign in to comment.