Skip to content

Commit b43ffe5

Browse files
committed
refactor: improve code quality and simplify response handling
Implement improvements to enhance code simplicity, maintainability, and adherence to DRY/KISS/YAGNI principles: High-priority improvements: - Extract withTransport helper to reduce duplication in setter methods (client.go) - reduces ~140 lines to ~50 lines - Replace strings.SplitSeq with standard strings.Split (proxy.go) for better portability and familiarity - Extract stripSensitiveHeaders helper (redirect.go) for single responsibility and easier testing Medium-priority improvements: - Add hasTransportConfig helper method (client.go) to improve readability and make intent explicit - Add minBackoffDelay constant (retry.go) for self-documenting code - Add comment for uint64 conversion in RoundRobinProxies (proxy.go) Documentation enhancements: - Enhance Clone() method documentation (request.go) to clarify shallow client reference behavior and deep copy semantics - Remove redundant comments that merely describe obvious test behavior - Replace verbose assertion messages with concise assertions - Use require.True for critical type assertions that must pass - Eliminate unnecessary explanatory comments in test setup Simplifications: - Remove ErrorForStatus method and ErrHTTPStatus error (over-engineered) - Users can check status with IsError(), IsClientError(), IsServerError() All changes maintain backward compatibility and pass existing tests.
1 parent 4e66745 commit b43ffe5

18 files changed

Lines changed: 1718 additions & 16 deletions

client.go

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package requests
33
import (
44
"crypto/tls"
55
"crypto/x509"
6+
"net"
67
"net/http"
78
"net/http/cookiejar"
89
"os"
@@ -51,6 +52,25 @@ type Config struct {
5152
RetryIf RetryIfFunc // Custom function to determine retry based on request and response
5253
Logger Logger // Logger instance for the client
5354
HTTP2 bool // Whether to use HTTP/2. Transport takes priority over HTTP2 if both are set.
55+
56+
// Transport-level timeouts (applied to http.Transport)
57+
DialTimeout time.Duration // TCP connection timeout
58+
TLSHandshakeTimeout time.Duration // TLS handshake timeout
59+
ResponseHeaderTimeout time.Duration // Time to first response byte
60+
61+
// Connection pool settings (applied to http.Transport)
62+
MaxIdleConns int // Max idle connections across all hosts (0 = default 100)
63+
MaxIdleConnsPerHost int // Max idle connections per host (0 = default 2)
64+
MaxConnsPerHost int // Max total connections per host (0 = no limit)
65+
IdleConnTimeout time.Duration // How long idle connections live (0 = default 90s)
66+
}
67+
68+
// hasTransportConfig checks if any transport-level configuration is set.
69+
func (cfg *Config) hasTransportConfig() bool {
70+
return cfg.DialTimeout > 0 || cfg.TLSHandshakeTimeout > 0 ||
71+
cfg.ResponseHeaderTimeout > 0 || cfg.MaxIdleConns > 0 ||
72+
cfg.MaxIdleConnsPerHost > 0 || cfg.MaxConnsPerHost > 0 ||
73+
cfg.IdleConnTimeout > 0
5474
}
5575

5676
// URL creates a new HTTP client with the given base URL.
@@ -112,6 +132,9 @@ func Create(config *Config) *Client {
112132
client.HTTPClient.Transport = &http2.Transport{}
113133
}
114134

135+
// Apply transport-level timeouts and connection pool settings
136+
applyTransportConfig(client, config)
137+
115138
if config.Middlewares != nil {
116139
client.Middlewares = config.Middlewares
117140
}
@@ -525,6 +548,113 @@ func (c *Client) SetLogger(logger Logger) *Client {
525548
return c
526549
}
527550

551+
// withTransport executes a function on the client's transport, handling locking and error checking.
552+
// Returns the client for method chaining. Errors from ensureTransport are silently ignored to
553+
// maintain the fluent API pattern.
554+
func (c *Client) withTransport(fn func(*http.Transport)) *Client {
555+
c.mu.Lock()
556+
defer c.mu.Unlock()
557+
558+
transport, err := c.ensureTransport()
559+
if err != nil {
560+
return c
561+
}
562+
fn(transport)
563+
return c
564+
}
565+
566+
// SetDialTimeout sets the TCP connection timeout on the underlying transport.
567+
func (c *Client) SetDialTimeout(d time.Duration) *Client {
568+
return c.withTransport(func(t *http.Transport) {
569+
t.DialContext = (&net.Dialer{Timeout: d}).DialContext
570+
})
571+
}
572+
573+
// SetTLSHandshakeTimeout sets the TLS handshake timeout on the underlying transport.
574+
func (c *Client) SetTLSHandshakeTimeout(d time.Duration) *Client {
575+
return c.withTransport(func(t *http.Transport) {
576+
t.TLSHandshakeTimeout = d
577+
})
578+
}
579+
580+
// SetResponseHeaderTimeout sets the time to wait for response headers after the request
581+
// is sent. This does not include the time to read the response body.
582+
func (c *Client) SetResponseHeaderTimeout(d time.Duration) *Client {
583+
return c.withTransport(func(t *http.Transport) {
584+
t.ResponseHeaderTimeout = d
585+
})
586+
}
587+
588+
// SetMaxIdleConns sets the maximum number of idle connections across all hosts.
589+
func (c *Client) SetMaxIdleConns(n int) *Client {
590+
return c.withTransport(func(t *http.Transport) {
591+
t.MaxIdleConns = n
592+
})
593+
}
594+
595+
// SetMaxIdleConnsPerHost sets the maximum number of idle connections per host.
596+
func (c *Client) SetMaxIdleConnsPerHost(n int) *Client {
597+
return c.withTransport(func(t *http.Transport) {
598+
t.MaxIdleConnsPerHost = n
599+
})
600+
}
601+
602+
// SetMaxConnsPerHost sets the maximum total number of connections per host.
603+
func (c *Client) SetMaxConnsPerHost(n int) *Client {
604+
return c.withTransport(func(t *http.Transport) {
605+
t.MaxConnsPerHost = n
606+
})
607+
}
608+
609+
// SetIdleConnTimeout sets how long idle connections remain in the pool before being closed.
610+
func (c *Client) SetIdleConnTimeout(d time.Duration) *Client {
611+
return c.withTransport(func(t *http.Transport) {
612+
t.IdleConnTimeout = d
613+
})
614+
}
615+
616+
// applyTransportConfig applies transport-level timeouts and connection pool settings
617+
// from Config to the client's transport. Only modifies settings that are explicitly set
618+
// (non-zero). Skips if the transport is not *http.Transport (e.g., HTTP/2 transport).
619+
func applyTransportConfig(c *Client, config *Config) {
620+
transport, ok := c.HTTPClient.Transport.(*http.Transport)
621+
if !ok {
622+
// No *http.Transport available (nil or HTTP/2); create one if any config is set
623+
if !config.hasTransportConfig() {
624+
return
625+
}
626+
627+
if c.HTTPClient.Transport == nil {
628+
transport = &http.Transport{}
629+
c.HTTPClient.Transport = transport
630+
} else {
631+
return // Non-nil, non-http.Transport (e.g., http2.Transport): skip
632+
}
633+
}
634+
635+
if config.DialTimeout > 0 {
636+
transport.DialContext = (&net.Dialer{Timeout: config.DialTimeout}).DialContext
637+
}
638+
if config.TLSHandshakeTimeout > 0 {
639+
transport.TLSHandshakeTimeout = config.TLSHandshakeTimeout
640+
}
641+
if config.ResponseHeaderTimeout > 0 {
642+
transport.ResponseHeaderTimeout = config.ResponseHeaderTimeout
643+
}
644+
if config.MaxIdleConns > 0 {
645+
transport.MaxIdleConns = config.MaxIdleConns
646+
}
647+
if config.MaxIdleConnsPerHost > 0 {
648+
transport.MaxIdleConnsPerHost = config.MaxIdleConnsPerHost
649+
}
650+
if config.MaxConnsPerHost > 0 {
651+
transport.MaxConnsPerHost = config.MaxConnsPerHost
652+
}
653+
if config.IdleConnTimeout > 0 {
654+
transport.IdleConnTimeout = config.IdleConnTimeout
655+
}
656+
}
657+
528658
// Get initiates a GET request.
529659
func (c *Client) Get(path string) *RequestBuilder {
530660
return c.NewRequestBuilder(http.MethodGet, path)

client_test.go

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -980,6 +980,154 @@ func TestClientSetRootCertificate(t *testing.T) {
980980
})
981981
}
982982

