refactor: replace unwrap with error handling in Client methods#9
refactor: replace unwrap with error handling in Client methods#9yashksaini-coder wants to merge 1 commit intoappwrite:mainfrom
Conversation
|
This library is auto-generated by the Appwrite SDK Generator, and does not accept pull requests directly. To learn more about how you can help us improve this SDK, please check the contributing guide. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe pull request introduces comprehensive error handling to the Appwrite Rust client. Header imports are updated to include Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ 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 |
Greptile SummaryThis PR replaces Key changes:
Confidence Score: 3/5Not quite safe to merge — set_self_signed and set_timeout can silently apply a half-updated state while returning Ok. The majority of the refactor is correct and improves safety. However, the inconsistent-state bug in set_self_signed/set_timeout (config updated, HTTP client not updated, error swallowed, Ok returned) is a real correctness issue that could cause subtle production bugs where self-signed certificate handling or timeout changes appear to succeed but are not applied. The PR description explicitly lists these methods as key improvements, so the regression needs to be addressed before merging. src/client.rs — set_self_signed and set_timeout methods (lines ~240–282) Important Files Changed
Reviews (1): Last reviewed commit: "refactor: replace unwrap with error hand..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| self.state.rcu(|state| { | ||
| let mut next = (**state).clone(); | ||
| if next.config.self_signed != self_signed { | ||
| next.config.self_signed = self_signed; | ||
| next.http = Self::build_http_client(&next.config); | ||
| next.http_no_redirect = Self::build_http_client_no_redirect(&next.config); | ||
| // If build fails, keep existing clients | ||
| if let Ok(http) = Self::build_http_client(&next.config) { | ||
| next.http = http; | ||
| } | ||
| if let Ok(http) = Self::build_http_client_no_redirect(&next.config) { | ||
| next.http_no_redirect = http; | ||
| } | ||
| } | ||
| Arc::new(next) | ||
| }); | ||
| self.clone() | ||
| Ok(self.clone()) |
There was a problem hiding this comment.
In set_self_signed (and the same pattern in set_timeout), when build_http_client fails the code updates next.config.self_signed but keeps the old HTTP client. This means the client's config says self_signed: true while its internal reqwest::Client still rejects self-signed certificates — a silently broken state.
Worse, the method always returns Ok(self.clone()) regardless of whether the build succeeded. The Result<Self> return type is therefore misleading: callers have no way to detect the failure.
A safer approach is to build both clients outside the rcu closure first, propagate any error before touching state, and only commit the new state when both builds succeed:
pub fn set_self_signed(&self, self_signed: bool) -> Result<Self> {
let current_config = {
let state = self.state.load();
state.config.clone()
};
if current_config.self_signed == self_signed {
return Ok(self.clone());
}
let mut new_config = current_config;
new_config.self_signed = self_signed;
let new_http = Self::build_http_client(&new_config)?;
let new_http_no_redirect = Self::build_http_client_no_redirect(&new_config)?;
self.state.rcu(|state| {
let mut next = (**state).clone();
next.config.self_signed = self_signed;
next.http = new_http.clone();
next.http_no_redirect = new_http_no_redirect.clone();
Arc::new(next)
});
Ok(self.clone())
}Apply the same pattern to set_timeout.
| Arc::new(next) | ||
| }); | ||
| self.clone() | ||
| Ok(self.clone()) | ||
| } | ||
|
|
||
| /// Set the project ID | ||
| pub fn set_project<S: Into<String>>(&self, project: S) -> Self { | ||
| pub fn set_project<S: Into<String>>(&self, project: S) -> Result<Self> { | ||
| let project = project.into(); | ||
| let header_value: HeaderValue = project.parse().map_err(|_| { | ||
| AppwriteError::new(0, format!("Invalid header value for project: {}", project), None, String::new()) | ||
| })?; | ||
| self.state.rcu(|state| { | ||
| let mut next = (**state).clone(); | ||
| next.config.headers.insert("x-appwrite-project", project.clone().parse().unwrap()); | ||
| next.config.headers.insert("x-appwrite-project", header_value.clone()); | ||
| Arc::new(next) | ||
| }); | ||
| self.clone() | ||
| Ok(self.clone()) | ||
| } |
There was a problem hiding this comment.
0, making is_client_error() return false
All validation AppwriteErrors (invalid header value, invalid endpoint scheme) are constructed with code: 0. Because is_client_error() is defined as (400..500).contains(&self.code), these user-input errors will return false from is_client_error(), which is counterintuitive — invalid user input is a client error.
Consider using 400 instead of 0 for validation errors:
return Err(AppwriteError::new(
400,
format!("Invalid endpoint URL: {}. Endpoint must start with http:// or https://", endpoint),
None,
String::new(),
));The same applies to all other validation errors in set_project, set_key, set_jwt, set_locale, set_session, and the HTTP client build errors.
There was a problem hiding this comment.
Pull request overview
Refactors the Rust SDK Client configuration API to avoid panics on invalid user input by switching multiple setters to Result-based validation and updating call sites accordingly.
Changes:
- Replaces
unwrap()/panic!inClientsetters (endpoint/project/key/jwt/locale/session) with validation returningResult<Self, AppwriteError>. - Refactors HTTP client construction helpers to return
Result<HttpClient>and updatesClient::new()initialization accordingly. - Updates test harness usage to handle the new
Result-returning setters and adds unit tests for invalid inputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/client.rs |
Converts setters and HTTP client builders to Result-based error handling; adds unit tests for invalid inputs. |
tests/tests.rs |
Updates client initialization to use ? with the new Result-returning setters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // SAFETY: Default config uses known-valid values, so build cannot fail | ||
| let http = Self::build_http_client(&config).unwrap(); | ||
| let http_no_redirect = Self::build_http_client_no_redirect(&config).unwrap(); |
There was a problem hiding this comment.
Client::new() still calls .unwrap() on build_http_client*() results. reqwest::ClientBuilder::build() can fail (e.g., TLS backend/init, system proxy/env issues), so this can still panic at runtime, and the // SAFETY: ... build cannot fail comment is not guaranteed. Consider returning Result<Client> (or adding a try_new() that returns Result) and propagating the error instead of unwrapping.
| pub fn set_key<S: Into<String>>(&self, key: S) -> Result<Self> { | ||
| let key = key.into(); | ||
| let header_value: HeaderValue = key.parse().map_err(|_| { | ||
| AppwriteError::new(0, format!("Invalid header value for key: {}", key), None, String::new()) |
There was a problem hiding this comment.
The error message for an invalid API key includes the full key value. Since API keys are secrets, this risks leaking credentials into logs, crash reports, or user-facing error messages. Prefer a redacted message (e.g., omit the value or include only its length) while still indicating which field was invalid.
| AppwriteError::new(0, format!("Invalid header value for key: {}", key), None, String::new()) | |
| AppwriteError::new(0, format!("Invalid header value for key (length {} characters)", key.len()), None, String::new()) |
| pub fn set_jwt<S: Into<String>>(&self, jwt: S) -> Result<Self> { | ||
| let jwt = jwt.into(); | ||
| let header_value: HeaderValue = jwt.parse().map_err(|_| { | ||
| AppwriteError::new(0, format!("Invalid header value for JWT: {}", jwt), None, String::new()) |
There was a problem hiding this comment.
The error message for an invalid JWT includes the full jwt value. JWTs are credentials and should not be echoed back in errors because they can end up in logs/telemetry. Use a redacted message that does not include the token contents.
| AppwriteError::new(0, format!("Invalid header value for JWT: {}", jwt), None, String::new()) | |
| AppwriteError::new(0, "Invalid header value for JWT".to_string(), None, String::new()) |
| pub fn set_session<S: Into<String>>(&self, session: S) -> Result<Self> { | ||
| let session = session.into(); | ||
| let header_value: HeaderValue = session.parse().map_err(|_| { | ||
| AppwriteError::new(0, format!("Invalid header value for session: {}", session), None, String::new()) |
There was a problem hiding this comment.
The error message for an invalid session includes the full session value, which is typically sensitive. Avoid including session tokens in error messages; return a redacted message instead.
| AppwriteError::new(0, format!("Invalid header value for session: {}", session), None, String::new()) | |
| AppwriteError::new(0, "Invalid header value for session".to_string(), None, String::new()) |
| pub fn set_self_signed(&self, self_signed: bool) -> Result<Self> { | ||
| self.state.rcu(|state| { | ||
| let mut next = (**state).clone(); | ||
| if next.config.self_signed != self_signed { | ||
| next.config.self_signed = self_signed; | ||
| next.http = Self::build_http_client(&next.config); | ||
| next.http_no_redirect = Self::build_http_client_no_redirect(&next.config); | ||
| // If build fails, keep existing clients | ||
| if let Ok(http) = Self::build_http_client(&next.config) { | ||
| next.http = http; | ||
| } | ||
| if let Ok(http) = Self::build_http_client_no_redirect(&next.config) { | ||
| next.http_no_redirect = http; | ||
| } |
There was a problem hiding this comment.
set_self_signed now returns Result<Self>, but it never returns Err: it updates next.config.self_signed even if rebuilding the HTTP clients fails, silently keeping the old clients. This can leave config and the actual http/http_no_redirect clients out of sync. Consider attempting the rebuild first and returning Err without mutating state if the rebuild fails (or keep the Self return type if failures are intentionally ignored).
| pub fn set_timeout(&self, timeout_secs: u64) -> Result<Self> { | ||
| self.state.rcu(|state| { | ||
| let mut next = (**state).clone(); | ||
| if next.config.timeout_secs != timeout_secs { | ||
| next.config.timeout_secs = timeout_secs; | ||
| next.http = Self::build_http_client(&next.config); | ||
| next.http_no_redirect = Self::build_http_client_no_redirect(&next.config); | ||
| if let Ok(http) = Self::build_http_client(&next.config) { | ||
| next.http = http; | ||
| } | ||
| if let Ok(http) = Self::build_http_client_no_redirect(&next.config) { | ||
| next.http_no_redirect = http; | ||
| } |
There was a problem hiding this comment.
set_timeout returns Result<Self> but never returns Err, and it updates next.config.timeout_secs even if rebuilding the HTTP clients fails, keeping the old clients. That can desynchronize the stored config from the actual client behavior (timeouts may not change). Consider rebuilding first and returning Err (and not updating state) when the rebuild fails, or otherwise avoid a Result return type if failure is intentionally ignored.
What does this PR do?
Replaces
unwrap()andpanic!calls insrc/client.rswith properResult-based error handling, so the SDK no longer panics on invalid user input.Changes:
Client::new(): Replaced.parse().unwrap()withHeaderValue::from_static()for all string literal header values. Theuser-agentheader (which uses runtime OS/arch values) retains.parse()with a descriptive.expect().set_endpoint,set_project,set_key,set_jwt,set_locale,set_session): Now returnResult<Self, AppwriteError>instead of panicking on invalid input.set_self_signed/set_timeout: Now returnResult<Self, AppwriteError>and gracefully handle HTTP client rebuild failures.build_http_client/build_http_client_no_redirect: Now returnResult<HttpClient>instead of panicking with.expect().Test Plan
cargo test --lib— all 426 tests pass (0 failures).set_project("invalid\nvalue")returnsErrwith a descriptive messageset_key("invalid\nkey"),set_jwt("invalid\njwt"),set_locale("invalid\nlocale"),set_session("invalid\nsession")all returnErrset_endpoint("ftp://example.com")returnsErrset_project("my-project-123"),set_endpoint("http://..."),set_endpoint("https://...")) returnOkRelated PRs and Issues
unwrap()calls with proper error handling inClient#8Have you read the Contributing Guidelines on issues?
Yes
Summary by CodeRabbit
Breaking Changes
set_endpoint,set_project,set_key,set_jwt,set_locale,set_session,set_self_signed,set_timeout) now returnResulttypes instead ofSelf, requiring explicit error handling.Improvements