[ENH]: (Rust client) parse and return API errors #5656
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Improve Rust Client Error Handling: Parse and Return Structured API Errors This pull request enhances error handling in the Rust client for Chroma by introducing logic to parse structured API error responses and return them to the user rather than discarding them. It introduces a new Key Changes• Added new Affected Areas• This summary was automatically generated by @propel-code-bot |
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
| pub enum ChromaClientError { | ||
| #[error("Request error: {0:?}")] | ||
| RequestError(#[from] reqwest::Error), | ||
| #[error("API error: {0:?} ({1})")] |
There was a problem hiding this comment.
[BestPractice]
Using {0:?} in the thiserror message format will add quotes around the error string when displayed. Using {0} will display the string directly, which is usually more desirable for error messages.
Context for Agents
[**BestPractice**]
Using `{0:?}` in the `thiserror` message format will add quotes around the error string when displayed. Using `{0}` will display the string directly, which is usually more desirable for error messages.
File: rust/chroma/src/client/chroma_client.rs
Line: 28| if let Some(response) = maybe_response { | ||
| let status = response.status(); | ||
| let json = response.json::<serde_json::Value>().await?; | ||
|
|
||
| if tracing::enabled!(tracing::Level::TRACE) { | ||
| tracing::trace!( | ||
| url = %url, | ||
| method =? method, | ||
| "Received response: {}", | ||
| serde_json::to_string_pretty(&json).unwrap_or_else(|_| "<failed to serialize>".to_string()) | ||
| ); | ||
| } | ||
|
|
||
| if let Ok(api_error) = serde_json::from_value::<ErrorResponse>(json) { | ||
| return Err(ChromaClientError::ApiError( | ||
| format!("{}: {}", api_error.error, api_error.message), | ||
| status, | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
[BestPractice]
The logic here for deserializing and logging the JSON response is very similar to the logic in the success path that follows this match block. To avoid code duplication and improve maintainability, consider extracting this into a private helper function.
For example, a helper like this could serve both paths:
async fn deserialize_with_logging<T: serde::de::DeserializeOwned>(
response: reqwest::Response,
url: &reqwest::Url,
method: &reqwest::Method,
) -> Result<T, ChromaClientError> {
let json = response.json::<serde_json::Value>().await?;
if tracing::enabled!(tracing::Level::TRACE) {
tracing::trace!(
url = %url,
method =? method,
"Received response: {}",
serde_json::to_string_pretty(&json).unwrap_or_else(|_| "<failed to serialize>".to_string())
);
}
serde_json::from_value::<T>(json).map_err(Into::into)
}This would allow you to simplify both the error-handling and success paths significantly.
Context for Agents
[**BestPractice**]
The logic here for deserializing and logging the JSON response is very similar to the logic in the success path that follows this `match` block. To avoid code duplication and improve maintainability, consider extracting this into a private helper function.
For example, a helper like this could serve both paths:
```rust
async fn deserialize_with_logging<T: serde::de::DeserializeOwned>(
response: reqwest::Response,
url: &reqwest::Url,
method: &reqwest::Method,
) -> Result<T, ChromaClientError> {
let json = response.json::<serde_json::Value>().await?;
if tracing::enabled!(tracing::Level::TRACE) {
tracing::trace!(
url = %url,
method =? method,
"Received response: {}",
serde_json::to_string_pretty(&json).unwrap_or_else(|_| "<failed to serialize>".to_string())
);
}
serde_json::from_value::<T>(json).map_err(Into::into)
}
```
This would allow you to simplify both the error-handling and success paths significantly.
File: rust/chroma/src/client/chroma_client.rs
Line: 418
codetheweb
force-pushed
the
feat-chroma-rust-client-create-list-collections
branch
from
703a271 to
0e9ef4c
Compare
codetheweb
force-pushed
the
feat-chroma-rust-client-custom-error
branch
from
e4f70b0 to
c14bd60
Compare
Merge activity
|
codetheweb
changed the base branch from
feat-chroma-rust-client-create-list-collections
to
graphite-base/5656
codetheweb
force-pushed
the
feat-chroma-rust-client-custom-error
branch
from
c14bd60 to
8246ef4
Compare
2 participants
Description of changes
Prior to this change, responses containing error messages would never be visible.
Test plan
How are these changes tested?
pytestfor python,yarn testfor js,cargo testfor rustMigration plan
Are there any migrations, or any forwards/backwards compatibility changes needed in order to make sure this change deploys reliably?
Observability plan
What is the plan to instrument and monitor this change?
Documentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?