Add rust tests #271
Merged
Add rust tests #271
Conversation
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
- Make pyo3 dependency optional behind 'python' feature flag - Add rlib crate-type to support both cdylib (Python) and rlib (fuzzing) - Gate all Python-specific code with #[cfg(feature = "python")] - Make pure Rust functions (escape_xml, wrap_cdata, etc.) public for fuzzing - Add comprehensive unit tests for XML utility functions Fixes linker error with Python 3.14 where PyUnicode_DATA and PyUnicode_KIND symbols are now inline macros, not exported functions. Fuzz targets can now build without linking against Python. Amp-Thread-ID: https://ampcode.com/threads/T-019c0425-ac62-76d8-9d59-4d6aba3edf45 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c0425-ac62-76d8-9d59-4d6aba3edf45 Co-authored-by: Amp <amp@ampcode.com>
Use more specific pattern matching (` key="`) instead of just checking if key exists as substring. This avoids false positives with overlapping keys (e.g. 'a' vs 'aa') or malformed attribute formatting. Suggested-by: sourcery-ai Amp-Thread-ID: https://ampcode.com/threads/T-019c0425-ac62-76d8-9d59-4d6aba3edf45 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c0438-3a17-7033-9c8b-a5dd94144dd5 Co-authored-by: Amp <amp@ampcode.com>
Contributor
Reviewer's GuideAdds unit tests and fuzzing for core Rust XML helper functions, makes them public and buildable without the Python extension, introduces a Python feature flag in the Rust crate, wires Rust unit tests into CI, and refreshes benchmark documentation and references. Class diagram for core Rust XML helpers, tests, and fuzz targetsclassDiagram
class json2xml_rs {
+escape_xml(s: &str) String
+wrap_cdata(s: &str) String
+is_valid_xml_name(key: &str) bool
+make_valid_xml_name(key: &str) (String, Option<(String, String)>)
+make_attr_string(attrs: &[(String, String)]) String
}
class ConvertConfig {
<<cfg_feature_python>>
-attr_type: bool
-cdata: bool
-item_wrap: bool
-list_headers: bool
}
class escape_xml_tests {
<<test_module>>
+escapes_ampersand()
+escapes_double_quote()
+escapes_single_quote()
+escapes_less_than()
+escapes_greater_than()
+escapes_all_special_chars()
+handles_empty_string()
+handles_no_special_chars()
+handles_unicode()
}
class wrap_cdata_tests {
<<test_module>>
+wraps_simple_string()
+wraps_empty_string()
+escapes_cdata_end_sequence()
+handles_multiple_cdata_end_sequences()
+handles_special_xml_chars()
}
class is_valid_xml_name_tests {
<<test_module>>
+accepts_simple_name()
+accepts_name_with_underscore_prefix()
+accepts_name_with_numbers()
+accepts_name_with_hyphens()
+accepts_name_with_dots()
+accepts_name_with_colons()
+rejects_empty_string()
+rejects_name_starting_with_number()
+rejects_name_starting_with_hyphen()
+rejects_name_with_spaces()
+rejects_xml_prefix_lowercase()
+rejects_xml_prefix_uppercase()
+rejects_xml_prefix_mixed_case()
}
class make_valid_xml_name_tests {
<<test_module>>
+returns_valid_name_unchanged()
+prepends_n_to_numeric_key()
+replaces_spaces_with_underscores()
+falls_back_to_key_with_name_attr()
+escapes_special_chars_in_name()
}
class make_attr_string_tests {
<<test_module>>
+returns_empty_for_empty_attrs()
+formats_single_attr()
+formats_multiple_attrs()
+escapes_attr_values()
}
class fuzz_escape_xml {
<<fuzz_target>>
+run(input: String)
}
class fuzz_wrap_cdata {
<<fuzz_target>>
+run(input: String)
}
class fuzz_is_valid_xml_name {
<<fuzz_target>>
+run(input: String)
}
class fuzz_make_valid_xml_name {
<<fuzz_target>>
+run(input: String)
}
class fuzz_make_attr_string {
<<fuzz_target>>
+run(attrs: Vec<(String, String)>)
}
json2xml_rs <|.. ConvertConfig
escape_xml_tests --> json2xml_rs : uses escape_xml
wrap_cdata_tests --> json2xml_rs : uses wrap_cdata
is_valid_xml_name_tests --> json2xml_rs : uses is_valid_xml_name
make_valid_xml_name_tests --> json2xml_rs : uses make_valid_xml_name
make_attr_string_tests --> json2xml_rs : uses make_attr_string
fuzz_escape_xml --> json2xml_rs : uses escape_xml
fuzz_wrap_cdata --> json2xml_rs : uses wrap_cdata
fuzz_is_valid_xml_name --> json2xml_rs : uses is_valid_xml_name
fuzz_make_valid_xml_name --> json2xml_rs : uses make_valid_xml_name
fuzz_make_attr_string --> json2xml_rs : uses make_attr_string
File-Level Changes
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 #271 +/- ##
=======================================
Coverage 95.89% 95.89%
=======================================
Files 5 5
Lines 463 463
=======================================
Hits 444 444
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. |
![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, and left some high level feedback:
- The core helper functions (
escape_xml,wrap_cdata,is_valid_xml_name,make_valid_xml_name,make_attr_string) are nowpub, which expands the crate’s public API; consider usingpub(crate)or gating them behind a feature (e.g.test/fuzzing) if they are not intended for external consumers. - The
fuzz_make_attr_stringtarget asserts that each key yields a{key}="fragment in the result, which may be brittle for unusual keys (e.g. containing whitespace or characters that might be normalized/escaped); you may want to relax this invariant or derive expectations from the actual formatting/escaping rules used inmake_attr_stringto avoid false positives.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The core helper functions (`escape_xml`, `wrap_cdata`, `is_valid_xml_name`, `make_valid_xml_name`, `make_attr_string`) are now `pub`, which expands the crate’s public API; consider using `pub(crate)` or gating them behind a feature (e.g. `test`/`fuzzing`) if they are not intended for external consumers.
- The `fuzz_make_attr_string` target asserts that each key yields a ` {key}="` fragment in the result, which may be brittle for unusual keys (e.g. containing whitespace or characters that might be normalized/escaped); you may want to relax this invariant or derive expectations from the actual formatting/escaping rules used in `make_attr_string` to avoid false positives.
## Individual Comments
### Comment 1
<location> `rust/fuzz/fuzz_targets/fuzz_make_attr_string.rs:28-37` </location>
<code_context>
+ );
+ }
+
+ // 4. Values should be escaped (no raw & < > " ' in values)
+ // The make_attr_string calls escape_xml on values
+
</code_context>
<issue_to_address>
**suggestion (testing):** The comment about escaped values is not enforced; consider adding an explicit check to strengthen the fuzz target.
Since the fuzz target doesn’t currently check that escaping actually happens, it won’t catch regressions in `escape_xml`. Consider adding an assertion (or a minimal parser) that inspects attribute values and verifies they contain no unescaped `&`, `<`, `>`, `"`, or `'` characters before concluding the output is valid.
```suggestion
for (key, _value) in &input.attrs {
let expected_fragment = format!(" {}=\"", key);
assert!(
result.contains(&expected_fragment),
"Attribute fragment '{}' should appear in result '{}'",
expected_fragment,
result
);
}
// Additionally, verify that attribute values are properly escaped:
// - No raw <, >, " or ' characters may appear inside attribute values.
// - Any '&' inside a value must be part of an entity (it must be followed
// by some characters and then a terminating ';' before the closing quote).
for (key, _value) in &input.attrs {
let expected_prefix = format!(" {}=\"", key);
if let Some(start) = result.find(&expected_prefix) {
let value_start = start + expected_prefix.len();
if let Some(rel_end) = result[value_start..].find('"') {
let value_end = value_start + rel_end;
let value = &result[value_start..value_end];
// 1. Forbid raw <, >, " and ' in attribute values.
for forbidden in ['<', '>', '"', '\''] {
assert!(
!value.chars().any(|c| c == forbidden),
"Unescaped '{}' found in attribute value for key '{}' in '{}'",
forbidden,
key,
result
);
}
// 2. Ensure any '&' is part of something that at least looks like an entity:
// '&' must be followed by at least one non-';' character and then a ';'
// before the end of the value.
let bytes = value.as_bytes();
let mut i = 0;
while i < bytes.len() {
if bytes[i] == b'&' {
// There must be at least one character after '&'
assert!(
i + 1 < bytes.len(),
"Dangling '&' at end of attribute value for key '{}' in '{}'",
key,
result
);
// Find the next ';' after '&'
let mut j = i + 1;
while j < bytes.len() && bytes[j] != b';' {
j += 1;
}
assert!(
j < bytes.len(),
"Found '&' in attribute value for key '{}' that is not terminated by ';' in '{}'",
key,
result
);
// Require at least one character between '&' and ';'
assert!(
j > i + 1,
"Empty entity reference '&;' in attribute value for key '{}' in '{}'",
key,
result
);
// Continue scanning after the ';'
i = j + 1;
} else {
i += 1;
}
}
}
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+28
to
+37
| for (key, _value) in &input.attrs { | ||
| let expected_fragment = format!(" {}=\"", key); | ||
| assert!( | ||
| result.contains(&expected_fragment), | ||
| "Attribute fragment '{}' should appear in result '{}'", | ||
| expected_fragment, | ||
| result | ||
| ); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
suggestion (testing): The comment about escaped values is not enforced; consider adding an explicit check to strengthen the fuzz target.
Since the fuzz target doesn’t currently check that escaping actually happens, it won’t catch regressions in escape_xml. Consider adding an assertion (or a minimal parser) that inspects attribute values and verifies they contain no unescaped &, <, >, ", or ' characters before concluding the output is valid.
Suggested change
| for (key, _value) in &input.attrs { | |
| let expected_fragment = format!(" {}=\"", key); | |
| assert!( | |
| result.contains(&expected_fragment), | |
| "Attribute fragment '{}' should appear in result '{}'", | |
| expected_fragment, | |
| result | |
| ); | |
| } | |
| for (key, _value) in &input.attrs { | |
| let expected_fragment = format!(" {}=\"", key); | |
| assert!( | |
| result.contains(&expected_fragment), | |
| "Attribute fragment '{}' should appear in result '{}'", | |
| expected_fragment, | |
| result | |
| ); | |
| } | |
| // Additionally, verify that attribute values are properly escaped: | |
| // - No raw <, >, " or ' characters may appear inside attribute values. | |
| // - Any '&' inside a value must be part of an entity (it must be followed | |
| // by some characters and then a terminating ';' before the closing quote). | |
| for (key, _value) in &input.attrs { | |
| let expected_prefix = format!(" {}=\"", key); | |
| if let Some(start) = result.find(&expected_prefix) { | |
| let value_start = start + expected_prefix.len(); | |
| if let Some(rel_end) = result[value_start..].find('"') { | |
| let value_end = value_start + rel_end; | |
| let value = &result[value_start..value_end]; | |
| // 1. Forbid raw <, >, " and ' in attribute values. | |
| for forbidden in ['<', '>', '"', '\''] { | |
| assert!( | |
| !value.chars().any(|c| c == forbidden), | |
| "Unescaped '{}' found in attribute value for key '{}' in '{}'", | |
| forbidden, | |
| key, | |
| result | |
| ); | |
| } | |
| // 2. Ensure any '&' is part of something that at least looks like an entity: | |
| // '&' must be followed by at least one non-';' character and then a ';' | |
| // before the end of the value. | |
| let bytes = value.as_bytes(); | |
| let mut i = 0; | |
| while i < bytes.len() { | |
| if bytes[i] == b'&' { | |
| // There must be at least one character after '&' | |
| assert!( | |
| i + 1 < bytes.len(), | |
| "Dangling '&' at end of attribute value for key '{}' in '{}'", | |
| key, | |
| result | |
| ); | |
| // Find the next ';' after '&' | |
| let mut j = i + 1; | |
| while j < bytes.len() && bytes[j] != b';' { | |
| j += 1; | |
| } | |
| assert!( | |
| j < bytes.len(), | |
| "Found '&' in attribute value for key '{}' that is not terminated by ';' in '{}'", | |
| key, | |
| result | |
| ); | |
| // Require at least one character between '&' and ';' | |
| assert!( | |
| j > i + 1, | |
| "Empty entity reference '&;' in attribute value for key '{}' in '{}'", | |
| key, | |
| result | |
| ); | |
| // Continue scanning after the ';' | |
| i = j + 1; | |
| } else { | |
| i += 1; | |
| } | |
| } | |
| } | |
| } | |
| } | |
pytest, pytest-cov, coverage, and setuptools were incorrectly listed as runtime dependencies, causing them to be installed with 'pip install json2xml'. Moved them to [project.optional-dependencies] under 'dev' group, so users get only defusedxml, urllib3, and xmltodict at runtime. Developers can install with 'pip install json2xml[dev]'. fixes #272
vinitkumar
force-pushed
the
add-rust-tests
branch
from
b86c1d6 to
6f5e399
Compare
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
Add a testable, non-Python Rust core for json2xml and strengthen validation of its XML helper utilities.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests: