Allow empty strings in plugin and custom attributes

[scpeters]

🦟 Bug fix

Fixes #725

Summary

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 in 0637c21 to confirm the bug and fixes the errors in e55a9e1 by setting _required == 0 when calling Element::AddAttribute.

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[azeey]

Looks good, but can you check the output of ToString() to make sure the custom attribute is included? I seem to recall that was a problem.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.44%. Comparing base (ff9b3ad) to head (e55a9e1).
Report is 13 commits behind head on sdf14.

❗ Current head e55a9e1 differs from pull request most recent head 39036ee

Please upload reports for the commit 39036ee to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##            sdf14    #1407      +/-   ##
==========================================
+ Coverage   92.42%   92.44%   +0.01%     
==========================================
  Files         134      135       +1     
  Lines       17751    17837      +86     
==========================================
+ Hits        16406    16489      +83     
- Misses       1345     1348       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[scpeters]

Looks good, but can you check the output of ToString() to make sure the custom attribute is included? I seem to recall that was a problem.

ok, thanks for bringing that up. I updated ElementPrivate::PrintAttributes in a4f811f to always print custom attributes

Empty attributes in the <plugin/> element won't be printed by ToString unless they are in a custom xml namespace, as I've noticed with the following test patch:

diff --git a/test/integration/plugin_attribute.cc b/test/integration/plugin_attribute.cc
index 7f4f9f4c..b66c9da1 100644
--- a/test/integration/plugin_attribute.cc
+++ b/test/integration/plugin_attribute.cc
@@ -65,4 +65,9 @@ TEST(PluginAttribute, ParseAttributes)
     EXPECT_EQ(value->Get<std::string>(""),
               std::string("value"));
   }
+  {
+    auto pluginToString = plugin->ToString("");
+    EXPECT_NE(std::string::npos, pluginToString.find("empty_attribute=''"))
+      << pluginToString;
+  }
 }

I don't have a good idea for how to fix it, so I'd prefer to open an issue documenting that behavior and potentially close it as won't fix

[azeey]

LGTM! Opening an issue to document the behavior in <plugin> sounds good.

[scpeters]

LGTM! Opening an issue to document the behavior in <plugin> sounds good.

opened a new issue: #1421