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 11 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.
templates/rust/tests/tests.rs
Outdated
| 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.
templates/rust/tests/tests.rs
Outdated
|
|
||
| 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 Confidence Score: 5/5Safe to merge — all remaining findings are P2 style/test-coverage suggestions that do not affect runtime correctness. The core logic change (panic → Result) is correct and consistent across all setters, templates/rust/src/client.rs.twig —
|
| Filename | Overview |
|---|---|
| templates/rust/src/client.rs.twig | All six setter methods and add_header now return Result<Self> instead of Self; set_endpoint adds proper URL parsing and scheme validation; add_header switches from silently ignoring bad headers to propagating errors; new unit tests added. Minor: // SAFETY: comment convention misused (not an unsafe block), and test_invalid_endpoint only covers the wrong-scheme path. |
| templates/rust/src/error.rs.twig | Adds From<InvalidHeaderValue> and From<InvalidHeaderName> impls, both mapping to code 400. Both are exercised by add_header's ? operator. |
| templates/rust/tests/tests.rs | The old no-op println! for the invalid-endpoint case is replaced with a real match that propagates an error if validation unexpectedly passes; all client builder calls updated with ?. |
| templates/rust/examples/basic_usage.rs.twig | Builder chain calls updated to propagate ?; no logic changes. |
| templates/rust/src/lib.rs.twig | Doc-comment example updated to use ? on setter chain; no logic changes. |
Reviews (7): Last reviewed commit: "Merge branch 'appwrite:master' into fix/..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
templates/rust/src/client.rs.twig:95
- The
// SAFETY:note claims the header values “below are compile-time constants … and always valid ASCII”, but the following inserts include a runtime-formatteduser-agentvalue. This makes the safety rationale inaccurate and also leaves additionalparse().unwrap()calls under that blanket statement. Consider narrowing the comment to only the truly static header values (or switching those toHeaderValue::from_static), and documenting/handling theuser-agentparse separately (e.g., with an explicit expectation message or a non-panicking fallback).
// 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.
|
closing this one as it doesnt feel like a need in the sdk, and will be a breaking one forcing users to add optional handling |
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