Skip to content

Commit

Permalink
ParamPassing: sdfwarns to sdf::Errors when warnings policy set to sdf…
Browse files Browse the repository at this point in the history 
…::EnforcementPolicy::ERR (#1135)

Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
  • Loading branch information
@azeey
Marco A. Gutierrez and azeey authored Nov 25, 2022
1 parent 8fcbe1e commit 702dc37
Show file tree
Hide file tree
Showing 3 changed files with 174 additions and 25 deletions.
59 changes: 37 additions & 22 deletions src/ParamPassing.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

#include "ParamPassing.hh"
#include "parser_private.hh"
#include "Utils.hh"
#include "XmlUtils.hh"

namespace sdf
Expand Down Expand Up @@ -176,11 +177,11 @@ void updateParams(const ParserConfig &_config,
}
else if (actionStr == "modify")
{
modify(childElemXml, elem, _errors);
modify(childElemXml, _config, elem, _errors);
}
else if (actionStr == "remove")
{
remove(childElemXml, elem, _errors);
remove(childElemXml, _config, elem, _errors);
}
else if (actionStr == "replace")
{
Expand Down Expand Up @@ -292,6 +293,8 @@ bool isValidAction(const std::string &_action)
//////////////////////////////////////////////////
ElementPtr getElementByName(const ElementPtr _elem,
const tinyxml2::XMLElement *_xml,
const sdf::ParserConfig _config,
sdf::Errors &_errors,
const bool _isModifyAction)
{
std::string elemName = _xml->Name();
Expand Down Expand Up @@ -324,10 +327,14 @@ ElementPtr getElementByName(const ElementPtr _elem,
else if (elem->HasAttribute("name")
&& elem->GetAttribute("name")->GetRequired())
{
sdfwarn << "The original element [" << elemName << "] contains the "
<< "attribute 'name' but none was provided in the element modifier."
<< " The assumed element to be modified is: <" << elemName
<< " name='" << elem->Get<std::string>("name") << "'>\n";
std::stringstream ss;
ss << "The original element [" << elemName << "] contains the "
<< "attribute 'name' but none was provided in the element modifier."
<< " The assumed element to be modified is: <" << elemName
<< " name='" << elem->Get<std::string>("name") << "'>";
Error err(ErrorCode::WARNING, ss.str());
enforceConfigurablePolicyCondition(
_config.WarningsPolicy(), err, _errors);
}

return elem;
Expand Down Expand Up @@ -397,7 +404,7 @@ void handleIndividualChildActions(const ParserConfig &_config,

if (actionStr == "remove")
{
ElementPtr e = getElementByName(_elem, xmlChild);
ElementPtr e = getElementByName(_elem, xmlChild, _config, _errors);
if (e == nullptr)
{
_errors.push_back({ErrorCode::ELEMENT_MISSING,
Expand All @@ -409,14 +416,14 @@ void handleIndividualChildActions(const ParserConfig &_config,
}
else
{
remove(xmlChild, e, _errors);
remove(xmlChild, _config, e, _errors);
}

continue;
}
else if (actionStr == "modify")
{
ElementPtr e = getElementByName(_elem, xmlChild, true);
ElementPtr e = getElementByName(_elem, xmlChild, _config, _errors, true);
if (e == nullptr)
{
_errors.push_back({ErrorCode::ELEMENT_MISSING,
Expand All @@ -428,7 +435,7 @@ void handleIndividualChildActions(const ParserConfig &_config,
}
else
{
modify(xmlChild, e, _errors);
modify(xmlChild, _config, e, _errors);
}

continue;
Expand Down Expand Up @@ -472,7 +479,7 @@ void handleIndividualChildActions(const ParserConfig &_config,
}
else if (actionStr == "replace")
{
ElementPtr e = getElementByName(_elem, xmlChild);
ElementPtr e = getElementByName(_elem, xmlChild, _config, _errors);
if (e == nullptr)
{
_errors.push_back({ErrorCode::ELEMENT_MISSING,
Expand Down Expand Up @@ -562,7 +569,8 @@ void modifyAttributes(tinyxml2::XMLElement *_xml,

//////////////////////////////////////////////////
void modifyChildren(tinyxml2::XMLElement *_xml,
ElementPtr _elem, Errors &_errors)
const sdf::ParserConfig &_config, ElementPtr _elem,
Errors &_errors)
{
for (tinyxml2::XMLElement *xmlChild = _xml->FirstChildElement();
xmlChild;
Expand All @@ -579,7 +587,8 @@ void modifyChildren(tinyxml2::XMLElement *_xml,
continue;
}

ElementPtr elemChild = getElementByName(_elem, xmlChild, true);
ElementPtr elemChild = getElementByName(_elem, xmlChild,
_config, _errors, true);
ParamPtr paramChild = elemChild->GetValue();

if (xmlChild->GetText())
Expand Down Expand Up @@ -609,21 +618,26 @@ void modifyChildren(tinyxml2::XMLElement *_xml,
else
{
// sdf has child elements but no children were specified in xml
sdfwarn << "No modifications for element "
<< ElementToString(xmlChild)
<< " provided, skipping modification for:\n"
<< ElementToString(_xml) << "\n";
std::stringstream ss;
ss << "No modifications for element "
<< ElementToString(xmlChild)
<< " provided, skipping modification for:\n"
<< ElementToString(_xml);
Error err(ErrorCode::WARNING, ss.str());
enforceConfigurablePolicyCondition(
_config.WarningsPolicy(), err, _errors);
}
}
else
{
modify(xmlChild, elemChild, _errors);
modify(xmlChild, _config, elemChild, _errors);
}
}
}

//////////////////////////////////////////////////
void modify(tinyxml2::XMLElement *_xml, ElementPtr _elem, Errors &_errors)
void modify(tinyxml2::XMLElement *_xml, const sdf::ParserConfig &_config,
ElementPtr _elem, Errors &_errors)
{
modifyAttributes(_xml, _elem, _errors);

Expand All @@ -643,12 +657,13 @@ void modify(tinyxml2::XMLElement *_xml, ElementPtr _elem, Errors &_errors)
else
{
// modify children elements
modifyChildren(_xml, _elem, _errors);
modifyChildren(_xml, _config, _elem, _errors);
}
}

//////////////////////////////////////////////////
void remove(const tinyxml2::XMLElement *_xml, ElementPtr _elem, Errors &_errors)
void remove(const tinyxml2::XMLElement *_xml, const sdf::ParserConfig &_config,
ElementPtr _elem, Errors &_errors)
{
if (_xml->NoChildren())
{
Expand All @@ -663,7 +678,7 @@ void remove(const tinyxml2::XMLElement *_xml, ElementPtr _elem, Errors &_errors)
xmlChild;
xmlChild = xmlChild->NextSiblingElement())
{
elemChild = getElementByName(_elem, xmlChild);
elemChild = getElementByName(_elem, xmlChild, _config, _errors);
if (elemChild == nullptr)
{
const tinyxml2::XMLElement *xmlParent = _xml->Parent()->ToElement();
Expand Down
17 changes: 14 additions & 3 deletions src/ParamPassing.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,13 +85,17 @@ namespace sdf
/// \param[in] _elem The sdf (parent) element used to find the matching
/// child element described in xml
/// \param[in] _xml The xml element to find
/// \param[in] _config Custom parser configuration.
/// \param[out] _errors Vector of errros.
/// \param[in] _isModifyAction Is true if the action is modify, the
/// attribute 'name' may not be in the sdf element (i.e., may be a
/// modified/added attribute such as //camera)
/// \return ElementPtr to the child element matching the xml, nullptr if
/// element could not be found
ElementPtr getElementByName(const ElementPtr _elem,
const tinyxml2::XMLElement *_xml,
const sdf::ParserConfig _config,
sdf::Errors &_errors,
const bool _isModifyAction = false);

/// \brief Initialize an sdf element description from the xml element
Expand Down Expand Up @@ -143,24 +147,31 @@ namespace sdf
/// \brief Modifies the children elements of the included model
/// \param[in] _xml Pointer to the xml element which contains the elements
/// to be modified
/// \param[in] _config Custom parser configuration.
/// \param[out] _elem The element from the included model to modify
/// \param[out] _errors Vector of errors.
void modifyChildren(tinyxml2::XMLElement *_xml,
ElementPtr _elem, Errors &_errors);
const sdf::ParserConfig &_config, ElementPtr _elem,
Errors &_errors);

/// \brief Modifies element values and/or attributes of an element from the
/// included model
/// \param[in] _xml Pointer to the xml element which contains the elements
/// to be modified
/// \param[in] _config Custom parser configuration.
/// \param[out] _elem The element from the included model to modify
/// \param[out] _errors Captures errors found during parsing
void modify(tinyxml2::XMLElement *_xml, ElementPtr _elem, Errors &_errors);
void modify(tinyxml2::XMLElement *_xml, const sdf::ParserConfig &_config,
ElementPtr _elem, Errors &_errors);

/// \brief Removes an element specified in xml
/// \param[in] _xml Pointer to the xml element(s) to be removed from _elem
/// \param[in] _config Custom parser configuration.
/// \param[out] _elem The element from the included model to remove
/// elements from
/// \param[out] _errors Captures errors found during parsing
void remove(const tinyxml2::XMLElement *_xml, ElementPtr _elem,
void remove(const tinyxml2::XMLElement *_xml,
const sdf::ParserConfig &_config, ElementPtr _elem,
Errors &_errors);

/// \brief Replace an element with another element
Expand Down
123 changes: 123 additions & 0 deletions src/ParamPassing_TEST.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "ParamPassing.hh"
#include "sdf/Element.hh"
#include "sdf/parser.hh"
#include "test_utils.hh"

/////////////////////////////////////////////////
TEST(ParamPassing, GetElement)
Expand Down Expand Up @@ -91,3 +92,125 @@ TEST(ParamPassing, GetElement)
"model::test_link::test_visual");
EXPECT_EQ(nullptr, paramPassElem);
}

////////////////////////////////////////
// Test warnings outputs for GetElementByName
TEST(ParamPassing, GetElementByNameWarningOutput)
{
std::stringstream buffer;
sdf::testing::RedirectConsoleStream redir(
sdf::Console::Instance()->GetMsgStream(), &buffer);

std::ostringstream stream;
stream << "<?xml version=\"1.0\"?>"
<< "<sdf version='1.8'>"
<< " <model name='test_model'>"
<< " <link name='link_name'/>"
<< " </model>"
<< "</sdf>";

sdf::SDFPtr sdfParsed(new sdf::SDF());
sdf::init(sdfParsed);

sdf::Errors errors;
sdf::ParserConfig parserConfig;
parserConfig.SetWarningsPolicy(sdf::EnforcementPolicy::ERR);
sdf::readString(stream.str(), parserConfig, sdfParsed, errors);
EXPECT_TRUE(errors.empty());

std::ostringstream stream2;
stream2 << " <model>"
<< " </model>";
tinyxml2::XMLDocument doc;
doc.Parse(stream2.str().c_str());

sdf::ParamPassing::getElementByName(sdfParsed->Root(),
doc.FirstChildElement("model"),
parserConfig,
errors);
ASSERT_EQ(errors.size(), 1u);
EXPECT_EQ(errors[0].Code(), sdf::ErrorCode::WARNING);
EXPECT_NE(std::string::npos, errors[0].Message().find(
"The original element [model] contains the attribute 'name' but none was"
" provided in the element modifier. The assumed element to be modified "
"is: <model name='test_model'>"));
errors.clear();

// Check nothing has been printed
EXPECT_TRUE(buffer.str().empty()) << buffer.str();

parserConfig.SetWarningsPolicy(sdf::EnforcementPolicy::WARN);
sdf::ParamPassing::getElementByName(sdfParsed->Root(),
doc.FirstChildElement("model"),
parserConfig,
errors);
EXPECT_TRUE(errors.empty());
// Check the warning has been printed
EXPECT_NE(std::string::npos, buffer.str().find(
"The original element [model] contains the attribute 'name' but none "
"was provided in the element modifier. The assumed element to be "
"modified is: <model name='test_model'>"));
}

////////////////////////////////////////
// Test warnings outputs for GetElementByName
TEST(ParamPassing, ModifyChildrenNameWarningOutput)
{
std::stringstream buffer;
sdf::testing::RedirectConsoleStream redir(
sdf::Console::Instance()->GetMsgStream(), &buffer);

std::ostringstream stream;
stream << "<?xml version=\"1.0\"?>"
<< "<sdf version='1.8'>"
<< " <model name='test_model'>"
<< " <link name='link_name'/>"
<< " </model>"
<< "</sdf>";

sdf::SDFPtr sdfParsed(new sdf::SDF());
sdf::init(sdfParsed);

sdf::Errors errors;
sdf::ParserConfig parserConfig;
parserConfig.SetWarningsPolicy(sdf::EnforcementPolicy::ERR);
sdf::readString(stream.str(), parserConfig, sdfParsed, errors);
EXPECT_TRUE(errors.empty());

std::ostringstream stream2;
stream2 << "<sdf version='1.8'>"
<< " <model name='test'>"
<< " </model>"
<< "</sdf>";
tinyxml2::XMLDocument doc;
doc.Parse(stream2.str().c_str());

sdf::ParamPassing::modifyChildren(
doc.FirstChildElement("sdf"),
parserConfig,
sdfParsed->Root(),
errors);
ASSERT_EQ(errors.size(), 1u);
EXPECT_EQ(errors[0].Code(), sdf::ErrorCode::WARNING);
EXPECT_NE(std::string::npos, errors[0].Message().find(
"No modifications for element <model name=\"test\"/>\n provided, "
"skipping modification for:\n<sdf version=\"1.8\">\n"
" <model name=\"test\"/>\n</sdf>"));
errors.clear();

// Check nothing has been printed
EXPECT_TRUE(buffer.str().empty()) << buffer.str();

parserConfig.SetWarningsPolicy(sdf::EnforcementPolicy::WARN);
sdf::ParamPassing::modifyChildren(
doc.FirstChildElement("sdf"),
parserConfig,
sdfParsed->Root(),
errors);
EXPECT_TRUE(errors.empty());
// Check the warning has been printed
EXPECT_NE(std::string::npos, buffer.str().find(
"No modifications for element <model name=\"test\"/>\n provided, "
"skipping modification for:\n<sdf version=\"1.8\">\n"
" <model name=\"test\"/>\n</sdf>"));
}

0 comments on commit 702dc37

Please sign in to comment.