🔥 feat: Add support for creating Fiber client from existing FastHTTP client #3214
Conversation
mitulagr2
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThe changes in this pull request primarily enhance the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3214 +/- ##
==========================================
- Coverage 82.85% 82.79% -0.07%
==========================================
Files 114 114
Lines 11189 11193 +4
==========================================
- Hits 9271 9267 -4
- Misses 1520 1526 +6
- Partials 398 400 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
docs/client/rest.md (3)
98-112: Enhance the documentation for new methodsThe documentation for the new methods could be improved by:
- Adding more details about the default configuration for
New()- Explaining the benefits of using
NewWithClient()- Including parameter description for
NewWithClient(c *fasthttp.Client)- Adding example usage for both methods
Consider updating the documentation as follows:
### New -New creates and returns a new Client object. +New creates and returns a new Client object with default configuration. This method internally creates a new fasthttp.Client with default settings. ```go title="Signature" func New() *Client
+
go title="Example" +cc := client.New() +NewWithClient
-NewWithClient creates and returns a new Client object from an existing client object.
+NewWithClient creates and returns a new Client object using an existing fasthttp.Client. This method is useful when you need to:
+- Reuse an existing client configuration
+- Fine-tune the underlying fasthttp.Client settings
+- Share the same client across multiple Fiber Client instances
+
+Parameters:
+-c *fasthttp.Client: The existing fasthttp.Client to usefunc NewWithClient(c *fasthttp.Client) *Client
+```go title="Example"
+existingClient := &fasthttp.Client{
- MaxConnsPerHost: 100,
- ReadTimeout: 5 * time.Second,
- WriteTimeout: 5 * time.Second,
+}
+cc := client.NewWithClient(existingClient)
+```--- Line range hint `115-124`: **Improve consistency in REST method documentation** The REST method descriptions could be more consistent and informative. For example, the Get method's description "Get provides an API like axios which sends a get request" could be improved. Consider updating the REST method descriptions to be more descriptive and consistent: ```diff ### Get -Get provides an API like axios which sends a get request. +Get sends an HTTP GET request to the specified URL. It accepts optional configuration parameters to customize the request. ```go title="Signature" func (c *Client) Get(url string, cfg ...Config) (*Response, error)
+```go title="Example"
+resp, err := client.Get("https://api.example.com/users")
+if err != nil {
- // Handle error
+}
+fmt.Printf("Status: %d\n", resp.StatusCode())
+```--- Line range hint `1-15`: **Enhance introduction to highlight new client customization capabilities** The introduction section could better emphasize the client customization capabilities introduced in this PR. Consider updating the introduction to highlight the flexibility in client configuration: ```diff The Fiber Client for Fiber v3 is a powerful HTTP client optimized for high performance and ease of use in server-side applications. Built on top of the robust FastHTTP library, it inherits FastHTTP's high-speed HTTP protocol implementation. The client is designed to make HTTP requests both internally within services or externally to other web services. ## Features - **Lightweight & Fast**: Leveraging the minimalistic design of FastHTTP, the Fiber Client is lightweight and extremely fast. - **Flexible Configuration**: Configure client-level settings such as timeouts, headers, and more, which apply to all requests. Specific requests can further override or merge these settings. + **Flexible Configuration**: Configure client-level settings such as timeouts, headers, and more, which apply to all requests. You can either use the default client configuration, create a custom configuration, or reuse an existing fasthttp.Client for fine-grained control. Specific requests can further override or merge these settings. - **Connection Pooling**: Manages a pool of persistent connections that reduce the overhead of repeatedly establishing connections. - **Timeouts & Retries**: Supports setting request timeouts and retry mechanisms to handle transient failures.client/client.go (3)
683-684: Fix typo in commentThere's a typo in the comment above: "trie" should be "try".
-// FOllOW-UP performance optimization -// trie to use a pool to reduce the cost of memory allocation +// FOLLOW-UP performance optimization +// try to use a pool to reduce the cost of memory allocationThe implementation correctly maintains backward compatibility while leveraging the new
NewWithClientfunction.
686-687: Fix typo in function documentationThere's a typo in the function comment: "eisting" should be "existing".
-// NewWithClient creates and returns a new Client object from an eisting client object. +// NewWithClient creates and returns a new Client object from an existing client object.
Line range hint
687-707: Add nil check for client parameterThe function should validate that the input client is not nil to prevent potential panics.
func NewWithClient(c *fasthttp.Client) *Client { + if c == nil { + c = &fasthttp.Client{} + } return &Client{ fasthttp: c, // ... rest of initialization } }The implementation correctly initializes all required fields and maintains thread safety by creating new instances of dependencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
client/client.go(1 hunks)docs/client/rest.md(1 hunks)
🔇 Additional comments (1)
client/client.go (1)
Line range hint 686-707: Verify custom client configuration persistence
The implementation looks good but let's verify that custom client configurations are properly maintained.
✅ Verification successful
Custom client configurations are properly maintained and used
The verification shows that:
- The
NewWithClientimplementation correctly accepts and stores the custom client without modification - Test cases demonstrate successful usage with various client configurations including:
- TLS configurations
- Custom headers behavior (NoDefaultUserAgentHeader)
- Path normalization settings
- Custom dial functions
- The custom configurations persist and are respected in the client operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for examples of custom client configuration in tests or documentation
rg -A 5 "NewWithClient.*fasthttp.Client"
# Search for potential configuration options that users might want to customize
ast-grep --pattern 'fasthttp.Client {
$$$
}'
Length of output: 2528
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
client/client.go (2)
Line range hint
689-707: Consider adding configuration validation hooks.The initialization of hooks is critical for proper client operation. Consider adding validation hooks to ensure the provided fasthttp.Client is properly configured (e.g., timeout settings, TLS configuration).
Add unit tests for NewWithClient constructor
The codebase has extensive tests for the default
New()constructor and various client functionalities, but lacks specific tests for theNewWithClientconstructor. Please add test cases covering:
- Creating client with custom fasthttp.Client
- Handling nil client
- Verifying all fields are properly initialized
Example locations where tests should be added:
client/client_test.go🔗 Analysis chain
Line range hint
687-707: Add unit tests for the new constructor.As mentioned in previous reviews, we need unit tests for this function. Key test cases should include:
- Creating client with custom fasthttp.Client
- Handling nil client
- Verifying all fields are properly initialized
Let's check for existing tests:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files and test cases rg -l "TestNewWithClient|TestNew" --type goLength of output: 45
Script:
#!/bin/bash # Let's try a broader search for test files in the client directory fd ".*_test.go" client/ # Also search for any test functions related to client creation rg "func Test.*Client" --type goLength of output: 2759
Script:
#!/bin/bash # Let's check the content of client_test.go for any tests related to client creation or New functions rg "func Test.*New" client/client_test.go -A 5 # Also check for any existing test setup that might be testing client creation rg "New\(|NewWithClient\(" client/client_test.go -A 2Length of output: 5924
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
client/client.go(1 hunks)
🔇 Additional comments (1)
client/client.go (1)
683-684: LGTM! Good refactoring.
The modification to use NewWithClient reduces code duplication and maintains consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
client/client.go (2)
680-682: Consider tracking the performance optimization as a separate issue.The comment suggests a valuable performance optimization using object pooling. This could significantly reduce memory allocations and improve performance.
Would you like me to create a GitHub issue to track this performance optimization task? The issue would cover:
- Implementation of object pooling for Client instances
- Pooling for associated structs (header, cookie, query param, path param)
- Benchmarking to measure the impact
686-693: Consider setting secure TLS defaults.While the implementation is solid, consider setting secure TLS defaults (like minimum TLS version) when initializing the client to ensure secure communication by default.
func NewWithClient(c *fasthttp.Client) *Client { if c == nil { panic("fasthttp.Client must not be nil") } + // Set secure defaults if TLS config is not already set + if c.TLSConfig == nil { + c.TLSConfig = &tls.Config{ + MinVersion: tls.VersionTLS12, + } + } return &Client{
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
client/client.go(1 hunks)client/client_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.61.0)
client/client_test.go
68-68: File is not gofumpt-ed with -extra
(gofumpt)
68-68: File is not goimports-ed
(goimports)
🔇 Additional comments (3)
client/client.go (2)
683-684: LGTM! Clean implementation maintaining backward compatibility.
The New method correctly delegates to NewWithClient while maintaining the existing behavior by creating a new fasthttp.Client.
Line range hint 686-716: LGTM! Well-structured initialization with proper defaults.
The implementation:
- Correctly validates the input client
- Properly initializes all required fields
- Sets appropriate default hooks and marshalers
- Maintains extensibility through the hooks system
client/client_test.go (1)
58-79: Good test structure, but test coverage could be enhanced.
The test follows good practices with parallel execution and clear assertions. However, as mentioned in the previous review, consider expanding the test coverage to verify that custom client settings are preserved and to test different configurations.
🧰 Tools
🪛 golangci-lint (1.61.0)
68-68: File is not gofumpt-ed with -extra
(gofumpt)
68-68: File is not goimports-ed
(goimports)
gaby
changed the title
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
Description
Add support for passing a custom fasthttp client to the fiber http client, to allow more granular fine tuning by the users.
Fixes #3107
Type of change
NewWithClientmethod to client.Newmethod callsNewWithClientinternally.