fix(clp-rust-utils): Make api_server and log_ingestor config values optional (fixes #1911).#1927
Conversation
Walkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@components/api-server/src/client.rs`:
- Around line 137-142: In submit_query, extract the api_server config to a local
variable like you do in fetch_results instead of using the long chained call;
e.g., create a local api_server_config =
self.config.api_server.as_ref().expect("api_server configuration is missing")
and then set search_job_config.max_num_results =
api_server_config.default_max_num_query_results so the access pattern matches
fetch_results and improves readability and consistency across the codebase.
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@components/api-server/src/client.rs`:
- Around line 109-115: Validate the API server configuration before creating any
database clients: call get_api_server_config() (or otherwise check that
config.api_server is Some) at the start of the constructor/initializer that
builds Self, and return an error (or propagate Result) if missing, instead of
constructing mongodb_client and sql_pool first; move the validation out of the
post-construction call and perform it before allocating mongo_client/sql_pool so
resources aren't wasted when api_server is None.
- Around line 460-473: The method get_api_server_config is incorrectly marked
const fn despite taking &self (not usable in const contexts); remove the const
qualifier so the signature becomes a normal fn, keeping the same receiver
(&self), return type (&clp_rust_utils::clp_config::package::config::ApiServer)
and the same body (using self.config.as_ref().expect(...)) so callers and
behavior are unchanged.
|
|
||
| /// Returns a reference to the API server configuration. | ||
| /// | ||
| /// # Panics | ||
| /// | ||
| /// Panics if `self.config.api_server` is `None`. | ||
| const fn get_api_server_config( | ||
| &self, | ||
| ) -> &clp_rust_utils::clp_config::package::config::ApiServer { | ||
| self.config | ||
| .api_server | ||
| .as_ref() | ||
| .expect("api_server configuration is missing") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find rust-toolchain or MSRV settings
echo "=== Looking for Rust version specification ==="
fd -t f "rust-toolchain" . --max-depth 3 2>/dev/null | head -5
echo "---"
fd -t f "Cargo.toml" . --max-depth 2 2>/dev/null | xargs grep -l "rust-version\|msrv" 2>/dev/null || echo "No MSRV found"
echo "---"
# Check the root Cargo.toml
cat Cargo.toml 2>/dev/null | grep -A 2 -B 2 "rust-version\|msrv" || echo "No rust-version in root Cargo.toml"Repository: y-scope/clp
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Check CI workflows for Rust version
echo "=== Checking CI workflows for Rust version ==="
fd -t f "\.yml\|\.yaml" .github/workflows/ 2>/dev/null | head -5 | xargs grep -h "toolchain\|rust" 2>/dev/null | grep -i "1\.[0-9]\+" | head -10 || echo "No Rust version found in CI"Repository: y-scope/clp
Length of output: 101
🏁 Script executed:
#!/bin/bash
# Check if const fn with &self is actually valid in the codebase
echo "=== Checking other const fn methods with &self ==="
rg -n "const fn.*&self" components/api-server/src/ || echo "No other const fn with &self found"Repository: y-scope/clp
Length of output: 140
🌐 Web query:
When was Option::expect stabilized as const fn in Rust
💡 Result:
Option::expect was const-stabilized in Rust 1.83.0 (released November 28, 2024). It is marked #[rustc_const_stable(... since = "1.83.0")] in core::option::Option. [1], [2]
Remove the unnecessary const fn marker.
This method takes &self and cannot practically be used in const contexts. While Option::expect is now const-evaluable (Rust 1.83.0+), marking an &self method as const fn is misleading since such methods cannot be evaluated at compile time in normal usage. The const fn provides no practical benefit here.
♻️ Suggested simplification
- const fn get_api_server_config(
+ fn get_api_server_config(
&self,
) -> &clp_rust_utils::clp_config::package::config::ApiServer {
self.config
.api_server
.as_ref()
.expect("api_server configuration is missing")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Returns a reference to the API server configuration. | |
| /// | |
| /// # Panics | |
| /// | |
| /// Panics if `self.config.api_server` is `None`. | |
| const fn get_api_server_config( | |
| &self, | |
| ) -> &clp_rust_utils::clp_config::package::config::ApiServer { | |
| self.config | |
| .api_server | |
| .as_ref() | |
| .expect("api_server configuration is missing") | |
| } | |
| /// Returns a reference to the API server configuration. | |
| /// | |
| /// # Panics | |
| /// | |
| /// Panics if `self.config.api_server` is `None`. | |
| fn get_api_server_config( | |
| &self, | |
| ) -> &clp_rust_utils::clp_config::package::config::ApiServer { | |
| self.config | |
| .api_server | |
| .as_ref() | |
| .expect("api_server configuration is missing") | |
| } |
🤖 Prompt for AI Agents
In `@components/api-server/src/client.rs` around lines 460 - 473, The method
get_api_server_config is incorrectly marked const fn despite taking &self (not
usable in const contexts); remove the const qualifier so the signature becomes a
normal fn, keeping the same receiver (&self), return type
(&clp_rust_utils::clp_config::package::config::ApiServer) and the same body
(using self.config.as_ref().expect(...)) so callers and behavior are unchanged.
| sql_pool, | ||
| }) | ||
| }; | ||
| client.get_api_server_config(); |
There was a problem hiding this comment.
I agree with the rabbit. I think we should make the check explicit and return as an error maybe?
| /// | ||
| /// # Panics | ||
| /// | ||
| /// Panics if `self.config.api_server` is `None`. |
There was a problem hiding this comment.
I don't think we need to document the implicit panics because it can easily get convoluted...
Let's drop them all except for get_api_server_config.
LinZhihao-723
left a comment
There was a problem hiding this comment.
One more nit fix and we should be good to go.
Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
LinZhihao-723
left a comment
There was a problem hiding this comment.
For the PR title, how about:
fix(clp-rust-utils): Make `api_server` and `log_ingestor` config values optional (fixes #1911).
It looks like the value is optional, not the field, lol
api_server and log_ingestor config fields optional (fixes #1911).api_server and log_ingestor config values optional (fixes #1911).
…es optional (fixes y-scope#1911). (y-scope#1927) Co-authored-by: Lin Zhihao <59785146+LinZhihao-723@users.noreply.github.com>
Description
Checklist
breaking change.
Validation performed
log_ingestor: nullinclp-config.yaml. The package starts correctly after the fix.Summary by CodeRabbit
Bug Fixes
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.