🧹 chore: Update documentation for Fiber client #3249
Conversation
gaby
requested review from
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThis pull request introduces a series of documentation enhancements across multiple files within the client module. The changes primarily focus on improving the clarity and consistency of comments associated with various structs and methods, including Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3249 +/- ##
==========================================
+ Coverage 82.70% 82.72% +0.02%
==========================================
Files 115 115
Lines 11389 11365 -24
==========================================
- Hits 9419 9402 -17
+ Misses 1562 1557 -5
+ Partials 408 406 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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 (15)
client/response.go (3)
Line range hint
52-82: Consider pre-allocating the string slice for better performance.In the Headers method, consider pre-allocating the
valsStrslice with the known capacity to avoid potential reallocations:- valsStr := make([]string, len(vals)) + valsStr := make([]string, 0, len(vals))
87-114: Consider wrapping errors for better context.The unmarshaling methods could benefit from wrapping their errors to provide more context about where the error occurred.
-func (r *Response) JSON(v any) error { - return r.client.jsonUnmarshal(r.Body(), v) +func (r *Response) JSON(v any) error { + if err := r.client.jsonUnmarshal(r.Body(), v); err != nil { + return fmt.Errorf("failed to unmarshal JSON response: %w", err) + } + return nil }
Line range hint
112-159: Consider splitting the Save method for better maintainability.The Save method handles two distinct cases (file paths and io.Writer). Consider splitting it into separate methods for better maintainability and testing:
+func (r *Response) saveToFile(path string) error { + file := filepath.Clean(path) + dir := filepath.Dir(file) + + if _, err := os.Stat(dir); err != nil { + if !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("failed to check directory: %w", err) + } + if err = os.MkdirAll(dir, 0o750); err != nil { + return fmt.Errorf("failed to create directory: %w", err) + } + } + + outFile, err := os.Create(file) + if err != nil { + return fmt.Errorf("failed to create file: %w", err) + } + defer func() { _ = outFile.Close() }() + + if _, err = io.Copy(outFile, bytes.NewReader(r.Body())); err != nil { + return fmt.Errorf("failed to write response body to file: %w", err) + } + return nil +} + +func (r *Response) saveToWriter(w io.Writer) error { + if _, err := io.Copy(w, bytes.NewReader(r.Body())); err != nil { + return fmt.Errorf("failed to write response body to io.Writer: %w", err) + } + if wc, ok := w.(io.WriteCloser); ok { + _ = wc.Close() + } + return nil +} + func (r *Response) Save(v any) error { switch p := v.(type) { case string: - file := filepath.Clean(p) - dir := filepath.Dir(file) - // Create directory if it doesn't exist - if _, err := os.Stat(dir); err != nil { - if !errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("failed to check directory: %w", err) - } - if err = os.MkdirAll(dir, 0o750); err != nil { - return fmt.Errorf("failed to create directory: %w", err) - } - } - // Create and write to file - outFile, err := os.Create(file) - if err != nil { - return fmt.Errorf("failed to create file: %w", err) - } - defer func() { _ = outFile.Close() }() - if _, err = io.Copy(outFile, bytes.NewReader(r.Body())); err != nil { - return fmt.Errorf("failed to write response body to file: %w", err) - } - return nil + return r.saveToFile(p) case io.Writer: - if _, err := io.Copy(p, bytes.NewReader(r.Body())); err != nil { - return fmt.Errorf("failed to write response body to io.Writer: %w", err) - } - defer func() { - if pc, ok := p.(io.WriteCloser); ok { - _ = pc.Close() - } - }() - return nil + return r.saveToWriter(p) default: return ErrNotSupportSaveMethod } }client/core.go (4)
Line range hint
19-44: Consider using constants for default ports.Define HTTP and HTTPS port constants for better maintainability and clarity:
+const ( + defaultHTTPPort = 80 + defaultHTTPSPort = 443 +) func addMissingPort(addr string, isTLS bool) string { n := strings.Index(addr, ":") if n >= 0 { return addr } - port := 80 - if isTLS { - port = 443 - } + port := defaultHTTPPort + if isTLS { + port = defaultHTTPSPort + } return net.JoinHostPort(addr, strconv.Itoa(port)) }
Line range hint
70-132: Consider improving context cancellation handling.The goroutine might continue running briefly after context cancellation. Consider adding a select statement in the goroutine:
go func() { + select { + case <-c.ctx.Done(): + return + default: + } + respv := fasthttp.AcquireResponse() defer func() { fasthttp.ReleaseRequest(reqv) fasthttp.ReleaseResponse(respv) }() // ... rest of the function }()
133-172: Consider wrapping hook errors for better context.Add error context to help identify which hook failed:
func (c *core) preHooks() error { c.client.mu.Lock() defer c.client.mu.Unlock() for i, f := range c.client.userRequestHooks { if err := f(c.client, c.req); err != nil { - return err + return fmt.Errorf("user request hook %d failed: %w", i, err) } } // Similar changes for other hook executions }
Line range hint
218-255: Consider adding godoc comments for error variables.Add documentation for each error variable to help users understand when these errors might occur:
var ( + // ErrTimeoutOrCancel is returned when a request times out or is cancelled ErrTimeoutOrCancel = errors.New("timeout or cancel") + // ErrURLFormat is returned when the provided URL is malformed ErrURLFormat = errors.New("the URL is incorrect") // ... similar comments for other errors )client/cookiejar.go (3)
Line range hint
37-108: Address the performance optimization TODO.There's a noted performance optimization regarding race conditions in the Reset method. Consider implementing the suggested copy-based solution:
Would you like me to help implement the copy-based solution for the Reset method to resolve the race condition?
Line range hint
109-172: Consider adding error handling for cookie parsing.The SetKeyValue and SetKeyValueBytes methods could benefit from error handling for invalid key/value pairs:
func (cj *CookieJar) SetKeyValue(host, key, value string) { + if key == "" { + return // or return an error + } c := fasthttp.AcquireCookie() c.SetKey(key) c.SetValue(value) cj.SetByHost(utils.UnsafeBytes(host), c) }
Line range hint
173-230: Consider adding error handling for cookie parsing in parseCookiesFromResp.The ParseBytes error is currently ignored. Consider handling it:
- _ = c.ParseBytes(value) //nolint:errcheck // ignore error + if err := c.ParseBytes(value); err != nil { + if created { + fasthttp.ReleaseCookie(c) + } + continue + }client/hooks.go (4)
Line range hint
411-420: Enhance comment to explain the rationale for preserving filesBody.The current comment could be more explicit about why we preserve filesBody when setting formBody. Consider updating it to explain that this prevents accidentally clearing file attachments when form data is added.
-// resetBody clears the existing body. If the current body type is filesBody and -// the new type is formBody, the formBody setting is ignored to preserve files. +// resetBody clears the existing body. When the current body type is filesBody and +// the new type is formBody, we preserve the filesBody type to prevent accidentally +// clearing file attachments when adding form data, as both can coexist in a multipart request.🧰 Tools
🪛 golangci-lint (1.62.2)
31-31: File is not
gofumpt-ed with-extra(gofumpt)
31-31: File is not
goimports-ed(goimports)
224-229: Clarify rationale for ignoring close errors.The comment should explain why it's safe to ignore close errors in this context.
- // Close errors are typically ignored. + // Close errors are ignored here as they typically indicate + // the connection was already closed or the write was already + // completed. Any critical write errors would have been caught + // during the actual write operations.
Line range hint
630-632: Convert performance optimization TODOs to tracked issues.These performance optimization suggestions should be tracked in the issue system rather than as code comments for better visibility and tracking.
Would you like me to create GitHub issues for tracking these optimization tasks:
- Implement object pooling for Fiber client and fasthttp client
- Implement pooling for request headers, cookies, query parameters, and path parameters
🧰 Tools
🪛 golangci-lint (1.62.2)
31-31: File is not
gofumpt-ed with-extra(gofumpt)
31-31: File is not
goimports-ed(goimports)
Line range hint
286-305: Enhance documentation regarding error handling in cookie parsing.The function's error handling behavior could be more explicitly documented.
-// parserResponseCookie parses the Set-Cookie headers from the response and stores them. +// parserResponseCookie parses the Set-Cookie headers from the response and stores them. +// If a cookie fails to parse, the function returns immediately with the parsing error, +// and any cookies parsed up to that point are still stored in the response.client/client.go (1)
630-632: Convert TODO comment into tracked tasks.The performance optimization suggestions should be tracked in the issue tracker for better visibility and follow-up.
Would you like me to create GitHub issues to track these optimization tasks:
- Implement object pooling for the Fiber client and fasthttp client
- Implement pooling for request headers, cookies, query parameters, and path parameters
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
client/client.go(15 hunks)client/cookiejar.go(12 hunks)client/core.go(8 hunks)client/hooks.go(9 hunks)client/request.go(18 hunks)client/response.go(7 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
client/hooks.go
31-31: File is not gofumpt-ed with -extra
(gofumpt)
31-31: File is not goimports-ed
(goimports)
client/client.go
48-48: File is not gofumpt-ed with -extra
(gofumpt)
48-48: File is not goimports-ed
(goimports)
545-545: File is not gofumpt-ed with -extra
(gofumpt)
🔇 Additional comments (11)
client/response.go (3)
Line range hint 18-38: LGTM! Clean struct definition and setter methods.
The Response struct and its core methods are well-documented and correctly implemented.
37-51: LGTM! Well-implemented status and protocol methods.
The methods provide clean wrappers around the underlying fasthttp.Response functionality.
Line range hint 161-209: LGTM! Excellent resource management implementation.
The Reset, Close, and pool-related methods are well-implemented with proper cleanup and documentation.
client/core.go (2)
Line range hint 45-69: LGTM! Well-implemented core structure and retry configuration.
The core struct and retry configuration handling are properly implemented with good documentation.
Line range hint 173-217: LGTM! Well-structured timeout and execution flow.
The timeout handling and execution flow are properly implemented with good error handling and cleanup.
client/cookiejar.go (2)
Line range hint 1-36: LGTM! Well-implemented object pooling.
The sync.Pool implementation and type assertion handling are properly implemented.
Line range hint 231-241: LGTM! Well-implemented utility function.
The searchCookieByKeyAndPath function is efficiently implemented with proper byte comparison.
client/hooks.go (1)
31-33: LGTM! Clear and accurate documentation for bit manipulation constants.
🧰 Tools
🪛 golangci-lint (1.62.2)
31-31: File is not gofumpt-ed with -extra
(gofumpt)
31-31: File is not goimports-ed
(goimports)
client/client.go (1)
27-31: LGTM! Documentation improvements enhance clarity.
The updated documentation for the Client struct clearly explains its purpose and capabilities, particularly the ability to override settings at the request level.
client/request.go (2)
1018-1022: LGTM! Clear documentation of function limitations.
The documentation for SetValWithStruct clearly states its limitations regarding nested structs, which is important for users to understand.
951-952: LGTM! Important concurrency warning added.
The added warnings about potential data races when using released objects are crucial for preventing concurrency issues.
Also applies to: 1011-1012
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: d437cc6 | Previous: 154c74d | Ratio |
|---|---|---|---|
Benchmark_Compress_Levels/Brotli_LevelBestCompression - B/op |
6 B/op |
0 B/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
|
@gaby can you check benchmark and lint |
errors are caused by a not updated bench which was also changed in main(master) as we switched from go 1.22 to 1.23 |
Let me give it a try |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
client/client.go (5)
Line range hint
27-60: Consider reorganizing struct fields for better readabilityThe Client struct's fields could be organized better by grouping related fields together. Consider this organization:
- Core fields (fasthttp, logger)
- Request-related fields (header, params, cookies, path)
- Serialization handlers (jsonMarshal, jsonUnmarshal, etc.)
- Configuration fields (baseURL, timeout, etc.)
- Hook-related fields (userRequestHooks, builtinRequestHooks, etc.)
- Synchronization and debug fields (mu, debug)
type Client struct { + // Core components logger log.CommonLogger fasthttp *fasthttp.Client + // Request-related fields header *Header params *QueryParam cookies *Cookie path *PathParam + // Serialization handlers jsonMarshal utils.JSONMarshal jsonUnmarshal utils.JSONUnmarshal xmlMarshal utils.XMLMarshal xmlUnmarshal utils.XMLUnmarshal cborMarshal utils.CBORMarshal cborUnmarshal utils.CBORUnmarshal + // Configuration cookieJar *CookieJar retryConfig *RetryConfig baseURL string userAgent string referer string timeout time.Duration + // Hooks userRequestHooks []RequestHook builtinRequestHooks []RequestHook userResponseHooks []ResponseHook builtinResponseHooks []ResponseHook + // Synchronization and debug mu sync.RWMutex debug bool }🧰 Tools
🪛 golangci-lint (1.62.2)
48-48: File is not
gofumpt-ed with-extra(gofumpt)
48-48: File is not
goimports-ed(goimports)
633-635: Enhance performance optimization comments with concrete examplesThe performance optimization comments are valuable but could be more specific. Consider expanding them with concrete implementation suggestions:
Client pooling:
var clientPool = sync.Pool{ New: func() interface{} { return New() }, }Request component pooling:
var headerPool = sync.Pool{ New: func() interface{} { return &Header{RequestHeader: &fasthttp.RequestHeader{}} }, }Would you like me to create a GitHub issue to track these performance optimizations?
544-546: Enhance Config documentation with examples and implicationsThe documentation about serialization priority could be more explicit. Consider adding examples and implications:
-// Config is used to easily set request parameters. Note that when setting a request body, -// JSON is used as the default serialization mechanism. The priority is: -// Body > FormData > File. +// Config is used to easily set request parameters. When multiple body-setting fields are provided, +// they are processed in the following priority order: +// 1. Body: Serialized as JSON by default +// Example: {Body: map[string]string{"key": "value"}} +// 2. FormData: Sent as application/x-www-form-urlencoded +// Example: {FormData: map[string]string{"field": "value"}} +// 3. File: Sent as multipart/form-data +// Example: {File: []*File{{Name: "file", Path: "path/to/file"}}} +// +// Note: Only the highest priority field will be processed, others will be ignored.
Line range hint
619-619: Remove or utilize the defaultUserAgent variableThe
defaultUserAgentvariable is defined but never used in the code. Either remove it or set it as the default user agent in theNew()function.-var defaultUserAgent = "fiber"Or utilize it:
func New() *Client { - return NewWithClient(&fasthttp.Client{}) + client := NewWithClient(&fasthttp.Client{}) + client.SetUserAgent(defaultUserAgent) + return client }
Line range hint
674-689: Document thread-safety implications of Replace functionThe
Replacefunction uses a global mutex to ensure thread-safe replacement of the default client. This should be documented clearly.-// Replace replaces the defaultClient with a new one, returning a function to restore the old client. +// Replace safely replaces the defaultClient with a new one in a thread-safe manner, +// returning a function that can be used to restore the previous client. +// +// Example usage: +// +// restore := Replace(newClient) +// defer restore() // Ensures the original client is restored
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/client.go(15 hunks)client/request.go(18 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/request.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
client/client.go
48-48: File is not gofumpt-ed with -extra
(gofumpt)
48-48: File is not goimports-ed
(goimports)
548-548: File is not gofumpt-ed with -extra
(gofumpt)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
docs/client/request.md (3)
106-110: Consider adding error handling details to Custom method documentation.The documentation should specify what errors might be returned, particularly for invalid HTTP methods.
696-700: Add details about JSON marshaling behavior.The SetJSON documentation should mention:
- How marshaling errors are handled
- Whether it automatically sets the Content-Type header
- Any limitations on the types that can be marshaled
178-186: Consider rewording to avoid repetition.The current text has three successive sentences beginning with "Context". Consider rewording to improve readability:
-## Context - -**Context** returns the `context.Context` of the request, or `context.Background()` if none is set. +## Request Context + +**Context** returns the `context.Context` of the request, or `context.Background()` if none is set.🧰 Tools
🪛 LanguageTool
[style] ~178-~178: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...`` ## Context Context returns thecontext.Contextof the request, or `context.Background...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/client/request.md(18 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/client/request.md
[style] ~178-~178: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...`` ## Context Context returns the context.Context of the request, or `context.Background...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (2)
docs/client/request.md (2)
9-17: LGTM! Clear and well-structured introduction.
The introduction effectively explains the Request structure and its components using clear, technical language and well-organized bullet points.
Line range hint 1-1433: Documentation is comprehensive and well-structured.
The documentation provides thorough coverage of the Request API with clear explanations and helpful examples. The suggested improvements are minor and don't impact the overall quality of the documentation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
docs/client/hooks.md (5)
50-50: Consider enhancing the example's error handling documentation.While the example is clear, it would be beneficial to document best practices for error handling in hooks, especially since errors interrupt the request chain.
- // Add a request hook that modifies the request URL before sending. + // Add a request hook that modifies the request URL before sending. + // Note: If this hook returns an error, the request will be interrupted + // and the error will be returned immediately.
95-97: Consider adding usage examples for built-in hooks.The documentation of built-in hooks would be more helpful with small code snippets demonstrating their typical usage patterns.
201-202: Consider adding configuration details for built-in hooks.The logger hook documentation would benefit from information about:
- How to configure the logger
- Available logging options
- Example of customizing the logging format
254-255: Consider adding performance considerations.While the FIFO order explanation is clear, it would be helpful to add a note about:
- Performance impact of multiple hooks
- Best practices for hook chain optimization
- Recommendations for the maximum number of hooks
Line range hint
1-256: Overall documentation looks great! 👍The documentation is well-structured, clear, and provides practical examples. Consider these future improvements:
- Add a troubleshooting section
- Include more real-world use cases
- Add a section on hook composition patterns
docs/client/rest.md (2)
Line range hint
37-42: Consider enhancing error handling in the example.While the example demonstrates basic usage, it could better showcase error handling best practices instead of using
panic.Consider updating the example to handle errors more gracefully:
- if err != nil { - panic(err) - } + if err != nil { + fmt.Printf("Error making request: %v\n", err) + return + }
844-844: Enhance concurrent modification warning.The caution about concurrent modification could be more specific about potential risks and consequences.
Consider expanding the caution message:
-Do not modify the default client concurrently. +Do not modify the default client concurrently as it may lead to race conditions and undefined behavior. If you need concurrent modifications, create separate client instances.docs/client/response.md (4)
11-13: Maintain consistent status code formatting.For better readability and consistency, consider including the numeric status codes alongside their text representations:
-- **Status Code**: The HTTP status code returned by the server (e.g., `200 OK`, `404 Not Found`). ++ **Status Code**: The HTTP status code returned by the server (e.g., `200` for "OK", `404` for "Not Found").
20-22: Add field-level documentation.Consider adding documentation for each field in the struct definition to explain their purposes:
type Response struct { - client *Client - request *Request - cookie []*fasthttp.Cookie - RawResponse *fasthttp.Response + // client holds a reference to the HTTP client that created this response + client *Client + // request stores the original request that generated this response + request *Request + // cookie stores any cookies received in the response + cookie []*fasthttp.Cookie + // RawResponse provides access to the underlying fasthttp Response + RawResponse *fasthttp.Response }
97-98: Clarify maps package import requirement.The documentation mentions
maps.Collect()but doesn't specify the required import. Consider adding:-- **Headers** returns an iterator over all response headers. Use `maps.Collect()` to convert them into a map if desired. ++ **Headers** returns an iterator over all response headers. Use `maps.Collect()` from the `golang.org/x/exp/maps` package to convert them into a map if desired.
260-261: Enhance Save method documentation with error handling examples.The Save method documentation would benefit from examples showing common error scenarios and their handling:
- File permission issues
- Directory creation failures
- IO errors during writing
Consider adding an example that demonstrates proper error handling:
resp, err := client.Get("https://example.com/file") if err != nil { log.Fatal(err) } if err := resp.Save("path/to/file.txt"); err != nil { switch { case os.IsPermission(err): log.Fatal("Permission denied") case os.IsNotExist(err): log.Fatal("Directory does not exist") default: log.Fatal("Failed to save file:", err) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/client.go(15 hunks)client/hooks.go(9 hunks)docs/client/hooks.md(6 hunks)docs/client/response.md(10 hunks)docs/client/rest.md(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/hooks.go
- client/client.go
🔇 Additional comments (4)
docs/client/hooks.md (1)
5-5: LGTM! Clear and comprehensive introduction.
The updated introduction effectively explains the purpose of hooks and provides clear, practical use cases.
Also applies to: 9-14
docs/client/rest.md (3)
10-10: LGTM! Clear and comprehensive introduction.
The introduction and features section effectively communicates the client's capabilities and key features. The content is well-structured and accurately represents the client's functionality.
Also applies to: 14-17
68-77: LGTM! Clear distinction between hook types.
The documentation clearly differentiates between user-defined and built-in hooks, making it easier for developers to understand the hook system.
Line range hint 1-844: Overall documentation quality is excellent.
The documentation is comprehensive, well-structured, and effectively communicates the client's capabilities. The updates improve clarity while maintaining technical accuracy.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
gaby
changed the title
Description
ChatGPT - o1 proChanges introduced
Type of change