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
refactor: explain why mapping the IP databases is sound - #1682
I went over everything that touches maxminddb after the bump to 0.33.0. The calls (open_mmap, lookup, LookupResult::decode, the geoip2 types) already match the current API, and nothing in the 0.31 to 0.33 upgrade notes affects us: we don't write newtype visitors, and a record that hits the new decode limits only costs that proxy its data, which a test already covers.
The one thing left was the unsafe mmap open, which silenced clippy::undocumented_unsafe_blocks instead of saying why it's sound. It now has a SAFETY comment: a database file is never written after it's mapped, because a download fills the temp file before checking it and then renames it into place.
I also measured the lookups against the current GeoLite2 City and ASN databases (release build, 20,000 random IPv4 addresses):
opening both databases: 0.4 ms
ASN and City lookup, decode, English-only names and JSON for all 20,000: about 30 ms
Reader::verify() would add 305 ms (City) and 37 ms (ASN) per download
So the lookups don't need optimising, even though both JSON files repeat them. I left verify() out: TLS and the length check already catch a damaged download, and the verifier is stricter than the format spec, so it could turn away a database that works.
Testing
cargo +beta clippy --all-targets --features tui -- and cargo +beta clippy --all-targets -- pass. Clippy would flag the block if the comment weren't recognised.
cargo +nightly fmt --check and uvx prek run --all-files pass.
The mmap-opening unsafe block continues to suppress the unsafe_code lint. Its annotation no longer suppresses Clippy’s undocumented-unsafe-block lint. The safety rationale remains in comments.
Changes: Unsafe block lint
Layer / File(s)
Summary
Lint annotation src/ipdb.rs
The mmap-opening unsafe block’s annotation now suppresses only the unsafe_code lint. The safety explanation remains in comments.
This change only adjusts a lint annotation and the safety comment on the database open. It does not change runtime behavior, so there is no meaningful merge risk.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title uses the Conventional Commits 'refactor:' prefix and describes the main change: a SAFETY comment explaining why memory-mapping the IP databases is sound. The raw summary confirms the only co…
Description check
The description follows the template well. It has a Summary explaining the change and why, including the SAFETY rationale and the upgrade review. It has a Testing section listing the commands run. The…
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 uses the Conventional Commits 'refactor:' prefix and describes the main change: a SAFETY comment explaining why memory-mapping the IP databases is sound. The raw summary confirms the only code change is in src/ipdb.rs, where the open_reader annotation now suppresses only unsafe_code and the rationale is kept in comments. The title is specific and related to the changeset. Note that the change removes a clippy suppression rather than adding new code, so 'refactor' is a reasonable fit.
Full details: Description check
Explanation
The description follows the template well. It has a Summary explaining the change and why, including the SAFETY rationale and the upgrade review. It has a Testing section listing the commands run. The checklist is filled in, with the test-suite items left unchecked. The Related issues section is missing, but the template says to remove it when there are none, so that omission is acceptable. The description does not link any issue, and the template's Related issues section is optional. The unchecked test item is a gap, since the template says new behavior needs tests. This change adds no new behavior, so the gap is minor. The description is mostly complete.
Finishing Touches
Generate unit tests (beta)
Commit to this branch
Create a new PR
Simplify code
Commit to this branch
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.
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 91999d01-4483-424d-ab18-dc2d2fe814fa
Commits
Reviewing files that changed from the base of the PR and between 4e2f6de and e7aa9e6.
Files selected for processing: 1
src/ipdb.rs
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
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.
Summary
I went over everything that touches maxminddb after the bump to 0.33.0. The calls (
open_mmap,lookup,LookupResult::decode, thegeoip2types) already match the current API, and nothing in the 0.31 to 0.33 upgrade notes affects us: we don't write newtype visitors, and a record that hits the new decode limits only costs that proxy its data, which a test already covers.The one thing left was the
unsafemmap open, which silencedclippy::undocumented_unsafe_blocksinstead of saying why it's sound. It now has aSAFETYcomment: a database file is never written after it's mapped, because a download fills the temp file before checking it and then renames it into place.I also measured the lookups against the current GeoLite2 City and ASN databases (release build, 20,000 random IPv4 addresses):
Reader::verify()would add 305 ms (City) and 37 ms (ASN) per downloadSo the lookups don't need optimising, even though both JSON files repeat them. I left
verify()out: TLS and the length check already catch a damaged download, and the verifier is stricter than the format spec, so it could turn away a database that works.Testing
cargo +beta clippy --all-targets --features tui --andcargo +beta clippy --all-targets --pass. Clippy would flag the block if the comment weren't recognised.cargo +nightly fmt --checkanduvx prek run --all-filespass.cargo +beta test --locked -- ipdb outputpasses.Checklist
uvx prek run --all-filespasses.cargo +nightly fmt --checkpasses.cargo +beta clippy --all-targets --features tui --andcargo +beta clippy --all-targets --pass.cargo +beta test --features tui --lockedandcargo +beta test --lockedpass, and new behavior has tests.config.toml,config-schema.jsonandREADME.mdmatch any change to the options or the output.