Skip to content

Commit

Permalink
Merge pull request #537 from osrf/aaron/fix-string-parsing-error-bug
Browse files Browse the repository at this point in the history 
Fixed file path in errors when parsing sdf or urdf from string
  • Loading branch information
@aaronchongth
aaronchongth authored Apr 28, 2021
2 parents 25004f0 + b922496 commit 689ee84
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 26 deletions.
8 changes: 8 additions & 0 deletions include/sdf/Types.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,14 @@ namespace sdf

const std::string kSdfScopeDelimiter = "::";

/// \brief The source path replacement if it was parsed from a string,
/// instead of a file.
constexpr char kSdfStringSource[] = "data-string";

/// \brief The source path replacement if the urdf was parsed from a string,
/// instead of a file.
constexpr char kUrdfStringSource[] = "urdf string";

/// \brief Split a string using the delimiter in splitter.
/// \param[in] str The string to split.
/// \param[in] splitter The delimiter to use.
Expand Down
2 changes: 1 addition & 1 deletion src/Element.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ void Element::SetParent(const ElementPtr _parent)

// If this element doesn't have a path, get it from the parent
if (nullptr != _parent && (this->FilePath().empty() ||
this->FilePath() == "data-string"))
this->FilePath() == std::string(kSdfStringSource)))
{
this->SetFilePath(_parent->FilePath());
}
Expand Down
57 changes: 32 additions & 25 deletions src/parser.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,8 @@ bool readStringInternal(const std::string &_xmlString, const bool _convert,
sdferr << "Error parsing XML from string: " << xmlDoc.ErrorStr() << '\n';
return false;
}
if (readDoc(&xmlDoc, _sdf, "data-string", _convert, _config, _errors))
if (readDoc(&xmlDoc, _sdf, std::string(kSdfStringSource), _convert, _config,
_errors))
{
return true;
}
Expand All @@ -584,7 +585,8 @@ bool readStringInternal(const std::string &_xmlString, const bool _convert,
auto doc = makeSdfDoc();
u2g.InitModelString(_xmlString, &doc);

if (sdf::readDoc(&doc, _sdf, "urdf string", _convert, _config, _errors))
if (sdf::readDoc(&doc, _sdf, std::string(kUrdfStringSource), _convert,
_config, _errors))
{
sdfdbg << "Parsing from urdf.\n";
return true;
Expand Down Expand Up @@ -629,7 +631,8 @@ bool readString(const std::string &_xmlString, const ParserConfig &_config,
sdferr << "Error parsing XML from string: " << xmlDoc.ErrorStr() << '\n';
return false;
}
if (readDoc(&xmlDoc, _sdf, "data-string", true, _config, _errors))
if (readDoc(&xmlDoc, _sdf, std::string(kSdfStringSource), true, _config,
_errors))
{
return true;
}
Expand Down Expand Up @@ -665,7 +668,7 @@ bool readDoc(tinyxml2::XMLDocument *_xmlDoc, SDFPtr _sdf,
return false;
}

if (_source != "data-string")
if (_source != std::string(kSdfStringSource))
{
_sdf->SetFilePath(_source);
}
Expand Down Expand Up @@ -745,7 +748,7 @@ bool readDoc(tinyxml2::XMLDocument *_xmlDoc, ElementPtr _sdf,
return false;
}

if (_source != "data-string")
if (_source != std::string(kSdfStringSource))
{
_sdf->SetFilePath(_source);
}
Expand Down Expand Up @@ -929,6 +932,10 @@ std::string getModelFilePath(const std::string &_modelDirPath)
bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
const ParserConfig &_config, const std::string &_source, Errors &_errors)
{
std::string sourcePath = _source;
if (_source == kSdfStringSource || _source == kUrdfStringSource)
sourcePath = "<" + _source + ">";

// Check if the element pointer is deprecated.
if (_sdf->GetRequired() == "-1")
{
Expand All @@ -947,7 +954,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
_errors.push_back({
ErrorCode::ELEMENT_MISSING,
"SDF Element<" + _sdf->GetName() + "> is missing",
_source});
sourcePath});
return false;
}
else
Expand Down Expand Up @@ -1021,7 +1028,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
"'" + std::string(attribute->Value()) +
"' is reserved; it cannot be used as a value of "
"attribute [" + p->GetKey() + "]",
_source,
sourcePath,
attribute->GetLineNum()});
}
}
Expand All @@ -1031,7 +1038,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
_errors.push_back({
ErrorCode::ATTRIBUTE_INVALID,
"Unable to read attribute[" + p->GetKey() + "]",
_source,
sourcePath,
attribute->GetLineNum()});
return false;
}
Expand All @@ -1050,7 +1057,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
Error(
ErrorCode::ATTRIBUTE_INCORRECT_TYPE,
ss.str(),
_source,
sourcePath,
_xml->GetLineNum()),
_errors);
}
Expand All @@ -1068,7 +1075,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
ErrorCode::ATTRIBUTE_MISSING,
"Required attribute[" + p->GetKey() + "] in element[" + _xml->Value()
+ "] is not specified in SDF.",
_source,
sourcePath,
_xml->GetLineNum()});
return false;
}
Expand Down Expand Up @@ -1104,7 +1111,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
_errors.push_back({
ErrorCode::URI_LOOKUP,
"Unable to find uri[" + uri + "]",
_source,
sourcePath,
uriElement->GetLineNum()});
continue;
}
Expand All @@ -1122,7 +1129,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
"Unable to resolve uri[" + uri + "] to model path [" +
modelPath + "] since it does not contain a model.config " +
"file.",
_source,
sourcePath,
uriElement->GetLineNum()});
continue;
}
Expand All @@ -1141,7 +1148,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
_errors.push_back({
ErrorCode::ATTRIBUTE_MISSING,
"<include> element missing 'uri' attribute",
_source,
sourcePath,
elemXml->GetLineNum()});
continue;
}
Expand Down Expand Up @@ -1170,7 +1177,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
_errors.push_back({
ErrorCode::FILE_READ,
"Unable to read file[" + filename + "]",
_source,
sourcePath,
uriElement->GetLineNum()});
return false;
}
Expand Down Expand Up @@ -1212,7 +1219,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
ErrorCode::ELEMENT_MISSING,
"Failed to find top level <model> / <actor> / <light> for "
"<include>\n",
_source,
sourcePath,
uriElement->GetLineNum()});
continue;
}
Expand Down Expand Up @@ -1287,7 +1294,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
ErrorCode::MODEL_PLACEMENT_FRAME_INVALID,
"<pose> is required when specifying the placement_frame "
"element",
_source,
sourcePath,
elemXml->GetLineNum()});
return false;
}
Expand All @@ -1301,7 +1308,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
"'" + placementFrameVal +
"' is reserved; it cannot be used as a value of "
"element [placement_frame]",
_source,
sourcePath,
placementFrameElem->GetLineNum()});
}
topLevelElem->GetAttribute("placement_frame")
Expand All @@ -1320,12 +1327,12 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
pluginElem = topLevelElem->AddElement("plugin");

