feat: support proxy in transport layer #3
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds HTTP/HTTPS proxy support to the httpx library, enabling clients to route requests through proxy servers with authentication. The implementation provides proxy configuration across all client types (ClientBuilder, GenericClient, and HTTPRetryClient) while maintaining backward compatibility.
Changes:
- Added proxy configuration options to all client builders with URL validation and graceful error handling
- Implemented transport cloning to avoid mutating shared transports when configuring proxies
- Added comprehensive test coverage for proxy functionality including authentication, invalid URLs, and interaction with other client options
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| http_client.go | Added proxy URL field and WithProxy method to ClientBuilder, implemented proxy configuration in Build() |
| http_retrier.go | Added proxy support to retry client with WithProxyRetry option and URL parsing logic |
| http_generic_client.go | Added proxy configuration to GenericClient with WithProxy option |
| http_client_test.go | Added test cases for ClientBuilder proxy functionality |
| http_retrier_test.go | Added comprehensive tests for retry client proxy configuration |
| http_generic_client_test.go | Added tests for GenericClient proxy support |
| example_proxy_test.go | Added example code demonstrating proxy usage across all client types |
| docs.go | Added proxy configuration documentation and examples |
| README.md | Added detailed proxy configuration section with examples and security notes |
| // WithProxy sets the proxy URL for HTTP requests. | ||
| // The proxy URL should be in the format "http://proxy.example.com:8080" or "https://proxy.example.com:8080". | ||
| // Pass an empty string to disable proxy (default behavior). | ||
| // and returns the ClientBuilder for method chaining |
There was a problem hiding this comment.
The comment "and returns the ClientBuilder for method chaining" should be part of the main description, not a separate line. Reformat to: "WithProxy sets the proxy URL for HTTP requests and returns the ClientBuilder for method chaining."
| if transport.Proxy != nil { | ||
| t.Error("Expected Proxy to be nil when not configured") | ||
| } |
There was a problem hiding this comment.
This test assumes the default transport has no proxy configured, but http.DefaultTransport may have Proxy set to http.ProxyFromEnvironment. The test should either check that Proxy equals http.ProxyFromEnvironment or skip this validation entirely since it depends on environment state.
| builder := NewClientBuilder() | ||
| invalidProxyURL := "://invalid-url" | ||
|
|
||
| client := builder.WithProxy(invalidProxyURL).Build() | ||
|
|
||
| // Should still build successfully, but proxy will be ignored | ||
| assertNotNil(t, client) | ||
|
|
||
| // Verify that the transport has no proxy configured due to parse error | ||
| if rt, ok := client.Transport.(*retryTransport); ok { | ||
| if transport, ok := rt.Transport.(*http.Transport); ok { | ||
| if transport.Proxy != nil { | ||
| t.Error("Expected Proxy to be nil when invalid URL provided") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This assertion is incorrect. When an invalid proxy URL is provided, the code logs a warning but does not set Proxy to nil—it leaves the transport with its default Proxy configuration (likely http.ProxyFromEnvironment from DefaultTransport). The test should verify that Proxy remains unchanged from the default rather than expecting nil.
| builder := NewClientBuilder() | |
| invalidProxyURL := "://invalid-url" | |
| client := builder.WithProxy(invalidProxyURL).Build() | |
| // Should still build successfully, but proxy will be ignored | |
| assertNotNil(t, client) | |
| // Verify that the transport has no proxy configured due to parse error | |
| if rt, ok := client.Transport.(*retryTransport); ok { | |
| if transport, ok := rt.Transport.(*http.Transport); ok { | |
| if transport.Proxy != nil { | |
| t.Error("Expected Proxy to be nil when invalid URL provided") | |
| } | |
| } | |
| } | |
| // Capture the default proxy configuration from a client built without any proxy. | |
| defaultClient := NewClientBuilder().Build() | |
| assertNotNil(t, defaultClient) | |
| var defaultProxyFunc any | |
| if rt, ok := defaultClient.Transport.(*retryTransport); ok { | |
| if transport, ok := rt.Transport.(*http.Transport); ok { | |
| defaultProxyFunc = transport.Proxy | |
| } | |
| } | |
| if defaultProxyFunc == nil { | |
| t.Fatal("Expected default proxy function to be non-nil") | |
| } | |
| // Now build a client with an invalid proxy URL; the proxy configuration | |
| // should remain unchanged (i.e., still the default proxy function). | |
| builder := NewClientBuilder() | |
| invalidProxyURL := "://invalid-url" | |
| client := builder.WithProxy(invalidProxyURL).Build() | |
| // Should still build successfully. | |
| assertNotNil(t, client) | |
| var actualProxyFunc any | |
| if rt, ok := client.Transport.(*retryTransport); ok { | |
| if transport, ok := rt.Transport.(*http.Transport); ok { | |
| actualProxyFunc = transport.Proxy | |
| } | |
| } | |
| if actualProxyFunc == nil { | |
| t.Fatal("Expected proxy function to remain set when invalid URL provided") | |
| } | |
| defaultPtr := reflect.ValueOf(defaultProxyFunc).Pointer() | |
| actualPtr := reflect.ValueOf(actualProxyFunc).Pointer() | |
| if defaultPtr != actualPtr { | |
| t.Error("Expected Proxy to remain unchanged when invalid URL provided") | |
| } |
| if transport, ok := rt.Transport.(*http.Transport); ok { | ||
| // Note: http.DefaultTransport may have Proxy set to use environment variables | ||
| // so we just verify the client was created successfully | ||
| _ = transport.Proxy |
There was a problem hiding this comment.
The blank identifier assignment serves no purpose here. Either remove this line or add a meaningful comment explaining why the field is being accessed but not validated.
| _ = transport.Proxy |
| if rt, ok := client.Transport.(*retryTransport); ok { | ||
| if transport, ok := rt.Transport.(*http.Transport); ok { | ||
| // Transport is created, but proxy configuration failed gracefully | ||
| _ = transport.Proxy |
There was a problem hiding this comment.
The blank identifier assignment serves no purpose here. Either remove this line or add a meaningful comment explaining why the field is being accessed but not validated.
No description provided.