Fix JSON conversion edge cases and URL reader robustness#284
Merged
Conversation
Contributor
Reviewer's GuideRefactors JSON input handling and URL reading to use a shared JSONValue type, harden error handling and timeouts, and fixes XML conversion edge cases around falsy values, invalid names, @attrs/@Val mutation, and @Flat behavior, with tests and docs updated accordingly. Sequence diagram for hardened readfromurl JSON loadingsequenceDiagram
actor Caller
participant utils_readfromurl as utils.readfromurl
participant http_pool as urllib3.PoolManager
Caller->>utils_readfromurl: readfromurl(url, params)
utils_readfromurl->>http_pool: request(GET, url, fields=params, timeout=DEFAULT_URL_TIMEOUT, retries=false)
alt network_error
http_pool-->>utils_readfromurl: HTTPError
utils_readfromurl-->>Caller: raise URLReadError
else http_status_200
utils_readfromurl->>utils_readfromurl: decode response.data as utf8
alt valid_json
utils_readfromurl-->>Caller: JSONValue
else invalid_json_or_decode
utils_readfromurl-->>Caller: raise URLReadError
end
else http_status_not_200
http_pool-->>utils_readfromurl: response(status!=200)
utils_readfromurl-->>Caller: raise URLReadError
end
Class diagram for JSONValue usage and XML conversion helpersclassDiagram
class JSONValue {
<<type_alias>>
dict_str_any
list_any
str
int
float
bool
NoneType
}
class Json2xml {
- data JSONValue
- wrapper str
- root bool
- pretty bool
- attr_type bool
- item_wrap bool
- cdata bool
- list_headers bool
+ Json2xml(data JSONValue, wrapper str, root bool, pretty bool, attr_type bool, item_wrap bool, cdata bool, list_headers bool)
+ to_xml() Any
}
class utils_module {
+ readfromjson(filename str) JSONValue
+ readfromurl(url str, params dict_str_str) JSONValue
+ readfromstring(jsondata object) JSONValue
+ JSONReadError
+ URLReadError
+ StringReadError
}
class dicttoxml_module {
+ dicttoxml(data JSONValue, root bool, wrapper str, attr_type bool, item_wrap bool, cdata bool, list_headers bool) bytes
+ get_xml_type(val Any) str
+ escape_xml(s str_int_float_number_none) str
+ make_valid_xml_name(key str, attr dict_str_any) tuple_str_dict
+ dict2xml_str(parent str, item Any, attr dict_str_any, attr_type bool, item_name str, item_func function, cdata bool, list_headers bool) str
+ convert(obj Any, parent str, attr_type bool, item_name str, item_func function, cdata bool, item_wrap bool, list_headers bool) str
+ convert_dict(obj dict_str_any, parent str, attr_type bool, item_func function, cdata bool, item_wrap bool, list_headers bool) str
}
Json2xml ..> JSONValue : holds
Json2xml ..> dicttoxml_module : calls dicttoxml
utils_module ..> JSONValue : returns
dicttoxml_module ..> JSONValue : consumes
dicttoxml_module ..> JSONValue : serializes falsy_values
dicttoxml_module ..> JSONValue : respects_attrs_and_val
dicttoxml_module ..> JSONValue : handles_flat_keys
File-Level Changes
Assessment against linked issues
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 #284 +/- ##
==========================================
+ Coverage 95.93% 96.07% +0.14%
==========================================
Files 5 6 +1
Lines 467 484 +17
==========================================
+ Hits 448 465 +17
Misses 19 19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
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="680-681" />
<code_context>
+ # @lat: [[tests#Conversion behavior#Special attributes do not mutate input]]
</code_context>
<issue_to_address>
**suggestion (testing):** Add explicit tests for @val with None and boolean values to cover new dict2xml_str primitive handling
The new `dict2xml_str` logic adds special handling for `None` and booleans in `rawitem`, but the tests here only cover `@attrs/@val` mutation, invalid XML names, and `@flat`. To cover the new behavior, please also add tests like:
- `{"field": {"@val": None}}` to verify we emit an empty element and don’t raise.
- `@val` set to `True`/`False` to verify we serialize as lowercase `"true"`/`"false"` and that attributes/element names are still correct.
This will better protect against regressions in the new primitive-handling path.
```suggestion
# @lat: [[tests#Conversion behavior#Special attributes do not mutate input]]
def test_dict2xml_val_none_emits_empty_element(self) -> None:
"""@val=None should serialize as an empty element without raising."""
xml = dicttoxml.dict2xml_str({"field": {"@val": None}})
# We should get a <field> element, but never the string "None"
assert "<field" in xml
assert "None" not in xml
# Accept either self-closing or explicit close-tag forms
assert (
"<field/>" in xml
or "<field />" in xml
or "<field></field>" in xml
)
def test_dict2xml_val_bool_serialization(self) -> None:
"""@val with boolean values should serialize to lowercase true/false."""
xml_true = dicttoxml.dict2xml_str({"active": {"@val": True}})
xml_false = dicttoxml.dict2xml_str({"active": {"@val": False}})
# Booleans should be serialized as lowercase true/false
assert "<active>true</active>" in xml_true
assert "<active>false</active>" in xml_false
# Uppercase Python reprs must not leak into the XML
assert "True" not in xml_true
assert "False" not in xml_false
# Attributes must still be preserved on the element
xml_with_attr = dicttoxml.dict2xml_str(
{"active": {"@attrs": {"flag": "yes"}, "@val": True}}
)
assert '<active flag="yes">true</active>' in xml_with_attr
def test_dicttoxml_does_not_mutate_special_attribute_input(self) -> None:
```
</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
2 participants
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
Fixes #283.
Json2xmlAPIJSONValuetyping for readers, CLI, and converter inputsURLReadErrorwrapping@attrs/@valcaller data during conversion@flatout of scalar/dict tagsxvsconfig optionValidation
make lintmake typecheckpytest -q testsmake test-rustlat_checkSummary by Sourcery
Harden JSON input handling and XML conversion semantics while aligning tests and documentation with the new behavior.
New Features:
Bug Fixes:
Enhancements:
Tests:
Chores: