fix: pass non-string values through clean_string (CDATA support) - #542
Sanjays2402 wants to merge 3 commits into
Conversation
clean_string() called .strip() on any truthy value, so passing an lxml.etree.CDATA object as a description (or any other cleaned string field) raised AttributeError: 'lxml.etree.CDATA' object has no attribute 'strip'. This made it impossible to emit unescaped HTML in a description, which worked before 1.0. Non-string values are now returned unchanged, as suggested on the issue, so a CDATA description round-trips as <![CDATA[...]]>. Closes cleder#418
|
Review changes with |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Reviewer's GuideAdjusts clean_string to safely handle non-string values (notably lxml.etree.CDATA) and adds regression tests ensuring CDATA descriptions in Placemark are preserved as CDATA rather than escaped. Sequence diagram for CDATA description handling via clean_stringsequenceDiagram
actor User
participant Placemark
participant clean_string
participant CDATA
participant Serializer
User->>Placemark: set_description(CDATA)
Placemark->>clean_string: clean_string(CDATA)
clean_string->>clean_string: [value is not str]
clean_string-->>Placemark: CDATA (unchanged)
Placemark->>Serializer: write_description(CDATA)
Serializer-->>User: <![CDATA[description]] in output
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR Review SummaryThe changes in this PR correctly fix issue #418 by allowing Feedback Summary
🤖 Reviewed by codereviewbot.ai - Catch bugs before your team does. |
PR Summary
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. Walkthrough
ChangesCDATA preservation
Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to This change preserves CDATA description values while retaining string cleanup behavior, with regression coverage for CDATA serialization and existing string cases. No merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit keeps strings neat, Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 7 high |
🟢 Metrics 0 complexity · 0 duplication
Metric Results Complexity 0 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
Tick the box to add this pull request to the merge queue (same as
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="fastkml/helpers.py" line_range="93-100" />
<code_context>
def clean_string(value: str | None) -> str | None:
"""Clean and validate a string value, returning None if empty."""
- return value.strip() or None if value else None
+ if not value:
+ return None
+ if not isinstance(value, str):
+ # Pass non-string values (e.g. ``lxml.etree.CDATA``) through unchanged.
+ return value
+ return value.strip() or None
</code_context>
<issue_to_address>
**suggestion:** The type hints and docstring no longer match the actual behavior, which can now return non-string values.
Given the new branch that returns non-string values unchanged, `clean_string` can now return the original input (e.g. a CDATA-like object), not just `str | None`. The parameter type `value: str | None` is also inaccurate now that non-string inputs are explicitly supported. Please update the parameter and return annotations to reflect the actual behavior, or adjust the implementation so it only accepts and returns strings, depending on the intended public API.
Suggested implementation:
```python
def clean_string(value: object | None) -> object | None:
"""Clean and validate string-like values.
For string inputs, returns the stripped string or ``None`` if the result is empty.
For non-string inputs (e.g. ``lxml.etree.CDATA`` or other objects), returns the
value unchanged.
"""
if value is None:
return None
if isinstance(value, str):
return value.strip() or None
# Pass non-string values (e.g. ``lxml.etree.CDATA``) through unchanged.
return value
```
Depending on your preferred typing style, you may want to:
1. Replace ``object`` with ``typing.Any`` and add ``from typing import Any`` at the top of the file, updating the signature to ``def clean_string(value: Any | None) -> Any | None:``.
2. If this helper is part of a public API, consider documenting explicitly that it is intended to be used with both plain strings and string-like objects such as CDATA, and that non-string inputs are returned unchanged.
</issue_to_address>
### Comment 2
<location path="tests/helper_test.py" line_range="171-178" />
<code_context>
+
+ assert clean_string(cdata) is cdata
+
+ def test_placemark_description_cdata_is_not_escaped(self) -> None:
+ """A CDATA description round-trips unescaped (#418)."""
+ placemark = Placemark(
+ ns="{http://www.opengis.net/kml/2.2}",
+ description=lxml.etree.CDATA("<b>bold</b>"),
+ )
+
+ assert "<![CDATA[<b>bold</b>]]>" in placemark.to_string()
</code_context>
<issue_to_address>
**suggestion (testing):** Consider also asserting that the escaped form is absent to fully capture the regression scenario.
Right now the test only checks that the CDATA wrapper is present in the serialized output. To fully cover the regression, also assert that the escaped version (e.g. `"<b>bold</b>"`) does not appear in `placemark.to_string()`, so the description isn’t both wrapped in CDATA and escaped at the same time.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| @@ -92,7 +92,12 @@ | |||
|
|
|||
| def clean_string(value: str | None) -> str | None: | |||
There was a problem hiding this comment.
-
Type Annotation: Since
clean_stringis now designed to accept and return non-string objects likelxml.etree.CDATA, thestr | Noneparameter and return type annotations cause static type checkers (mypy/pyright) to report type errors when passing or returning non-string instances. Annotating withAny(orstr | Any | None) accurately reflects the expected inputs and outputs. -
Control Flow:
if not value:runs before checkingisinstance(value, str). If a non-string object that evaluates as falsy in a boolean context is passed,clean_stringwill returnNonerather than passing the object through unchanged. Checkingif value is None:or performing theisinstancecheck first fixes this edge case.
| def clean_string(value: str | None) -> str | None: | |
| def clean_string(value: Any) -> Any: | |
| """Clean and validate a string value, returning None if empty.""" | |
| if value is None: | |
| return None | |
| if not isinstance(value, str): | |
| # Pass non-string values (e.g. ``lxml.etree.CDATA``) through unchanged. | |
| return value | |
| return value.strip() or None |
There was a problem hiding this comment.
Fixed — updated the annotation to Any -> Any and fixed the control flow so only None becomes None; non-strings pass through unchanged.
Greptile SummaryThis PR extends string cleaning to preserve non-string values such as CDATA while retaining existing normalization for strings.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported type-contract mismatch was addressed by broadening the helper signature to match its non-string passthrough behavior.
|
| Filename | Overview |
|---|---|
| fastkml/helpers.py | Broadens the helper contract and safely preserves non-string values while maintaining existing string-cleaning behavior. |
| tests/helper_test.py | Adds focused regression coverage for string normalization and unescaped CDATA serialization. |
Reviews (3): Last reviewed commit: "Address review: align clean_string type ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/helper_test.py (1)
155-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a Hypothesis property for
clean_string.The current test covers only four fixed inputs. Add a property over normal
strinputs that assertsclean_string(value) == value.strip() or None, and keep the named regression cases and the CDATA pass-through test where needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/helper_test.py` around lines 155 - 159, Add a Hypothesis-based property test for clean_string using arbitrary normal str values, asserting the result equals value.strip() when non-empty and None otherwise. Retain the existing named regression cases and any separate CDATA pass-through coverage required by the current tests.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fastkml/helpers.py`:
- Around line 95-100: Update clean_string’s type contract and documentation to
reflect that non-string XML text values such as lxml.etree.CDATA are returned
unchanged, and revise all _Feature field annotations that use this helper
accordingly. Prefer an overload or generic preserving the input type for
supported values; otherwise explicitly narrow and diagnose the CDATA passthrough
so strict type checking matches runtime behavior.
---
Nitpick comments:
In `@tests/helper_test.py`:
- Around line 155-159: Add a Hypothesis-based property test for clean_string
using arbitrary normal str values, asserting the result equals value.strip()
when non-empty and None otherwise. Retain the existing named regression cases
and any separate CDATA pass-through coverage required by the current tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 07c47e90-c5a0-42e1-b5e6-181fb370a9ff
📒 Files selected for processing (2)
fastkml/helpers.pytests/helper_test.py
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
LlamaPReview — Verification needed
How does to_string() serialize a CDATA object assigned as element text through the registry binding, and what happens with the stdlib ElementTree fallback?
Owner action: Read fastkml/base.py _XMLObject.to_string() and the relevant registry binding in fastkml/features.py to confirm how description is bound as element text and whether the configured etree preserves CDATA objects through serialization. Alternatively, run the new test_placemark_description_cdata_is_not_escaped test with lxml configured.
1 further check in details.
Review details and evidence
| Priority | File | Finding | Evidence |
|---|---|---|---|
| P2 | tests/helper_test.py |
Unconditional import lxml.etree breaks test collection when lxml is absent |
confirmed |
Material unknowns
- How does
to_string()serialize a CDATA object assigned as element text through the registry binding, and what happens with the stdlib ElementTree fallback? The PR's test asserts<![CDATA[<b>bold</b>]]>appears inplacemark.to_string(), but the serialization path (_XMLObject.to_string()/ registry element-text binding) was not retrieved by evidence collection. If the etree or binding silently coerces CDATA to a plain string, the fix does not deliver the claimed unescaped round-trip and issue #418 remains unresolved. - Analysis surfaced 1 candidate concern whose adjudication did not complete in this review.
- Check: Have a maintainer look over the changed areas once more, or re-trigger the review to complete adjudication.
LlamaPReview checks
- Reviewed changed regions in
tests/helper_test.py. - Read bounded PR-head context from
tests/base.py.
Automated review by LlamaPReview · Free for public open-source projects.
| from unittest.mock import patch | ||
|
|
||
| import lxml.etree |
There was a problem hiding this comment.
P2 | Confidence: High
The module-level import lxml.etree added by this PR raises ImportError when lxml is not installed, preventing collection of all tests in tests/helper_test.py — including the existing StdLibrary-based tests that do not require lxml. The repository supports both stdlib ElementTree and lxml backends: tests/base.py defines a StdLibrary base class and a Lxml base class guarded by @pytest.mark.skipif(not LXML, reason="lxml not installed"), confirming lxml is optional. The TestLxml class inherits that guard, but the skip decorator never executes because the module-level import fails first.
Code Suggestion:
Move `import lxml.etree` inside the `TestLxml` class (e.g., in `setup_method` or each test method), or wrap it in `try: import lxml.etree except ImportError: lxml = None` paired with the existing skip logic. Since `TestLxml` already inherits from `Lxml` which is guarded by `@pytest.mark.skipif(not LXML, ...)`, placing the import inside the class body or `setup_method` is safe.Evidence: changed region in tests/helper_test.py; bounded PR-head context from tests/base.py.
There was a problem hiding this comment.
You're right, and it was reproducible: with lxml uninstalled the module-level import lxml.etree fails during collection, so pytest errors out on the whole file and the seven StdLibrary tests never run either — the Lxml skipif on TestLxml never gets a chance to fire.
Fixed in 88677d1 using the same try/except ImportError guard tests/base.py and tests/config_test.py already use.
Before: ERROR tests/helper_test.py / Interrupted: 1 error during collection.
After: 7 passed, 2 skipped without lxml, 9 passed with it.
On the str | None signature point from the other bot — I left that alone deliberately. Widening the annotation would push the CDATA type into every caller's field type across features/links/model/network_link_control, which is a bigger API decision than this fix; happy to do it if you'd prefer.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #542 +/- ##
========================================
Coverage 99.95% 99.95%
========================================
Files 90 90
Lines 7160 7180 +20
Branches 172 174 +2
========================================
+ Hits 7157 7177 +20
Misses 2 2
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
LGTM! The changes to 🤖 Reviewed by codereviewbot.ai - Catch bugs before your team does. |
|
LGTM! The changes to 🤖 Reviewed by codereviewbot.ai - Catch bugs before your team does. |
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: Customizable Safeguards
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
This branch has not been deployed
1 participant
Closes #418
clean_string()called.strip()on any truthy value, so passing anlxml.etree.CDATAobject as adescriptionraisedAttributeError: 'lxml.etree.CDATA' object has no attribute 'strip'. Non-string values are now passed through unchanged — the approach suggested on the issue — so a CDATA description serialises as<![CDATA[...]]>instead of being escaped.Regression tests added to
tests/helper_test.py; both fail without the change and pass with it.This change was prepared with AI assistance; the regression test was run locally and fails without the fix.
Summary by Sourcery
Preserve non-string description values while continuing to normalize string inputs.
Bug Fixes:
clean_string, allowing lxml CDATA descriptions to serialize correctly without escaping.Tests:
Summary by CodeRabbit
Bug Fixes
Tests