From 0ca53d16cc8c629edf7e0d8e2add8de0b687376e Mon Sep 17 00:00:00 2001 From: BoxBoxJason Date: Tue, 2 Jun 2026 01:12:45 +0200 Subject: [PATCH 1/2] feat: apply a default request timeout to the managed HTTP client Signed-off-by: BoxBoxJason --- sonar/client.go | 93 ++++++++++++++++++++++++++---------- sonar/client_timeout_test.go | 78 ++++++++++++++++++++++++++++++ sonar/middleware_test.go | 5 +- sonar/transport_test.go | 7 ++- 4 files changed, 155 insertions(+), 28 deletions(-) create mode 100644 sonar/client_timeout_test.go diff --git a/sonar/client.go b/sonar/client.go index da9ba4e..c11fd56 100644 --- a/sonar/client.go +++ b/sonar/client.go @@ -11,10 +11,17 @@ import ( "net/url" "strings" "sync" + "time" "github.com/google/go-querystring/query" ) +// defaultHTTPTimeout is the request timeout applied to the SDK-managed HTTP +// client when the caller does not supply their own *http.Client. It bounds the +// total time of a request (connection, redirects and body read) so a stalled +// connection cannot hang a goroutine indefinitely. +const defaultHTTPTimeout = 30 * time.Second + // ============================================= // TYPES AND STRUCTS // ============================================= @@ -44,6 +51,7 @@ type Client struct { transportConfig *TransportConfig middlewares []Middleware userAgent string + timeout time.Duration Applications *ApplicationsService AuditLogs *AuditLogsService @@ -227,45 +235,63 @@ func setDefaults(client *Client) error { } if client.httpClient == nil { - if client.transportConfig != nil { - //nolint:exhaustruct // Timeout, Jar, CheckRedirect intentionally left at zero values - client.httpClient = &http.Client{Transport: buildTransport(*client.transportConfig)} - } else { - client.httpClient = http.DefaultClient - } + client.httpClient = newDefaultHTTPClient(client.timeout, client.transportConfig) } - // Middleware is applied first so that retry sits outermost. Each retry attempt - // therefore passes through the full middleware chain, letting middleware observe - // every individual attempt rather than just the first one. - if len(client.middlewares) > 0 { - baseTransport := client.httpClient.Transport - if baseTransport == nil { - baseTransport = http.DefaultTransport - } + applyTransportWrappers(client) + + if client.userAgent == "" { + client.userAgent = buildUserAgent() + } + + return nil +} + +// newDefaultHTTPClient builds the SDK-managed HTTP client used when the caller +// does not supply their own via WithHTTPClient. A fresh *http.Client is created +// (rather than reusing http.DefaultClient) so the timeout never leaks onto the +// shared global client. A nil Transport falls back to http.DefaultTransport at +// request time. +func newDefaultHTTPClient(timeout time.Duration, cfg *TransportConfig) *http.Client { + if timeout == 0 { + timeout = defaultHTTPTimeout + } + + var transport http.RoundTripper + if cfg != nil { + transport = buildTransport(*cfg) + } + + //nolint:exhaustruct // Jar and CheckRedirect intentionally left at zero values + return &http.Client{Transport: transport, Timeout: timeout} +} - transport := applyMiddlewares(baseTransport, client.middlewares) +// applyTransportWrappers wraps the client transport with middleware and retry. +// Middleware is applied first so that retry sits outermost: each retry attempt +// therefore passes through the full middleware chain, letting middleware observe +// every individual attempt rather than just the first one. +func applyTransportWrappers(client *Client) { + if len(client.middlewares) > 0 { clone := *client.httpClient - clone.Transport = transport + clone.Transport = applyMiddlewares(baseTransport(client.httpClient), client.middlewares) client.httpClient = &clone } if client.retryOptions != nil { - baseTransport := client.httpClient.Transport - if baseTransport == nil { - baseTransport = http.DefaultTransport - } - clone := *client.httpClient - clone.Transport = &retryRoundTripper{base: baseTransport, opts: *client.retryOptions} + clone.Transport = &retryRoundTripper{base: baseTransport(client.httpClient), opts: *client.retryOptions} client.httpClient = &clone } +} - if client.userAgent == "" { - client.userAgent = buildUserAgent() +// baseTransport returns the client's transport, falling back to +// http.DefaultTransport when none is set. +func baseTransport(httpClient *http.Client) http.RoundTripper { + if httpClient.Transport != nil { + return httpClient.Transport } - return nil + return http.DefaultTransport } // WithToken is a ClientOptionFunc that sets the token for private token authentication. @@ -343,6 +369,23 @@ func WithUserAgent(userAgent string) ClientOptionFunc { } } +// WithTimeout is a ClientOptionFunc that sets the request timeout on the +// SDK-managed HTTP client. It bounds the total duration of each request. A zero +// value keeps the default of defaultHTTPTimeout; pass a custom *http.Client via +// WithHTTPClient if you need to disable the timeout entirely. It is ignored when +// WithHTTPClient is also used. +func WithTimeout(timeout time.Duration) ClientOptionFunc { + return func(c *Client) error { + if timeout < 0 { + return fmt.Errorf("WithTimeout: timeout must not be negative, got %s", timeout) + } + + c.timeout = timeout + + return nil + } +} + // ============================================= // SETTERS // ============================================= diff --git a/sonar/client_timeout_test.go b/sonar/client_timeout_test.go new file mode 100644 index 0000000..50ffb57 --- /dev/null +++ b/sonar/client_timeout_test.go @@ -0,0 +1,78 @@ +package sonar + +import ( + "net/http" + "testing" + "time" +) + +func TestNewClient_DefaultTimeout(t *testing.T) { + t.Parallel() + + client, err := NewClient(nil) + if err != nil { + t.Fatalf("NewClient returned error: %v", err) + } + + if client.httpClient.Timeout != defaultHTTPTimeout { + t.Errorf("expected default timeout %s, got %s", defaultHTTPTimeout, client.httpClient.Timeout) + } +} + +func TestWithTimeout(t *testing.T) { + t.Parallel() + + const custom = 5 * time.Second + + client, err := NewClient(nil, WithTimeout(custom)) + if err != nil { + t.Fatalf("NewClient returned error: %v", err) + } + + if client.httpClient.Timeout != custom { + t.Errorf("expected timeout %s, got %s", custom, client.httpClient.Timeout) + } +} + +func TestWithTimeout_Negative(t *testing.T) { + t.Parallel() + + _, err := NewClient(nil, WithTimeout(-time.Second)) + if err == nil { + t.Fatal("expected error for negative timeout, got nil") + } +} + +func TestNewClient_TransportConfigKeepsDefaultTimeout(t *testing.T) { + t.Parallel() + + client, err := NewClient(nil, WithTransportConfig(TransportConfig{MaxIdleConns: 10})) //nolint:exhaustruct + if err != nil { + t.Fatalf("NewClient returned error: %v", err) + } + + if client.httpClient.Timeout != defaultHTTPTimeout { + t.Errorf("expected default timeout %s, got %s", defaultHTTPTimeout, client.httpClient.Timeout) + } + + if client.httpClient.Transport == nil { + t.Error("expected transport to be configured from TransportConfig") + } +} + +func TestWithHTTPClient_TimeoutNotOverridden(t *testing.T) { + t.Parallel() + + const custom = 2 * time.Second + //nolint:exhaustruct // only Timeout is relevant for this test + provided := &http.Client{Timeout: custom} + + client, err := NewClient(nil, WithHTTPClient(provided), WithTimeout(time.Minute)) + if err != nil { + t.Fatalf("NewClient returned error: %v", err) + } + + if client.httpClient.Timeout != custom { + t.Errorf("caller-provided client timeout should be preserved: expected %s, got %s", custom, client.httpClient.Timeout) + } +} diff --git a/sonar/middleware_test.go b/sonar/middleware_test.go index d298200..696c95f 100644 --- a/sonar/middleware_test.go +++ b/sonar/middleware_test.go @@ -101,14 +101,15 @@ func TestWithMiddleware_WithHTTPClient(t *testing.T) { } // TestWithMiddleware_NoMiddleware_DefaultUnchanged verifies that when no middleware is -// provided the client uses http.DefaultClient without wrapping it. +// provided the client transport is left unwrapped (nil, falling back to +// http.DefaultTransport at request time). func TestWithMiddleware_NoMiddleware_DefaultUnchanged(t *testing.T) { t.Parallel() client, err := NewClient(nil) require.NoError(t, err) - assert.Same(t, http.DefaultClient, client.httpClient, "httpClient should be http.DefaultClient when no middleware is provided") + assert.Nil(t, client.httpClient.Transport, "transport should be unwrapped when no middleware is provided") } // roundTripFunc is a convenience type that implements http.RoundTripper via a function. diff --git a/sonar/transport_test.go b/sonar/transport_test.go index b1841de..2c7b9fb 100644 --- a/sonar/transport_test.go +++ b/sonar/transport_test.go @@ -58,7 +58,12 @@ func TestWithTransportConfig_DefaultUnchanged(t *testing.T) { require.NoError(t, err) require.NotNil(t, client) - assert.Same(t, http.DefaultClient, client.httpClient, "expected http.DefaultClient when no options are provided") + // The default client is a dedicated *http.Client (not the shared + // http.DefaultClient) with the default timeout and no explicit transport + // (it falls back to http.DefaultTransport at request time). + assert.NotSame(t, http.DefaultClient, client.httpClient, "expected a dedicated http.Client, not the shared default") + assert.Equal(t, defaultHTTPTimeout, client.httpClient.Timeout, "expected the default timeout to be applied") + assert.Nil(t, client.httpClient.Transport, "expected no explicit transport when no options are provided") } func TestWithTransportConfig_TLSConfig(t *testing.T) { From 332795a5fde048253cc598048f1c50dcc82d3cf7 Mon Sep 17 00:00:00 2001 From: BoxBoxJason Date: Sat, 13 Jun 2026 12:39:30 +0200 Subject: [PATCH 2/2] feat: enable Timeout selection in ClientCreateOptions Signed-off-by: BoxBoxJason --- README.md | 26 +++++++++++++++++++++++++- sonar/client.go | 11 +++++++++++ sonar/client_timeout_test.go | 26 ++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 19ccf13..45fe550 100644 --- a/README.md +++ b/README.md @@ -294,11 +294,35 @@ client, err := sonar.NewClient(&sonar.ClientCreateOptions{ ### Advanced Usage +**Custom request timeout:** + +The SDK-managed HTTP client applies a 30-second default timeout. Use `WithTimeout` to override it: + +```go +import "time" + +client, err := sonar.NewClient( + &sonar.ClientCreateOptions{URL: &url, Token: &token}, + sonar.WithTimeout(60*time.Second), +) +``` + +Or pass it directly in `ClientCreateOptions`: + +```go +timeout := 60 * time.Second + +client, err := sonar.NewClient(&sonar.ClientCreateOptions{ + URL: &url, + Token: &token, + Timeout: &timeout, +}) +``` + **Custom HTTP client:** ```go import "net/http" -import "time" httpClient := &http.Client{Timeout: 60 * time.Second} diff --git a/sonar/client.go b/sonar/client.go index c11fd56..b34419b 100644 --- a/sonar/client.go +++ b/sonar/client.go @@ -144,6 +144,10 @@ type ClientCreateOptions struct { HttpClient *http.Client // UserAgent is the User-Agent header to use for API requests. UserAgent *string + // Timeout sets the request timeout on the SDK-managed HTTP client. + // Ignored when HttpClient is also set. A zero value keeps the default of + // defaultHTTPTimeout. + Timeout *time.Duration } // ClientOptionFunc can be used to customize a new SonarQube API client. @@ -208,6 +212,13 @@ func applyCreateOptions(client *Client, createOpts *ClientCreateOptions) error { assignPtrIfNotNil(&client.httpClient, createOpts.HttpClient) assignIfNotNil(&client.userAgent, createOpts.UserAgent) + if createOpts.Timeout != nil { + err := WithTimeout(*createOpts.Timeout)(client) + if err != nil { + return err + } + } + return nil } diff --git a/sonar/client_timeout_test.go b/sonar/client_timeout_test.go index 50ffb57..5b57101 100644 --- a/sonar/client_timeout_test.go +++ b/sonar/client_timeout_test.go @@ -60,6 +60,32 @@ func TestNewClient_TransportConfigKeepsDefaultTimeout(t *testing.T) { } } +func TestClientCreateOptions_Timeout(t *testing.T) { + t.Parallel() + + custom := 7 * time.Second + + client, err := NewClient(&ClientCreateOptions{Timeout: &custom}) //nolint:exhaustruct + if err != nil { + t.Fatalf("NewClient returned error: %v", err) + } + + if client.httpClient.Timeout != custom { + t.Errorf("expected timeout %s, got %s", custom, client.httpClient.Timeout) + } +} + +func TestClientCreateOptions_Timeout_Negative(t *testing.T) { + t.Parallel() + + neg := -time.Second + + _, err := NewClient(&ClientCreateOptions{Timeout: &neg}) //nolint:exhaustruct + if err == nil { + t.Fatal("expected error for negative timeout, got nil") + } +} + func TestWithHTTPClient_TimeoutNotOverridden(t *testing.T) { t.Parallel()