feat(crate): init soar-registry crate #119
Conversation
|
Warning Rate limit exceeded@QaidVoid has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughNew crate Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
soar-core/src/database/nests/models.rs (1)
9-13: Potential panic if name lacks "nest-" prefix.
strip_prefixreturnsNonewhen the prefix is absent, causing.unwrap()to panic. If the database ever contains a name without this prefix (corruption, migration issue, or manual edit), this will crash.Consider using
unwrap_oror returning the original name:name: row .get::<_, String>("name")? .strip_prefix("nest-") - .unwrap() + .unwrap_or_else(|| row.get::<_, String>("name").unwrap_or_default().as_str()) .to_string(),Or more cleanly:
name: { let raw = row.get::<_, String>("name")?; raw.strip_prefix("nest-").unwrap_or(&raw).to_string() }
🧹 Nitpick comments (6)
crates/soar-registry/src/error.rs (2)
26-31: Consider using Display formatting instead of Debug for UreqError.The
{0:?}Debug formatting may expose internal implementation details of the ureq library. Using{0}(Display) would provide cleaner, more user-friendly error messages.Apply this diff:
- #[error("HTTP request error: {0:?}")] + #[error("HTTP request error: {0}")] #[diagnostic( code(soar_registry::http), help("Check your network connection and the repository URL") )] UreqError(#[from] ureq::Error),
1-124: Well-structured error handling module.The error module is well-designed with comprehensive error variants, diagnostic codes, and helpful error messages. The
ErrorContexttrait provides a clean pattern for enriching IO errors with context.Consider expanding test coverage to include more error variants when time permits. The current coverage (3/10 variants) provides a foundation but could be more comprehensive.
soar-cli/src/nest.rs (1)
9-23: Consider whetherasyncis necessary here.This function and others in this file are marked
asyncbut don't contain any.awaitcalls. If all database operations are synchronous, theasynckeyword adds overhead without benefit.If these are intentionally async for API consistency or planned future async database operations, this is fine to keep.
crates/soar-registry/src/package.rs (2)
26-35: Consider parsing directly asu64to avoid unnecessary conversion.The current implementation parses as
i64, filters negatives, then converts tou64. This adds overhead for a straightforward unsigned number parse.Apply this diff to simplify the parsing:
fn optional_number<'de, D>(deserializer: D) -> Result<Option<u64>, D::Error> where D: Deserializer<'de>, { let s: Option<String> = Option::deserialize(deserializer)?; Ok(s.filter(|s| !s.is_empty()) - .and_then(|s| s.parse::<i64>().ok()) - .filter(|&n| n >= 0) - .map(|n| n as u64)) + .and_then(|s| s.parse::<u64>().ok())) }
234-270: Consider adding tests for edge cases.The current tests validate basic functionality, but additional test coverage for edge cases would improve robustness:
- Empty string handling in
empty_is_none- Invalid number strings in
optional_number- Invalid boolean strings in
flexible_bool- Field aliases deserialization
crates/soar-registry/src/metadata.rs (1)
364-410: Consider using RAII for temporary file cleanup.The temporary file cleanup (lines 392-393, 400-401) is only performed in the success paths. If an error occurs during zstd decoding (line 379-380) or JSON parsing (line 399), the temporary file will be left behind, potentially accumulating over time.
Consider ensuring cleanup happens even on error:
// After creating the temp file let tmp_path = format!("{}.part", metadata_db_path.display()); let _cleanup = scopeguard::guard(tmp_path.clone(), |path| { let _ = fs::remove_file(path); }); // ... rest of the processingOr structure the code to handle cleanup in a single location using Rust's drop semantics.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
Cargo.toml(3 hunks)crates/soar-registry/Cargo.toml(1 hunks)crates/soar-registry/src/error.rs(1 hunks)crates/soar-registry/src/lib.rs(1 hunks)crates/soar-registry/src/metadata.rs(1 hunks)crates/soar-registry/src/nest.rs(1 hunks)crates/soar-registry/src/package.rs(1 hunks)soar-cli/Cargo.toml(1 hunks)soar-cli/src/nest.rs(1 hunks)soar-cli/src/state.rs(7 hunks)soar-core/Cargo.toml(1 hunks)soar-core/src/constants.rs(0 hunks)soar-core/src/database/connection.rs(1 hunks)soar-core/src/database/models.rs(1 hunks)soar-core/src/database/nests/models.rs(1 hunks)soar-core/src/database/nests/repository.rs(1 hunks)soar-core/src/database/repository.rs(1 hunks)soar-core/src/lib.rs(0 hunks)soar-core/src/metadata.rs(0 hunks)
💤 Files with no reviewable changes (3)
- soar-core/src/constants.rs
- soar-core/src/metadata.rs
- soar-core/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (3)
crates/soar-registry/src/nest.rs (3)
soar-cli/src/state.rs (1)
new(67-77)soar-core/src/database/connection.rs (1)
new(20-27)soar-core/src/database/repository.rs (1)
new(15-21)
crates/soar-registry/src/error.rs (3)
soar-cli/src/download.rs (1)
download(67-89)crates/soar-dl/src/http.rs (1)
json(72-79)crates/soar-db/src/models/types.rs (1)
serde_json(20-20)
soar-cli/src/state.rs (2)
crates/soar-registry/src/metadata.rs (3)
fetch_metadata(243-321)fetch_nest_metadata(88-166)write_metadata_db(435-444)crates/soar-registry/src/nest.rs (1)
new(37-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: coverage
🔇 Additional comments (28)
soar-core/Cargo.toml (1)
23-23: LGTM! Clean integration of the new registry crate.The workspace dependency addition is straightforward and consistent with the existing dependency management pattern.
Cargo.toml (3)
7-7: LGTM! New workspace member added correctly.
47-47: LGTM! Workspace dependency path is correct.
57-57: zstd version 0.13.3 is current and secure.Verification confirms that 0.13.3 is the latest stable version, and no security advisories are registered for this crate. No further action is required.
soar-cli/Cargo.toml (1)
37-37: LGTM! Consistent workspace dependency addition.soar-core/src/database/repository.rs (1)
4-6: LGTM! RemotePackage import migration is consistent.The refactoring maintains the same functionality while using the external
soar_registry::RemotePackagetype.crates/soar-registry/Cargo.toml (1)
1-25: LGTM! Well-structured crate configuration.The package metadata and workspace dependencies are properly configured for the new registry crate. The dependency set is appropriate for handling registry operations (HTTP requests, JSON parsing, error handling, compression).
soar-core/src/database/connection.rs (1)
8-10: LGTM! Clean migration to external RemotePackage type.The import refactoring from the local module to
soar_registry::RemotePackageis clean and maintains backward compatibility with the existing API. Verification confirms allRemotePackagereferences have been consistently migrated—no legacy imports remain, and the new import pattern is used uniformly across the codebase.soar-core/src/database/nests/repository.rs (1)
2-2: Nest type migration is correct and consistent.The import refactoring to use
soar_registry::Nestaligns with the overall migration strategy. Verified that all Nest type references have been migrated consistently across the codebase with no remaining old import patterns or conflicting definitions.crates/soar-registry/src/nest.rs (1)
1-40: Well-structured data type with clear documentation.The
Neststruct is well-documented with clear explanations of URL formats and field purposes. The derives are appropriate for serialization needs.soar-cli/src/nest.rs (1)
2-7: Import refactoring looks correct.The import path for
Nesthas been correctly updated to usesoar_registry::Nest, aligning with the type extraction to the new crate.soar-core/src/database/models.rs (1)
4-4: Clean import simplification.The serde import correctly removes the unused
deandDeserializermodules now thatRemotePackageand its custom deserialization helpers have been moved tosoar-registry.crates/soar-registry/src/lib.rs (1)
1-48: Well-documented crate with clean public API.The crate documentation is comprehensive and provides a clear usage example. All imports in the example are valid:
Repositoryis properly exported viasoar_config::repository, andfetch_metadataandMetadataContentare correctly re-exported from the metadata module. The module structure and re-exports create an intentional, clean public API surface.crates/soar-registry/src/package.rs (4)
1-16: LGTM!The module documentation clearly describes the purpose and the
FlexiBoolenum is well-designed for handling flexible boolean deserialization from varied JSON formats.
18-24: LGTM!The
empty_is_nonedeserializer correctly normalizes empty strings toNone, which is a common requirement for handling legacy or inconsistent JSON formats.
37-58: LGTM!The
flexible_booldeserializer comprehensively handles boolean values in multiple formats (actual booleans, yes/no, true/false, 1/0) with appropriate error handling for invalid inputs.
60-232: LGTM!The
RemotePackagestruct is well-documented with comprehensive field coverage. The custom deserializers are applied consistently, and field aliases ensure backward compatibility with legacy metadata formats.soar-cli/src/state.rs (4)
1-29: LGTM!The imports are well-organized and correctly include all necessary items from the new
soar_registrycrate.
34-53: LGTM!The
handle_json_metadatahelper function correctly handles JSON metadata by creating a fresh database, applying migrations, and importing the remote package data. The approach of removing the existing file before creating a new one ensures a clean state.
84-135: LGTM!The
sync_nestsfunction correctly handles both metadata content variants. The use of anest-prefix for repository names ensures nests are distinguishable from regular repositories in the database.
137-174: LGTM!The
init_repo_dbsfunction correctly handles both metadata content variants using the same pattern assync_nests, ensuring consistency across the codebase.crates/soar-registry/src/metadata.rs (7)
1-48: LGTM!The module documentation is clear, and the
MetadataContentenum provides a clean abstraction for handling both SQLite and JSON metadata formats.
50-62: LGTM!The
construct_nest_urlfunction correctly handles thegithub:shorthand by expanding it to a full GitHub releases URL, with appropriate URL validation.
88-166: LGTM!The
fetch_nest_metadatafunction correctly implements conditional HTTP requests with ETag-based caching and respects the configured sync interval to minimize unnecessary network requests.
182-198: LGTM!The
fetch_public_keyfunction correctly handles public key downloads with idempotent behavior (skipping if already exists).
243-321: LGTM!The
fetch_metadatafunction implements the same robust caching strategy asfetch_nest_metadata, with the additional capability to fetch public keys for signature verification.
435-444: LGTM!The
write_metadata_dbfunction correctly uses buffered I/O for efficient writing and provides clear error context. TheBufWriterautomatically flushes on drop, ensuring data integrity.
324-339: [Your rewritten review comment text here]
[Exactly ONE classification tag]
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/soar-db/src/migration.rs (1)
38-49: Pre-existing bug:mark_first_pendingalways usesCORE_MIGRATIONS.The
mark_first_pendingfunction hardcodesCORE_MIGRATIONSon Line 41, but it's called fromapply_migrationswhich handles three different migration types (Core, Metadata, Nest). When applying Metadata or Nest migrations that encounter "already exists" errors, this function would incorrectly mark Core migrations as pending instead of the appropriate type.Apply this diff to fix the function signature and usage:
fn mark_first_pending( conn: &mut SqliteConnection, + source: EmbeddedMigrations, ) -> Result<(), Box<dyn Error + Send + Sync + 'static>> { - let pending = conn.pending_migrations(CORE_MIGRATIONS)?; + let pending = conn.pending_migrations(source)?; if let Some(first) = pending.first() { sql_query("INSERT INTO __diesel_schema_migrations (version) VALUES (?1)") .bind::<diesel::sql_types::Text, _>(first.name().version()) .execute(conn)?; } Ok(()) }And update the call site:
match conn.run_pending_migrations(source) { Ok(_) => break, Err(e) if e.to_string().contains("already exists") => { - mark_first_pending(conn)?; + mark_first_pending(conn, source)?; } Err(e) => return Err(e), }
🧹 Nitpick comments (2)
soar-cli/src/state.rs (2)
40-43: Non-atomic metadata replacement may leave inconsistent state on failure.If migration or population fails after removing the existing file, the old metadata is lost without a successful replacement. Consider writing to a temporary file first, then atomically renaming on success.
This is acceptable for a cache that can be rebuilt, but worth noting for robustness.
112-115: Error conversion loses context.Using
.map_err(|e| SoarError::Custom(e.to_string()))discards the original error's chain and context. Consider implementingFrom<RegistryError>forSoarErroror using a more context-preserving approach.- write_metadata_db(&db_bytes, &metadata_db_path) - .map_err(|e| SoarError::Custom(e.to_string()))?; + write_metadata_db(&db_bytes, &metadata_db_path) + .map_err(|e| SoarError::Custom(format!("writing nest metadata: {e:#}")))?;The same pattern appears at line 157 for repository sync.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/soar-db/src/migration.rs(1 hunks)crates/soar-registry/src/nest.rs(1 hunks)soar-cli/src/nest.rs(1 hunks)soar-cli/src/state.rs(9 hunks)soar-core/src/database/connection.rs(1 hunks)soar-core/src/database/repository.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- soar-cli/src/nest.rs
🧰 Additional context used
🧬 Code graph analysis (1)
soar-cli/src/state.rs (2)
crates/soar-registry/src/metadata.rs (3)
fetch_metadata(243-321)fetch_nest_metadata(88-166)write_metadata_db(435-444)soar-core/src/utils.rs (1)
get_nests_db_conn(92-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: coverage
- GitHub Check: test
🔇 Additional comments (5)
crates/soar-registry/src/nest.rs (1)
1-44: Well-structured data type with good documentation.The
Neststruct is clean, appropriately derived, and well-documented with clear explanations of URL formats. No issues found.soar-cli/src/state.rs (2)
27-29: Imports align with new registry API.The new imports from
soar_registryproperly expose the required functions and types for metadata handling.
150-164: Content handling follows consistent pattern.The metadata content handling correctly distinguishes between SQLite database bytes and JSON packages, applying appropriate processing for each. The flow of writing content then validating packages is logically sound.
soar-core/src/database/connection.rs (1)
7-9: Import path updated correctly.The import change mirrors the one in
repository.rsand is consistent with the crate refactoring. The usage infrom_remote_metadatais straightforward pass-through logic.soar-core/src/database/repository.rs (1)
3-5: Import change verified—external RemotePackage type is fully compatible.The transition from local to
soar_registry::RemotePackageis correct. Verification confirms that the external type insoar-registrydefines all fields accessed ininsert_package(lines 59-161), including all optional and required fields with compatible types:disabled_reason,licenses,ghcr_files,homepages,notes,src_urls,tags,categories,snapshots,repology,replaces,recurse_provides,app_id, and all others. No type mismatches found.
Summary by CodeRabbit
New Features
Refactor
Chores
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.