Add pyuppsala as an alternative lxml-compatible etree backend#529
Add pyuppsala as an alternative lxml-compatible etree backend#529cleder wants to merge 1 commit into
Conversation
Extends fastkml's swappable etree backend (stdlib / lxml) to also support pyuppsala>=0.8.0, a Rust-based lxml-compatible XML library. Fixes a handful of places that special-cased lxml specifically instead of "any non-stdlib backend" (default-namespace element construction, recover-mode parsing fallback), and makes schema validation backend-agnostic: - get_schema_parser() now builds XMLSchema(file=schema) instead of from an already-parsed tree, so relative xsd:import/xsd:include schemaLocations resolve correctly, and its cache is keyed on the active etree module too (fixes a latent bug where switching backends mid-process could return a schema object built for the wrong backend). - validate() works uniformly via a new assert_valid() helper, since pyuppsala's XMLSchema has no assert_() alias. - handle_validation_error() tolerates error-log entries without a `.path` attribute (pyuppsala's ValidationError doesn't have one). - parse_and_validate (renamed from lxml_parse_and_validate) falls back gracefully when a backend doesn't support recover-mode parsing. Mirrors lxml test coverage for pyuppsala across the whole test suite, including hypothesis property-based tests (via a PyuppsalaNoSchemaValidation mixin that stubs out schema validation, working around a real pyuppsala 0.8 bug where xs:double range validation rejects in-range scientific-notation values), plus CI, packaging, and doc updates. Detailed pyuppsala compatibility findings (good/bad/ugly) are written up in pyuppsala-integration-notes.md for upstream feedback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reviewer's GuideAdds pyuppsala>=0.8.0 as a third, lxml-compatible etree backend alongside stdlib and lxml, makes KML parsing/validation and default-namespace handling backend-agnostic, and mirrors lxml’s test/CI coverage for pyuppsala, including workarounds for current pyuppsala XSD validation limitations. Sequence diagram for backend-agnostic KML parsing and validationsequenceDiagram
actor User
participant KML as KML
participant config as config
participant etree as config.etree
participant validator as validator
User->>KML: parse(file, strict, validate)
KML->>KML: parse_and_validate(file, strict, validate)
KML->>etree: XMLParser(huge_tree=True, recover=True)
alt [backend supports recover]
etree-->>KML: parser
else [backend raises NotImplementedError]
etree-->>KML: NotImplementedError
KML->>etree: XMLParser(huge_tree=True)
etree-->>KML: parser
end
KML->>etree: parse(file, parser)
etree-->>KML: tree
KML->>KML: getroot()
KML-->>User: root (if validate is False)
opt [validate is True]
KML->>validator: validate(element=root, schema=None)
validator->>validator: get_schema_parser(schema=None)
validator->>validator: _build_schema_parser(config.etree, schema_path)
validator-->>validator: schema_parser
validator->>validator: assert_valid(schema_parser, root)
alt [validation passes]
validator-->>User: OK
else [validation fails]
validator->>validator: handle_validation_error(schema_parser, root)
validator-->>User: AssertionError
end
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
WalkthroughAdds pyuppsala as an alternative optional XML etree backend to lxml. Updates fastkml's parsing ( ChangesPyuppsala Backend Integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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. Comment |
|
Tick the box to add this pull request to the merge queue (same as
|
PR Summary
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 1 high |
🟢 Metrics 25 complexity · 30 duplication
Metric Results Complexity 25 Duplication 30
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.
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| if validate: | ||
| validator.validate(element=root) |
There was a problem hiding this comment.
If validator.validate(element=root) raises an exception due to invalid KML, the exception will propagate without context or user-friendly error reporting. This can make debugging difficult for users.
Recommendation: Catch validation exceptions and raise a more informative error, or log the validation failure with details about the file and error cause. For example:
try:
validator.validate(element=root)
except Exception as e:
raise ValueError(f"KML validation failed: {e}") from e| f"{self.ns}{self.get_tag_name()}", | ||
| nsmap={None: self.ns[1:-1]}, # ty: ignore[invalid-argument-type] |
There was a problem hiding this comment.
The code uses self.ns[1:-1] to extract the namespace URI, assuming self.ns is always in the format '{namespace}'. If self.ns is not in this format, this will result in incorrect namespace mapping or a runtime error.
Recommendation: Add validation for the namespace format before slicing, or use a more robust method to extract the namespace URI. For example:
if self.ns.startswith('{') and self.ns.endswith('}'):
ns_uri = self.ns[1:-1]
else:
raise ValueError(f"Invalid namespace format: {self.ns}")| # attributes, even though the real lxml class supports both. | ||
| log = cast("Iterable[_LogEntry]", schema_parser.error_log) | ||
| for error_entry in log: | ||
| path = getattr(error_entry, "path", None) | ||
| try: | ||
| matches = cast("list[Element]", element.xpath(error_entry.path)) | ||
| matches = cast("list[Element]", element.xpath(path) if path else []) | ||
| parent = matches[0].getparent() | ||
| except (config.etree.XPathEvalError, IndexError): | ||
| parent = element |
There was a problem hiding this comment.
Potential backend incompatibility and error context loss in handle_validation_error
The use of element.xpath(path) assumes that the backend supports XPath and that path is a valid expression. For etree-compatible backends that do not support XPath, this may raise an exception or fail silently, leading to loss of error context. Additionally, the fallback to element as the parent may obscure the actual location of the validation error.
Recommendation:
- Check for XPath support in the backend before attempting to use it.
- Provide a more informative fallback or log when XPath is not supported or fails, to aid debugging.
- Consider logging the original error context and path for traceability.
| element = config.etree.parse(file_to_validate).getroot() | ||
| assert element is not None # noqa: S101 | ||
| try: | ||
| schema_parser.assert_(element) # noqa: PT009 | ||
| assert_valid(schema_parser, element) | ||
| except AssertionError: | ||
| handle_validation_error(schema_parser, element) | ||
| raise |
There was a problem hiding this comment.
Uncaught parsing errors in validate function
When file_to_validate is provided, the code parses the file using config.etree.parse(file_to_validate).getroot(). If the file is malformed or unreadable, this will raise an exception that is not caught, potentially causing the application to crash or obscure the source of the error.
Recommendation:
- Wrap the parsing operation in a try-except block to catch parsing-related exceptions (e.g.,
XMLSyntaxErrororOSError). - Log the error and raise a more informative exception or return
Noneto signal failure gracefully.
| def teardown_method(self) -> None: | ||
| """Restore the real schema parser.""" | ||
| self._get_schema_parser_patcher.stop() |
There was a problem hiding this comment.
In teardown_method, if setup_method fails before self._get_schema_parser_patcher is assigned, calling self._get_schema_parser_patcher.stop() will raise an AttributeError. To make teardown more robust, check for the existence of the patcher before stopping it:
if hasattr(self, '_get_schema_parser_patcher'):
self._get_schema_parser_patcher.stop()This ensures that teardown does not fail if setup was interrupted.
| @@ -57,6 +71,21 @@ def test_register_namespaces() -> None: | |||
| assert config.etree._namespace_map[v] == k | |||
There was a problem hiding this comment.
The test directly accesses and asserts on the private attribute _namespace_map of the etree implementation. This approach tightly couples the test to the internal implementation, making it fragile to refactoring and breaking encapsulation. It is recommended to verify the effect of namespace registration through the public API or observable behavior, rather than relying on internal state. If necessary, consider exposing a public method or property for testability.
| empty_namespace_map: dict[str, str] = {} | ||
| config.etree._namespace_map = empty_namespace_map | ||
|
|
||
| config.set_default_namespaces() | ||
|
|
||
| for k, v in config.DEFAULT_NAME_SPACES.items(): | ||
| assert config.etree._namespace_map[v] == k |
There was a problem hiding this comment.
Direct manipulation and assertion of the private attribute _namespace_map in the test introduces maintainability risks. Tests should avoid depending on internal implementation details. Instead, validate the outcome of set_default_namespaces() through public interfaces or observable effects. If such verification is not possible, consider refactoring the code to provide a testable public API.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates pyuppsala (a pure-Rust, memory-safe XML library) as an optional alternative XML backend alongside lxml and the standard library's xml.etree.ElementTree. The changes include updating core parsing, validation, and element generation logic to accommodate pyuppsala's API differences, adding detailed integration notes, and expanding the test suite to run standard and property-based tests against the new backend. There are no review comments, so no feedback is provided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
PR Summary by QodoAdd pyuppsala as an lxml-compatible etree backend (plus CI/docs/tests)
AI Description
Diagram
High-Level Assessment
Files changed (51)
|
![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 1 issue, and left some high level feedback:
- The manual aliasing of
get_schema_parser.cache_clearto_build_schema_parser.cache_clearwith multiple type ignores is a bit obscure; consider exposing a small explicitclear_schema_cache()helper instead of rebinding internals to make cache invalidation clearer and easier to maintain. - The Pyuppsala Hypothesis classes repeatedly re-wrap
TestLxmlmethods withsettings(suppress_health_check=[HealthCheck.differing_executors], ...); factoring this into a shared helper or mixin would reduce duplication and make it easier to adjust Hypothesis configuration in one place. - In
_NoOpSchema,error_logis typed aslist[str]while real implementations expose objects withmessage/path; aligning the stand‑in’s shape with the real protocol (e.g., using a lightweight object or namedtuple) would avoid surprises if tests start relying onerror_logcontents.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The manual aliasing of `get_schema_parser.cache_clear` to `_build_schema_parser.cache_clear` with multiple type ignores is a bit obscure; consider exposing a small explicit `clear_schema_cache()` helper instead of rebinding internals to make cache invalidation clearer and easier to maintain.
- The Pyuppsala Hypothesis classes repeatedly re-wrap `TestLxml` methods with `settings(suppress_health_check=[HealthCheck.differing_executors], ...)`; factoring this into a shared helper or mixin would reduce duplication and make it easier to adjust Hypothesis configuration in one place.
- In `_NoOpSchema`, `error_log` is typed as `list[str]` while real implementations expose objects with `message`/`path`; aligning the stand‑in’s shape with the real protocol (e.g., using a lightweight object or namedtuple) would avoid surprises if tests start relying on `error_log` contents.
## Individual Comments
### Comment 1
<location path="docs/configuration.rst" line_range="13-15" />
<code_context>
implementation.
+``pyuppsala.etree`` (a pure-Rust, memory-safe implementation of the
+``lxml.etree`` API, installable via the ``pyuppsala`` extra) can be used the
+same way, as an alternative to ``lxml.etree``:
+
</code_context>
<issue_to_address>
**nitpick (typo):** Minor grammar tweak: consider "can be used in the same way".
For smoother reading, consider "can be used in the same way, as an alternative to ``lxml.etree``" (adding "in" before "the same way").
```suggestion
``pyuppsala.etree`` (a pure-Rust, memory-safe implementation of the
``lxml.etree`` API, installable via the ``pyuppsala`` extra) can be used in the
same way, as an alternative to ``lxml.etree``:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ``pyuppsala.etree`` (a pure-Rust, memory-safe implementation of the | ||
| ``lxml.etree`` API, installable via the ``pyuppsala`` extra) can be used the | ||
| same way, as an alternative to ``lxml.etree``: |
There was a problem hiding this comment.
nitpick (typo): Minor grammar tweak: consider "can be used in the same way".
For smoother reading, consider "can be used in the same way, as an alternative to lxml.etree" (adding "in" before "the same way").
| ``pyuppsala.etree`` (a pure-Rust, memory-safe implementation of the | |
| ``lxml.etree`` API, installable via the ``pyuppsala`` extra) can be used the | |
| same way, as an alternative to ``lxml.etree``: | |
| ``pyuppsala.etree`` (a pure-Rust, memory-safe implementation of the | |
| ``lxml.etree`` API, installable via the ``pyuppsala`` extra) can be used in the | |
| same way, as an alternative to ``lxml.etree``: |
|
Failed to generate code suggestions for PR |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates Failed
(2 files with Code Duplication, Lines of Code in a Single File)
Our agent can fix these. Install it.
Gates Passed
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| config_test.py | 1 advisory rule | 10.00 → 9.39 | Suppress |
| kml_test.py | 1 advisory rule | 7.07 → 6.66 | Suppress |
Quality Gate Profile: Customizable Safeguards
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
| def test_register_namespaces_pyuppsala() -> None: | ||
| """Register namespaces for use in etree.""" | ||
| config.set_etree_implementation(pyuppsala.etree) | ||
| ns = { | ||
| "real_person": "http://people.example.com", | ||
| "role": "http://characters.example.com", | ||
| } | ||
|
|
||
| config.register_namespaces(**ns) | ||
|
|
||
| for k, v in ns.items(): | ||
| assert config.etree._namespace_map[v] == k |
There was a problem hiding this comment.
❌ New issue: Code Duplication
The module contains 2 functions with similar structure: test_register_namespaces,test_register_namespaces_pyuppsala
| import pytest | ||
| from pygeoif.geometry import Polygon | ||
|
|
||
| from fastkml import config |
There was a problem hiding this comment.
❌ New issue: Lines of Code in a Single File
This module has 628 lines of code, improve code health by reducing it to 600
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #529 +/- ##
===========================================
- Coverage 99.98% 99.22% -0.76%
===========================================
Files 80 80
Lines 6544 6853 +309
Branches 166 169 +3
===========================================
+ Hits 6543 6800 +257
- Misses 1 52 +51
- Partials 0 1 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review by Qodo
Context used✅ Tickets:
🎫 pyuppsala 0.8.0 1. Recover fallback misses TypeError
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| elif config.etree.__name__ != "xml.etree.ElementTree": | ||
| # lxml and lxml-compatible backends (e.g. pyuppsala) support a | ||
| # `None` key in `nsmap` to declare a default namespace; the | ||
| # stdlib's `Element` doesn't accept `nsmap` at all. lxml-stubs' | ||
| # `_NSMapArg` doesn't model this. | ||
| root = config.etree.Element( | ||
| f"{self.ns}{self.get_tag_name()}", | ||
| nsmap={None: self.ns[1:-1]}, # ty: ignore[invalid-argument-type] |
There was a problem hiding this comment.
1. Unsafe nsmap backend check 🐞 Bug ≡ Correctness
KML.etree_element decides whether to pass nsmap based on `config.etree.__name__ != "xml.etree.ElementTree"`, which is not a capability check. Since fastkml explicitly allows swapping in any ElementTree-compatible module, this can raise TypeError at runtime for backends that don’t accept nsmap but also aren’t named exactly like the stdlib module.
Agent Prompt
## Issue description
`KML.etree_element()` uses a module-name comparison (`config.etree.__name__ != "xml.etree.ElementTree"`) to decide whether to call `Element(..., nsmap=...)`. This is brittle because `fastkml.config.set_etree_implementation()` explicitly allows *any* ElementTree-compatible module, and many such modules won’t accept an `nsmap` kwarg.
## Issue Context
This can break callers who plug in an alternative ElementTree implementation: root element construction fails with `TypeError` when `nsmap` is passed.
## Fix Focus Areas
- fastkml/kml.py[165-183]
### Suggested approach
Replace the name-based check with a capability check:
- Prefer `try: Element(tag, nsmap=...)` and `except TypeError: Element(tag)`.
- Or gate on a known capability attribute (less ideal than try/except).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| try: | ||
| parser = config.etree.XMLParser(huge_tree=True, recover=True) | ||
| except NotImplementedError: | ||
| # Some backends (e.g. pyuppsala) don't support recover-mode parsing. | ||
| parser = config.etree.XMLParser(huge_tree=True) | ||
| tree = config.etree.parse(file, parser=parser) |
There was a problem hiding this comment.
2. Recover fallback misses typeerror 🐞 Bug ≡ Correctness
parse_and_validate() only falls back when XMLParser(..., recover=True) raises NotImplementedError; if a backend rejects recover (or other kwargs) with TypeError, KML.parse() catches that TypeError and silently falls back to a non-validating parse path. This can unintentionally disable schema validation for backends that otherwise support validation but not recover-mode parsing.
Agent Prompt
## Issue description
`parse_and_validate()` only catches `NotImplementedError` when constructing `XMLParser(huge_tree=True, recover=True)`. If the backend signals unsupported kwargs via `TypeError` (common for unexpected keyword args), that `TypeError` bubbles up to `KML.parse()`, which catches it and falls back to `config.etree.parse(file).getroot()`—skipping schema validation entirely.
## Issue Context
This undermines the intent of the new “recover-mode parsing fallback” and can make `strict/validate` behave inconsistently depending on how a backend reports unsupported options.
## Fix Focus Areas
- fastkml/kml.py[88-99]
- fastkml/kml.py[229-233]
### Suggested approach
In `parse_and_validate()`, broaden the fallback to also catch `TypeError` (and possibly `ValueError`) when `recover=True` is unsupported:
- First try `XMLParser(huge_tree=True, recover=True)`
- On `(NotImplementedError, TypeError)`, retry `XMLParser(huge_tree=True)`
- (Optional hardening) If that still raises `TypeError` due to `huge_tree`, retry `XMLParser()`
This keeps validation behavior consistent by ensuring parser-option incompatibility doesn’t trigger the outer `TypeError` fallback that skips validation.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Pull request overview
Adds pyuppsala.etree as a third, optional “rich etree” backend (alongside stdlib xml.etree.ElementTree and lxml.etree) and updates validation/parsing/namespace-handling to be backend-agnostic, with extensive mirrored test coverage.
Changes:
- Add
pyuppsala>=0.8.0as an optional extra and introduce a CI job to run the full test suite against it. - Make schema parsing/validation backend-neutral (schema cache keyed by active etree module; uniform
AssertionErrorcontract; tolerant error-log handling). - Mirror lxml-backed tests for pyuppsala across unit + Hypothesis suites (with a pyuppsala-only schema-validation stub for a known
xs:doublevalidator bug).
Reviewed changes
Copilot reviewed 51 out of 51 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/views_test.py | Adds a Pyuppsala backend test variant for views. |
| tests/validator_test.py | Adds pyuppsala-specific validator tests + cache invalidation in setup. |
| tests/utils_test.py | Adds a Pyuppsala backend test variant for utils find/find_all. |
| tests/times_test.py | Adds a Pyuppsala backend test variant for times. |
| tests/styles_test.py | Adds a Pyuppsala backend test variant for styles. |
| tests/repr_eq_test.py | Adds a Pyuppsala backend variant for repr/eq/str round-trips. |
| tests/overlays_test.py | Adds Pyuppsala backend variants for overlay tests. |
| tests/network_link_control_test.py | Adds a Pyuppsala backend test variant for network link control. |
| tests/model_test.py | Adds a Pyuppsala backend test variant for model. |
| tests/links_test.py | Adds a Pyuppsala backend test variant for links. |
| tests/kml_test.py | Adds pyuppsala-specific expectations for KML parse/serialize edge cases. |
| tests/hypothesis/views_test.py | Adds pyuppsala Hypothesis variant with health-check suppression. |
| tests/hypothesis/times_test.py | Adds pyuppsala Hypothesis variant with health-check suppression. |
| tests/hypothesis/style_test.py | Adds pyuppsala Hypothesis variant with health-check suppression. |
| tests/hypothesis/overlay_test.py | Adds pyuppsala Hypothesis variant with health-check suppression. |
| tests/hypothesis/network_link_control_test.py | Adds pyuppsala Hypothesis variant with health-check suppression. |
| tests/hypothesis/multi_geometry_test.py | Adds pyuppsala Hypothesis variant; merges Hypothesis settings for reused tests. |
| tests/hypothesis/model_test.py | Adds pyuppsala Hypothesis variant with health-check suppression. |
| tests/hypothesis/links_test.py | Adds pyuppsala Hypothesis variant with health-check suppression. |
| tests/hypothesis/kml_test.py | Adds pyuppsala Hypothesis variant with health-check suppression. |
| tests/hypothesis/gx/track_test.py | Adds pyuppsala Hypothesis variant for gx track with health-check suppression. |
| tests/hypothesis/gx/data_test.py | Adds pyuppsala Hypothesis variant for gx data with health-check suppression. |
| tests/hypothesis/geometry_test.py | Adds pyuppsala Hypothesis variant; merges settings for a pre-settings-decorated test. |
| tests/hypothesis/feature_test.py | Adds pyuppsala Hypothesis variant with health-check suppression. |
| tests/hypothesis/data_test.py | Adds pyuppsala Hypothesis variant with health-check suppression. |
| tests/hypothesis/container_test.py | Adds pyuppsala Hypothesis variant with health-check suppression. |
| tests/hypothesis/atom_test.py | Adds pyuppsala Hypothesis variant with health-check suppression. |
| tests/gx/track_test.py | Adds Pyuppsala backend variants for gx track unit tests. |
| tests/gx/data_test.py | Adds Pyuppsala backend variants for gx data unit tests. |
| tests/geometries/polygon_test.py | Adds Pyuppsala backend variant for polygon geometry tests. |
| tests/geometries/point_test.py | Adds Pyuppsala backend variant for point geometry tests. |
| tests/geometries/multigeometry_test.py | Adds Pyuppsala backend variants for multigeometry tests. |
| tests/geometries/linestring_test.py | Adds Pyuppsala backend variant for linestring tests. |
| tests/geometries/linearring_test.py | Adds Pyuppsala backend variant for linearring tests. |
| tests/geometries/geometry_test.py | Adds Pyuppsala backend variants for geometry factory/round-trip tests. |
| tests/geometries/coordinates_test.py | Adds Pyuppsala backend variant for coordinates tests. |
| tests/geometries/boundaries_test.py | Adds Pyuppsala backend variant for boundaries tests. |
| tests/features_test.py | Adds Pyuppsala backend variant for features tests. |
| tests/data_test.py | Adds Pyuppsala backend variant for data tests. |
| tests/containers_test.py | Adds Pyuppsala backend variant for containers tests. |
| tests/config_test.py | Adds config tests for selecting pyuppsala + namespace registration/defaults. |
| tests/base.py | Introduces Pyuppsala / PyuppsalaNoSchemaValidation test mixins. |
| tests/base_test.py | Adds pyuppsala-specific base object serialization/parsing tests. |
| tests/atom_test.py | Adds Pyuppsala backend variant for atom tests. |
| README.rst | Documents installing/using the pyuppsala extra. |
| pyuppsala-integration-notes.md | Adds an integration report/compatibility notes for pyuppsala. |
| pyproject.toml | Adds pyuppsala optional extra; includes it in dev extras. |
| fastkml/validator.py | Makes schema parsing/validation backend-agnostic + caches by active etree module. |
| fastkml/kml.py | Generalizes parse+validate helper and expands default-namespace handling beyond lxml-only. |
| docs/configuration.rst | Documents configuring fastkml to use pyuppsala.etree. |
| .github/workflows/run-all-tests.yml | Adds a dedicated pyuppsala test job and makes publishing depend on it. |
| if not self.ns: | ||
| root = config.etree.Element( | ||
| f"{self.ns}{self.get_tag_name()}", | ||
| ) | ||
| root.set("xmlns", config.KMLNS[1:-1]) | ||
| elif hasattr(config.etree, "LXML_VERSION"): | ||
| # lxml supports a `None` key in `nsmap` to declare a default | ||
| # namespace; lxml-stubs' `_NSMapArg` doesn't model this. | ||
| elif config.etree.__name__ != "xml.etree.ElementTree": |
| test_coordinates_str_roundtrip = TestLxml.test_coordinates_str_roundtrip | ||
| test_coordinates_str_roundtrip._hypothesis_internal_use_settings = settings( | ||
| parent=test_coordinates_str_roundtrip._hypothesis_internal_use_settings, | ||
| suppress_health_check=[HealthCheck.differing_executors], | ||
| ) |
| _multipoint_repr_roundtrip = TestLxml.test_multipoint_repr_roundtrip | ||
| _multipoint_repr_roundtrip._hypothesis_internal_use_settings = settings( | ||
| _multipoint_repr_roundtrip._hypothesis_internal_use_settings, | ||
| suppress_health_check=[HealthCheck.differing_executors], | ||
| ) |
Greptile SummaryThis PR extends fastkml's swappable etree backend to support
Confidence Score: 4/5Safe to merge; all production changes are defensive guards that leave the lxml and stdlib paths unaffected. The core logic changes in validator.py and kml.py are small, well-targeted, and the existing lxml and stdlib paths are preserved exactly. Test coverage mirrors the lxml suite across the whole test tree. The findings are confined to test infrastructure: a teardown guard that could mask errors on unexpected failures, reliance on a private Hypothesis internal that could break with a Hypothesis update, and a missing coverage gate in CI for the new backend. tests/base.py (teardown fragility) and tests/hypothesis/geometry_test.py (private Hypothesis API mutation) deserve a second look before the next Hypothesis version bump. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Caller
participant KML
participant parse_and_validate
participant config_etree
participant validator
participant _build_schema_parser
Caller->>KML: parse(file, strict, validate)
KML->>parse_and_validate: file, strict, validate
parse_and_validate->>config_etree: "XMLParser(huge_tree=True, recover=True)"
alt NotImplementedError (pyuppsala)
config_etree-->>parse_and_validate: raise NotImplementedError
parse_and_validate->>config_etree: "XMLParser(huge_tree=True)"
else TypeError (stdlib)
config_etree-->>parse_and_validate: raise TypeError
Note over KML: outer try/except catches TypeError, falls back to plain parse
end
config_etree-->>parse_and_validate: parser
parse_and_validate->>config_etree: parse(file, parser)
config_etree-->>parse_and_validate: tree
parse_and_validate->>validator: "validate(element=root)"
validator->>_build_schema_parser: (etree_module, schema_path)
_build_schema_parser-->>validator: XMLSchema
validator->>validator: assert_valid(schema, element)
alt has assert_() lxml
validator->>validator: schema.assert_(element)
else has assertValid() pyuppsala
validator->>validator: schema.assertValid(element) re-raise as AssertionError
end
validator-->>parse_and_validate: True / AssertionError
parse_and_validate-->>KML: root Element
KML-->>Caller: KML object
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Caller
participant KML
participant parse_and_validate
participant config_etree
participant validator
participant _build_schema_parser
Caller->>KML: parse(file, strict, validate)
KML->>parse_and_validate: file, strict, validate
parse_and_validate->>config_etree: "XMLParser(huge_tree=True, recover=True)"
alt NotImplementedError (pyuppsala)
config_etree-->>parse_and_validate: raise NotImplementedError
parse_and_validate->>config_etree: "XMLParser(huge_tree=True)"
else TypeError (stdlib)
config_etree-->>parse_and_validate: raise TypeError
Note over KML: outer try/except catches TypeError, falls back to plain parse
end
config_etree-->>parse_and_validate: parser
parse_and_validate->>config_etree: parse(file, parser)
config_etree-->>parse_and_validate: tree
parse_and_validate->>validator: "validate(element=root)"
validator->>_build_schema_parser: (etree_module, schema_path)
_build_schema_parser-->>validator: XMLSchema
validator->>validator: assert_valid(schema, element)
alt has assert_() lxml
validator->>validator: schema.assert_(element)
else has assertValid() pyuppsala
validator->>validator: schema.assertValid(element) re-raise as AssertionError
end
validator-->>parse_and_validate: True / AssertionError
parse_and_validate-->>KML: root Element
KML-->>Caller: KML object
Reviews (1): Last reviewed commit: "Add pyuppsala as an alternative lxml-com..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def teardown_method(self) -> None: | ||
| """Restore the real schema parser.""" | ||
| self._get_schema_parser_patcher.stop() |
There was a problem hiding this comment.
teardown_method unconditionally calls self._get_schema_parser_patcher.stop(), but _get_schema_parser_patcher is only assigned after super().setup_method() succeeds. If super().setup_method() raises (e.g. a pyuppsala import error slips through, or get_schema_parser() fails), pytest will still invoke teardown_method, which will then fail with AttributeError: 'PyuppsalaNoSchemaValidation' object has no attribute '_get_schema_parser_patcher', masking the original error. A getattr guard makes this safe.
| def teardown_method(self) -> None: | |
| """Restore the real schema parser.""" | |
| self._get_schema_parser_patcher.stop() | |
| def teardown_method(self) -> None: | |
| """Restore the real schema parser.""" | |
| patcher = getattr(self, "_get_schema_parser_patcher", None) | |
| if patcher is not None: | |
| patcher.stop() |
| # happening here, deliberately, so it's suppressed rather than avoided. | ||
| # | ||
| # `test_coordinates_str_roundtrip` already carries an explicit | ||
| # `@settings(deadline=None)`. `settings.__call__` refuses to decorate a | ||
| # test that has already been settings-decorated, so instead of | ||
| # rewrapping it with `settings(...)(...)` like the others below, merge | ||
| # the extra `suppress_health_check` into its existing settings object |
There was a problem hiding this comment.
_hypothesis_internal_use_settings is an undocumented internal attribute. Hypothesis has no stability contract on it — a minor release could rename or restructure it, silently converting the test into one that runs without suppress_health_check (and therefore fails with HealthCheck.differing_executors) or fails at collection time. The comment explains why settings()(already_decorated_fn) isn't usable here, but it's worth tracking the upstream issue or pinning hypothesis tightly if this pattern is relied upon across all the new TestPyuppsala hypothesis classes.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| - name: Test with pytest | ||
| run: | | ||
| pytest tests |
There was a problem hiding this comment.
cpython-pyuppsala job runs pytest tests without a coverage threshold, unlike the cpython-lxml job which enforces --cov-fail-under=95. This means coverage regressions introduced specifically through the pyuppsala path won't be caught in CI.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Approve with suggestions
This PR successfully adds pyuppsala as a third etree backend with comprehensive test coverage and backend-agnostic refactoring. However, a few defensive-coding and maintainability concerns in the new validation and parsing code should be addressed before merging to avoid subtle runtime failures.
📄 Documentation Diagram
This diagram illustrates the backend-agnostic parse, validate, and schema caching flow introduced in the PR.
sequenceDiagram
participant Client
participant PAV as parse_and_validate
participant Backend as etree Backend
participant Svc as Schema Parser
Client->>PAV: parse_and_validate(file, strict, validate)
PAV->>Backend: try XMLParser(huge_tree=True, recover=True)
alt recover not supported
Backend-->>PAV: NotImplementedError or TypeError
PAV->>Backend: XMLParser(huge_tree=True) or default
end
PAV->>Backend: parse(file, parser)
Backend-->>PAV: element tree root
alt validate
PAV->>Svc: assert_valid(parser, element)
Svc->>Svc: get_schema_parser() with cache keyed on (module, path)
Svc-->>PAV: raises AssertionError if invalid
end
PAV-->>Client: root element
note over PAV,Backend: PR #35;529: Added recover fallback and<br/>backend-agnostic assert_valid
note over Svc: Build XMLSchema from file path<br/>to preserve relative imports
🌟 Strengths
- Excellent test mirroring across lxml and pyuppsala, including property-based tests with hypothesis.
- Thoughtful workarounds for pyuppsala limitations (e.g., recover mode, assert_valid helper, schema file path resolution).
Summary of Findings
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P2 | fastkml/validator.py |
Maintainability | Overly broad exception catch in assert_valid | symbol:assert_valid |
| P2 | fastkml/validator.py |
Maintainability | Fragile cache_clear delegation pattern | symbol:get_schema_parser |
| P2 | fastkml/kml.py |
Bug | Incomplete exception handling in parser fallback | symbol:parse_and_validate |
🔍 Notable Themes
- All three findings relate to defensive error handling in the new backend-agnostic code: overly broad exception catches, fragile delegation, and missing exception types. These could lead to silent failures or confusion during maintenance.
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| try: | ||
| schema_parser.assertValid(element) | ||
| except Exception as error: | ||
| raise AssertionError(str(error)) from error |
There was a problem hiding this comment.
P2 | Confidence: High
assert_valid catches a bare Exception in the fallback path for backends that don't provide assert_(). This is overly broad: any unexpected runtime error (e.g., MemoryError, KeyboardInterrupt, or an unrelated programming error inside assertValid) will be silently converted into an AssertionError, obscuring the real problem and potentially halting cleanup during a shutdown. The code should catch the specific exception type that DocumentInvalid or similar validation exceptions raise (e.g., lxml.etree.DocumentInvalid or its pyuppsala equivalent). Since the point of the helper is to normalize the exception for backends that lack assert_(), a more precise catch protects debugging and avoids swallowing unrelated failures.
Code Suggestion:
try:
schema_parser.assertValid(element)
except (schema_parser.DocumentInvalid, ...) as error:
raise AssertionError(str(error)) from error
(Note: The exact exception class depends on the active backend; you could use `type(schema_parser).__module__` to derive the correct one, or catch a common base like `ValueError` if the validation API guarantees that. Alternatively, use `lxml.etree.DocumentInvalid` as a standard—pyuppsala should expose the same hierarchy per its documentation.)Evidence: symbol:assert_valid
| get_schema_parser.cache_clear = ( # type: ignore[attr-defined] # ty: ignore[unresolved-attribute] | ||
| _build_schema_parser.cache_clear | ||
| ) |
There was a problem hiding this comment.
P2 | Confidence: Medium
The cache-clearing delegation reassigns get_schema_parser.cache_clear to the inner _build_schema_parser.cache_clear. This works but introduces fragility: if a future developer mistakenly adds @lru_cache on get_schema_parser itself (e.g., thinking it is still cached), the reassignment will silently break, and the type: ignore comments suppress type-checking warnings that would normally flag this. The pattern also loses the original cache_clear attribute on get_schema_parser (which was never decorated, so it didn't have one). While functionally correct, it is an unusual indirection that may confuse maintenance. A simpler, more readable approach would be to keep @lru_cache on get_schema_parser but include config.etree in its arguments (already done via the helper) and then use get_schema_parser.cache_clear() directly—the cache would already be keyed on (config.etree, schema). The current design adds an unnecessary extra level of indirection.
Code Suggestion:
@lru_cache(maxsize=16)
def get_schema_parser(
schema: pathlib.Path | None = None,
) -> "etree.XMLSchema":
if schema is None:
schema = pathlib.Path(__file__).parent / "schema" / "ogckml22.xsd"
# Build from file path to preserve relative import resolution
return config.etree.XMLSchema(file=schema)
(Note: The cache then automatically depends on `config.etree` because `config.etree` is a module object that is hashable and will key correctly as long as the module reference doesn't change identity. If it does change, the cache will naturally create a new entry. The `cache_clear` method is available automatically.)Evidence: symbol:get_schema_parser
| try: | ||
| parser = config.etree.XMLParser(huge_tree=True, recover=True) | ||
| except NotImplementedError: | ||
| # Some backends (e.g. pyuppsala) don't support recover-mode parsing. | ||
| parser = config.etree.XMLParser(huge_tree=True) |
There was a problem hiding this comment.
P2 | Confidence: Medium
The try block assumes that a NotImplementedError is raised only for the recover=True argument, and that huge_tree=True is universally supported. However, the standard library's xml.etree.ElementTree.XMLParser does not accept huge_tree either—it will raise TypeError (because the keyword argument is not defined). This TypeError is not caught here, so when the active backend is stdlib, the exception propagates out of parse_and_validate and is caught by KML.parse's broader except TypeError fallback. While that fallback exists (and the overall behavior is correct), the intent of the PR is to make parse_and_validate backend-agnostic; the current design inadvertently relies on the caller to handle stdlib-specific failures. This is a design inconsistency: the function's docstring states it is for "rich etree backends", but it is called by KML.parse which also handles stdlib. A cleaner approach would be to detect support for huge_tree and recover upfront (e.g., via a capability check or by catching both NotImplementedError and TypeError here) so that the function works uniformly across all three backends without forcing the caller to know about internal limitations.
Code Suggestion:
try:
parser = config.etree.XMLParser(huge_tree=True, recover=True)
except (NotImplementedError, TypeError):
# Backends may not support recover or huge_tree (e.g., stdlib
# doesn't accept huge_tree, pyuppsala doesn't accept recover).
try:
parser = config.etree.XMLParser(huge_tree=True)
except (NotImplementedError, TypeError):
parser = config.etree.XMLParser() # fallback to standard parser
(Adapt the fallback chain as needed; the key point is to handle both exceptions at both levels.)Evidence: symbol:parse_and_validate
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/hypothesis/atom_test.py (1)
92-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect, but this boilerplate is repeated near-identically across ~16 test files.
Every
TestPyuppsalaclass in this cohort follows the same shape: rebind each inheritedTestLxmlmethod throughsettings(suppress_health_check=[HealthCheck.differing_executors])(...). A small helper intests/base.py(e.g. a class decorator that iterates over a class'stest_*attributes carrying_hypothesis_internal_use_settingsand rewraps them) would collapse this to one line per file, and would also auto-cover any new fuzz test added toTestLxmlin the future without requiring everyTestPyuppsalasubclass to be updated by hand.🤖 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/hypothesis/atom_test.py` around lines 92 - 105, The repeated hypothesis rebindings in TestPyuppsala should be centralized instead of duplicated across each test module. Add a helper in tests/base.py, ideally a class decorator or similar utility, that walks inherited test_* attributes from TestLxml, detects those carrying _hypothesis_internal_use_settings, and reapplies settings(suppress_health_check=[HealthCheck.differing_executors]) automatically. Then replace the per-method rewraps in TestPyuppsala subclasses with the helper so new fuzz tests added to TestLxml are covered without manual updates.tests/hypothesis/gx/data_test.py (1)
86-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrectly reuses
TestLxml's fuzz tests; both methods are wrapped.The reasoning for suppressing
HealthCheck.differing_executorsis well documented. As an optional improvement, since this exact same boilerplate (importHealthCheck/settings, defineTestPyuppsala(PyuppsalaNoSchemaValidation, TestLxml), manually re-wrap everytest_fuzz_*method) is duplicated across ~16 hypothesis test files in this cohort, consider extracting a small helper (e.g. intests/base.py) that automatically discovers and wraps alltest_fuzz_*methods from a base class, reducing copy-paste and the risk of forgetting to wrap a newly added fuzz test.♻️ Example helper to reduce duplication
def reuse_fuzz_tests(source_cls: type) -> dict[str, object]: """Reuse all `test_fuzz_*` methods from `source_cls`, suppressing the `differing_executors` health check triggered by invoking the same underlying hypothesis-wrapped test from multiple classes.""" return { name: settings(suppress_health_check=[HealthCheck.differing_executors])(method) for name, method in vars(source_cls).items() if name.startswith("test_fuzz") }Usage:
class TestPyuppsala(PyuppsalaNoSchemaValidation, TestLxml): locals().update(reuse_fuzz_tests(TestLxml))🤖 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/hypothesis/gx/data_test.py` around lines 86 - 99, The current `TestPyuppsala` setup correctly re-wraps the `TestLxml` fuzz tests, but the same `HealthCheck.differing_executors` boilerplate is duplicated across many hypothesis test modules. Add a small shared helper (for example in `tests/base.py`) that discovers all `test_fuzz_*` methods from a source class and applies the `settings(...)(...)` wrapper, then use it from `TestPyuppsala` and the other cohort classes to reduce copy-paste and prevent missing a new fuzz test.fastkml/kml.py (1)
170-182: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider centralizing backend-capability detection.
The
config.etree.__name__ != "xml.etree.ElementTree"string check works, but hardcoding the stdlib module's dotted name here duplicates backend-detection logic that likely belongs infastkml/config.py(e.g. aconfig.is_stdlib_etree()helper). This would keep the "which backends support X" knowledge in one place as more backends are added.🤖 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 `@fastkml/kml.py` around lines 170 - 182, Centralize the backend check used in KML root creation: the string comparison in Kml-like element construction duplicates knowledge about which etree backends support nsmap. Add or reuse a helper in config.py such as config.is_stdlib_etree(), then update the root element creation logic in fastkml/kml.py to call that helper instead of comparing config.etree.__name__ directly. Keep the backend-capability decision in one place so future backends only need config.py changes.
🤖 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 @.github/workflows/run-all-tests.yml:
- Around line 54-72: The new cpython-pyuppsala GitHub Actions job should be
hardened by explicitly restricting the job’s GITHUB_TOKEN permissions and
disabling checkout credential persistence. Update the workflow job definition
for cpython-pyuppsala to set only the minimal permissions it needs, and adjust
the actions/checkout usage in that job to avoid persisting credentials after
checkout.
In `@tests/model_test.py`:
- Around line 161-164: The backend mixin order is wrong in the test classes, so
`setup_method` is resolving from `StdLibrary` through `TestModel` before the
backend-specific mixin can install its etree behavior. Update
`TestModelPyuppsala` to put `Pyuppsala` before `TestModel`, and make the same
base-class order fix in `TestModelLxml` so the backend mixin is applied first
during test setup.
In `@tests/utils_test.py`:
- Around line 213-218: The backend test classes are missing the shared
TestFindAll mixin, so they only inherit backend-specific behavior and skip the
common test_* cases. Update the class definitions for TestFindAllLxml and
TestFindAllPyuppsala to include TestFindAll in their base classes, alongside
Lxml and Pyuppsala, so both backend variants run the shared find-all tests.
---
Nitpick comments:
In `@fastkml/kml.py`:
- Around line 170-182: Centralize the backend check used in KML root creation:
the string comparison in Kml-like element construction duplicates knowledge
about which etree backends support nsmap. Add or reuse a helper in config.py
such as config.is_stdlib_etree(), then update the root element creation logic in
fastkml/kml.py to call that helper instead of comparing config.etree.__name__
directly. Keep the backend-capability decision in one place so future backends
only need config.py changes.
In `@tests/hypothesis/atom_test.py`:
- Around line 92-105: The repeated hypothesis rebindings in TestPyuppsala should
be centralized instead of duplicated across each test module. Add a helper in
tests/base.py, ideally a class decorator or similar utility, that walks
inherited test_* attributes from TestLxml, detects those carrying
_hypothesis_internal_use_settings, and reapplies
settings(suppress_health_check=[HealthCheck.differing_executors]) automatically.
Then replace the per-method rewraps in TestPyuppsala subclasses with the helper
so new fuzz tests added to TestLxml are covered without manual updates.
In `@tests/hypothesis/gx/data_test.py`:
- Around line 86-99: The current `TestPyuppsala` setup correctly re-wraps the
`TestLxml` fuzz tests, but the same `HealthCheck.differing_executors`
boilerplate is duplicated across many hypothesis test modules. Add a small
shared helper (for example in `tests/base.py`) that discovers all `test_fuzz_*`
methods from a source class and applies the `settings(...)(...)` wrapper, then
use it from `TestPyuppsala` and the other cohort classes to reduce copy-paste
and prevent missing a new fuzz test.
🪄 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
Run ID: b043d015-fc24-4719-b2bf-99b689210252
📒 Files selected for processing (51)
.github/workflows/run-all-tests.ymlREADME.rstdocs/configuration.rstfastkml/kml.pyfastkml/validator.pypyproject.tomlpyuppsala-integration-notes.mdtests/atom_test.pytests/base.pytests/base_test.pytests/config_test.pytests/containers_test.pytests/data_test.pytests/features_test.pytests/geometries/boundaries_test.pytests/geometries/coordinates_test.pytests/geometries/geometry_test.pytests/geometries/linearring_test.pytests/geometries/linestring_test.pytests/geometries/multigeometry_test.pytests/geometries/point_test.pytests/geometries/polygon_test.pytests/gx/data_test.pytests/gx/track_test.pytests/hypothesis/atom_test.pytests/hypothesis/container_test.pytests/hypothesis/data_test.pytests/hypothesis/feature_test.pytests/hypothesis/geometry_test.pytests/hypothesis/gx/data_test.pytests/hypothesis/gx/track_test.pytests/hypothesis/kml_test.pytests/hypothesis/links_test.pytests/hypothesis/model_test.pytests/hypothesis/multi_geometry_test.pytests/hypothesis/network_link_control_test.pytests/hypothesis/overlay_test.pytests/hypothesis/style_test.pytests/hypothesis/times_test.pytests/hypothesis/views_test.pytests/kml_test.pytests/links_test.pytests/model_test.pytests/network_link_control_test.pytests/overlays_test.pytests/repr_eq_test.pytests/styles_test.pytests/times_test.pytests/utils_test.pytests/validator_test.pytests/views_test.py
| cpython-pyuppsala: | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| python-version: ['3.10', '3.11', '3.12', '3.13', '3.14'] | ||
| steps: | ||
| - uses: actions/checkout@v7 | ||
| - name: Set up Python ${{ matrix.python-version }} | ||
| uses: actions/setup-python@v6 | ||
| with: | ||
| python-version: ${{ matrix.python-version }} | ||
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip wheel | ||
| pip install -e ".[tests, pyuppsala]" | ||
| - name: Test with pytest | ||
| run: | | ||
| pytest tests | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Harden the new CI job: pin token permissions and disable credential persistence.
Static analysis flags this new job for default (overly broad) GITHUB_TOKEN permissions and for not disabling credential persistence on checkout, both of which are easy to close off.
🔒 Suggested hardening
cpython-pyuppsala:
runs-on: ubuntu-latest
+ permissions:
+ contents: read
strategy:
matrix:
python-version: ['3.10', '3.11', '3.12', '3.13', '3.14']
steps:
- uses: actions/checkout@v7
+ with:
+ persist-credentials: false
- name: Set up Python ${{ matrix.python-version }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cpython-pyuppsala: | |
| runs-on: ubuntu-latest | |
| strategy: | |
| matrix: | |
| python-version: ['3.10', '3.11', '3.12', '3.13', '3.14'] | |
| steps: | |
| - uses: actions/checkout@v7 | |
| - name: Set up Python ${{ matrix.python-version }} | |
| uses: actions/setup-python@v6 | |
| with: | |
| python-version: ${{ matrix.python-version }} | |
| - name: Install dependencies | |
| run: | | |
| python -m pip install --upgrade pip wheel | |
| pip install -e ".[tests, pyuppsala]" | |
| - name: Test with pytest | |
| run: | | |
| pytest tests | |
| cpython-pyuppsala: | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: read | |
| strategy: | |
| matrix: | |
| python-version: ['3.10', '3.11', '3.12', '3.13', '3.14'] | |
| steps: | |
| - uses: actions/checkout@v7 | |
| with: | |
| persist-credentials: false | |
| - name: Set up Python ${{ matrix.python-version }} | |
| uses: actions/setup-python@v6 | |
| with: | |
| python-version: ${{ matrix.python-version }} | |
| - name: Install dependencies | |
| run: | | |
| python -m pip install --upgrade pip wheel | |
| pip install -e ".[tests, pyuppsala]" | |
| - name: Test with pytest | |
| run: | | |
| pytest tests |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 60-60: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 54-71: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 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 @.github/workflows/run-all-tests.yml around lines 54 - 72, The new
cpython-pyuppsala GitHub Actions job should be hardened by explicitly
restricting the job’s GITHUB_TOKEN permissions and disabling checkout credential
persistence. Update the workflow job definition for cpython-pyuppsala to set
only the minimal permissions it needs, and adjust the actions/checkout usage in
that job to avoid persisting credentials after checkout.
Source: Linters/SAST tools
|
|
||
|
|
||
| class TestModelPyuppsala(TestModel, Pyuppsala): | ||
| pass |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'class StdLibrary {
$$$
}' --lang python tests/base.pyRepository: cleder/fastkml
Length of output: 470
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== tests/base.py outline ==\n'
ast-grep outline tests/base.py --view expanded || true
printf '\n== relevant class definitions ==\n'
rg -n "class (StdLibrary|Pyuppsala|Lxml|TestModelPyuppsala|TestModelLxml|TestGetGeometryPyuppsala|TestGetGeometryLxml)" tests -S
printf '\n== tests/base.py around StdLibrary and mixins ==\n'
python3 - <<'PY'
from pathlib import Path
p = Path("tests/base.py")
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if any(k in line for k in ["class StdLibrary", "class Pyuppsala", "class Lxml", "setup_method", "teardown_method", "def __init__"]):
start = max(1, i-8)
end = min(len(lines), i+30)
print(f"\n--- lines {start}-{end} ---")
for j in range(start, end+1):
print(f"{j:4}: {lines[j-1]}")
PY
printf '\n== tests/model_test.py around target classes ==\n'
python3 - <<'PY'
from pathlib import Path
p = Path("tests/model_test.py")
lines = p.read_text().splitlines()
for start in [130, 150, 155]:
end = min(len(lines), start+30)
print(f"\n--- lines {start}-{end} ---")
for j in range(start, end+1):
print(f"{j:4}: {lines[j-1]}")
PY
printf '\n== tests/geometries/geometry_test.py relevant classes ==\n'
python3 - <<'PY'
from pathlib import Path
p = Path("tests/geometries/geometry_test.py")
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if "TestGetGeometryPyuppsala" in line or "TestGetGeometryLxml" in line:
start = max(1, i-5)
end = min(len(lines), i+8)
print(f"\n--- lines {start}-{end} ---")
for j in range(start, end+1):
print(f"{j:4}: {lines[j-1]}")
PYRepository: cleder/fastkml
Length of output: 17196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' tests/base.py | cat -nRepository: cleder/fastkml
Length of output: 4880
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== tests/base.py outline =='
ast-grep outline tests/base.py --view expanded || true
echo
echo '== relevant definitions across tests/ =='
rg -n "class (StdLibrary|Pyuppsala|Lxml|TestModelPyuppsala|TestModelLxml|TestGetGeometryPyuppsala|TestGetGeometryLxml)" tests -S || true
echo
echo '== tests/base.py excerpts =='
python3 - <<'PY'
from pathlib import Path
p = Path("tests/base.py")
lines = p.read_text().splitlines()
keys = ["class StdLibrary", "class Pyuppsala", "class Lxml", "setup_method", "teardown_method"]
seen = set()
for i, line in enumerate(lines, 1):
if any(k in line for k in keys):
start = max(1, i-8)
end = min(len(lines), i+25)
if (start, end) in seen:
continue
seen.add((start, end))
print(f"\n--- lines {start}-{end} ---")
for j in range(start, end+1):
print(f"{j:4}: {lines[j-1]}")
PY
echo
echo '== tests/model_test.py excerpts =='
python3 - <<'PY'
from pathlib import Path
p = Path("tests/model_test.py")
lines = p.read_text().splitlines()
for start in [130, 150, 155]:
end = min(len(lines), start+30)
print(f"\n--- lines {start}-{end} ---")
for j in range(start, end+1):
print(f"{j:4}: {lines[j-1]}")
PY
echo
echo '== tests/geometries/geometry_test.py excerpts =='
python3 - <<'PY'
from pathlib import Path
p = Path("tests/geometries/geometry_test.py")
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if "TestGetGeometryPyuppsala" in line or "TestGetGeometryLxml" in line:
start = max(1, i-5)
end = min(len(lines), i+8)
print(f"\n--- lines {start}-{end} ---")
for j in range(start, end+1):
print(f"{j:4}: {lines[j-1]}")
PYRepository: cleder/fastkml
Length of output: 15868
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in ["tests/base.py", "tests/model_test.py", "tests/geometries/geometry_test.py"]:
p = Path(path)
print(f"\n### {path} ({sum(1 for _ in p.open())} lines)")
lines = p.read_text().splitlines()
for needle in ["class StdLibrary", "class Pyuppsala", "class Lxml",
"class TestModelPyuppsala", "class TestModelLxml",
"class TestGetGeometryPyuppsala", "class TestGetGeometryLxml",
"setup_method", "teardown_method"]:
for i, line in enumerate(lines, 1):
if needle in line:
start = max(1, i-6)
end = min(len(lines), i+20)
print(f"\n-- {needle} at line {i} --")
for j in range(start, end+1):
print(f"{j:4}: {lines[j-1]}")
break
PYRepository: cleder/fastkml
Length of output: 6806
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in ["tests/base.py", "tests/model_test.py", "tests/geometries/geometry_test.py"]:
p = Path(path)
lines = p.read_text().splitlines()
print(f"\n### {path} ({len(lines)} lines)")
for needle in ["class StdLibrary", "class Pyuppsala", "class Lxml",
"class TestModelPyuppsala", "class TestModelLxml",
"class TestGetGeometryPyuppsala", "class TestGetGeometryLxml",
"setup_method", "teardown_method"]:
for i, line in enumerate(lines, 1):
if needle in line:
start = max(1, i-6)
end = min(len(lines), i+20)
print(f"\n-- {needle} at line {i} --")
for j in range(start, end+1):
print(f"{j:4}: {lines[j-1]}")
break
PYRepository: cleder/fastkml
Length of output: 6806
Put the backend mixin first here (and in TestModelLxml). TestModel inherits StdLibrary, so class TestModelPyuppsala(TestModel, Pyuppsala) resolves setup_method from StdLibrary before Pyuppsala, and the pyuppsala etree never gets installed. Swap the bases to class TestModelPyuppsala(Pyuppsala, TestModel); TestModelLxml has the same issue.
🤖 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/model_test.py` around lines 161 - 164, The backend mixin order is wrong
in the test classes, so `setup_method` is resolving from `StdLibrary` through
`TestModel` before the backend-specific mixin can install its etree behavior.
Update `TestModelPyuppsala` to put `Pyuppsala` before `TestModel`, and make the
same base-class order fix in `TestModelLxml` so the backend mixin is applied
first during test setup.
| class TestFindAllLxml(Lxml): | ||
| """Run the tests using lxml.""" | ||
|
|
||
|
|
||
| class TestFindAllPyuppsala(Pyuppsala): | ||
| """Run the tests using pyuppsala.""" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -A2 'class TestFindAll' tests/utils_test.pyRepository: cleder/fastkml
Length of output: 403
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline tests/utils_test.py --view expandedRepository: cleder/fastkml
Length of output: 572
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test class hierarchy and any pytest-relevant markers.
sed -n '1,260p' tests/utils_test.pyRepository: cleder/fastkml
Length of output: 7422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how pytest treats inherited test methods in classes with multiple inheritance.
python3 - <<'PY'
import sys, textwrap, tempfile, subprocess, os, json, pathlib
print("Python:", sys.version)
PYRepository: cleder/fastkml
Length of output: 210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' tests/utils_test.py | cat -nRepository: cleder/fastkml
Length of output: 8948
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant class definitions and nearby context in tests/utils_test.py.
sed -n '1,240p' tests/utils_test.py | cat -n | sed -n '1,240p'Repository: cleder/fastkml
Length of output: 8948
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A6 -B6 'class (StdLibrary|Lxml|Pyuppsala|TestFindAll)' tests/utils_test.pyRepository: cleder/fastkml
Length of output: 897
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check pytest collection semantics with a minimal in-memory example, if pytest is available.
python3 - <<'PY'
import importlib.util, sys, tempfile, textwrap, subprocess, pathlib, os
spec = importlib.util.find_spec("pytest")
print("pytest_available=", bool(spec))
if not spec:
sys.exit(0)
tmp = tempfile.mkdtemp()
path = pathlib.Path(tmp) / "test_sample.py"
path.write_text(textwrap.dedent("""
class Base:
def test_base(self):
pass
class Child(Base):
pass
class TestBase:
def test_base(self):
pass
class TestChild(TestBase):
pass
"""))
r = subprocess.run([sys.executable, "-m", "pytest", "--collect-only", "-q", str(path)], capture_output=True, text=True)
print("returncode=", r.returncode)
print(r.stdout)
print(r.stderr)
PYRepository: cleder/fastkml
Length of output: 292
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A8 -B4 'class (StdLibrary|Lxml|Pyuppsala)\b' tests/utils_test.pyRepository: cleder/fastkml
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' tests/utils_test.py | cat -n | sed -n '1,220p'Repository: cleder/fastkml
Length of output: 8948
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A6 -B4 'class (Lxml|Pyuppsala)\b|from .* import (Lxml|Pyuppsala)\b' testsRepository: cleder/fastkml
Length of output: 31792
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,180p' tests/base.py | cat -nRepository: cleder/fastkml
Length of output: 4880
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' tests/base.py | cat -nRepository: cleder/fastkml
Length of output: 4880
Add TestFindAll to the backend test classes.
TestFindAllLxml and TestFindAllPyuppsala only inherit the backend mixins, so they don’t pick up the shared test_* methods. Add TestFindAll to each base list.
🐛 Proposed fix
-class TestFindAllLxml(Lxml):
+class TestFindAllLxml(Lxml, TestFindAll):
"""Run the tests using lxml."""
-class TestFindAllPyuppsala(Pyuppsala):
+class TestFindAllPyuppsala(Pyuppsala, TestFindAll):
"""Run the tests using pyuppsala."""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class TestFindAllLxml(Lxml): | |
| """Run the tests using lxml.""" | |
| class TestFindAllPyuppsala(Pyuppsala): | |
| """Run the tests using pyuppsala.""" | |
| class TestFindAllLxml(Lxml, TestFindAll): | |
| """Run the tests using lxml.""" | |
| class TestFindAllPyuppsala(Pyuppsala, TestFindAll): | |
| """Run the tests using pyuppsala.""" |
🤖 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/utils_test.py` around lines 213 - 218, The backend test classes are
missing the shared TestFindAll mixin, so they only inherit backend-specific
behavior and skip the common test_* cases. Update the class definitions for
TestFindAllLxml and TestFindAllPyuppsala to include TestFindAll in their base
classes, alongside Lxml and Pyuppsala, so both backend variants run the shared
find-all tests.
Extends fastkml's swappable etree backend (stdlib / lxml) to also support pyuppsala>=0.8.0, a Rust-based lxml-compatible XML library. Fixes a handful of places that special-cased lxml specifically instead of "any non-stdlib backend" (default-namespace element construction, recover-mode parsing fallback), and makes schema validation backend-agnostic:
.pathattribute (pyuppsala's ValidationError doesn't have one).Mirrors lxml test coverage for pyuppsala across the whole test suite, including hypothesis property-based tests (via a PyuppsalaNoSchemaValidation mixin that stubs out schema validation, working around a real pyuppsala 0.8 bug where xs:double range validation rejects in-range scientific-notation values), plus CI, packaging, and doc updates.
Summary by Sourcery
Add pyuppsala as an optional lxml-compatible etree backend for fastkml and make validation, parsing, and namespace handling backend-agnostic.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation