Allow empty strings in plugin and custom attributes (gazebosim#1407)

Currently errors are generated when adding an attribute containing an empty string to a <plugin> block or as a custom attribute. This adds failing tests to confirm the bug and fixes the errors in by setting `_required == 0` when calling Element::AddAttribute. This also changes Element::ToString to print empty custom attributes. Fixes gazebosim#725. Signed-off-by: Steve Peters <[email protected]>
rudislabs · May 20, 2024 · c9779cb · c9779cb
1 parent 321d85e
commit c9779cb
Show file tree

Hide file tree

Showing 6 changed files with 24 additions and 5 deletions.
diff --git a/src/Element.cc b/src/Element.cc
@@ -671,7 +671,8 @@ void ElementPrivate::PrintAttributes(sdf::Errors &_errors,
     // better separation of concerns if the conversion process set the
     // required attributes with their default values.
     if ((*aiter)->GetSet() || (*aiter)->GetRequired() ||
-        _includeDefaultAttributes)
+        _includeDefaultAttributes ||
+        ((*aiter)->GetKey().find(':') != std::string::npos))
     {
       const std::string key = (*aiter)->GetKey();
       const auto it = attributeExceptions.find(key);

diff --git a/src/Utils.cc b/src/Utils.cc
@@ -366,7 +366,8 @@ void copyChildren(ElementPtr _sdf, tinyxml2::XMLElement *_xml,
       for (const tinyxml2::XMLAttribute *attribute = elemXml->FirstAttribute();
            attribute; attribute = attribute->Next())
       {
-        element->AddAttribute(attribute->Name(), "string", "", 1, "");
+        // Add with required == 0 to allow unrecognized attribute to be empty
+        element->AddAttribute(attribute->Name(), "string", "", 0, "");
         element->GetAttribute(attribute->Name())->SetFromString(
             attribute->Value());
       }

diff --git a/src/parser.cc b/src/parser.cc
@@ -1440,7 +1440,8 @@ static bool readAttributes(tinyxml2::XMLElement *_xml, ElementPtr _sdf,
     // attribute is found
     if (std::strchr(attribute->Name(), ':') != nullptr)
     {
-      _sdf->AddAttribute(attribute->Name(), "string", "", 1, "");
+      // Add with required == 0 to allow custom attribute to be empty
+      _sdf->AddAttribute(attribute->Name(), "string", "", 0, "");
       _sdf->GetAttribute(attribute->Name())->SetFromString(attribute->Value());
       attribute = attribute->Next();
       continue;

diff --git a/test/integration/custom_elems_attrs.sdf b/test/integration/custom_elems_attrs.sdf
@@ -4,7 +4,7 @@
     <mysim:description>Description of this world</mysim:description>
     <model name="M1">
       <link name="L1" mysim:custom_attr_str="A" mysim:custom_attr_int="5" />
-      <link name="L2" />
+      <link name="L2" mysim:empty_attr="" />
       <joint name="J1" type="revolute">
         <parent>L1</parent>
         <child>L2</child>

diff --git a/test/integration/plugin_attribute.cc b/test/integration/plugin_attribute.cc
@@ -30,7 +30,7 @@ std::string get_sdf_string()
     << "<model name='model'>"
     << "  <link name='link'/>"
     << "  <plugin name='example' filename='libexample.so'>"
-    << "    <user attribute='attribute' />"
+    << "    <user attribute='attribute' empty_attribute='' />"
     << "    <value attribute='attribute'>value</value>"
     << "  </plugin>"
     << "</model>"
@@ -54,6 +54,8 @@ TEST(PluginAttribute, ParseAttributes)
     EXPECT_TRUE(user->HasAttribute("attribute"));
     EXPECT_EQ(user->GetAttribute("attribute")->GetAsString(),
               std::string("attribute"));
+    EXPECT_TRUE(user->HasAttribute("empty_attribute"));
+    EXPECT_EQ(user->GetAttribute("empty_attribute")->GetAsString(), "");
   }
   {
     sdf::ElementPtr value = plugin->GetElement("value");

diff --git a/test/integration/sdf_custom.cc b/test/integration/sdf_custom.cc
@@ -60,6 +60,20 @@ TEST(SDFParser, CustomElements)
   auto customAttrInt = link1Element->Get<int>("mysim:custom_attr_int");
   EXPECT_EQ(5, customAttrInt);
 
+  // Check empty attribute in link L2
+  const sdf::Link *link2 = model->LinkByName("L2");
+  ASSERT_TRUE(link2 != nullptr);
+  sdf::ElementPtr link2Element = link2->Element();
+  ASSERT_TRUE(link2Element != nullptr);
+  EXPECT_TRUE(link2Element->HasAttribute("mysim:empty_attr"));
+  auto emptyAttrStr = link2Element->Get<std::string>("mysim:empty_attr");
+  EXPECT_EQ("", emptyAttrStr);
+
+  // Ensure that mysim:empty_attr appears when calling ToString("")
+  auto rootToString = link2Element->ToString("");
+  EXPECT_NE(std::string::npos, rootToString.find("mysim:empty_attr=''"))
+    << rootToString;
+
   // Use of sdf::Model::Element() to obtain an sdf::ElementPtr object
   sdf::ElementPtr modelElement = model->Element();