fix(rust): replace unwrap/panic with Result-based error handling#1398
fix(rust): replace unwrap/panic with Result-based error handling#1398yashksaini-coder wants to merge 3 commits intoappwrite:masterfrom
Conversation
📝 WalkthroughWalkthroughThis pull request converts six Rust client builder setter methods ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Pull request overview
This PR updates the Rust SDK template to avoid panics in user-input client setters by switching them to Result-returning APIs, improving error handling consistency and test validation for invalid inputs.
Changes:
- Convert
Clientsetters for endpoint/project/key/jwt/locale/session from infallible chaining toResult<Self>returns. - Add
From<reqwest::header::InvalidHeaderValue>conversion to the SDK error type. - Update docs/examples/tests to handle fallible setters and add unit tests for invalid endpoint/header values.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/rust/src/client.rs.twig | Make setters fallible (Result<Self>) and add tests; add SAFETY note for header unwraps. |
| templates/rust/src/error.rs.twig | Add error conversion from InvalidHeaderValue. |
| templates/rust/src/lib.rs.twig | Update crate-level usage docs to use ? with fallible setters. |
| templates/rust/examples/basic_usage.rs.twig | Update example code to use ? with fallible setters. |
| templates/rust/tests/tests.rs | Update template test harness to compile with fallible setters and print invalid-endpoint behavior. |
Comments suppressed due to low confidence (1)
templates/rust/src/client.rs.twig:95
- The
// SAFETY:comment claims the following header values are compile-time constants, but theuser-agentheader right after the loop includes runtime OS/ARCH values. Either scope the comment to only the truly constant spec headers (the loop), or adjust the comment/implementation so it accurately justifies everyunwrap()that it is meant to cover.
// SAFETY: Header values below are compile-time constants from the SDK spec and always valid ASCII.
{% for key, header in spec.global.defaultHeaders %}
headers.insert("{{ key }}", "{{ header }}".parse().unwrap());
{% endfor %}
headers.insert("user-agent", format!("{{ spec.title }}RustSDK/{{ sdk.version }} ({}; {})", std::env::consts::OS, std::env::consts::ARCH).parse().unwrap());
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let client = Client::new() | ||
| .set_endpoint("http://mockapi/v1") | ||
| .set_project("appwrite") | ||
| .set_key("apikey") | ||
| .set_endpoint("http://mockapi/v1").unwrap() | ||
| .set_project("appwrite").unwrap() | ||
| .set_key("apikey").unwrap() | ||
| .add_header("Origin", "http://localhost"); |
There was a problem hiding this comment.
Client::new() builder chain uses .unwrap() for the fallible setters even though main() already returns Result. This reintroduces panics for what is now modeled as recoverable error handling; prefer propagating with ? (or explicitly handling the error) so the test harness matches the SDK’s intended usage.
|
|
||
| println!("Invalid endpoint URL: htp://cloud.appwrite.io/v1"); | ||
| match Client::new().set_endpoint("htp://cloud.appwrite.io/v1") { | ||
| Ok(_) => println!("Expected error for invalid endpoint"), |
There was a problem hiding this comment.
The match message for the success branch is inverted: Ok(_) indicates the invalid endpoint was accepted, but it prints "Expected error for invalid endpoint". This makes failures misleading; the Ok(_) arm should indicate unexpected success (or assert/return an error) and the Err(_) arm should be treated as the expected path.
| Ok(_) => println!("Expected error for invalid endpoint"), | |
| Ok(_) => println!("Unexpected success for invalid endpoint"), |
| pub fn set_endpoint<S: Into<String>>(&self, endpoint: S) -> Result<Self> { | ||
| let endpoint = endpoint.into(); | ||
| if !endpoint.starts_with("http://") && !endpoint.starts_with("https://") { | ||
| panic!("Invalid endpoint URL: {}. Endpoint must start with http:// or https://", endpoint); | ||
| return Err({{ spec.title | caseUcfirst }}Error::new( | ||
| 400, | ||
| format!("Invalid endpoint URL: {}", endpoint), | ||
| None, | ||
| String::new(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
set_endpoint returns an error with message "Invalid endpoint URL" but the validation only checks the scheme prefix. This is misleading and allows other malformed URLs through until request time. Consider parsing/validating the endpoint with url::Url here (and still enforcing http/https), and include a clear message about the requirement (e.g., must start with http:// or https://).
Greptile SummaryThis PR replaces Key observations:
Confidence Score: 5/5Safe to merge; all remaining findings are P2 style and quality suggestions that do not affect correctness or runtime behavior. The core change — replacing panics with Result-based error handling — is correct and well-structured. The three issues found are all P2: a misused comment convention, an unused From impl that is a missed opportunity for better error messages rather than a bug, and a slightly misleading test print statement. None block a merge. templates/rust/src/error.rs.twig and templates/rust/src/client.rs.twig — the unused From impl and the // SAFETY: comment are worth cleaning up before the next release. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| impl From<reqwest::header::InvalidHeaderValue> for {{ spec.title | caseUcfirst }}Error { | ||
| fn from(err: reqwest::header::InvalidHeaderValue) -> Self { | ||
| Self::new(400, format!("Invalid header value: {}", err), None, String::new()) | ||
| } | ||
| } |
There was a problem hiding this comment.
From<InvalidHeaderValue> impl is never exercised by the setters
None of the setter methods (set_project, set_key, set_jwt, set_locale, set_session) actually use this From impl. They all use .map_err(|_| AppwriteError::new(...)), which silently discards the original InvalidHeaderValue and its diagnostic message. Switching them to ? would leverage the From impl and produce richer error messages:
// Instead of:
let value = key.parse().map_err(|_| AppwriteError::new(400, "Invalid header value for x-appwrite-key", None, String::new()))?;
// Use the From impl:
let value: HeaderValue = key.parse()?; // converts via From<InvalidHeaderValue>| match Client::new().set_endpoint("htp://cloud.appwrite.io/v1") { | ||
| Ok(_) => println!("Expected error for invalid endpoint"), | ||
| Err(e) => println!("{}", e.message), | ||
| } |
There was a problem hiding this comment.
Ok branch message in invalid-endpoint test
If set_endpoint unexpectedly returns Ok, the message "Expected error for invalid endpoint" reads as a confirmation rather than an indication that validation surprisingly passed.
| match Client::new().set_endpoint("htp://cloud.appwrite.io/v1") { | |
| Ok(_) => println!("Expected error for invalid endpoint"), | |
| Err(e) => println!("{}", e.message), | |
| } | |
| match Client::new().set_endpoint("htp://cloud.appwrite.io/v1") { | |
| Ok(_) => println!("ERROR: Expected validation failure for invalid endpoint but got Ok"), | |
| Err(e) => println!("{}", e.message), | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
templates/rust/src/client.rs.twig (1)
269-286: Consider makingadd_headerconsistent with other setters.The
add_headermethod silently ignores invalid header names/values, while the newset_*methods return errors. This inconsistency could surprise users who expect uniform error handling.Consider either returning
Result<Self>or documenting the silent-failure behavior.💡 Optional: Return Result for consistency
/// Add a custom header - pub fn add_header<K: AsRef<str>, V: AsRef<str>>(&self, key: K, value: V) -> Self { + pub fn add_header<K: AsRef<str>, V: AsRef<str>>(&self, key: K, value: V) -> Result<Self> { use reqwest::header::{HeaderName, HeaderValue}; let key = key.as_ref().to_string(); let value = value.as_ref().to_string(); + let header_name = key.parse::<HeaderName>().map_err(|_| {{ spec.title | caseUcfirst }}Error::new( + 400, format!("Invalid header name: {}", key), None, String::new(), + ))?; + let header_value = value.parse::<HeaderValue>().map_err(|_| {{ spec.title | caseUcfirst }}Error::new( + 400, format!("Invalid header value for {}", key), None, String::new(), + ))?; + self.state.rcu(|state| { let mut next = (**state).clone(); - if let (Ok(header_name), Ok(header_value)) = ( - key.parse::<HeaderName>(), - value.parse::<HeaderValue>(), - ) { - next.config.headers.insert(header_name, header_value); - } + next.config.headers.insert(header_name.clone(), header_value.clone()); Arc::new(next) }); - self.clone() + Ok(self.clone()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/rust/src/client.rs.twig` around lines 269 - 286, The add_header method currently swallows parse failures; make it consistent with the set_* setters by returning a Result<Self, E> instead of silently ignoring invalid headers: change add_header to return Result<Self, Error> (use the crate/common/error type or anyhow), perform key.parse::<HeaderName>() and value.parse::<HeaderValue>() and if either fails return Err with that parse error, otherwise insert into next.config.headers inside the rcu closure and return Ok(self.clone()); update any callers/tests and the method doc comment to reflect the new Result return signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@templates/rust/src/client.rs.twig`:
- Around line 269-286: The add_header method currently swallows parse failures;
make it consistent with the set_* setters by returning a Result<Self, E> instead
of silently ignoring invalid headers: change add_header to return Result<Self,
Error> (use the crate/common/error type or anyhow), perform
key.parse::<HeaderName>() and value.parse::<HeaderValue>() and if either fails
return Err with that parse error, otherwise insert into next.config.headers
inside the rcu closure and return Ok(self.clone()); update any callers/tests and
the method doc comment to reflect the new Result return signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 96b082c4-1325-4dcb-9a99-da2d59e49eb6
📒 Files selected for processing (5)
templates/rust/examples/basic_usage.rs.twigtemplates/rust/src/client.rs.twigtemplates/rust/src/error.rs.twigtemplates/rust/src/lib.rs.twigtemplates/rust/tests/tests.rs
Summary
.parse().unwrap()andpanic!withResult<Self>returns in all user-input client setters (set_endpoint,set_project,set_key,set_jwt,set_locale,set_session)From<reqwest::header::InvalidHeaderValue>impl to the error type// SAFETY:comment for compile-time constant header values innew()println!Test plan
examples/rust/output verifiedCloses #1397
Summary by CodeRabbit