if (!readXml(
childElemXml, pluginElem, _config, _source, _errors))
childElemXml, pluginElem, _config, sourcePath, _errors))
{
_errors.push_back({
ErrorCode::ELEMENT_INVALID,
"Error reading plugin element",
_source,
sourcePath,
childElemXml->GetLineNum()});
return false;
}
Expand Down Expand Up @@ -1360,7 +1367,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
{
ElementPtr element = elemDesc->Clone();
element->SetParent(_sdf);
if (readXml(elemXml, element, _config, _source, _errors))
if (readXml(elemXml, element, _config, sourcePath, _errors))
{
_sdf->InsertElement(element);
}
Expand All @@ -1370,7 +1377,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
ErrorCode::ELEMENT_INVALID,
std::string("Error reading element <") +
elemXml->Value() + ">",
_source,
sourcePath,
elemXml->GetLineNum()});
return false;
}
Expand All @@ -1392,7 +1399,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
Error(
ErrorCode::ELEMENT_INCORRECT_TYPE,
ss.str(),
_source,
sourcePath,
elemXml->GetLineNum()),
_errors);

Expand Down Expand Up @@ -1420,7 +1427,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
ErrorCode::ELEMENT_MISSING,
"XML Missing required element[" + elemDesc->GetName() +
"], child of element[" + _sdf->GetName() + "]",
_source,
sourcePath,
elemXml->GetLineNum()});
return false;
}
Expand Down Expand Up @@ -1595,8 +1602,8 @@ bool convertString(const std::string &_sdfString, const std::string &_version,
if (sdf::Converter::Convert(&xmlDoc, _version, true))
{
Errors errors;
bool result =
sdf::readDoc(&xmlDoc, _sdf, "data-string", false, _config, errors);
bool result = sdf::readDoc(&xmlDoc, _sdf, std::string(kSdfStringSource),
false, _config, errors);

// Output errors
for (auto const &e : errors)
Expand Down
78 changes: 78 additions & 0 deletions src/parser_TEST.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,84 @@ TEST_F(ValueConstraintsFixture, ElementMinMaxValues)
}
}

/////////////////////////////////////////////////
/// Check for parsing errors while reading strings
TEST(Parser, ReadSingleLineStringError)
{
sdf::SDFPtr sdf = InitSDF();
std::ostringstream stream;
stream
<< "<sdf version='1.8'>"
<< "<model name=\"test\">"
<< " <link name=\"test1\">"
<< " <visual name=\"good\">"
<< " <geometry>"
<< " <box><size>1 1 1</size></box>"
<< " </geometry>"
<< " </visual>"
<< " </link>"
<< " <link>"
<< " <visual name=\"good2\">"
<< " <geometry>"
<< " <box><size>1 1 1</size></box>"
<< " </geometry>"
<< " </visual>"
<< " </link>"
<< "</model>"
<< "</sdf>";
sdf::Errors errors;
EXPECT_FALSE(sdf::readString(stream.str(), sdf, errors));
ASSERT_NE(errors.size(), 0u);

std::cerr << errors[0] << std::endl;

EXPECT_EQ(errors[0].Code(), sdf::ErrorCode::ATTRIBUTE_MISSING);
ASSERT_TRUE(errors[0].FilePath().has_value());
EXPECT_EQ(errors[0].FilePath().value(),
"<" + std::string(sdf::kSdfStringSource) + ">");
ASSERT_TRUE(errors[0].LineNumber().has_value());
EXPECT_EQ(errors[0].LineNumber().value(), 1);
}

/////////////////////////////////////////////////
/// Check for parsing errors while reading strings
TEST(Parser, ReadMultiLineStringError)
{
sdf::SDFPtr sdf = InitSDF();
std::ostringstream stream;
stream
<< "<sdf version='1.8'>\n"
<< "<model name=\"test\">\n"
<< " <link name=\"test1\">\n"
<< " <visual name=\"good\">\n"
<< " <geometry>\n"
<< " <box><size>1 1 1</size></box>\n"
<< " </geometry>\n"
<< " </visual>\n"
<< " </link>\n"
<< " <link>\n"
<< " <visual name=\"good2\">\n"
<< " <geometry>\n"
<< " <box><size>1 1 1</size></box>\n"
<< " </geometry>\n"
<< " </visual>\n"
<< " </link>\n"
<< "</model>\n"
<< "</sdf>\n";
sdf::Errors errors;
EXPECT_FALSE(sdf::readString(stream.str(), sdf, errors));
ASSERT_NE(errors.size(), 0u);

std::cerr << errors[0] << std::endl;

EXPECT_EQ(errors[0].Code(), sdf::ErrorCode::ATTRIBUTE_MISSING);
ASSERT_TRUE(errors[0].FilePath().has_value());
EXPECT_EQ(errors[0].FilePath().value(),
"<" + std::string(sdf::kSdfStringSource) + ">");
ASSERT_TRUE(errors[0].LineNumber().has_value());
EXPECT_EQ(errors[0].LineNumber().value(), 10);
}

/////////////////////////////////////////////////
/// Main
int main(int argc, char **argv)
Expand Down

0 comments on commit 689ee84

Please sign in to comment.