You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(data): report unreadable polygon holes in GeoRSS feeds - #2049
When reading a GeoRSS feed containing a GML polygon (georss:where with gml:Polygon), any unreadable or malformed gml:interior rings (e.g. invalid coordinates or fewer than 4 points once closed) were silently dropped without being counted or reported in ReadReport.messages.
the KML and OSM readers leave out unreadable coordinates, holes and parts without one, and GeoRSS its unreadable holes (GML says so since #1686) (open).
This PR counts unreadable polygon holes during GeoRSS feed ingestion and reports them via ReadReport.messages (e.g. 1 hole in a polygon was unreadable and left out), satisfying the "Never silent" guideline.
Testing
Added unit test unreadable_interior_ring_is_said in crates/data/src/georss.rs verifying that an unreadable interior ring produces the notice while keeping the valid exterior polygon, and that clean feeds produce no notice.
Ran cargo test -p rustgis-data georss (4 passed, 0 failed).
GeoRSS imports now retain polygons when an interior ring cannot be read. The unreadable ring is omitted, while the polygon and any other valid rings remain available. Import reports state how many interior rings were omitted, with singular or plural wording as appropriate. This applies when one or multiple unreadable rings are encountered, including polygons that also contain valid interior rings.
…2048)
When reading GML polygons in GeoRSS feeds, unreadable or malformed
gml:interior rings were silently dropped.
Now unreadable holes are counted and reported in ReadReport.messages
(e.g. '1 hole in a polygon was unreadable and left out'), fulfilling the
'Never silent' guideline and the roadmap item under Foundations / Correct
results.
Closesopengeos#2048
No actionable comments were generated in the recent review.
Walkthrough
The GeoRSS reader now counts unreadable GML polygon interior rings that it omits. It adds a singular or plural message to the read report. Tests cover unreadable and valid rings, including mixed cases.
Changes: GeoRSS polygon-hole reporting
Layer / File(s)
Summary
Parse and report unreadable rings crates/data/src/georss.rs
place counts unreadable interior rings while retaining the polygon. read reports how many rings were omitted. Tests cover unreadable, valid, multiple, and mixed interior rings.
GeoRSS feeds with unreadable polygon holes now produce a notice instead of silently dropping the holes. The change is small, covered by tests, and easy to revert, so merge risk is minimal.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title names the main change: GeoRSS feed polygon holes that cannot be read are now reported instead of silently dropped. The raw summary confirms this: an omitted-ring counter feeds a report messa…
Description check
The description follows the template. It has Summary, Testing, UI evidence, and Related issue sections. Summary explains the silent data loss and the fix. Testing lists a unit test and the cargo test,…
Linked Issues check
[#2048] The issue requires the GeoRSS reader to count unreadable or malformed gml:interior rings, report them in ReadReport.messages, and keep the valid exterior polygon. The whole-PR diff of `cra…
Out of Scope Changes check
The whole-PR diff for crates/data/src/georss.rs contains only changes that serve #2048. These are the module doc line on counted hole omissions, the lost_holes parameter on place, the increment …
Full details: Title check
Explanation
The title names the main change: GeoRSS feed polygon holes that cannot be read are now reported instead of silently dropped. The raw summary confirms this: an omitted-ring counter feeds a report message in read(). The title uses the conventional fix(data) prefix and is specific, so it passes.
Full details: Description check
Explanation
The description follows the template. It has Summary, Testing, UI evidence, and Related issue sections. Summary explains the silent data loss and the fix. Testing lists a unit test and the cargo test, clippy, fmt, and xtask runs. UI evidence keeps the 'No visible UI change.' line. Related issue reads 'Closes #2048'. The template's HTML comments were removed, which is acceptable. The description is complete and specific.
Full details: Linked Issues check
Explanation
[#2048] The issue requires the GeoRSS reader to count unreadable or malformed gml:interior rings, report them in ReadReport.messages, and keep the valid exterior polygon. The whole-PR diff of crates/data/src/georss.rs (merge base 0eeb756 to head 3aee036) implements this. place takes lost_holes: &mut usize and increments it in the else branch when an interior ring fails .and_then(ring). read adds 1 hole in a polygon was unreadable and left out for one hole and N holes in polygons were unreadable and left out for several. The exterior is still returned as Geometry::Polygon(vec![rings]). The test unreadable_interior_ring_is_said covers: one unreadable hole (feature kept, geometry present, singular notice), a clean ring (no hole notice), two unreadable holes (plural notice), and one valid plus one corrupt hole (singular notice, exterior and valid ring kept). The ring helper, which enforces the fewer-than-4-points rule, is unchanged and is treated as existing behavior. The incremental changes since bd060bc and 3928b0e were assessed through the whole-PR diff at head, which contains the required behavior.
Full details: Out of Scope Changes check
Explanation
The whole-PR diff for crates/data/src/georss.rs contains only changes that serve #2048. These are the module doc line on counted hole omissions, the lost_holes parameter on place, the increment on ring failure, the singular and plural message in read, and the unit test. The test cases for plural and mixed holes support the same objective. No unrelated refactoring or feature appears in the inspected diff. The ROADMAP.md reference is documentation context, not a code change. The repository file listing was truncated, so files other than georss.rs were not individually verified at head. The PR description and prior review both identify georss.rs as the only changed code file.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 reads the rings with care It counts the holes that cannot parse The polygon stays within the feed A message tells what was left out Valid rings pass without a sound The rabbit hops through tested ground
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: d9c0e215-08cf-4851-ac99-94539137ea5a
Commits
Reviewing files that changed from the base of the PR and between 3928b0e and 3aee036.
Files selected for processing: 1
crates/data/src/georss.rs
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 2 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Nit (quality, low priority): the doc comment above place still says it returns Err for a place that can't be read, and the module doc at the top of the file says unreadable items are "skipped and counted". Holes that can't be read are now counted too, but they only go to the lost_holes out-parameter and the item stays in. Consider one sentence in each doc saying that unreadable polygon holes are left out and counted. Threading &mut usize through place also works; returning the lost-hole count alongside the geometry would keep place free of side effects, if you prefer that.
The reason will be displayed to describe this comment to others. Learn more.
Good point, thanks! Updated both doc comments in commit 3928b0e to explicitly describe that unreadable polygon interior rings (holes) are omitted from the geometry and counted in lost_holes while keeping the containing feature.
I found no bugs, security issues, or performance problems in this change. I checked the new hole counter on every path through place(). It is incremented only after the exterior ring has parsed, so an item that is skipped never adds to the count. Holes that are readable but have fewer than four points after closing are also counted. The notice goes through ReadReport.messages, and the test covers both a lossy and a clean feed.
Bugs: None found. Confidence: high.
Security: None found.
Performance: None found.
Quality:
The place doc comment and the module doc still describe only skipped items. Holes that can't be read are now counted as well. Posted inline on crates/data/src/georss.rs:74 as a low-priority nit. Confidence: high that the docs are stale, low that it matters.
ROADMAP.md:76-78 still lists "GeoRSS its unreadable holes" as open. This PR closes that part, so the GeoRSS clause should come out. The file is not in the diff, so I did not post it inline. Confidence: high.
The plural message branch and the hole path that fails the four-point check have no tests. Both are simple and look correct. Confidence: medium.
The notice is attached to the first non-empty layer's report, which is the points layer when a feed has points. The code comment says this is intentional. The message also does not name the item it came from, which matches the existing GeoRSS messages. Confidence: low.
AGENTS.md: No violations found. Non-test code has no unwrap, expect or panic. Losses are reported as a single count. The lossy and clean test pair is present.
Already raised by earlier reviews: 0. There were no prior threads on this PR, so nothing was left alone.
Note on my inline comment: I appended the <!-- quality-sweep --> marker to the inline comment at georss.rs:74. That marker belongs only in a reply that defers a quality point to the release pass. It was not the right marker for a new finding, and I can't delete the comment with the tools I have. Please remove that line from the comment, or ignore it.
Change size
Compared with base: the whole PR against main (merge base 0eeb756 to PR head bd060bc). Sizes are file sizes, not line counts.
The reason will be displayed to describe this comment to others. Learn more.
Quality nit (optional, can wait for the release quality pass): the new plural branch ("{lost_holes} holes in polygons were unreadable and left out") and a mixed case (one good hole plus one bad hole, so the count is 1 and the valid hole is kept) have no test. The current test only covers a single unreadable hole. Adding a feed with two bad holes would pin both the plural wording and the count.
The reason will be displayed to describe this comment to others. Learn more.
Added test coverage in commit 3aee036 for both multiple unreadable holes (asserting the plural notice wording) and mixed valid/invalid holes (asserting the count of 1 and verifying that the valid interior ring is retained in the resulting geometry).
The reason will be displayed to describe this comment to others. Learn more.
Additional comments: 1
crates/data/src/georss.rs: 1
66-66: Minor · Functional Correctness · Quick win
Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.
Reject degenerate interior rings.
An interior posList of 0 0 0 0 0 0 0 0 parses as four identical coordinates. This length check accepts it, so place retains the malformed ring and does not report it. Check that the ring has at least three distinct vertices before accepting it.
Based on learnings, lossy input must produce a notice, while clean input must not.
Source: Learnings
Review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 752bf236-2a96-40ce-88a2-29b88f7e5741
Commits
Reviewing files that changed from the base of the PR and between bd060bc and 3928b0e.
Files selected for processing: 1
crates/data/src/georss.rs
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 2 remain after this review.
Bugs: None found. Checked the hole counter on every path. The exterior and outer ring parses run before the loop, so an item that ends in Err never has holes counted, and the Ok path counts each lost hole once. Single-count notices go to the first non-empty layer, as before. Holes that fail lat_lons or ring (fewer than 4 points) are all counted. (confidence: high)
Security: None found. No new input handling, parsing or allocation.
Performance: None found. The added work is one counter and a per-hole find.
Quality:
The new plural message path and the mixed good/bad hole case have no test. The existing test covers only one bad hole. Posted inline on the test at crates/data/src/georss.rs:284. Per AGENTS.md this is a quality point and may wait for the release quality pass. (confidence: high)
Low confidence (~40%): a valid GML 3 hole written with gml:pos or gml:coordinates instead of gml:posList is counted and reported as "unreadable". The text is accurate about the loss, but "unreadable" may mislead for valid data. I could not confirm from the crate whether other GML readers accept gml:pos rings, so this is not verified. The exterior ring has the same posList-only limit, so this may be a pre-existing gap rather than something this PR should fix.
AGENTS.md / repository rules:
CONTRIBUTING.md says a PR that lands a roadmap item must remove it or narrow it in the same PR. ROADMAP.md:76–78 still lists "GeoRSS its unreadable holes" as open. The line is outside the diff, so I could not comment inline. Suggested change: keep the "(open)" marker for KML and OSM, and drop the GeoRSS clause, for example "…without one (GML says so since fix(data): the GML reader says what it leaves out (#1620) #1686; GeoRSS since this change) (open)." (confidence: high)
The earlier doc-comment nit is addressed in 3928b0e: both doc comments now describe the omitted holes. The "never silent" rule is met: the loss is counted and reported with one notice per kind.
Not raised (out of scope, pre-existing): In place (georss.rs:96), a GML where with MultiSurface, MultiPoint or MultiCurve reads only the first member geometry, so the other members are dropped without a notice. This is a silent-loss bug in the same function, but this PR does not touch it.
Already raised, left alone: 1 earlier thread (the doc-comment nit on place at georss.rs:75). The author addressed it in 3928b0e, so I did not repeat it.
Change size
Compared: the whole PR against main (merge base 0eeb756 to PR head 3928b0e).
None found. The new counter only increments after the exterior ring has validated (crates/data/src/georss.rs:107-122), so a counted hole never belongs to an item that gets dropped. Lossy and clean inputs are both covered by the new test, and the singular and plural notices match their counts. Confidence: high.
Security
None found. The change only counts. Input stays bounded by the existing XML node cap. Confidence: high.
Performance
None found. The added cost is one integer increment per interior ring. Confidence: high.
Quality
Nit (stale docs): ROADMAP.md:77-78 still lists "GeoRSS its unreadable holes" as (open), and this PR closes that part ([Bug]: GeoRSS reader silently drops unreadable polygon holes (interior rings) #2048). The bullet also covers KML and OSM, which are still open, so trim the GeoRSS clause rather than deleting the bullet. Confidence: high that it is stale.
AGENTS.md
Never silent: met. Lossy input gives the notice, clean input gives none, and the notice is one per file with a count.
Never crash: no unwrap, expect or panic in non-test code, and no unsafe. The test module's unwrap calls match the existing tests in this file.
Layering and wasm: no new dependencies, no std::time, no new crate edges.
Translation: the notices are plain format! strings, not tl!. The data crate has no tl! or i18n::fmt calls at all, so this matches the layer. No action. Confidence: medium-high.
Earlier review threads
Two earlier concerns were left alone, both already answered by the PR author and reflected in the code. The doc comments were updated in 3928b0e, which is visible at lines 7, 19 and 74. The plural and mixed valid/invalid tests were added in 3aee036.
No inline comments were posted, because no finding survived to a changed line.
Change size
Compared with main (merge base 0eeb756 to PR head 3aee036).
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
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.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
When reading a GeoRSS feed containing a GML polygon (
georss:wherewithgml:Polygon), any unreadable or malformedgml:interiorrings (e.g. invalid coordinates or fewer than 4 points once closed) were silently dropped without being counted or reported inReadReport.messages.This silent data loss was tracked in ROADMAP.md:
This PR counts unreadable polygon holes during GeoRSS feed ingestion and reports them via
ReadReport.messages(e.g.1 hole in a polygon was unreadable and left out), satisfying the "Never silent" guideline.Testing
unreadable_interior_ring_is_saidincrates/data/src/georss.rsverifying that an unreadable interior ring produces the notice while keeping the valid exterior polygon, and that clean feeds produce no notice.cargo test -p rustgis-data georss(4 passed, 0 failed).cargo clippy -p rustgis-data --all-targets -- -D warnings(clean, 0 warnings).cargo fmt --check(clean).cargo xtask layers(layers: ok).UI evidence
No visible UI change.
Related issue
Closes #2048
Summary by CodeRabbit