refactor: fix cargo lint checks for collapsible ifs and mismatched_lifetime_syntaxes #1404
Conversation
WalkthroughThis change refactors multiple Rust source files to collapse nested Changes
Sequence Diagram(s)Not included: Changes are purely syntactic refactors and a function signature update, with no new or altered control flow. Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Suggested labels
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/handlers/livetail.rs (1)
276-289: Fix lifetime lint is correct; also remove allocation and avoid panic on malformed cookies.Current parsing uses
expect("cookie is parseable"), which can panic on bad input, and allocates aVecunnecessarily. Stream the iterator and drop invalid cookies gracefully:-fn extract_cookie(header: &MetadataMap) -> Option<Cookie<'_>> { - // extract the cookie from the request - let cookies = header.get_all("cookie"); - let cookies: Vec<_> = cookies - .iter() - .filter_map(|value| value.to_str().ok()) - .flat_map(Cookie::split_parse) - .map(|value| value.expect("cookie is parseable")) - .collect(); - - cookies - .into_iter() - .find(|cookie| cookie.name() == SESSION_COOKIE_NAME) -} +fn extract_cookie(header: &MetadataMap) -> Option<Cookie<'_>> { + header + .get_all("cookie") + .iter() + .filter_map(|value| value.to_str().ok()) + .flat_map(|s| Cookie::split_parse(s).filter_map(Result::ok)) + .find(|cookie| cookie.name() == SESSION_COOKIE_NAME) +}src/handlers/http/query.rs (1)
372-375: Field name consistency: use “end_time” (snake_case) instead of “endTime”.Counts API fields were standardized as ["start_time", "end_time", "count"]. Current responses mix cases and can break clients.
- "fields": vec!["start_time", "endTime", "count"], + "fields": vec!["start_time", "end_time", "count"],- "fields": vec!["start_time", "endTime", "count"], + "fields": vec!["start_time", "end_time", "count"],Also applies to: 386-390
🧹 Nitpick comments (4)
src/handlers/http/logstream.rs (1)
71-75: Optional: avoid pre-check to reduce TOCTOU window.If
delete_hot_tieris idempotent or returns a well-defined “not found” error, consider calling it directly (whenHotTierManager::global()isSome(_)) and ignoring the “not found” case. This removes the check-then-act race and slightly simplifies the code further.src/handlers/http/audit.rs (1)
121-123: Nit: typo in comment (“reponse” → “response”).- // Use error information from reponse object if an error + // Use error information from response object if an errorsrc/about.rs (1)
155-157: Nit: fix typo in comment (“Cargol.toml” → “Cargo.toml”).- // CARGO_PKG_VERSION is set from Cargol.toml file at build time + // CARGO_PKG_VERSION is set from Cargo.toml file at build timesrc/catalog/mod.rs (1)
295-295: Nit: variable name typo (mainfest_file_name)Minor naming typo; consider renaming to
manifest_file_namefor clarity.- let mainfest_file_name = manifest_path("").to_string(); - let path = partition_path(stream_name, lower_bound, upper_bound).join(&mainfest_file_name); + let manifest_file_name = manifest_path("").to_string(); + let path = partition_path(stream_name, lower_bound, upper_bound).join(&manifest_file_name);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/about.rs(1 hunks)src/catalog/manifest.rs(1 hunks)src/catalog/mod.rs(2 hunks)src/event/format/json.rs(1 hunks)src/handlers/http/audit.rs(1 hunks)src/handlers/http/logstream.rs(1 hunks)src/handlers/http/query.rs(1 hunks)src/handlers/livetail.rs(1 hunks)src/livetail.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: de-sh
PR: parseablehq/parseable#0
File: :0-0
Timestamp: 2025-03-20T15:50:45.435Z
Learning: Pay close attention to code comments for typos and semantic clarity during reviews for the Parseable project.
📚 Learning: 2025-02-14T09:49:25.818Z
Learnt from: de-sh
PR: parseablehq/parseable#1185
File: src/handlers/http/logstream.rs:255-261
Timestamp: 2025-02-14T09:49:25.818Z
Learning: In Parseable's logstream handlers, stream existence checks must be performed for both query and standalone modes. The pattern `!PARSEABLE.streams.contains(&stream_name) && (PARSEABLE.options.mode != Mode::Query || !PARSEABLE.create_stream_and_schema_from_storage(&stream_name).await?)` ensures proper error handling in both modes.
Applied to files:
src/handlers/http/logstream.rssrc/handlers/http/audit.rssrc/catalog/mod.rs
📚 Learning: 2025-06-16T02:04:58.990Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1346
File: src/handlers/http/health_check.rs:81-90
Timestamp: 2025-06-16T02:04:58.990Z
Learning: In the shutdown function in src/handlers/http/health_check.rs, the design approach is to log errors from sync operations rather than propagate them. This is intentional because the shutdown function is called on SIGTERM/SIGINT signals, and the goal is to perform best-effort cleanup (syncing pending files to object storage) while allowing the shutdown to proceed regardless of sync failures. Logging provides debugging information without blocking the shutdown process.
Applied to files:
src/handlers/http/logstream.rs
📚 Learning: 2025-06-18T12:44:31.983Z
Learnt from: parmesant
PR: parseablehq/parseable#1347
File: src/handlers/http/query.rs:0-0
Timestamp: 2025-06-18T12:44:31.983Z
Learning: The counts API in src/handlers/http/query.rs does not currently support group_by functionality in COUNT queries, so the hard-coded fields array ["start_time", "end_time", "count"] is appropriate for the current scope.
Applied to files:
src/handlers/http/query.rs
📚 Learning: 2025-05-01T10:27:56.858Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:0-0
Timestamp: 2025-05-01T10:27:56.858Z
Learning: The `add_tile()` function in `src/handlers/http/users/dashboards.rs` should use `get_dashboard_by_user(dashboard_id, &user_id)` instead of `get_dashboard(dashboard_id)` to ensure proper authorization checks when modifying a dashboard.
Applied to files:
src/handlers/http/query.rs
📚 Learning: 2025-07-28T17:10:39.448Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1392
File: src/migration/stream_metadata_migration.rs:303-322
Timestamp: 2025-07-28T17:10:39.448Z
Learning: In Parseable's migration system (src/migration/stream_metadata_migration.rs), each migration function updates the metadata to the current latest format using CURRENT_OBJECT_STORE_VERSION and CURRENT_SCHEMA_VERSION constants, rather than producing incremental versions. For example, v5_v6 function produces v7 format output when these constants are set to "v7", not v6 format.
Applied to files:
src/catalog/mod.rs
📚 Learning: 2025-06-16T05:20:18.593Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1346
File: src/parseable/streams.rs:351-355
Timestamp: 2025-06-16T05:20:18.593Z
Learning: In the Parseable codebase, arrow files are expected to always have valid creation or modified timestamps as a basic system assumption. The conversion flow uses expect() on file metadata operations to enforce this invariant with fail-fast behavior rather than graceful error handling, as violations represent fundamental system issues that should cause immediate failures.
Applied to files:
src/catalog/mod.rs
📚 Learning: 2025-06-16T09:50:38.636Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1346
File: src/parseable/streams.rs:319-331
Timestamp: 2025-06-16T09:50:38.636Z
Learning: In Parseable's Ingest or Query mode, the node_id is always available because it's generated during server initialization itself, before the get_node_id_string() function in streams.rs would be called. This makes the .expect() calls on QUERIER_META.get() and INGESTOR_META.get() safe in this context.
Applied to files:
src/catalog/mod.rs
⏰ 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). (10)
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: coverage
🔇 Additional comments (10)
src/handlers/http/logstream.rs (1)
71-75: Consolidated let-chain reads well; logic preserved.Good cleanup addressing Clippy’s collapsible_if. Behavior remains the same and is easier to read.
src/handlers/http/audit.rs (2)
54-57: Let-chain refactor looks good and keeps behavior identical.
54-57: MSRV 1.88.0 already supports let-chains
Theif let … && let …syntax was stabilized in Rust 1.65.0. Your Cargo.toml declaresrust-version = "1.88.0", so the chain ofletpatterns is fully supported—no changes needed.src/handlers/http/query.rs (1)
349-349: Nice micro-optimization.Using
std::slice::from_ref(&body.stream)avoids an unnecessary clone/allocation compared to building a one-element Vec.src/about.rs (1)
115-118: Let-chain refactor is clean and preserves semantics.src/livetail.rs (1)
163-167: MSRV supportsif let-chains
The repository’s Cargo.toml declaresrust-version = "1.88.0", which is well above the 1.65 stabilization of let-chains. We also already use this pattern in several places (e.g.,src/catalog/mod.rs,src/event/format/json.rs). No changes are needed.src/event/format/json.rs (1)
318-320: LGTM: collapsed condition is correct and preserves behaviorCombining the boolean flag with the pattern match is idiomatic and functionally equivalent.
src/catalog/mod.rs (2)
271-293: LGTM: let-chain refactor preserves first_event_at initialization logicCondition is equivalent to the previous nested checks. No change to control flow or side effects.
369-371: LGTM:if letread of cachedfirst_eventsimplifies control flowEquivalent to pre-refactor intent without extra nesting.
src/catalog/manifest.rs (1)
115-121: LGTM: concise check that all row groups share the same sort orderThe pop+all check is clear and equivalent to the prior nested form.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/handlers/http/cluster/mod.rs (1)
1076-1095: Minor cleanup + restore error observability in scheduler
- Prefer
is_ok()overmatches!(..., Ok(()))for clarity.- Using a
matchoncluster_metricspreserves an error log for failed fetches (currently not logged if theif letdoesn’t match).- if let Ok(metrics) = cluster_metrics - && !metrics.is_empty() - { - info!("Cluster metrics fetched successfully from all ingestors"); - if let Ok(metrics_bytes) = serde_json::to_vec(&metrics) { - if matches!( - ingest_internal_stream( - INTERNAL_STREAM_NAME.to_string(), - bytes::Bytes::from(metrics_bytes), - ) - .await, - Ok(()) - ) { - info!("Cluster metrics successfully ingested into internal stream"); - } else { - error!("Failed to ingest cluster metrics into internal stream"); - } - } else { - error!("Failed to serialize cluster metrics"); - } - } + match cluster_metrics { + Ok(metrics) if !metrics.is_empty() => { + info!("Cluster metrics fetched successfully from all ingestors"); + if let Ok(metrics_bytes) = serde_json::to_vec(&metrics) { + if ingest_internal_stream( + INTERNAL_STREAM_NAME.to_string(), + bytes::Bytes::from(metrics_bytes), + ) + .await + .is_ok() + { + info!("Cluster metrics successfully ingested into internal stream"); + } else { + error!("Failed to ingest cluster metrics into internal stream"); + } + } else { + error!("Failed to serialize cluster metrics"); + } + } + Err(err) => { + error!("Failed to fetch cluster metrics: {:?}", err); + } + _ => { /* no metrics to ingest */ } + }src/storage/object_storage.rs (2)
607-632: Merging log sources: consider deterministic ordering and avoid a minor re-parseBehavior looks correct, but two small improvements can help:
- Deterministic output: HashMap/HashSet iteration order is non-deterministic; consider sorting field lists to produce stable JSON (useful for diffs/tests).
- Optional: Avoid re-parsing the first JSON later by caching it during iteration.
Example diff for deterministic field ordering (no functional change):
- for (log_source_format, fields) in log_source_map { - merged_log_sources.push(LogSourceEntry { - log_source_format, - fields: fields.into_iter().collect(), - }); - } + for (log_source_format, fields) in log_source_map { + let mut fields_vec: Vec<_> = fields.into_iter().collect(); + fields_vec.sort_unstable(); + merged_log_sources.push(LogSourceEntry { + log_source_format, + fields: fields_vec, + }); + }
634-641: Nit: prefer .first() over [0] after the non-empty guardIndexing with [0] is correct given the guard, but .first() reads clearer and is safer against future refactors.
Apply:
- let stream_ob_metadata = - serde_json::from_slice::<ObjectStoreFormat>(&stream_metadata_obs[0])?; + let stream_ob_metadata = serde_json::from_slice::<ObjectStoreFormat>( + stream_metadata_obs.first().expect("non-empty by guard"), + )?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/handlers/http/cluster/mod.rs(2 hunks)src/handlers/http/modal/query/querier_logstream.rs(1 hunks)src/handlers/http/modal/ssl_acceptor.rs(1 hunks)src/handlers/http/modal/utils/ingest_utils.rs(1 hunks)src/hottier.rs(1 hunks)src/livetail.rs(1 hunks)src/metadata.rs(1 hunks)src/migration/metadata_migration.rs(3 hunks)src/otel/otel_utils.rs(1 hunks)src/parseable/streams.rs(2 hunks)src/query/mod.rs(1 hunks)src/query/stream_schema_provider.rs(1 hunks)src/stats.rs(2 hunks)src/storage/object_storage.rs(2 hunks)
✅ Files skipped from review due to trivial changes (8)
- src/handlers/http/modal/ssl_acceptor.rs
- src/hottier.rs
- src/metadata.rs
- src/query/mod.rs
- src/stats.rs
- src/otel/otel_utils.rs
- src/parseable/streams.rs
- src/handlers/http/modal/utils/ingest_utils.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/livetail.rs
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: de-sh
PR: parseablehq/parseable#0
File: :0-0
Timestamp: 2025-03-20T15:50:45.435Z
Learning: Pay close attention to code comments for typos and semantic clarity during reviews for the Parseable project.
📚 Learning: 2025-02-14T09:49:25.818Z
Learnt from: de-sh
PR: parseablehq/parseable#1185
File: src/handlers/http/logstream.rs:255-261
Timestamp: 2025-02-14T09:49:25.818Z
Learning: In Parseable's logstream handlers, stream existence checks must be performed for both query and standalone modes. The pattern `!PARSEABLE.streams.contains(&stream_name) && (PARSEABLE.options.mode != Mode::Query || !PARSEABLE.create_stream_and_schema_from_storage(&stream_name).await?)` ensures proper error handling in both modes.
Applied to files:
src/handlers/http/modal/query/querier_logstream.rssrc/storage/object_storage.rssrc/query/stream_schema_provider.rs
📚 Learning: 2025-06-16T02:04:58.990Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1346
File: src/handlers/http/health_check.rs:81-90
Timestamp: 2025-06-16T02:04:58.990Z
Learning: In the shutdown function in src/handlers/http/health_check.rs, the design approach is to log errors from sync operations rather than propagate them. This is intentional because the shutdown function is called on SIGTERM/SIGINT signals, and the goal is to perform best-effort cleanup (syncing pending files to object storage) while allowing the shutdown to proceed regardless of sync failures. Logging provides debugging information without blocking the shutdown process.
Applied to files:
src/handlers/http/modal/query/querier_logstream.rssrc/storage/object_storage.rssrc/handlers/http/cluster/mod.rs
📚 Learning: 2025-06-16T09:50:38.636Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1346
File: src/parseable/streams.rs:319-331
Timestamp: 2025-06-16T09:50:38.636Z
Learning: In Parseable's Ingest or Query mode, the node_id is always available because it's generated during server initialization itself, before the get_node_id_string() function in streams.rs would be called. This makes the .expect() calls on QUERIER_META.get() and INGESTOR_META.get() safe in this context.
Applied to files:
src/handlers/http/modal/query/querier_logstream.rssrc/handlers/http/cluster/mod.rs
📚 Learning: 2025-07-28T17:10:39.448Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1392
File: src/migration/stream_metadata_migration.rs:303-322
Timestamp: 2025-07-28T17:10:39.448Z
Learning: In Parseable's migration system (src/migration/stream_metadata_migration.rs), each migration function updates the metadata to the current latest format using CURRENT_OBJECT_STORE_VERSION and CURRENT_SCHEMA_VERSION constants, rather than producing incremental versions. For example, v5_v6 function produces v7 format output when these constants are set to "v7", not v6 format.
Applied to files:
src/storage/object_storage.rssrc/migration/metadata_migration.rs
📚 Learning: 2025-03-26T06:44:53.362Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1263
File: src/handlers/http/ingest.rs:300-310
Timestamp: 2025-03-26T06:44:53.362Z
Learning: In Parseable, every stream is always associated with a log_source - no stream can exist without a log_source. For otel-traces and otel-metrics, strict restrictions are implemented where ingestion is rejected if a stream already has a different log_source format. However, regular logs from multiple log_sources can coexist in a single stream.
Applied to files:
src/storage/object_storage.rs
📚 Learning: 2025-03-26T06:13:48.898Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1271
File: src/prism/home/mod.rs:207-224
Timestamp: 2025-03-26T06:13:48.898Z
Learning: In the Parseable codebase, if a stream is found, the stream_jsons array will always have at least one element. Additionally, for any valid stream_json object, the log_source array will always have at least one element. This is a design invariant that makes additional null checks unnecessary.
Applied to files:
src/storage/object_storage.rs
📚 Learning: 2025-06-18T06:45:37.070Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1340
File: src/storage/object_storage.rs:832-843
Timestamp: 2025-06-18T06:45:37.070Z
Learning: Stats calculation for parquet files in Parseable is done synchronously during the upload process because files are deleted from staging after upload. This prevents race conditions and ensures stats are calculated while files are still available locally.
Applied to files:
src/storage/object_storage.rs
📚 Learning: 2025-06-18T06:39:04.775Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1340
File: src/query/mod.rs:64-66
Timestamp: 2025-06-18T06:39:04.775Z
Learning: In src/query/mod.rs, QUERY_SESSION_STATE and QUERY_SESSION serve different architectural purposes: QUERY_SESSION_STATE is used for stats calculation and allows dynamic registration of individual parquet files from the staging path (files created every minute), while QUERY_SESSION is used for object store queries with the global schema provider. Session contexts with schema providers don't support registering individual tables/parquets, so both session objects are necessary for their respective use cases.
Applied to files:
src/query/stream_schema_provider.rs
🔇 Additional comments (11)
src/migration/metadata_migration.rs (4)
88-92: LGTM: let-chain refactor is correct and preserves behaviorCondition now short-circuits cleanly; mutation remains exact to prior logic.
127-131: LGTM: identical semantics with improved readabilityThe privilege rename stays guarded by the same checks; no functional change.
188-192: LGTM: precise conditional removal of "tag"The chained pattern match avoids nesting without altering control flow.
88-92: MSRV check passed: Rust 1.88.0 supports let-chains
The workspaceCargo.tomlpinsrust-version = "1.88.0"(Rust 1.65+ stabilized if-let chains), so these chainedif letstatements are fully supported. No action required.src/handlers/http/cluster/mod.rs (2)
1189-1195: LGTM: let-chain for round-robin selection is cleanCombining selection and map access shrinks nesting; state updates remain identical.
1198-1204: LGTM: LRU fallback path similarly simplifiedSemantics unchanged; availability and timestamp update are intact.
src/handlers/http/modal/query/querier_logstream.rs (1)
79-83: LGTM: concise hot-tier cleanup guardThe combined
if letwith predicate keeps control flow simple without changing behavior.src/query/stream_schema_provider.rs (1)
566-579: LGTM: let-chain guards hot-tier scan path cleanlyReadable, behavior-preserving consolidation. No changes to plan construction.
src/storage/object_storage.rs (3)
643-651: LGTM: returning serialized Bytes and persisting to storeCloning Bytes here is cheap (ref-counted); returning the same buffer after put_object is fine.
889-893: Let-chain flattening reads well; double-check MSRVNice collapse of nested if-lets. As above, ensure toolchain/MSRV is ≥ 1.70 for let-chains.
You can reuse the MSRV verification script from the earlier comment (Lines 595–606).
595-606: MSRV ≥1.70 confirmed – let-chains are safeThe project’s Cargo.toml declares
rust-version = "1.88.0"and CI always installs the stable toolchain, so the newif let … &&chains will compile without issue. Core refactors in both methods look correct and preserve existing behavior.Optional nits (no blockers):
src/storage/object_storage.rs –
create_stream_from_ingestor(≈ lines 605–651):
• Use.first().cloned()instead of.into_iter().next()and indexing to avoid consuming the Vec and improve readability.
• Parse the JSON payload once, store it in a local variable, and reuse it rather than parsing twice.
• If determinism is important, consider sorting merged fields/entries before returning.src/storage/object_storage.rs –
upload_files_from_staging(≈ lines 889–893):
• Likewise, prefer.first()over indexing for the initial element.
Fixes #1403
Summary by CodeRabbit