client: fix SetProxyURL functionality #3109
Conversation
efectn
requested review from
gaby,
sixcolors and
ReneWerner87
and removed request for
a team
WalkthroughThe changes involve a substantial refactoring of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy
participant Server
Client->>Proxy: SetProxyURL(proxyURL)
Proxy-->>Client: Proxy set successfully
Client->>Proxy: Send request
Proxy->>Server: Forward request
Server-->>Proxy: Return response
Proxy-->>Client: Return response
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3109 +/- ##
==========================================
+ Coverage 80.77% 80.82% +0.04%
==========================================
Files 116 116
Lines 8850 8846 -4
==========================================
+ Hits 7149 7150 +1
+ Misses 1302 1299 -3
+ Partials 399 397 -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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
Files selected for processing (2)
- client/client.go (5 hunks)
- client/client_test.go (3 hunks)
Additional context used
golangci-lint
client/client_test.go
1581-1581: go-require: require must only be used in the goroutine running the test function
(testifylint)
GitHub Check: lint
client/client_test.go
[failure] 1581-1581:
go-require: require must only be used in the goroutine running the test function (testifylint)
Additional comments not posted (2)
client/client.go (1)
227-229: Efficient proxy handling withfasthttpproxy.FasthttpHTTPDialer.The update to use
fasthttpproxy.FasthttpHTTPDialerin theSetProxyURLmethod simplifies proxy configuration and enhances efficiency by leveraging the external library's capabilities.client/client_test.go (1)
Line range hint
1543-1614: Effective proxy functionality testing.The test case now correctly verifies the proxy functionality by using a proxy server and checking the response for the "isProxy" header.
Tools
golangci-lint
1581-1581: go-require: require must only be used in the goroutine running the test function
(testifylint)
GitHub Check: lint
[failure] 1581-1581:
go-require: require must only be used in the goroutine running the test function (testifylint)
|
Code looks good. @efectn can you fix the lint error |
|
Nvm i will do it, its just one word change |
|
@efectn something seems to be not thread safe |
| } | ||
|
|
||
| // Create a simple proxy sever | ||
| proxyServer := fiber.New() |
There was a problem hiding this comment.
This is probably creating a server in a port already used
There was a problem hiding this comment.
Could we use a InmemoryListener for tests?
There was a problem hiding this comment.
@efectn can you check this
There was a problem hiding this comment.
Could we use a InmemoryListener for tests?
i dont't think we can, since FasthttpHTTPDialer only accepts a valid proxy url at ip:port format
However, i'll randomize de port instead of using fixed port
efectn
force-pushed
the
v3-client-proxy
branch
from
37d5b47 to
841a100
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
client/client_test.go (1)
Line range hint
1543-1618: Improve robustness and reliability of the test case.The updated test case correctly verifies the proxy functionality by checking the "isProxy" header. However, consider the following improvements:
- Use dynamic port allocation for the proxy server to avoid potential conflicts.
- Ensure proper cleanup and shutdown of the proxy server to prevent resource leaks.
Apply this diff to use dynamic port allocation and ensure proper cleanup:
- go func() { - assert.NoError(t, proxyServer.Listen(":0", fiber.ListenConfig{ - DisableStartupMessage: true, - ListenerAddrFunc: func(addr net.Addr) { - addrChan <- addr.String() - }, - })) - }() + listener, err := net.Listen("tcp", "127.0.0.1:0") // Dynamic port allocation + require.NoError(t, err) + defer listener.Close() + go func() { + assert.NoError(t, proxyServer.Listener(listener, fiber.ListenConfig{ + DisableStartupMessage: true, + })) + }() + addrChan <- listener.Addr().String()
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- client/client.go (5 hunks)
- client/client_test.go (3 hunks)
Additional comments not posted (3)
client/client.go (2)
21-21: LGTM!The import statement for
fasthttpproxyis correct and necessary for the updatedSetProxyURLmethod.
225-227: LGTM!The
SetProxyURLmethod correctly usesfasthttpproxy.FasthttpHTTPDialerto set the proxy URL, enhancing the efficiency of proxy handling.client/client_test.go (1)
22-22: LGTM!The import statement for
fasthttpis correct and necessary for the updated test case.
Description
Fixes #3085
Changes introduced
Type of change
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md