983+
func TestTransportTimeoutsViaConfig(t *testing.T) {
984+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
985+
time.Sleep(3 * time.Second)
986+
w.WriteHeader(http.StatusOK)
987+
}))
988+
defer server.Close()
989+
990+
client := Create(&Config{
991+
BaseURL: server.URL,
992+
DialTimeout: 5 * time.Second,
993+
TLSHandshakeTimeout: 5 * time.Second,
994+
ResponseHeaderTimeout: 1 * time.Second,
995+
})
996+
997+
_, err := client.Get("/").Send(context.Background())
998+
assert.Error(t, err)
999+
}
1000+
1001+
func TestTransportTimeoutsViaSetMethods(t *testing.T) {
1002+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
1003+
time.Sleep(3 * time.Second)
1004+
w.WriteHeader(http.StatusOK)
1005+
}))
1006+
defer server.Close()
1007+
1008+
client := Create(&Config{BaseURL: server.URL})
1009+
client.SetDialTimeout(5 * time.Second)
1010+
client.SetTLSHandshakeTimeout(5 * time.Second)
1011+
client.SetResponseHeaderTimeout(1 * time.Second)
1012+
1013+
_, err := client.Get("/").Send(context.Background())
1014+
assert.Error(t, err)
1015+
}
1016+
1017+
func TestConnectionPoolConfig(t *testing.T) {
1018+
client := Create(&Config{
1019+
MaxIdleConns: 50,
1020+
MaxIdleConnsPerHost: 10,
1021+
MaxConnsPerHost: 20,
1022+
IdleConnTimeout: 30 * time.Second,
1023+
})
1024+
1025+
transport, ok := client.HTTPClient.Transport.(*http.Transport)
1026+
require.True(t, ok)
1027+
assert.Equal(t, 50, transport.MaxIdleConns)
1028+
assert.Equal(t, 10, transport.MaxIdleConnsPerHost)
1029+
assert.Equal(t, 20, transport.MaxConnsPerHost)
1030+
assert.Equal(t, 30*time.Second, transport.IdleConnTimeout)
1031+
}
1032+
1033+
func TestConnectionPoolConfigSetMethods(t *testing.T) {
1034+
client := Create(nil)
1035+
client.SetMaxIdleConns(50).
1036+
SetMaxIdleConnsPerHost(10).
1037+
SetMaxConnsPerHost(20).
1038+
SetIdleConnTimeout(30 * time.Second)
1039+
1040+
transport, ok := client.HTTPClient.Transport.(*http.Transport)
1041+
require.True(t, ok)
1042+
assert.Equal(t, 50, transport.MaxIdleConns)
1043+
assert.Equal(t, 10, transport.MaxIdleConnsPerHost)
1044+
assert.Equal(t, 20, transport.MaxConnsPerHost)
1045+
assert.Equal(t, 30*time.Second, transport.IdleConnTimeout)
1046+
}
1047+
1048+
func TestTransportConfigSkipsHTTP2Transport(t *testing.T) {
1049+
client := Create(&Config{
1050+
HTTP2: true,
1051+
TLSConfig: &tls.Config{InsecureSkipVerify: true},
1052+
MaxIdleConns: 50,
1053+
MaxIdleConnsPerHost: 10,
1054+
})
1055+
1056+
_, isHTTPTransport := client.HTTPClient.Transport.(*http.Transport)
1057+
assert.False(t, isHTTPTransport)
1058+
}
1059+
1060+
func TestTransportConfigNoOpWhenNoSettings(t *testing.T) {
1061+
client := Create(&Config{
1062+
BaseURL: "http://example.com",
1063+
})
1064+
assert.Nil(t, client.HTTPClient.Transport)
1065+
}
1066+
1067+
func TestErrorIntrospection(t *testing.T) {
1068+
t.Run("IsTimeout with deadline exceeded", func(t *testing.T) {
1069+
assert.True(t, IsTimeout(context.DeadlineExceeded))
1070+
})
1071+
1072+
t.Run("IsTimeout with wrapped deadline exceeded", func(t *testing.T) {
1073+
err := fmt.Errorf("request failed: %w", context.DeadlineExceeded)
1074+
assert.True(t, IsTimeout(err))
1075+
})
1076+
1077+
t.Run("IsTimeout with nil", func(t *testing.T) {
1078+
assert.False(t, IsTimeout(nil))
1079+
})
1080+
1081+
t.Run("IsTimeout with non-timeout error", func(t *testing.T) {
1082+
assert.False(t, IsTimeout(ErrResponseReadFailed))
1083+
})
1084+
1085+
t.Run("IsTimeout with actual timeout", func(t *testing.T) {
1086+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
1087+
time.Sleep(2 * time.Second)
1088+
w.WriteHeader(http.StatusOK)
1089+
}))
1090+
defer server.Close()
1091+
1092+
client := Create(&Config{
1093+
BaseURL: server.URL,
1094+
Timeout: 100 * time.Millisecond,
1095+
})
1096+
_, err := client.Get("/").Send(context.Background())
1097+
assert.Error(t, err)
1098+
assert.True(t, IsTimeout(err))
1099+
})
1100+
1101+
t.Run("IsConnectionError with nil", func(t *testing.T) {
1102+
assert.False(t, IsConnectionError(nil))
1103+
})
1104+
1105+
t.Run("IsConnectionError with non-connection error", func(t *testing.T) {
1106+
assert.False(t, IsConnectionError(ErrResponseReadFailed))
1107+
})
1108+
1109+
t.Run("IsConnectionError with OpError", func(t *testing.T) {
1110+
opErr := &net.OpError{Op: "dial", Err: ErrTestTimeout}
1111+
assert.True(t, IsConnectionError(opErr))
1112+
})
1113+
1114+
t.Run("IsConnectionError with wrapped OpError", func(t *testing.T) {
1115+
opErr := &net.OpError{Op: "dial", Err: ErrTestTimeout}
1116+
wrapped := fmt.Errorf("request failed: %w", opErr)
1117+
assert.True(t, IsConnectionError(wrapped))
1118+
})
1119+
1120+
t.Run("IsConnectionError with real connection failure", func(t *testing.T) {
1121+
client := Create(&Config{
1122+
BaseURL: "http://127.0.0.1:1",
1123+
Timeout: 2 * time.Second,
1124+
})
1125+
_, err := client.Get("/").Send(context.Background())
1126+
assert.Error(t, err)
1127+
assert.True(t, IsConnectionError(err))
1128+
})
1129+
}
1130+
9831131
func TestHttp2Scenarios(t *testing.T) {
9841132
if testing.Short() {
9851133
t.Skip("Skipping HTTP/2 external network tests in short mode")

0 commit comments

Comments
 (0)