Repository navigation
test(data): exercise corrupted GeoPackage headers - #1138
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Mutated input test crates/data/src/source/tests.rs |
Adds coverage for mutations to SQLite file and page headers and three GeoPackage geometry blobs. The test attempts to open each mutated file and reads all features when opening succeeds. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Other
Possibly related issues
- opengeos/RustGIS#779 requests malformed-input testing for readers, including GeoPackage. This change adds coverage for mutated GeoPackage input.
Possibly related PRs
- opengeos/RustGIS#965 adds malformed-input panic-resistance coverage for a different data-reader format.
Suggested reviewers: giswqs
Merge Risk: 
d0c8c
-
This test-only change is mergeable after normal checks. The previously reported mutation-coverage gaps remain follow-up improvements.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title "test(data): exercise corrupted GeoPackage headers" matches the main change. The PR adds a test that mutates GeoPackage and SQLite header bytes and checks that opening and reading the file d… Description check The description follows the repository template. It has a Summary that explains the change, the malformed-input coverage, and the scope limits. It has a Testing section that lists the test command, pr… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "test(data): exercise corrupted GeoPackage headers" matches the main change. The PR adds a test that mutates GeoPackage and SQLite header bytes and checks that opening and reading the file does not panic. The title is short, specific, and uses a conventional commit prefix.
Full details: Description check
-
Explanation
The description follows the repository template. It has a Summary that explains the change, the malformed-input coverage, and the scope limits. It has a Testing section that lists the test command, pre-commit, cargo xtask ci, and the wasm build. It has a Related issue section that says "Part of
#779." The UI evidence section is omitted, which is correct because the change has no visible UI impact. The description does not name a test file or give a result for the test run, but those details are not required by the template.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 1 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
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 checks each header byte,
Then nudges geometry into sight.
The pages shift; the tests proceed,
Open, read, and check the feed.
Soft paws hop through every case,
While features find their place.
Comment @coderabbitai help to get the list of available commands.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| original.windows(4).enumerate().filter_map(|(i, bytes)| (bytes == b"GP\x00\x03").then_some(i)).take(3).collect(); | ||
| assert_eq!(geometries.len(), 3); | ||
| for i in geometries { | ||
| offsets.extend(i..i + 8); |
There was a problem hiding this comment.
Envelope is never mutated, despite the comment on line 387. The fixture writes each blob as an 8-byte header (GP, version, flags, srs_id) followed by a 32-byte XY envelope. i..i + 8 covers only the header, so the envelope that gpkg_mem reads via split_blob for pruning is untouched. Covering the envelope too is a one-line change (confidence: high).
| offsets.extend(i..i + 8); | |
| offsets.extend(i..i + 40); |
Also update the comment on line 387 to say it covers the header and the XY envelope.
| for page in (4096..original.len()).step_by(4096) { | ||
| offsets.extend(page..(page + 12).min(original.len())); | ||
| } |
There was a problem hiding this comment.
Page 1's B-tree header is skipped. The loop starts at 4096, so page 1 is never visited. Its B-tree header sits at byte 100, just past the 100-byte database header that line 383 covers, so bytes 100–111 are never mutated. The 4096 page size is also hard-coded; it is stored in the database header at bytes 16–17 (confidence: medium-high).
| for page in (4096..original.len()).step_by(4096) { | |
| offsets.extend(page..(page + 12).min(original.len())); | |
| } | |
| for page in (0..original.len()).step_by(4096) { | |
| let start = if page == 0 { 100 } else { page }; | |
| offsets.extend(start..(start + 12).min(original.len())); | |
| } |
| } | ||
| // Exercise flags and envelope metadata in actual GeoPackage geometry blobs. | ||
| let geometries: Vec<_> = | ||
| original.windows(4).enumerate().filter_map(|(i, bytes)| (bytes == b"GP\x00\x03").then_some(i)).take(3).collect(); |
There was a problem hiding this comment.
Nits, grouped:
100and4096are magic numbers. A named constant or a reference to the SQLite header layout would make the intent clearer.filter_map(|(i, bytes)| (bytes == b"GP\x00\x03").then_some(i))reads more simply as.filter(...).map(|(i, _)| i).- A mutation that makes the reader loop or over-allocate would hang the test rather than fail it. There is no timeout, so this is worth knowing about even though the test is correct as written.
Code reviewBugs
Security
Performance
Quality
AGENTS.md
Verified: Inline comments are posted on Change sizeCompared with the base branch: the whole PR, main at 90576c5 to PR head d0c8c55.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
📖 Docs preview
GitHub Pages takes a minute or two to publish a new commit. |
Summary
Exercise the browser's in-memory GeoPackage reader with deterministic byte mutations in a generated SQLite file: the database header, page headers, and three GeoPackage geometry headers. Every mutated file is opened and, when opening succeeds, read; errors are accepted but a panic fails the test. This adds focused malformed-input coverage and does not implement a fuzzing harness or claim a newly discovered crash.
Testing
cargo test -p rustgis-data --lib geopackage_in_memory_handles_mutated_headersuvx pre-commit run --all-filescargo xtask ci(fmt, clippy, workspace tests and layers)cargo build --locked --target wasm32-unknown-unknown -p rustgis-webwith clangRelated issue
Part of #779.
Summary by CodeRabbit