-
Notifications
You must be signed in to change notification settings - Fork 132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(api-client-framework): initial implementation #1161
Conversation
WalkthroughThe pull request introduces a new Rust library named Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
libs/api-client-framework/src/endpoint.rs (2)
38-39
: Consider handling potential URL parse errors instead of panicking.
Calling .unwrap() here could cause a runtime panic if self.path() yields invalid input. For robust error handling, consider propagating an error using a custom endpoint-specific error or returning a Result.-fn url(&self, base_url: &Url) -> Url { - let mut url = base_url.join(&self.path()).unwrap(); +fn url(&self, base_url: &Url) -> Result<Url, SomeError> { + let mut url = base_url.join(&self.path())?; url.set_query(self.query().as_deref()); Ok(url) }
53-55
: Graceful fallback for query serialization.
Using .ok() to ignore errors in query serialization is functional, but consider capturing the serialization error for better debugging if unexpected data is passed.libs/api-client-framework/src/async_client.rs (2)
35-48
: Smooth construction of the middleware-enabled client.
Your use of exponential backoff and default headers for every request is a solid design. However, verify that the backoff configuration is appropriate for high-throughput scenarios, and ensure errors are traced or logged adequately to ease debugging.
73-90
: Potential to cover more status codes.
Currently, only 200 (OK) and 404 (NotFound) are specifically handled, leaving other codes flagged as generic InvalidStatusCode. Consider distinguishing additional status codes (e.g., 401, 403) if important to your domain logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/Cargo.toml
(1 hunks)libs/api-client-framework/Cargo.toml
(1 hunks)libs/api-client-framework/src/async_client.rs
(1 hunks)libs/api-client-framework/src/endpoint.rs
(1 hunks)libs/api-client-framework/src/lib.rs
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/api-client-framework/Cargo.toml
🔇 Additional comments (6)
libs/api-client-framework/src/endpoint.rs (1)
10-10
: Great use of associated type for the response.
Ensuring the response type implements both Deserialize and Debug is a sound approach that fosters strong compile-time guarantees and easier debugging.
libs/api-client-framework/src/async_client.rs (2)
19-27
: Default configuration is well-defined.
Having a default timeout of 30 seconds and a single retry is a reasonable baseline, ensuring the caller can still override these options.
50-66
: Optional body handling is well-structured.
Setting the Content-Type conditionally and using the trait’s content_type() accessor is straightforward and extensible.
libs/api-client-framework/src/lib.rs (2)
11-26
: Comprehensive error enum.
This error design covers key failure points like middleware failures, request timeouts, deserialization issues, and unexpected status codes. However, if expected status codes beyond 404 exist, consider enumerating them for finer-grained error handling.
28-35
: Seamless error conversion.
Mapping reqwest_middleware::Error into your custom Error is a clear and maintainable approach that keeps the rest of the code straightforward.
libs/Cargo.toml (1)
18-18
: Workspace expansion approved.
Adding "api-client-framework" as a workspace member is properly aligned with the new library structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
libs/api-client-framework/src/lib.rs (3)
11-28
: Add documentation for Error enum variants.While the error messages are descriptive, adding documentation comments for each variant would improve API documentation and help users better understand when each error might occur.
Example improvement:
#[derive(Debug, thiserror::Error)] +/// Represents all possible errors that can occur when making API requests pub enum Error { + /// Error occurred in the middleware layer #[error("middleware error: {0}")] Middleware(anyhow::Error), + /// Error occurred while making the HTTP request #[error("request error: {0}")] Request(#[from] reqwest::Error), // ... add docs for other variants
21-25
: Consider security implications of error messages.The
InvalidStatusCode
variant includes the raw error message from the server. This could potentially leak sensitive information if the server's error messages contain internal details.Consider sanitizing or limiting the error message content:
#[error("request returned with invalid status code: {status_code} - {message}")] InvalidStatusCode { status_code: reqwest::StatusCode, - message: String, + message: String, // TODO: Consider implementing message sanitization or using a bounded/filtered message },
1-37
: Consider adding framework usage examples and testing guidelines.As this is a framework for building API clients, it would be valuable to:
- Add example usage in the module documentation
- Provide testing utilities or traits to help users test their API client implementations
- Consider adding a builder pattern for
HttpApiClientConfig
to make configuration more flexible
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/api-client-framework/Cargo.toml
(1 hunks)libs/api-client-framework/src/lib.rs
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/api-client-framework/Cargo.toml
🔇 Additional comments (2)
libs/api-client-framework/src/lib.rs (2)
30-37
: LGTM! Clean error conversion implementation.
The From
implementation correctly maps middleware errors while preserving the error context.
1-2
: Add license information for the adapted code.
While attribution to cloudflare-rs is good, please also include the original license information to ensure proper compliance with the source project's terms.
Let's check the original repository's license:
A framework to be used when writing custom http api clients.
Summary by CodeRabbit
New Features
Endpoint
trait.Bug Fixes
Documentation