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

✨ v3 (feature): client refactor #1986

Merged
merged 143 commits into from
Mar 4, 2024
Merged

Conversation

wangjq4214
Copy link
Member

@wangjq4214 wangjq4214 commented Jul 27, 2022

Aims

Please provide enough information so that others can review your pull request:

Move client.go to client/ to make project structure more tidy.
Re-create client by inspiring by go-resty/resty or axios/axios.

Details

Explain the details for making this change. What existing problem does the pull request solve?

This PR is related to #1906.

TODO List

  • Core request process, hook and plugin mechanism
  • URL and baseurl merge mechanism
  • Header setting
  • Query parameter setting
  • User agent setting
  • Cookie setting
  • Path parameter setting
  • Form data support
  • JSON body, XML body, Raw body, Files body
  • Refer setting
  • Unmarshal
  • Chain API (processing)
  • API like axios
  • Timeout
  • Response save to file or io.Writer
  • TLS support
  • Proxy
  • Retry (✨ v3 (feature): add retry mechanism #1972) (Thanks for it, I'll continue this work)
  • Throttle
  • Extending the axios-like API options
  • Redirect
  • Improve test coverage (95% up, 96.7% now)
  • Add benchmarks
  • Cookie jar
  • Logger (A simple impl, may be improve)
  • Trace (Need PR for fasthttp)
  • Add API docs and examples

Summary by CodeRabbit

  • New Features

    • Introduced a comprehensive Client struct in the Fiber Client for enhanced request handling, including support for custom headers, parameters, cookies, timeouts, user agents, TLS configurations, and more.
    • Added robust HTTP request and response handling capabilities, including methods for GET, POST, PUT, DELETE, etc., with extensive configuration options.
    • Implemented new test functions for validating HTTP response attributes, ensuring reliability in response parsing and saving.
  • Refactor

    • Refined core client functionalities for improved request and response processing, including error handling, retries, and timeouts.

@efectn efectn linked an issue Jul 27, 2022 that may be closed by this pull request
@efectn efectn added this to the v3 milestone Jul 27, 2022
@wangjq4214
Copy link
Member Author

wangjq4214 commented Aug 3, 2022

Can you add some features that I haven't thought of? Please! @efectn

@efectn
Copy link
Member

efectn commented Aug 3, 2022

Can you add some features that I haven't thought of? Please! @efectn

Sure ill take a look

@wangjq4214 wangjq4214 marked this pull request as ready for review August 5, 2022 02:11
@wangjq4214
Copy link
Member Author

wangjq4214 commented Aug 5, 2022

The refactoring work is done basically, then I will finish the rest of the work in TODO list and add unit tests.

Please do a part of the code review work first. Thanks! @efectn @ReneWerner87

Copy link
Member

@efectn efectn left a comment

Choose a reason for hiding this comment

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

We should add README.md to explain new client. We can also give some examples.

@efectn
Copy link
Member

efectn commented Aug 5, 2022

I reviewed API and it's fine to me. And we may extend request setter options more.

Also what's https://github.com/wangjq4214/fiber/blob/v3-client/client/plugins.go?

@wangjq4214
Copy link
Member Author

We should add README.md to explain new client. We can also give some examples.

Should I create a new README.md in the client package?

@wangjq4214
Copy link
Member Author

wangjq4214 commented Aug 5, 2022

I reviewed API and it's fine to me. And we may extend request setter options more.

Also what's https://github.com/wangjq4214/fiber/blob/v3-client/client/plugins.go?

More options

I will further expand the options after I finish the remaining three features.

Plugins

At first I wanted to provide users with the ability to change the request process through the plugin.

For example like as follows:

package client

import (
	"context"
	"time"
)

type TimeoutPlugin struct {
	time int
}

func (t *TimeoutPlugin) Name() string {
	return "timeout-plugin"
}

func (t *TimeoutPlugin) Check() bool {
	return t.time != 0
}

func (t *TimeoutPlugin) GenerateExecute(f ExecuteFunc) (ExecuteFunc, error) {
	return func(ctx context.Context, c *Client, r *Request) (*Response, error) {
		ctx, cancel := context.WithTimeout(ctx, time.Duration(t.time)*time.Second)
		defer func() { cancel() }()
		return f(ctx, c, r)
	}, nil
}

func NewTimeoutPlugin(time int) *TimeoutPlugin {
	return &TimeoutPlugin{time: time}
}

var _ Plugin = (*TimeoutPlugin)(nil)

But now I think it's an overdesign. Any Suggestions?

@efectn
Copy link
Member

efectn commented Aug 5, 2022

I reviewed API and it's fine to me. And we may extend request setter options more.
Also what's https://github.com/wangjq4214/fiber/blob/v3-client/client/plugins.go?

More options

I will further expand the options after I finish the remaining three features.

Plugins

At first I wanted to provide users with the ability to change the request process through the plugin.

For example like as follows:

package client

import (
	"context"
	"time"
)

type TimeoutPlugin struct {
	time int
}

func (t *TimeoutPlugin) Name() string {
	return "timeout-plugin"
}

func (t *TimeoutPlugin) Check() bool {
	return t.time != 0
}

func (t *TimeoutPlugin) GenerateExecute(f ExecuteFunc) (ExecuteFunc, error) {
	return func(ctx context.Context, c *Client, r *Request) (*Response, error) {
		ctx, cancel := context.WithTimeout(ctx, time.Duration(t.time)*time.Second)
		defer func() { cancel() }()
		return f(ctx, c, r)
	}, nil
}

func NewTimeoutPlugin(time int) *TimeoutPlugin {
	return &TimeoutPlugin{time: time}
}

var _ Plugin = (*TimeoutPlugin)(nil)

But now I think it's an overdesign. Any Suggestions?

I think we don't need a plugin mechanism. Hooks are enough for extendability.

Should I create a new README.md in the client package?

Yes

@efectn efectn changed the title ✨ v3(feature): client refactor ✨ v3 (feature): client refactor Aug 5, 2022
@@ -0,0 +1,766 @@
package client
Copy link
Member

Choose a reason for hiding this comment

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

@coderabbitai read client/client.go and generate usage and examples documentation in Markdown format.

@gofiber gofiber deleted a comment from coderabbitai bot Mar 2, 2024
@gofiber gofiber deleted a comment from coderabbitai bot Mar 2, 2024
@@ -0,0 +1,766 @@
package client

import (
Copy link
Member

Choose a reason for hiding this comment

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

@coderabbitai read client/client.go and generate usage examples in Markdown format. Include examples for doing GET, POST, PUT, using TLS config, using JSON for request and response, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gaby Based on the provided client/client.go file, here are some usage examples in Markdown format for doing GET, POST, PUT requests, using TLS config, and handling JSON for request and response.

GET Request Example

client := NewClient()
response, err := client.Get("https://example.com/api/resource")
if err != nil {
    log.Fatalf("Error making GET request: %v", err)
}
fmt.Println("Response status:", response.StatusCode())

POST Request with JSON Body Example

client := NewClient()
data := map[string]interface{}{
    "name": "John Doe",
    "age": 30,
}
response, err := client.Post("https://example.com/api/resource", Config{
    Body: data,
})
if err != nil {
    log.Fatalf("Error making POST request: %v", err)
}
fmt.Println("Response status:", response.StatusCode())

PUT Request with JSON Response Example

client := NewClient()
response, err := client.Put("https://example.com/api/resource/1", Config{
    Body: map[string]interface{}{"name": "Jane Doe"},
})
if err != nil {
    log.Fatalf("Error making PUT request: %v", err)
}
var result map[string]interface{}
if err := json.Unmarshal(response.Body(), &result); err != nil {
    log.Fatalf("Error parsing JSON response: %v", err)
}
fmt.Println("Updated resource:", result)

Using TLS Config

client := NewClient()
tlsConfig := &tls.Config{
    InsecureSkipVerify: true, // Example: Skip TLS verification, use with caution!
}
client.SetTLSConfig(tlsConfig)
response, err := client.Get("https://example.com/secure-resource")
if err != nil {
    log.Fatalf("Error making secure GET request: %v", err)
}
fmt.Println("Response status:", response.StatusCode())

These examples demonstrate basic usage of the client package for making HTTP requests, including setting custom headers, query parameters, and handling JSON data. Remember to replace "https://example.com/api/resource" with your actual resource URL and adjust the request body and response handling as needed for your specific use case.

Analysis chain
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Fetch the content of client/client.go for analysis.
cat client/client.go

Length of output: 20630

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.

Review Status

Actionable comments generated: 16

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between a140863 and 6b01572.
Files selected for processing (7)
  • client/client.go (1 hunks)
  • client/client_test.go (1 hunks)
  • client/core.go (1 hunks)
  • client/hooks.go (1 hunks)
  • client/hooks_test.go (1 hunks)
  • client/request.go (1 hunks)
  • client/request_test.go (1 hunks)
Files not summarized due to errors (2)
  • client/client_test.go: Error: Message exceeds token limit
  • client/request_test.go: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (4)
  • client/client.go
  • client/core.go
  • client/hooks_test.go
  • client/request.go
Additional comments: 28
client/hooks.go (1)
  • 36-56: Given the previous discussion about the use of math/rand for performance reasons, it's crucial to ensure that the randString function is not used in security-sensitive contexts. If it is used for non-security purposes, such as generating boundary strings for multipart forms, the performance benefit might justify its use. However, if there's any chance it could be used in a security context, reconsider the decision or clearly document its intended use to prevent security vulnerabilities.
client/client_test.go (26)
  • 94-120: The test Test_Client_Add_Hook_CheckOrder correctly checks the order of request hooks execution. This is an excellent addition to ensure the hooks feature behaves predictably.
  • 122-215: The tests for JSON and XML marshal/unmarshal functions are comprehensive and cover both success and error scenarios. This ensures the custom serialization and deserialization functionalities are working as expected.
  • 218-224: Setting the base URL for the client is a fundamental feature, and the test Test_Client_SetBaseURL correctly verifies this functionality.
  • 226-252: Tests for handling invalid URLs and unsupported protocols are crucial for ensuring the client behaves correctly in error scenarios. These tests help in validating error handling and resilience of the client.
  • 254-279: The test Test_Client_ConcurrencyRequests is a valuable addition to ensure the client can handle concurrent requests correctly. This test is crucial for assessing the client's performance and stability under load.
  • 281-319: The tests for the Get method, both the global function and the client instance method, are well-structured and ensure the functionality works as expected.
  • 321-358: Similar to the Get method tests, the tests for the Head method are well-implemented, ensuring the method behaves correctly.
  • 639-687: Testing the default and custom user agent settings is important for ensuring the client sends the correct user agent in requests. These tests are well-implemented.
  • 690-763: The tests for adding and setting headers, including case-insensitivity, are comprehensive and ensure headers are correctly managed by the client. This addresses the previous comment about testing case-insensitivity.
  • 765-789: The test Test_Client_Header_With_Server effectively verifies the client's ability to send multiple headers, including handling duplicates and overriding values. This is a good practice for ensuring header management works as expected.
  • 791-851: The tests for setting and managing cookies are comprehensive, covering various scenarios including setting individual cookies, setting multiple cookies, and deleting cookies. This ensures the client's cookie management functionality works as expected.
  • 853-870: The test Test_Client_Cookie_With_Server effectively verifies the client's ability to send cookies and handle cookie deletion. This is crucial for ensuring the client's cookie management works correctly in real-world scenarios.
  • 873-976: The tests for the cookie jar functionality, including handling cookies without expiration and with expiration, as well as overriding cookie values, are well-structured and ensure the cookie jar behaves as expected. This addresses the previous comment about ensuring the cookie jar correctly handles cookies with different paths, domains, and expiration times.
  • 978-987: The test for setting the referer header is straightforward and ensures the client correctly sets and sends the referer in requests.
  • 990-1113: The tests for adding, setting, and managing query parameters are comprehensive, covering various scenarios including adding individual parameters, setting multiple parameters, and deleting parameters. This ensures the client's query parameter management functionality works as expected.
  • 1115-1129: The test Test_Client_QueryParam_With_Server effectively verifies the client's ability to send query parameters. This is crucial for ensuring the client's query parameter management works correctly in real-world scenarios.
  • 1131-1191: The tests for setting and managing path parameters are comprehensive, covering various scenarios including setting individual path parameters, setting multiple path parameters, and deleting path parameters. This ensures the client's path parameter management functionality works as expected.
  • 1193-1209: The test Test_Client_PathParam_With_Server effectively verifies the client's ability to send path parameters. This is crucial for ensuring the client's path parameter management works correctly in real-world scenarios.
  • 1211-1240: The tests for TLS configurations, including setting custom TLS configurations and handling empty TLS configurations, are important for ensuring secure communication. These tests cover the necessary scenarios to ensure the client's TLS functionality works as expected.
  • 1242-1270: The test Test_Client_TLS_Empty_TLSConfig correctly handles the scenario where no TLS configuration is provided, ensuring the client behaves correctly in such cases. This addresses the previous comment about adding negative test cases for TLS handshake failures.
  • 1272-1280: Setting certificates for the client is a crucial feature for TLS communication. The test Test_Client_SetCertificates correctly verifies this functionality.
  • 1282-1287: The test Test_Client_SetRootCertificate ensures the client can correctly set a root certificate from a file. This is important for custom CA trust stores.
  • 1289-1301: The test Test_Client_SetRootCertificateFromString ensures the client can correctly set a root certificate from a string. This flexibility is important for scenarios where the certificate is not available as a file.
  • 1303-1311: The test Test_Client_R ensures the request builder function R() works as expected, returning a new request instance associated with the client. This is crucial for the usability of the client.
  • 1313-1346: The test Test_Replace verifies the functionality to replace the global client with a custom client for a scoped execution. This is important for testing and scenarios where temporary client configurations are needed.
  • 1348-1451: The tests for setting various configurations to a request, such as context, user agent, referer, headers, parameters, cookies, path parameters, timeout, max redirects, body, and file, are comprehensive and ensure the request builder functions work as expected.
client/request_test.go (1)
  • 638-660: The previous comment about adding a teardown step to close the listener and clean up resources after the test completes in Test_Request_Get is still relevant. Implementing a teardown step is crucial for preventing resource leaks and ensuring that each test runs in a clean state.

client/request_test.go Show resolved Hide resolved
client/hooks.go Show resolved Hide resolved
client/hooks.go Show resolved Hide resolved
client/hooks.go Show resolved Hide resolved
client/hooks.go Show resolved Hide resolved
client/hooks.go Show resolved Hide resolved
client/hooks.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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 6b01572 and 907b8a7.
Files selected for processing (2)
  • client/client.go (1 hunks)
  • client/core.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • client/client.go
  • client/core.go

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.

Review Status

Actionable comments generated: 13

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 907b8a7 and cb33aae.
Files selected for processing (2)
  • client/client_test.go (1 hunks)
  • client/response_test.go (1 hunks)
Files not summarized due to errors (1)
  • client/client_test.go: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (1)
  • client/response_test.go
Additional comments: 21
client/client_test.go (21)
  • 122-216: The tests for marshaling and unmarshaling (JSON and XML) are comprehensive and cover both successful and error scenarios. It's good practice to ensure that the custom marshal and unmarshal functions are invoked as expected. These tests effectively validate the functionality of setting custom marshal/unmarshal functions.
  • 218-224: Setting the base URL for the client is correctly tested. This is a crucial feature for clients that interact with a specific base endpoint. The test ensures that the base URL is properly set and retrieved.
  • 226-252: The tests for handling invalid URLs and unsupported protocols are important for ensuring robust error handling in the client. These tests correctly assert that the client returns an ErrURLFormat error when an invalid URL or an unsupported protocol is used.
  • 254-279: The concurrency test for making requests using different HTTP methods is well-structured. It ensures that the client can handle concurrent requests without issues. This test is crucial for validating the thread-safety and concurrency handling of the client.
  • 281-319: The tests for the Get method, both the global function and the client method, are correctly implemented. They ensure that the client can make GET requests and receive the expected response. It's good to see the server shutdown being handled in a deferred function to clean up resources.
  • 639-688: The tests for setting the user agent, both default and custom, are correctly implemented. These tests ensure that the client correctly sets and sends the user agent in the request headers.
  • 765-789: The test for header manipulation with a server is well-structured and ensures that headers are correctly sent and received. It's good to see a variety of header operations being tested, including adding, setting, and deleting headers.
  • 791-851: The tests for cookie manipulation cover setting, updating, and deleting cookies. These tests ensure that the client correctly handles cookies, which is crucial for maintaining session state and other functionalities that rely on cookies.
  • 853-870: The test for cookie manipulation with a server ensures that cookies are correctly sent and received. This test validates the client's ability to handle cookies in a real-world scenario.
  • 978-988: The test for setting the referer header is correctly implemented. This test ensures that the client correctly sets and sends the referer header in the request, which can be important for web analytics and logging purposes.
  • 990-1113: The tests for query parameter manipulation cover adding, setting, and deleting parameters. These tests ensure that the client correctly handles query parameters, which are crucial for making requests with specific data or instructions.
  • 1115-1129: The test for query parameter manipulation with a server ensures that query parameters are correctly sent and received. This test validates the client's ability to handle query parameters in a real-world scenario.
  • 1131-1191: The tests for path parameter manipulation cover setting, updating, and deleting path parameters. These tests ensure that the client correctly handles path parameters, which are crucial for making requests to specific resource locations.
  • 1193-1209: The test for path parameter manipulation with a server ensures that path parameters are correctly sent and received. This test validates the client's ability to handle path parameters in a real-world scenario.
  • 1242-1272: The test for TLS error handling is crucial for ensuring that the client behaves correctly when encountering TLS handshake errors due to incompatible configurations. This test validates the robustness of the client's TLS support.
  • 1274-1302: The test for an empty TLS configuration ensures that the client behaves correctly when no TLS configuration is provided. This test is important for validating the client's default behavior in the absence of explicit TLS settings.
  • 1304-1312: The test for setting certificates on the client is correctly implemented. This test ensures that the client can be configured with custom certificates, which is crucial for secure communication in environments with specific trust requirements.
  • 1314-1333: The tests for setting the root certificate, both from a file and from a string, are correctly implemented. These tests ensure that the client can trust custom root CAs, which is important for secure communication with servers using certificates signed by those CAs.
  • 1335-1343: The test for the R() method ensures that a new request object is correctly created and associated with the client. This test validates the client's ability to initiate new requests.
  • 1345-1378: The test for replacing the global client with a custom client configuration is well-structured. It ensures that the global client can be temporarily replaced for specific tests or operations, which is useful for testing or when different configurations are needed for different requests.
  • 1380-1483: The tests for setting various configurations to a request are comprehensive and cover a wide range of configurations including context, user agent, referer, headers, params, cookies, path params, timeout, max redirects, body, and file. These tests ensure that the client can be configured as needed for different requests.

client/client_test.go Outdated Show resolved Hide resolved
client/client_test.go Outdated Show resolved Hide resolved
client/client_test.go Show resolved Hide resolved
client/client_test.go Outdated Show resolved Hide resolved
client/client_test.go Show resolved Hide resolved
client/client_test.go Outdated 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.

Review Status

Actionable comments generated: 17

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between cb33aae and f56dfd2.
Files selected for processing (3)
  • client/client_test.go (1 hunks)
  • listen_test.go (2 hunks)
  • redirect_test.go (1 hunks)
Files not summarized due to errors (1)
  • client/client_test.go: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (2)
  • listen_test.go
  • redirect_test.go
Additional comments: 20
client/client_test.go (20)
  • 94-120: The test Test_Client_Add_Hook_CheckOrder correctly verifies the execution order of request hooks. This is a good practice and ensures that hooks are executed in the order they were added, which is critical for predictable behavior.
  • 218-224: The test Test_Client_SetBaseURL correctly verifies that the base URL is set and retrieved correctly.
  • 244-251: The test Test_Client_Unsupported_Protocol correctly checks for an error when an unsupported protocol is used. This is a good practice to ensure that the client behaves as expected with unsupported protocols.
  • 254-279: The test Test_Client_ConcurrencyRequests effectively tests the client's ability to handle concurrent requests. This is crucial for ensuring that the client can be used in a concurrent environment without issues.
  • 765-788: The test Test_Client_Header_With_Server effectively verifies the behavior of setting and manipulating headers with a server. This is a good practice to ensure that headers are correctly sent and received.
  • 853-870: The test Test_Client_Cookie_With_Server correctly verifies the behavior of setting and deleting cookies with a server. This ensures that cookies are correctly managed in client requests.
  • 873-995: The tests for the cookie jar functionality (Test_Client_CookieJar and Test_Client_CookieJar_Response) are comprehensive and cover various scenarios, including cookie expiration and overriding cookie values. These tests ensure that the cookie jar behaves as expected in different situations.
  • 998-1008: The test Test_Client_Referer correctly verifies that the Referer header is set and sent correctly in requests. This is important for ensuring that the client can correctly manage the Referer header.
  • 1135-1149: The test Test_Client_QueryParam_With_Server effectively verifies the behavior of query parameters with a server. This ensures that query parameters are correctly included in client requests.
  • 1213-1229: The test Test_Client_PathParam_With_Server correctly verifies the behavior of path parameters with a server. This ensures that path parameters are correctly replaced in the URL of client requests.
  • 1231-1260: The test Test_Client_TLS correctly verifies the behavior of setting custom TLS configurations for the client. This is crucial for ensuring that the client can establish secure connections with custom TLS settings.
  • 1262-1292: The test Test_Client_TLS_Error effectively verifies the behavior when the client and server TLS configurations are incompatible, ensuring that the client correctly handles TLS handshake failures. This is important for robust error handling in secure connections.
  • 1294-1322: The test Test_Client_TLS_Empty_TLSConfig correctly verifies the behavior when no TLS configuration is provided to the client. This ensures that the client behaves as expected in the absence of custom TLS settings.
  • 1324-1332: The test Test_Client_SetCertificates correctly verifies that certificates can be set for the client's TLS configuration. This is important for ensuring that the client can use custom certificates for secure connections.
  • 1334-1339: The test Test_Client_SetRootCertificate correctly verifies that a root certificate can be set for the client's TLS configuration from a file. This is crucial for ensuring that the client can trust custom certificate authorities.
  • 1341-1353: The test Test_Client_SetRootCertificateFromString correctly verifies that a root certificate can be set for the client's TLS configuration from a string. This provides flexibility in configuring the client's trust store.
  • 1355-1363: The test Test_Client_R correctly verifies that the R method returns a new request object associated with the client. This is important for ensuring that the client can create new requests correctly.
  • 1365-1398: The test Test_Replace effectively verifies the behavior of replacing the global client with a custom client and then restoring the original global client. This is crucial for ensuring that the global client can be temporarily overridden for specific tests.
  • 1400-1518: The tests in Test_Set_Config_To_Request comprehensively cover setting various configurations to a request, such as context, user agent, referer, headers, params, cookies, path params, timeout, max redirects, body, and file. These tests ensure that the request configuration is correctly applied.
  • 1564-1580: The test Test_Client_SetRetryConfig correctly verifies the behavior of setting a retry configuration for the client. This is important for ensuring that the client can retry requests according to the specified configuration.

client/client_test.go Outdated Show resolved Hide resolved
client/client_test.go Outdated Show resolved Hide resolved
client/client_test.go Outdated Show resolved Hide resolved
client/client_test.go Show resolved Hide resolved
client/client_test.go Outdated Show resolved Hide resolved
client/client_test.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.

Review Status

Actionable comments generated: 15

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between f56dfd2 and 96d6068.
Files selected for processing (1)
  • client/client_test.go (1 hunks)
Files not summarized due to errors (1)
  • client/client_test.go: Error: Message exceeds token limit
Additional comments: 6
client/client_test.go (6)
  • 23-54: The function startTestServerWithPort is a utility function for starting a test server. It's well-implemented and uses channels to communicate the server's address or any error that occurs during startup. This pattern ensures that the test does not proceed until the server is ready, which is a good practice for avoiding race conditions in tests.
  • 103-129: The test Test_Client_Add_Hook_CheckOrder correctly verifies the order of execution for request hooks. This is an important aspect to test, as the order in which hooks are executed can affect the behavior of the client. The use of a bytebufferpool to concatenate the hook identifiers is a clever way to assert the execution order.
  • 229-234: The test Test_Client_SetBaseURL correctly verifies that the base URL is set as expected. This is a straightforward test that ensures the SetBaseURL method functions correctly.
  • 255-262: The test Test_Client_Unsupported_Protocol correctly checks for an error when an unsupported protocol is used in the URL. This is a good negative test case to ensure that the client behaves as expected when encountering unsupported protocols.
  • 265-290: The test Test_Client_ConcurrencyRequests effectively tests the client's ability to handle concurrent requests. It iterates over a set of HTTP methods and performs requests in parallel, asserting the correct response for each. This test is crucial for ensuring that the client can handle a high volume of concurrent requests without issues.
  • 332-371: The tests for the Head method (Test_Head) correctly verify the behavior of both the global Head function and the client's Head method. These tests ensure that the correct headers are returned without a response body, which is the expected behavior for HEAD requests.

client/client_test.go Outdated Show resolved Hide resolved
client/client_test.go Show resolved Hide resolved
client/client_test.go Show resolved Hide resolved
client/client_test.go Show resolved Hide resolved
client/client_test.go Show resolved Hide resolved
client/client_test.go Show resolved Hide resolved
client/client_test.go Show resolved Hide resolved
client/client_test.go Outdated Show resolved Hide resolved
client/client_test.go Show resolved Hide resolved
client/client_test.go Show resolved Hide resolved
@efectn
Copy link
Member

efectn commented Mar 2, 2024

I think the PR should be ready-to-merge. Do you have any comments @gofiber/maintainers

@gaby gaby mentioned this pull request Mar 4, 2024
30 tasks
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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 96d6068 and 2627b52.
Files selected for processing (5)
  • client/client.go (1 hunks)
  • client/client_test.go (1 hunks)
  • log/default.go (2 hunks)
  • middleware/proxy/proxy_test.go (16 hunks)
  • redirect_test.go (1 hunks)
Files not summarized due to errors (1)
  • client/client_test.go: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (4)
  • client/client.go
  • log/default.go
  • middleware/proxy/proxy_test.go
  • redirect_test.go
Additional comments: 17
client/client_test.go (17)
  • 23-54: The utility function startTestServerWithPort is well-implemented and follows best practices for setting up a test server in a concurrent environment. Good use of channels for error handling and address passing.
  • 56-99: The tests for adding request and response hooks are correctly implemented, making good use of bytebufferpool for efficient string concatenation and require.Len for asserting the correct number of hooks. Parallel execution of sub-tests is appropriately handled.
  • 101-126: The test Test_Client_Add_Hook_CheckOrder correctly verifies the order of request hook execution. It efficiently uses bytebufferpool for string concatenation and require.Equal for asserting the execution order. Parallel execution is appropriately handled.
  • 128-224: The tests for custom JSON and XML marshal/unmarshal functions are well-implemented, covering both success and error scenarios. Good use of require assertions for checking outcomes and parallel execution of sub-tests.
  • 226-232: The test Test_Client_SetBaseURL correctly verifies the functionality of setting a base URL for the client. It uses require.Equal for asserting the expected value. Parallel execution is appropriately handled.
  • 234-250: The test Test_Client_Invalid_URL correctly verifies the client's error handling when provided with an invalid URL. It uses require.ErrorIs to assert the expected error type. Parallel execution is appropriately handled.
  • 252-260: The test Test_Client_Unsupported_Protocol correctly verifies the client's error handling when provided with an unsupported protocol. It uses require.ErrorIs to assert the expected error type. Parallel execution is appropriately handled.
  • 262-287: The test Test_Client_ConcurrencyRequests correctly verifies the client's ability to handle concurrent requests with different HTTP methods. It efficiently uses a wait group for synchronization and require assertions for checking outcomes.
  • 289-327: The tests for the global Get function and using the client directly for GET requests are well-implemented. Good use of utility functions for server setup and require assertions for checking outcomes. Parallel execution of sub-tests is appropriately handled.
  • 329-369: The tests for the global Head function and using the client directly for HEAD requests are correctly implemented. They efficiently check for the expected headers and empty body, using require assertions for checking outcomes. Parallel execution of sub-tests is appropriately handled.
  • 371-425: The tests for the global Post function and using the client directly for POST requests are well-implemented, correctly performing POST requests with form data. Good use of require assertions for checking outcomes and parallel execution of sub-tests.
  • 428-481: The tests for the global Put function and using the client directly for PUT requests are correctly implemented, performing PUT requests with form data. They make good use of require assertions for checking outcomes and parallel execution of sub-tests.
  • 484-540: The tests for the global Delete function and using the client directly for DELETE requests are well-implemented, correctly performing DELETE requests with form data. They efficiently use require assertions for checking outcomes and parallel execution of sub-tests.
  • 543-592: The tests for the global Options function and using the client directly for OPTIONS requests are correctly implemented, performing OPTIONS requests and checking for the expected headers. They make good use of require assertions for checking outcomes and parallel execution of sub-tests.
  • 594-649: The tests for the global Patch function and using the client directly for PATCH requests are well-implemented, correctly performing PATCH requests with form data. They efficiently use require assertions for checking outcomes and parallel execution of sub-tests.
  • 652-701: The tests for setting and using a custom User-Agent header in requests are correctly implemented, checking for the default and custom User-Agent values. They make good use of require assertions for checking outcomes and parallel execution of sub-tests.
  • 703-776: The tests for adding, setting, and manipulating headers in the client are well-implemented, including a test for case-insensitivity which is crucial given HTTP headers' case-insensitive nature. They efficiently use require assertions for checking outcomes and parallel execution of sub-tests.

@ReneWerner87
Copy link
Member

follow up ticket item :
image

same problem as when releasing the cookie pool, there is a reference somewhere instead of a copy in the cookies

this is then solved in the following ticket

@ReneWerner87 ReneWerner87 merged commit b38be4b into gofiber:main Mar 4, 2024
16 of 17 checks passed
grivera64 pushed a commit to grivera64/fiber that referenced this pull request Mar 16, 2024
* ✨ v3: Move the client module to the client folder and fix the error

* ✨ v3: add xml encoder and decoder

* 🚧 v3: design plugin and hook mechanism, complete simple get request

* 🚧 v3: reset add some field

* 🚧 v3: add doc and fix some error

* 🚧 v3: add header merge

* 🚧 v3: add query param

* 🚧 v3: change to fasthttp's header and args

* ✨ v3: add body and ua setting

* 🚧 v3: add cookie support

* 🚧 v3: add path param support

* ✅ v3: fix error test case

* 🚧 v3: add formdata and file support

* 🚧 v3: referer support

* 🚧 v3: reponse unmarshal

* ✨ v3: finish API design

* 🔥 v3: remove plugin mechanism

* 🚧 v3: add timeout

* 🚧 v3: change path params pattern and add unit test for core

* ✏️ v3: error spell

* ✅ v3: improve test coverage

* ✅ perf: change test func name to fit project format

* 🚧 v3: handle error

* 🚧 v3: add unit test and fix error

* ⚡️ chore: change func to improve performance

* ✅ v3: add some unit test

* ✅ v3: fix error test

* 🐛 fix: add cookie to response

* ✅ v3: add unit test

* ✨ v3: export raw field

* 🐛 fix: fix data race

* 🔒️ chore: change package

* 🐛 fix: data race

* 🐛 fix: test fail

* ✨ feat: move core to req

* 🐛 fix: connection reuse

* 🐛 fix: data race

* 🐛 fix: data race

* 🔀 fix: change to testify

* ✅ fix: fail test in windows

* ✨ feat: response body save to file

* ✨ feat: support tls config

* 🐛 fix: add err check

* 🎨 perf: fix some static check

* ✨ feat: add proxy support

* ✨ feat: add retry feature

* 🐛 fix: static check error

* 🎨 refactor: move som code

* docs: change readme

* ✨ feat: extend axios API

* perf: change field to export field

* ✅ chore: disable startup message

* 🐛 fix: fix test error

* chore: fix error test

* chore: fix test case

* feat: add some test to client

* chore: add test case

* chore: add test case

* ✨ feat: add peek for client

* ✅ chore: add test case

* ⚡️ feat: lazy generate rand string

* 🚧 perf: add config test case

* 🐛 fix: fix merge error

* 🐛 fix utils error

* ✨ add redirection

* 🔥 chore: delete deps

* perf: fix spell error

* 🎨 perf: spell error

* ✨ feat: add logger

* ✨ feat: add cookie jar

* ✨ feat: logger with level

* 🎨 perf: change the field name

* perf: add jar test

* fix proxy test

* improve test coverage

* fix proxy tests

* add cookiejar support from pending fasthttp PR

* fix some lint errors.

* add benchmark for SetValWithStruct

* optimize

* update

* fix proxy middleware

* use panicf instead of errorf and fix panic on default logger

* update

* update

* cleanup comments

* cleanup comments

* fix golang-lint errors

* Update helper_test.go

* add more test cases

* add hostclient pool

* make it more thread safe
-> there is still something which is shared between the requests

* fixed some golangci-lint errors

* fix Test_Request_FormData test

* create new test suite

* just create client for once

* use random port instead of 3000

* remove client pooling and fix test suite

* fix data races on logger tests

* fix proxy tests

* fix global tests

* remove unused code

* fix logger test

* fix proxy tests

* fix linter

* use lock instead of rlock

* fix cookiejar data-race

* fix(client): race conditions

* fix(client): race conditions

* apply some reviews

* change client property name

* apply review

* add parallel benchmark for simple request

* apply review

* apply review

* fix log tests

* fix linter

* fix(client): return error in SetProxyURL instead of panic

---------

Co-authored-by: Muhammed Efe Çetin <efectn@protonmail.com>
Co-authored-by: René Werner <rene.werner@verivox.com>
Co-authored-by: Joey <fenny@gofiber.io>
Co-authored-by: René <rene@gofiber.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

🚀 v3 Request: Add Throttle and Timeout with retry in client 🚀 v3 Request: Refactor Client
5 participants