Skip to content
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: Add support for creating Fiber client from existing FastHTTP client #3214

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mitulagr2
Copy link

@mitulagr2 mitulagr2 commented Nov 23, 2024

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

  • New feature (non-breaking change which adds functionality)
    • Add NewWithClient method to client.
  • Documentation update (changes to documentation)
  • Code consistency (non-breaking change which improves code reliability and robustness)
    • The New method calls NewWithClient internally.
    • Add unit tests for valid fasthttp client and nil client.

@mitulagr2 mitulagr2 requested a review from a team as a code owner November 23, 2024 12:16
@mitulagr2 mitulagr2 requested review from gaby, sixcolors, ReneWerner87 and efectn and removed request for a team November 23, 2024 12:16
Copy link

welcome bot commented Nov 23, 2024

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

Copy link
Contributor

coderabbitai bot commented Nov 23, 2024

Walkthrough

The changes in this pull request primarily enhance the Client structure within the client/client.go file by introducing a new method, NewWithClient, which allows the instantiation of a Client using an existing fasthttp.Client. The existing New method is updated to leverage this new capability. Additionally, the documentation in docs/client/rest.md is expanded to include detailed descriptions of these methods and the Client struct, improving clarity and usability for users.

Changes

File Change Summary
client/client.go - Added method: NewWithClient(c *fasthttp.Client) *Client
- Updated New() method to call NewWithClient(&fasthttp.Client{})
docs/client/rest.md - Expanded documentation to include New and NewWithClient methods and their signatures
- Detailed existing REST methods and added a "Request Configuration" section for the Config struct
- Clarified documentation for the Client struct and its functionalities
client/client_test.go - Added test: Test_New_With_Client to verify instantiation with a custom fasthttp.Client

Assessment against linked issues

Objective Addressed Explanation
Support to create client from existing client (#3107)

Possibly related PRs

  • 📚 Doc: Update What's New documentation #3181: The documentation update includes changes related to the Client structure, specifically the introduction of the New and NewWithClient methods, which are directly relevant to the changes made in the main PR regarding the client/client.go file.

Suggested labels

📒 Documentation, v3

Suggested reviewers

  • sixcolors
  • ReneWerner87
  • gaby

Poem

In the code where clients dwell,
A new path opens, all is well.
With NewWithClient, oh so bright,
Reuse your clients, what a delight!
Documentation now sings clear,
For every user, far and near! 🐇✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@gaby gaby added this to the v3 milestone Nov 23, 2024
Copy link

codecov bot commented Nov 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.79%. Comparing base (f8b490f) to head (1cb5ee0).

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     
Flag Coverage Δ
unittests 82.79% <100.00%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Outside diff range and nitpick comments (6)
docs/client/rest.md (3)

98-112: Enhance the documentation for new methods

The documentation for the new methods could be improved by:

  1. Adding more details about the default configuration for New()
  2. Explaining the benefits of using NewWithClient()
  3. Including parameter description for NewWithClient(c *fasthttp.Client)
  4. 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 use

func 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 comment

There'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 allocation

The implementation correctly maintains backward compatibility while leveraging the new NewWithClient function.


686-687: Fix typo in function documentation

There'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 parameter

The 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

📥 Commits

Reviewing files that changed from the base of the PR and between f8b490f and 4a70013.

📒 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 NewWithClient implementation 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

client/client.go Outdated Show resolved Hide resolved
client/client.go Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 the NewWithClient constructor. Please add test cases covering:

  1. Creating client with custom fasthttp.Client
  2. Handling nil client
  3. 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:

  1. Creating client with custom fasthttp.Client
  2. Handling nil client
  3. 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 go

Length 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 go

Length 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 2

Length of output: 5924

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 4a70013 and 37a5e6e.

📒 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.

client/client.go Show resolved Hide resolved
@mitulagr2 mitulagr2 requested a review from gaby November 23, 2024 13:12
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 37a5e6e and ec07efc.

📒 Files selected for processing (1)
  • client/client_test.go (1 hunks)

client/client_test.go Outdated Show resolved Hide resolved
client/client.go Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 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

📥 Commits

Reviewing files that changed from the base of the PR and between ec07efc and d2f7db5.

📒 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 gaby added the v3 label Nov 23, 2024
@gaby gaby changed the title 🔥 feat: Add support to create client from existing client 🔥 feat: Add support for creating Fiber client from existing FastHTTP client Nov 23, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between d2f7db5 and 1cb5ee0.

📒 Files selected for processing (1)
  • client/client_test.go (1 hunks)

client/client_test.go Show resolved Hide resolved
Copy link
Member

@gaby gaby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

📝 [Proposal]: Add support to create client from existing client
3 participants