Validate XML boundary names#306
Merged
Merged
Conversation
Contributor
Reviewer's GuideValidates XML attribute and root element names to prevent malformed XML, and aligns custom root handling with existing XML name normalization, with tests and LAT docs updated accordingly. Flow diagram for XML attribute name validation in make_attrstringflowchart TD
A[make_attrstring receives attr dict] --> B{attr is empty?}
B -- yes --> C[Return empty string]
B -- no --> D[validate_xml_attr_names]
D --> E{Iterate keys in attr}
E --> F[key_is_valid_xml_attr]
F --> G{minidom parseString succeeds?}
G -- no --> H[Raise ValueError Invalid XML attribute name]
G -- yes --> I[All keys valid]
I --> J[Build attribute string and return]
Flow diagram for custom_root XML name normalization in dicttoxmlflowchart TD
A[dicttoxml called with custom_root] --> B{root is True?}
B -- no --> C[Call convert with parent empty]
B -- yes --> D[Append XML declaration]
D --> E[make_valid_xml_name with custom_root and empty attrs]
E --> F[Receive normalized custom_root and root_attr]
F --> G[Call convert with parent normalized custom_root]
G --> H["Build root start tag with make_attrstring(root_attr) and namespace_str"]
H --> I[Append closing tag using normalized custom_root]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #306 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 552 570 +18
=========================================
+ Hits 552 570 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/test_dict2xml.py" line_range="511-519" />
<code_context>
+ b"<custom_root><key>value</key></custom_root>"
+ )
+
+ # @lat: [[tests#Conversion behavior#Invalid custom attributes are rejected]]
+ def test_dicttoxml_rejects_invalid_custom_attribute_names(self) -> None:
+ """Invalid custom attribute names should fail before dicttoxml returns malformed XML bytes."""
+ with pytest.raises(ValueError, match="Invalid XML attribute name"):
+ dicttoxml.dicttoxml(
+ {"key": {"@attrs": {"bad attr": "value"}, "@val": "payload"}},
+ root=False,
+ attr_type=False,
+ )
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider broadening attribute-name tests to cover multiple invalid and valid edge-case names
The new `test_dicttoxml_rejects_invalid_custom_attribute_names` currently exercises only one invalid attribute (`"bad attr"`). To strengthen coverage of `key_is_valid_xml_attr` / `validate_xml_attr_names`, please parameterize this test (or add more) to include:
- Several invalid names (e.g. `""`, `"1foo"`, `"foo>bar"`, something with a newline or quote).
- A few borderline valid names (e.g. `"a_b"`, `"a-b"`, `"xmlAttr"`) to confirm the validator isn’t over-restrictive.
This will better cover edge-case attribute names and the XML-based validation behavior.
```suggestion
# @lat: [[tests#Conversion behavior#Invalid custom attributes are rejected]]
@pytest.mark.parametrize(
"attr_name",
[
"bad attr", # space
"", # empty
"1foo", # starts with digit
"foo>bar", # invalid character
"foo\nbar", # newline
'foo"bar', # quote
],
)
def test_dicttoxml_rejects_invalid_custom_attribute_names(self, attr_name: str) -> None:
"""Invalid custom attribute names should fail before dicttoxml returns malformed XML bytes."""
with pytest.raises(ValueError, match="Invalid XML attribute name"):
dicttoxml.dicttoxml(
{"key": {"@attrs": {attr_name: "value"}, "@val": "payload"}},
root=False,
attr_type=False,
)
@pytest.mark.parametrize(
"attr_name",
[
"a_b", # underscore
"a-b", # hyphen
"xmlAttr", # mixed case, xml-prefixed but still syntactically valid
],
)
def test_dicttoxml_accepts_valid_custom_attribute_names(self, attr_name: str) -> None:
"""Borderline-but-valid attribute names should be accepted by the XML attribute validator."""
result = dicttoxml.dicttoxml(
{"key": {"@attrs": {attr_name: "value"}, "@val": "payload"}},
root=False,
attr_type=False,
)
# We don't assert exact XML shape here; reaching this point means no ValueError was raised.
assert isinstance(result, bytes)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Validate XML names for custom roots and attribute keys to prevent malformed XML output and ensure consistent name normalization.
Bug Fixes:
Documentation:
Tests: