From 1e9c2cad68039d52c2f0c6e15a4d04b760121741 Mon Sep 17 00:00:00 2001 From: Brad Moylan Date: Mon, 23 Sep 2024 17:02:54 -0700 Subject: [PATCH 1/5] improvement: WithAllowCreateWithEmptyURIs() suppresses errors due to empty URIs in NewClient() --- conjure-go-client/httpclient/authn_test.go | 6 ++-- .../httpclient/body_handler_test.go | 8 ++--- conjure-go-client/httpclient/client.go | 2 +- .../httpclient/client_builder.go | 24 +++++++++---- .../httpclient/client_builder_test.go | 2 +- conjure-go-client/httpclient/client_params.go | 15 ++++++++ .../httpclient/client_params_test.go | 3 +- conjure-go-client/httpclient/client_test.go | 28 +++++++-------- conjure-go-client/httpclient/close_test.go | 12 +++---- conjure-go-client/httpclient/config.go | 10 ++---- .../httpclient/config_refreshable_test.go | 36 +++++++++++++++++-- .../httpclient/error_decoder_test.go | 10 +++--- conjure-go-client/httpclient/metrics_test.go | 20 +++++------ conjure-go-client/httpclient/recovery_test.go | 2 +- conjure-go-client/httpclient/trace_test.go | 2 +- 15 files changed, 116 insertions(+), 64 deletions(-) diff --git a/conjure-go-client/httpclient/authn_test.go b/conjure-go-client/httpclient/authn_test.go index 30eb5994..48460b34 100644 --- a/conjure-go-client/httpclient/authn_test.go +++ b/conjure-go-client/httpclient/authn_test.go @@ -42,7 +42,7 @@ func TestRoundTripperWithToken(t *testing.T) { client, err := httpclient.NewClient( httpclient.WithHTTPTimeout(time.Minute), httpclient.WithAuthTokenProvider(tokenProvider), - httpclient.WithBaseURLs([]string{server.URL})) + httpclient.WithBaseURL(server.URL)) require.NoError(t, err) resp, err := client.Do(context.Background(), httpclient.WithRequestMethod(http.MethodGet)) @@ -71,7 +71,7 @@ func TestRoundTripperWithBasicAuth(t *testing.T) { client, err := httpclient.NewClient( httpclient.WithHTTPTimeout(time.Minute), httpclient.WithBasicAuth(expected.User, expected.Password), - httpclient.WithBaseURLs([]string{server.URL})) + httpclient.WithBaseURL(server.URL)) require.NoError(t, err) resp, err := client.Do(context.Background(), httpclient.WithRequestMethod(http.MethodGet)) @@ -106,7 +106,7 @@ func TestRoundTripperWithBasicAuthProvider(t *testing.T) { Password: "password", }, nil }), - httpclient.WithBaseURLs([]string{server.URL})) + httpclient.WithBaseURL(server.URL)) require.NoError(t, err) resp, err := client.Do(context.Background(), httpclient.WithRequestMethod(http.MethodGet)) diff --git a/conjure-go-client/httpclient/body_handler_test.go b/conjure-go-client/httpclient/body_handler_test.go index 5018b2fa..0ad71e46 100644 --- a/conjure-go-client/httpclient/body_handler_test.go +++ b/conjure-go-client/httpclient/body_handler_test.go @@ -48,7 +48,7 @@ func TestJSONBody(t *testing.T) { client, err := httpclient.NewClient( httpclient.WithUserAgent("TestNewRequest"), - httpclient.WithBaseURLs([]string{server.URL}), + httpclient.WithBaseURL(server.URL), ) require.NoError(t, err) @@ -80,7 +80,7 @@ func TestRawBody(t *testing.T) { client, err := httpclient.NewClient( httpclient.WithUserAgent("TestNewRequest"), - httpclient.WithBaseURLs([]string{server.URL}), + httpclient.WithBaseURL(server.URL), ) require.NoError(t, err) @@ -119,7 +119,7 @@ func TestRawRequestRetry(t *testing.T) { })) defer server.Close() - client, err := httpclient.NewClient(httpclient.WithBaseURLs([]string{server.URL})) + client, err := httpclient.NewClient(httpclient.WithBaseURL(server.URL)) assert.NoError(t, err) _, err = client.Do( @@ -154,7 +154,7 @@ func TestRedirectWithBodyAndBytesBuffer(t *testing.T) { client, err := httpclient.NewClient( httpclient.WithUserAgent("TestNewRequest"), - httpclient.WithBaseURLs([]string{server.URL}), + httpclient.WithBaseURL(server.URL), httpclient.WithBytesBufferPool(bytesbuffers.NewSizedPool(1, 10)), ) require.NoError(t, err) diff --git a/conjure-go-client/httpclient/client.go b/conjure-go-client/httpclient/client.go index 69df1d16..fe3d6076 100644 --- a/conjure-go-client/httpclient/client.go +++ b/conjure-go-client/httpclient/client.go @@ -84,7 +84,7 @@ func (c *clientImpl) Delete(ctx context.Context, params ...RequestParam) (*http. func (c *clientImpl) Do(ctx context.Context, params ...RequestParam) (*http.Response, error) { uris := c.uriScorer.CurrentURIScoringMiddleware().GetURIsInOrderOfIncreasingScore() if len(uris) == 0 { - return nil, werror.ErrorWithContextParams(ctx, "no base URIs are configured") + return nil, werror.WrapWithContextParams(ctx, ErrEmptyURIs, "") } attempts := 2 * len(uris) diff --git a/conjure-go-client/httpclient/client_builder.go b/conjure-go-client/httpclient/client_builder.go index a37f75ee..aa157a00 100644 --- a/conjure-go-client/httpclient/client_builder.go +++ b/conjure-go-client/httpclient/client_builder.go @@ -18,6 +18,7 @@ package httpclient import ( "context" "crypto/tls" + "fmt" "net/http" "time" @@ -46,12 +47,20 @@ const ( defaultMaxBackoff = 2 * time.Second ) +var ( + ErrEmptyURIs = fmt.Errorf("httpclient URLs must not be empty") +) + type clientBuilder struct { HTTP *httpClientBuilder URIs refreshable.StringSlice URIScorerBuilder func([]string) internal.URIScoringMiddleware + // If false, NewClient() will return an error when URIs.Current() is empty. + // This allows for a refreshable URI slice to be populated after construction but before use. + AllowEmptyURIs bool + ErrorDecoder ErrorDecoder BytesBufferPool bytesbuffers.Pool @@ -115,7 +124,7 @@ func NewClient(params ...ClientParam) (Client, error) { // We apply "sane defaults" before applying the provided params. func NewClientFromRefreshableConfig(ctx context.Context, config RefreshableClientConfig, params ...ClientParam) (Client, error) { b := newClientBuilder() - if err := newClientBuilderFromRefreshableConfig(ctx, config, b, nil, false); err != nil { + if err := newClientBuilderFromRefreshableConfig(ctx, config, b, nil); err != nil { return nil, err } return newClient(ctx, b, params...) @@ -131,7 +140,10 @@ func newClient(ctx context.Context, b *clientBuilder, params ...ClientParam) (Cl } } if b.URIs == nil { - return nil, werror.Error("httpclient URLs must not be empty", werror.SafeParam("serviceName", b.HTTP.ServiceNameTag.Value())) + return nil, werror.ErrorWithContextParams(ctx, "httpclient URLs must be set in configuration or by constructor param", werror.SafeParam("serviceName", b.HTTP.ServiceNameTag.Value())) + } + if !b.AllowEmptyURIs && len(b.URIs.CurrentStringSlice()) == 0 { + return nil, werror.WrapWithContextParams(ctx, ErrEmptyURIs, "", werror.SafeParam("serviceName", b.HTTP.ServiceNameTag.Value())) } var edm Middleware @@ -187,7 +199,7 @@ type RefreshableHTTPClient = refreshingclient.RefreshableHTTPClient // We apply "sane defaults" before applying the provided params. func NewHTTPClientFromRefreshableConfig(ctx context.Context, config RefreshableClientConfig, params ...HTTPClientParam) (RefreshableHTTPClient, error) { b := newClientBuilder() - if err := newClientBuilderFromRefreshableConfig(ctx, config, b, nil, true); err != nil { + if err := newClientBuilderFromRefreshableConfig(ctx, config, b, nil); err != nil { return nil, err } return b.HTTP.Build(ctx, params...) @@ -237,7 +249,7 @@ func newClientBuilder() *clientBuilder { } } -func newClientBuilderFromRefreshableConfig(ctx context.Context, config RefreshableClientConfig, b *clientBuilder, reloadErrorSubmitter func(error), isHTTPClient bool) error { +func newClientBuilderFromRefreshableConfig(ctx context.Context, config RefreshableClientConfig, b *clientBuilder, reloadErrorSubmitter func(error)) error { var err error b.HTTP.ServiceNameTag, err = metrics.NewTag(MetricTagServiceName, config.CurrentClientConfig().ServiceName) if err != nil { @@ -256,16 +268,16 @@ func newClientBuilderFromRefreshableConfig(ctx context.Context, config Refreshab } refreshingParams, err := refreshable.NewMapValidatingRefreshable(config, func(i interface{}) (interface{}, error) { - p, err := newValidatedClientParamsFromConfig(ctx, i.(ClientConfig), isHTTPClient) + p, err := newValidatedClientParamsFromConfig(ctx, i.(ClientConfig)) if reloadErrorSubmitter != nil { reloadErrorSubmitter(err) } return p, err }) - validParams := refreshingclient.NewRefreshingValidatedClientParams(refreshingParams) if err != nil { return err } + validParams := refreshingclient.NewRefreshingValidatedClientParams(refreshingParams) b.HTTP.DialerParams = validParams.Dialer() b.HTTP.TransportParams = validParams.Transport() diff --git a/conjure-go-client/httpclient/client_builder_test.go b/conjure-go-client/httpclient/client_builder_test.go index 2696e59b..de2c27df 100644 --- a/conjure-go-client/httpclient/client_builder_test.go +++ b/conjure-go-client/httpclient/client_builder_test.go @@ -47,7 +47,7 @@ func TestWrapTransport(t *testing.T) { httpclient.WithMiddleware(middleware(1)), httpclient.WithMiddleware(middleware(2)), httpclient.WithMiddleware(middleware(3)), - httpclient.WithBaseURLs([]string{server.URL}), + httpclient.WithBaseURL(server.URL), ) require.NoError(t, err) diff --git a/conjure-go-client/httpclient/client_params.go b/conjure-go-client/httpclient/client_params.go index 41c274a7..2fce18f2 100644 --- a/conjure-go-client/httpclient/client_params.go +++ b/conjure-go-client/httpclient/client_params.go @@ -450,6 +450,11 @@ func WithKeepAlive(keepAlive time.Duration) ClientOrHTTPClientParam { }) } +// WithBaseURL is a variadic-argument alias for WithBaseURLs. Any previous base URLs will be overwritten. +func WithBaseURL(urls ...string) ClientParam { + return WithBaseURLs(urls) +} + // WithBaseURLs sets the base URLs for every request. This is meant to be used in conjunction with WithPath. func WithBaseURLs(urls []string) ClientParam { return clientParamFunc(func(b *clientBuilder) error { @@ -466,6 +471,16 @@ func WithRefreshableBaseURLs(urls refreshable.StringSlice) ClientParam { }) } +// WithAllowCreateWithEmptyURIs prevents NewClient from returning an error when the URI slice is empty. +// This is useful when the URIs are not known at client creation time but will be populated by a refreshable. +// Requests will error if attempted before URIs are populated. +func WithAllowCreateWithEmptyURIs() ClientParam { + return clientParamFunc(func(b *clientBuilder) error { + b.AllowEmptyURIs = true + return nil + }) +} + // WithMaxBackoff sets the maximum backoff between retried calls to the same URI. // Defaults to 2 seconds. <= 0 indicates no limit. func WithMaxBackoff(maxBackoff time.Duration) ClientParam { diff --git a/conjure-go-client/httpclient/client_params_test.go b/conjure-go-client/httpclient/client_params_test.go index 56d20ee9..04ca1bf1 100644 --- a/conjure-go-client/httpclient/client_params_test.go +++ b/conjure-go-client/httpclient/client_params_test.go @@ -145,8 +145,7 @@ func TestBuilder(t *testing.T) { } { t.Run(test.Name, func(t *testing.T) { // Must provide URLs for client creation - urls := WithBaseURLs([]string{"https://localhost"}) - client, err := NewClient(urls, test.Param) + client, err := NewClient(WithBaseURL("https://localhost"), test.Param) require.NoError(t, err) test.Test(t, client.(*clientImpl)) }) diff --git a/conjure-go-client/httpclient/client_test.go b/conjure-go-client/httpclient/client_test.go index d16ffeff..9fcf67fe 100644 --- a/conjure-go-client/httpclient/client_test.go +++ b/conjure-go-client/httpclient/client_test.go @@ -35,7 +35,7 @@ import ( func TestNoBaseURIs(t *testing.T) { client, err := httpclient.NewClient() - require.EqualError(t, err, "httpclient URLs must not be empty") + require.EqualError(t, err, "httpclient URLs must be set in configuration or by constructor param") require.Nil(t, client) } @@ -57,7 +57,7 @@ func TestCanReadBodyWithBufferPool(t *testing.T) { client, err := httpclient.NewClient( httpclient.WithBytesBufferPool(bytesbuffers.NewSizedPool(1, 10)), - httpclient.WithBaseURLs([]string{server.URL}), + httpclient.WithBaseURL(server.URL), ) require.NoError(t, err) @@ -89,7 +89,7 @@ func TestCanUseRelocationURI(t *testing.T) { client, err := httpclient.NewClient( httpclient.WithBytesBufferPool(bytesbuffers.NewSizedPool(1, 10)), - httpclient.WithBaseURLs([]string{server.URL}), + httpclient.WithBaseURL(server.URL), ) assert.NoError(t, err) @@ -122,7 +122,7 @@ func TestCanUseSimpleRelocationURI(t *testing.T) { client, err := httpclient.NewClient( httpclient.WithBytesBufferPool(bytesbuffers.NewSizedPool(1, 10)), - httpclient.WithBaseURLs([]string{server.URL}), + httpclient.WithBaseURL(server.URL), ) assert.NoError(t, err) @@ -169,7 +169,7 @@ func TestMiddlewareCanReadBody(t *testing.T) { t.Run("NoByteBufferPool", func(t *testing.T) { client, err := httpclient.NewClient( withMiddleware, - httpclient.WithBaseURLs([]string{server.URL}), + httpclient.WithBaseURL(server.URL), ) require.NoError(t, err) resp, err := client.Do(context.Background(), httpclient.WithRequestBody(unencodedBody, codecs.Plain), httpclient.WithRequestMethod("GET")) @@ -180,7 +180,7 @@ func TestMiddlewareCanReadBody(t *testing.T) { client, err := httpclient.NewClient( httpclient.WithBytesBufferPool(bytesbuffers.NewSizedPool(1, 10)), withMiddleware, - httpclient.WithBaseURLs([]string{server.URL}), + httpclient.WithBaseURL(server.URL), ) require.NoError(t, err) resp, err := client.Do(context.Background(), httpclient.WithRequestBody(unencodedBody, codecs.Plain), httpclient.WithRequestMethod("GET")) @@ -317,7 +317,7 @@ func BenchmarkAllocWithBytesBufferPool(b *testing.B) { } b.Run("NoByteBufferPool", func(b *testing.B) { client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{server.URL}), + httpclient.WithBaseURL(server.URL), ) require.NoError(b, err) runBench(b, client) @@ -325,7 +325,7 @@ func BenchmarkAllocWithBytesBufferPool(b *testing.B) { b.Run("WithByteBufferPool", func(b *testing.B) { client, err := httpclient.NewClient( httpclient.WithBytesBufferPool(bytesbuffers.NewSizedPool(1, 10)), - httpclient.WithBaseURLs([]string{server.URL}), + httpclient.WithBaseURL(server.URL), ) require.NoError(b, err) runBench(b, client) @@ -376,42 +376,42 @@ func BenchmarkUnavailableURIs(b *testing.B) { } b.Run("OneAvailableServer", func(b *testing.B) { client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{server1.URL}), + httpclient.WithBaseURL(server1.URL), ) require.NoError(b, err) runBench(b, client) }) b.Run("FourAvailableServers", func(b *testing.B) { client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{server1.URL, server2.URL, server3.URL, server4.URL}), + httpclient.WithBaseURL(server1.URL, server2.URL, server3.URL, server4.URL), ) require.NoError(b, err) runBench(b, client) }) b.Run("OneOutOfFourUnavailableServers", func(b *testing.B) { client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{server1.URL, server2.URL, server3.URL, unavailableServer.URL}), + httpclient.WithBaseURL(server1.URL, server2.URL, server3.URL, unavailableServer.URL), ) require.NoError(b, err) runBench(b, client) }) b.Run("OneOutOfThreeUnavailableServers", func(b *testing.B) { client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{server1.URL, server2.URL, unavailableServer.URL}), + httpclient.WithBaseURL(server1.URL, server2.URL, unavailableServer.URL), ) require.NoError(b, err) runBench(b, client) }) b.Run("OneOutOfTwoUnavailableServers", func(b *testing.B) { client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{server1.URL, unavailableServer.URL}), + httpclient.WithBaseURL(server1.URL, unavailableServer.URL), ) require.NoError(b, err) runBench(b, client) }) b.Run("OneOutOfTwoUnstartedServers", func(b *testing.B) { client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{server1.URL, unstartedServer.URL}), + httpclient.WithBaseURL(server1.URL, unstartedServer.URL), ) require.NoError(b, err) runBench(b, client) diff --git a/conjure-go-client/httpclient/close_test.go b/conjure-go-client/httpclient/close_test.go index 0392753f..de5c15bc 100644 --- a/conjure-go-client/httpclient/close_test.go +++ b/conjure-go-client/httpclient/close_test.go @@ -18,7 +18,7 @@ import ( "bytes" "context" "fmt" - "io/ioutil" + "io" "net/http" "net/http/httptest" "runtime" @@ -38,7 +38,7 @@ func TestClose(t *testing.T) { _, _ = fmt.Fprintln(rw, "test") })) client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{ts.URL}), + httpclient.WithBaseURL(ts.URL), httpclient.WithHTTPTimeout(5*time.Second), ) require.NoError(t, err) @@ -65,7 +65,7 @@ func TestCloseOnError(t *testing.T) { before := runtime.NumGoroutine() // create test server and client with an HTTP Timeout of 5s client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{ts.URL}), + httpclient.WithBaseURL(ts.URL), httpclient.WithHTTPTimeout(5*time.Second), ) require.NoError(t, err) @@ -96,7 +96,7 @@ func TestCloseOnEmptyResponse(t *testing.T) { before := runtime.NumGoroutine() // create test server and client with an HTTP Timeout of 5s client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{ts.URL}), + httpclient.WithBaseURL(ts.URL), httpclient.WithHTTPTimeout(5*time.Second), ) require.NoError(t, err) @@ -138,14 +138,14 @@ func TestStreamingResponse(t *testing.T) { })) defer ts.Close() client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{ts.URL}), + httpclient.WithBaseURL(ts.URL), ) require.NoError(t, err) ctx := context.Background() resp, err := client.Get(ctx, httpclient.WithPath("/"), httpclient.WithRawResponseBody()) require.NoError(t, err) close(finishResponseChan) - b, err := ioutil.ReadAll(resp.Body) + b, err := io.ReadAll(resp.Body) require.NoError(t, err) assert.Equal(t, firstLine+"\n"+secondLine+"\n", string(b)) } diff --git a/conjure-go-client/httpclient/config.go b/conjure-go-client/httpclient/config.go index 99face43..ee495608 100644 --- a/conjure-go-client/httpclient/config.go +++ b/conjure-go-client/httpclient/config.go @@ -20,7 +20,7 @@ import ( "crypto/tls" "io/ioutil" "net/url" - "sort" + "slices" "time" "github.com/palantir/conjure-go-runtime/v2/conjure-go-client/httpclient/internal/refreshingclient" @@ -364,7 +364,7 @@ func RefreshableClientConfigFromServiceConfig(servicesConfig RefreshableServices })) } -func newValidatedClientParamsFromConfig(ctx context.Context, config ClientConfig, isHTTPClient bool) (refreshingclient.ValidatedClientParams, error) { +func newValidatedClientParamsFromConfig(ctx context.Context, config ClientConfig) (refreshingclient.ValidatedClientParams, error) { dialer := refreshingclient.DialerParams{ DialTimeout: derefDurationPtr(config.ConnectTimeout, defaultDialTimeout), KeepAlive: defaultKeepAlive, @@ -455,11 +455,7 @@ func newValidatedClientParamsFromConfig(ctx context.Context, config ClientConfig } uris = append(uris, uriStr) } - // Plain HTTP clients do not store URIs - if !isHTTPClient && len(uris) == 0 { - return refreshingclient.ValidatedClientParams{}, werror.ErrorWithContextParams(ctx, "httpclient URLs must not be empty") - } - sort.Strings(uris) + slices.Sort(uris) return refreshingclient.ValidatedClientParams{ APIToken: apiToken, diff --git a/conjure-go-client/httpclient/config_refreshable_test.go b/conjure-go-client/httpclient/config_refreshable_test.go index 60c9f477..7081ea43 100644 --- a/conjure-go-client/httpclient/config_refreshable_test.go +++ b/conjure-go-client/httpclient/config_refreshable_test.go @@ -105,10 +105,40 @@ func TestRefreshableClientConfig(t *testing.T) { } return c })) - refreshableClientConfig := RefreshableClientConfigFromServiceConfig(refreshableServicesConfig, serviceName) - _, err = NewClientFromRefreshableConfig(context.Background(), refreshableClientConfig) - require.EqualError(t, err, "httpclient URLs must not be empty") + t.Run("refreshable config without uris fails", func(t *testing.T) { + getClientURIs := func(client Client) []string { + return client.(*clientImpl).uriScorer.CurrentURIScoringMiddleware().GetURIsInOrderOfIncreasingScore() + } + refreshableClientConfig := RefreshableClientConfigFromServiceConfig(refreshableServicesConfig, serviceName) + client, err := NewClientFromRefreshableConfig(context.Background(), refreshableClientConfig) + require.EqualError(t, err, "httpclient URLs must not be empty") + require.Nil(t, client) + + client, err = NewClientFromRefreshableConfig(context.Background(), refreshableClientConfig, WithBaseURL("https://localhost")) + require.NoError(t, err, "expected to successfully create client using WithBaseURL even when config has no URIs") + require.Equal(t, []string{"https://localhost"}, getClientURIs(client), "expected URIs to be set") + + client, err = NewClientFromRefreshableConfig(context.Background(), refreshableClientConfig, WithRefreshableBaseURLs(refreshable.NewStringSlice(refreshable.NewDefaultRefreshable([]string{"https://localhost"})))) + require.NoError(t, err, "expected to successfully create client using WithRefreshableBaseURLs even when config has no URIs") + require.Equal(t, []string{"https://localhost"}, getClientURIs(client), "expected URIs to be set") + + t.Run("WithAllowCreateWithEmptyURIs", func(t *testing.T) { + client, err := NewClientFromRefreshableConfig(context.Background(), refreshableClientConfig, WithAllowCreateWithEmptyURIs()) + require.NoError(t, err, "expected to create a client from empty client config with WithAllowCreateWithEmptyURIs") + + // Expect error making request + _, err = client.Get(context.Background()) + require.EqualError(t, err, ErrEmptyURIs.Error()) + // Update config + initialConfig.Services[serviceName] = ClientConfig{ServiceName: serviceName, URIs: []string{"https://localhost"}} + updateRefreshableBytes(initialConfig) + + require.Equal(t, []string{"https://localhost"}, getClientURIs(client), "expected URIs to be set") + }) + }) + + refreshableClientConfig := RefreshableClientConfigFromServiceConfig(refreshableServicesConfig, serviceName) initialConfig.Services[serviceName] = ClientConfig{ServiceName: serviceName, URIs: []string{"https://localhost"}} updateRefreshableBytes(initialConfig) client, err := NewClientFromRefreshableConfig(context.Background(), refreshableClientConfig) diff --git a/conjure-go-client/httpclient/error_decoder_test.go b/conjure-go-client/httpclient/error_decoder_test.go index ddeb9220..bed8f978 100644 --- a/conjure-go-client/httpclient/error_decoder_test.go +++ b/conjure-go-client/httpclient/error_decoder_test.go @@ -45,7 +45,7 @@ func TestErrorDecoder(t *testing.T) { defer ts.Close() t.Run("ClientDefault", func(t *testing.T) { client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{ts.URL}), + httpclient.WithBaseURL(ts.URL), ) require.NoError(t, err) resp, err := client.Get(context.Background()) @@ -57,7 +57,7 @@ func TestErrorDecoder(t *testing.T) { }) t.Run("ClientNoop", func(t *testing.T) { client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{ts.URL}), + httpclient.WithBaseURL(ts.URL), httpclient.WithDisableRestErrors(), ) require.NoError(t, err) @@ -68,7 +68,7 @@ func TestErrorDecoder(t *testing.T) { }) t.Run("ClientCustom", func(t *testing.T) { client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{ts.URL}), + httpclient.WithBaseURL(ts.URL), httpclient.WithErrorDecoder(&customErrorDecoder{ statusCode: statusCode, message: clientDecoderMsg, @@ -81,7 +81,7 @@ func TestErrorDecoder(t *testing.T) { }) t.Run("RequestCustom", func(t *testing.T) { client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{ts.URL}), + httpclient.WithBaseURL(ts.URL), httpclient.WithErrorDecoder(&customErrorDecoder{ statusCode: statusCode, message: clientDecoderMsg, @@ -101,7 +101,7 @@ func TestErrorDecoder(t *testing.T) { }) t.Run("FallbackToClient", func(t *testing.T) { client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{ts.URL}), + httpclient.WithBaseURL(ts.URL), httpclient.WithErrorDecoder(&customErrorDecoder{ statusCode: statusCode, message: clientDecoderMsg, diff --git a/conjure-go-client/httpclient/metrics_test.go b/conjure-go-client/httpclient/metrics_test.go index 7883110c..ead24ab5 100644 --- a/conjure-go-client/httpclient/metrics_test.go +++ b/conjure-go-client/httpclient/metrics_test.go @@ -123,7 +123,7 @@ func TestRoundTripperWithMetrics(t *testing.T) { httpclient.WithHTTPTimeout(5*time.Second), httpclient.WithServiceName("my-service"), httpclient.WithMetrics(tagsProviders...), - httpclient.WithBaseURLs([]string{serverURLstr})) + httpclient.WithBaseURL(serverURLstr)) require.NoError(t, err) rpcMethodName, expectedMethodNameTag := getRpcNameAndExpectedTag() @@ -212,17 +212,17 @@ func TestMetricsMiddleware_HTTPClient(t *testing.T) { } func TestMetricsMiddleware_ClientTimeout(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { time.Sleep(time.Second) w.WriteHeader(200) })) - defer srv.Close() + defer server.Close() rootRegistry := metrics.NewRootMetricsRegistry() ctx := metrics.WithRegistry(context.Background(), rootRegistry) client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{srv.URL}), + httpclient.WithBaseURL(server.URL), httpclient.WithTLSInsecureSkipVerify(), httpclient.WithServiceName("test-service"), httpclient.WithHTTPTimeout(time.Millisecond), @@ -251,10 +251,10 @@ func TestMetricsMiddleware_ClientTimeout(t *testing.T) { } func TestMetricsMiddleware_ContextCanceled(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(200) })) - defer srv.Close() + defer server.Close() rootRegistry := metrics.NewRootMetricsRegistry() ctx := metrics.WithRegistry(context.Background(), rootRegistry) @@ -262,7 +262,7 @@ func TestMetricsMiddleware_ContextCanceled(t *testing.T) { cancel() client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{srv.URL}), + httpclient.WithBaseURL(server.URL), httpclient.WithTLSInsecureSkipVerify(), httpclient.WithServiceName("test-service"), httpclient.WithMetrics()) @@ -383,15 +383,15 @@ func TestMetricsMiddleware_InFlightRequests(t *testing.T) { ctx := metrics.WithRegistry(context.Background(), rootRegistry) serviceNameTag := metrics.MustNewTag("service-name", "test-service") - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { clientMetric := rootRegistry.Counter(httpclient.MetricRequestInFlight, serviceNameTag).Count() assert.Equal(t, int64(1), clientMetric, "%s should be nonzero during a request", httpclient.MetricRequestInFlight) w.WriteHeader(200) })) - defer srv.Close() + defer server.Close() client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{srv.URL}), + httpclient.WithBaseURL(server.URL), httpclient.WithServiceName("test-service"), httpclient.WithMetrics()) require.NoError(t, err) diff --git a/conjure-go-client/httpclient/recovery_test.go b/conjure-go-client/httpclient/recovery_test.go index d3b556eb..c67dad85 100644 --- a/conjure-go-client/httpclient/recovery_test.go +++ b/conjure-go-client/httpclient/recovery_test.go @@ -33,7 +33,7 @@ func TestRecoveryMiddleware(t *testing.T) { defer server.Close() client, err := httpclient.NewClient( - httpclient.WithBaseURLs([]string{server.URL}), + httpclient.WithBaseURL(server.URL), httpclient.WithMiddleware(panicMiddleware{err: helloErr}), ) require.NoError(t, err) diff --git a/conjure-go-client/httpclient/trace_test.go b/conjure-go-client/httpclient/trace_test.go index 6d390a13..ce37a34b 100644 --- a/conjure-go-client/httpclient/trace_test.go +++ b/conjure-go-client/httpclient/trace_test.go @@ -88,7 +88,7 @@ func TestTracing(t *testing.T) { })) defer server.Close() - client, err := httpclient.NewClient(httpclient.WithBaseURLs([]string{server.URL})) + client, err := httpclient.NewClient(httpclient.WithBaseURL(server.URL)) require.NoError(t, err) resp, err := client.Do(ctx, testCase.requestParams...) From 718342a511ffb7d2ae0ca8faef0a7ba426ca06fd Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Tue, 24 Sep 2024 00:06:29 +0000 Subject: [PATCH 2/5] Add generated changelog entries --- changelog/@unreleased/pr-693.v2.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/@unreleased/pr-693.v2.yml diff --git a/changelog/@unreleased/pr-693.v2.yml b/changelog/@unreleased/pr-693.v2.yml new file mode 100644 index 00000000..1b3c400b --- /dev/null +++ b/changelog/@unreleased/pr-693.v2.yml @@ -0,0 +1,6 @@ +type: improvement +improvement: + description: WithAllowCreateWithEmptyURIs() suppresses errors due to empty URIs + in NewClient() + links: + - https://github.com/palantir/conjure-go-runtime/pull/693 From ab8262e986916697fc69779fffa1598f1d47270b Mon Sep 17 00:00:00 2001 From: Brad Moylan Date: Tue, 24 Sep 2024 13:28:29 -0700 Subject: [PATCH 3/5] Revert "improvement: Add WithBaseURL param to avoid struct literals in WithBaseURLs" --- conjure-go-client/httpclient/authn_test.go | 6 ++--- .../httpclient/body_handler_test.go | 8 +++--- .../httpclient/client_builder_test.go | 2 +- conjure-go-client/httpclient/client_params.go | 5 ---- .../httpclient/client_params_test.go | 3 ++- conjure-go-client/httpclient/client_test.go | 26 +++++++++---------- conjure-go-client/httpclient/close_test.go | 12 ++++----- .../httpclient/config_refreshable_test.go | 2 +- .../httpclient/error_decoder_test.go | 10 +++---- conjure-go-client/httpclient/metrics_test.go | 20 +++++++------- conjure-go-client/httpclient/recovery_test.go | 2 +- conjure-go-client/httpclient/trace_test.go | 2 +- 12 files changed, 47 insertions(+), 51 deletions(-) diff --git a/conjure-go-client/httpclient/authn_test.go b/conjure-go-client/httpclient/authn_test.go index 48460b34..30eb5994 100644 --- a/conjure-go-client/httpclient/authn_test.go +++ b/conjure-go-client/httpclient/authn_test.go @@ -42,7 +42,7 @@ func TestRoundTripperWithToken(t *testing.T) { client, err := httpclient.NewClient( httpclient.WithHTTPTimeout(time.Minute), httpclient.WithAuthTokenProvider(tokenProvider), - httpclient.WithBaseURL(server.URL)) + httpclient.WithBaseURLs([]string{server.URL})) require.NoError(t, err) resp, err := client.Do(context.Background(), httpclient.WithRequestMethod(http.MethodGet)) @@ -71,7 +71,7 @@ func TestRoundTripperWithBasicAuth(t *testing.T) { client, err := httpclient.NewClient( httpclient.WithHTTPTimeout(time.Minute), httpclient.WithBasicAuth(expected.User, expected.Password), - httpclient.WithBaseURL(server.URL)) + httpclient.WithBaseURLs([]string{server.URL})) require.NoError(t, err) resp, err := client.Do(context.Background(), httpclient.WithRequestMethod(http.MethodGet)) @@ -106,7 +106,7 @@ func TestRoundTripperWithBasicAuthProvider(t *testing.T) { Password: "password", }, nil }), - httpclient.WithBaseURL(server.URL)) + httpclient.WithBaseURLs([]string{server.URL})) require.NoError(t, err) resp, err := client.Do(context.Background(), httpclient.WithRequestMethod(http.MethodGet)) diff --git a/conjure-go-client/httpclient/body_handler_test.go b/conjure-go-client/httpclient/body_handler_test.go index 0ad71e46..5018b2fa 100644 --- a/conjure-go-client/httpclient/body_handler_test.go +++ b/conjure-go-client/httpclient/body_handler_test.go @@ -48,7 +48,7 @@ func TestJSONBody(t *testing.T) { client, err := httpclient.NewClient( httpclient.WithUserAgent("TestNewRequest"), - httpclient.WithBaseURL(server.URL), + httpclient.WithBaseURLs([]string{server.URL}), ) require.NoError(t, err) @@ -80,7 +80,7 @@ func TestRawBody(t *testing.T) { client, err := httpclient.NewClient( httpclient.WithUserAgent("TestNewRequest"), - httpclient.WithBaseURL(server.URL), + httpclient.WithBaseURLs([]string{server.URL}), ) require.NoError(t, err) @@ -119,7 +119,7 @@ func TestRawRequestRetry(t *testing.T) { })) defer server.Close() - client, err := httpclient.NewClient(httpclient.WithBaseURL(server.URL)) + client, err := httpclient.NewClient(httpclient.WithBaseURLs([]string{server.URL})) assert.NoError(t, err) _, err = client.Do( @@ -154,7 +154,7 @@ func TestRedirectWithBodyAndBytesBuffer(t *testing.T) { client, err := httpclient.NewClient( httpclient.WithUserAgent("TestNewRequest"), - httpclient.WithBaseURL(server.URL), + httpclient.WithBaseURLs([]string{server.URL}), httpclient.WithBytesBufferPool(bytesbuffers.NewSizedPool(1, 10)), ) require.NoError(t, err) diff --git a/conjure-go-client/httpclient/client_builder_test.go b/conjure-go-client/httpclient/client_builder_test.go index de2c27df..2696e59b 100644 --- a/conjure-go-client/httpclient/client_builder_test.go +++ b/conjure-go-client/httpclient/client_builder_test.go @@ -47,7 +47,7 @@ func TestWrapTransport(t *testing.T) { httpclient.WithMiddleware(middleware(1)), httpclient.WithMiddleware(middleware(2)), httpclient.WithMiddleware(middleware(3)), - httpclient.WithBaseURL(server.URL), + httpclient.WithBaseURLs([]string{server.URL}), ) require.NoError(t, err) diff --git a/conjure-go-client/httpclient/client_params.go b/conjure-go-client/httpclient/client_params.go index c49e3353..ef3bbeb9 100644 --- a/conjure-go-client/httpclient/client_params.go +++ b/conjure-go-client/httpclient/client_params.go @@ -450,11 +450,6 @@ func WithKeepAlive(keepAlive time.Duration) ClientOrHTTPClientParam { }) } -// WithBaseURL is a variadic-argument alias for WithBaseURLs. Any previous base URLs will be overwritten. -func WithBaseURL(urls ...string) ClientParam { - return WithBaseURLs(urls) -} - // WithBaseURLs sets the base URLs for every request. This is meant to be used in conjunction with WithPath. func WithBaseURLs(urls []string) ClientParam { return clientParamFunc(func(b *clientBuilder) error { diff --git a/conjure-go-client/httpclient/client_params_test.go b/conjure-go-client/httpclient/client_params_test.go index 04ca1bf1..56d20ee9 100644 --- a/conjure-go-client/httpclient/client_params_test.go +++ b/conjure-go-client/httpclient/client_params_test.go @@ -145,7 +145,8 @@ func TestBuilder(t *testing.T) { } { t.Run(test.Name, func(t *testing.T) { // Must provide URLs for client creation - client, err := NewClient(WithBaseURL("https://localhost"), test.Param) + urls := WithBaseURLs([]string{"https://localhost"}) + client, err := NewClient(urls, test.Param) require.NoError(t, err) test.Test(t, client.(*clientImpl)) }) diff --git a/conjure-go-client/httpclient/client_test.go b/conjure-go-client/httpclient/client_test.go index 9fcf67fe..e3b838c3 100644 --- a/conjure-go-client/httpclient/client_test.go +++ b/conjure-go-client/httpclient/client_test.go @@ -57,7 +57,7 @@ func TestCanReadBodyWithBufferPool(t *testing.T) { client, err := httpclient.NewClient( httpclient.WithBytesBufferPool(bytesbuffers.NewSizedPool(1, 10)), - httpclient.WithBaseURL(server.URL), + httpclient.WithBaseURLs([]string{server.URL}), ) require.NoError(t, err) @@ -89,7 +89,7 @@ func TestCanUseRelocationURI(t *testing.T) { client, err := httpclient.NewClient( httpclient.WithBytesBufferPool(bytesbuffers.NewSizedPool(1, 10)), - httpclient.WithBaseURL(server.URL), + httpclient.WithBaseURLs([]string{server.URL}), ) assert.NoError(t, err) @@ -122,7 +122,7 @@ func TestCanUseSimpleRelocationURI(t *testing.T) { client, err := httpclient.NewClient( httpclient.WithBytesBufferPool(bytesbuffers.NewSizedPool(1, 10)), - httpclient.WithBaseURL(server.URL), + httpclient.WithBaseURLs([]string{server.URL}), ) assert.NoError(t, err) @@ -169,7 +169,7 @@ func TestMiddlewareCanReadBody(t *testing.T) { t.Run("NoByteBufferPool", func(t *testing.T) { client, err := httpclient.NewClient( withMiddleware, - httpclient.WithBaseURL(server.URL), + httpclient.WithBaseURLs([]string{server.URL}), ) require.NoError(t, err) resp, err := client.Do(context.Background(), httpclient.WithRequestBody(unencodedBody, codecs.Plain), httpclient.WithRequestMethod("GET")) @@ -180,7 +180,7 @@ func TestMiddlewareCanReadBody(t *testing.T) { client, err := httpclient.NewClient( httpclient.WithBytesBufferPool(bytesbuffers.NewSizedPool(1, 10)), withMiddleware, - httpclient.WithBaseURL(server.URL), + httpclient.WithBaseURLs([]string{server.URL}), ) require.NoError(t, err) resp, err := client.Do(context.Background(), httpclient.WithRequestBody(unencodedBody, codecs.Plain), httpclient.WithRequestMethod("GET")) @@ -317,7 +317,7 @@ func BenchmarkAllocWithBytesBufferPool(b *testing.B) { } b.Run("NoByteBufferPool", func(b *testing.B) { client, err := httpclient.NewClient( - httpclient.WithBaseURL(server.URL), + httpclient.WithBaseURLs([]string{server.URL}), ) require.NoError(b, err) runBench(b, client) @@ -325,7 +325,7 @@ func BenchmarkAllocWithBytesBufferPool(b *testing.B) { b.Run("WithByteBufferPool", func(b *testing.B) { client, err := httpclient.NewClient( httpclient.WithBytesBufferPool(bytesbuffers.NewSizedPool(1, 10)), - httpclient.WithBaseURL(server.URL), + httpclient.WithBaseURLs([]string{server.URL}), ) require.NoError(b, err) runBench(b, client) @@ -376,42 +376,42 @@ func BenchmarkUnavailableURIs(b *testing.B) { } b.Run("OneAvailableServer", func(b *testing.B) { client, err := httpclient.NewClient( - httpclient.WithBaseURL(server1.URL), + httpclient.WithBaseURLs([]string{server1.URL}), ) require.NoError(b, err) runBench(b, client) }) b.Run("FourAvailableServers", func(b *testing.B) { client, err := httpclient.NewClient( - httpclient.WithBaseURL(server1.URL, server2.URL, server3.URL, server4.URL), + httpclient.WithBaseURLs([]string{server1.URL, server2.URL, server3.URL, server4.URL}), ) require.NoError(b, err) runBench(b, client) }) b.Run("OneOutOfFourUnavailableServers", func(b *testing.B) { client, err := httpclient.NewClient( - httpclient.WithBaseURL(server1.URL, server2.URL, server3.URL, unavailableServer.URL), + httpclient.WithBaseURLs([]string{server1.URL, server2.URL, server3.URL, unavailableServer.URL}), ) require.NoError(b, err) runBench(b, client) }) b.Run("OneOutOfThreeUnavailableServers", func(b *testing.B) { client, err := httpclient.NewClient( - httpclient.WithBaseURL(server1.URL, server2.URL, unavailableServer.URL), + httpclient.WithBaseURLs([]string{server1.URL, server2.URL, unavailableServer.URL}), ) require.NoError(b, err) runBench(b, client) }) b.Run("OneOutOfTwoUnavailableServers", func(b *testing.B) { client, err := httpclient.NewClient( - httpclient.WithBaseURL(server1.URL, unavailableServer.URL), + httpclient.WithBaseURLs([]string{server1.URL, unavailableServer.URL}), ) require.NoError(b, err) runBench(b, client) }) b.Run("OneOutOfTwoUnstartedServers", func(b *testing.B) { client, err := httpclient.NewClient( - httpclient.WithBaseURL(server1.URL, unstartedServer.URL), + httpclient.WithBaseURLs([]string{server1.URL, unstartedServer.URL}), ) require.NoError(b, err) runBench(b, client) diff --git a/conjure-go-client/httpclient/close_test.go b/conjure-go-client/httpclient/close_test.go index de5c15bc..0392753f 100644 --- a/conjure-go-client/httpclient/close_test.go +++ b/conjure-go-client/httpclient/close_test.go @@ -18,7 +18,7 @@ import ( "bytes" "context" "fmt" - "io" + "io/ioutil" "net/http" "net/http/httptest" "runtime" @@ -38,7 +38,7 @@ func TestClose(t *testing.T) { _, _ = fmt.Fprintln(rw, "test") })) client, err := httpclient.NewClient( - httpclient.WithBaseURL(ts.URL), + httpclient.WithBaseURLs([]string{ts.URL}), httpclient.WithHTTPTimeout(5*time.Second), ) require.NoError(t, err) @@ -65,7 +65,7 @@ func TestCloseOnError(t *testing.T) { before := runtime.NumGoroutine() // create test server and client with an HTTP Timeout of 5s client, err := httpclient.NewClient( - httpclient.WithBaseURL(ts.URL), + httpclient.WithBaseURLs([]string{ts.URL}), httpclient.WithHTTPTimeout(5*time.Second), ) require.NoError(t, err) @@ -96,7 +96,7 @@ func TestCloseOnEmptyResponse(t *testing.T) { before := runtime.NumGoroutine() // create test server and client with an HTTP Timeout of 5s client, err := httpclient.NewClient( - httpclient.WithBaseURL(ts.URL), + httpclient.WithBaseURLs([]string{ts.URL}), httpclient.WithHTTPTimeout(5*time.Second), ) require.NoError(t, err) @@ -138,14 +138,14 @@ func TestStreamingResponse(t *testing.T) { })) defer ts.Close() client, err := httpclient.NewClient( - httpclient.WithBaseURL(ts.URL), + httpclient.WithBaseURLs([]string{ts.URL}), ) require.NoError(t, err) ctx := context.Background() resp, err := client.Get(ctx, httpclient.WithPath("/"), httpclient.WithRawResponseBody()) require.NoError(t, err) close(finishResponseChan) - b, err := io.ReadAll(resp.Body) + b, err := ioutil.ReadAll(resp.Body) require.NoError(t, err) assert.Equal(t, firstLine+"\n"+secondLine+"\n", string(b)) } diff --git a/conjure-go-client/httpclient/config_refreshable_test.go b/conjure-go-client/httpclient/config_refreshable_test.go index 2d869942..bf0d68c7 100644 --- a/conjure-go-client/httpclient/config_refreshable_test.go +++ b/conjure-go-client/httpclient/config_refreshable_test.go @@ -114,7 +114,7 @@ func TestRefreshableClientConfig(t *testing.T) { require.EqualError(t, err, "httpclient URLs must not be empty") require.Nil(t, client) - client, err = NewClientFromRefreshableConfig(context.Background(), refreshableClientConfig, WithBaseURL("https://localhost")) + client, err = NewClientFromRefreshableConfig(context.Background(), refreshableClientConfig, WithBaseURLs([]string{"https://localhost"})) require.NoError(t, err, "expected to successfully create client using WithBaseURL even when config has no URIs") require.Equal(t, []string{"https://localhost"}, getClientURIs(client), "expected URIs to be set") diff --git a/conjure-go-client/httpclient/error_decoder_test.go b/conjure-go-client/httpclient/error_decoder_test.go index bed8f978..ddeb9220 100644 --- a/conjure-go-client/httpclient/error_decoder_test.go +++ b/conjure-go-client/httpclient/error_decoder_test.go @@ -45,7 +45,7 @@ func TestErrorDecoder(t *testing.T) { defer ts.Close() t.Run("ClientDefault", func(t *testing.T) { client, err := httpclient.NewClient( - httpclient.WithBaseURL(ts.URL), + httpclient.WithBaseURLs([]string{ts.URL}), ) require.NoError(t, err) resp, err := client.Get(context.Background()) @@ -57,7 +57,7 @@ func TestErrorDecoder(t *testing.T) { }) t.Run("ClientNoop", func(t *testing.T) { client, err := httpclient.NewClient( - httpclient.WithBaseURL(ts.URL), + httpclient.WithBaseURLs([]string{ts.URL}), httpclient.WithDisableRestErrors(), ) require.NoError(t, err) @@ -68,7 +68,7 @@ func TestErrorDecoder(t *testing.T) { }) t.Run("ClientCustom", func(t *testing.T) { client, err := httpclient.NewClient( - httpclient.WithBaseURL(ts.URL), + httpclient.WithBaseURLs([]string{ts.URL}), httpclient.WithErrorDecoder(&customErrorDecoder{ statusCode: statusCode, message: clientDecoderMsg, @@ -81,7 +81,7 @@ func TestErrorDecoder(t *testing.T) { }) t.Run("RequestCustom", func(t *testing.T) { client, err := httpclient.NewClient( - httpclient.WithBaseURL(ts.URL), + httpclient.WithBaseURLs([]string{ts.URL}), httpclient.WithErrorDecoder(&customErrorDecoder{ statusCode: statusCode, message: clientDecoderMsg, @@ -101,7 +101,7 @@ func TestErrorDecoder(t *testing.T) { }) t.Run("FallbackToClient", func(t *testing.T) { client, err := httpclient.NewClient( - httpclient.WithBaseURL(ts.URL), + httpclient.WithBaseURLs([]string{ts.URL}), httpclient.WithErrorDecoder(&customErrorDecoder{ statusCode: statusCode, message: clientDecoderMsg, diff --git a/conjure-go-client/httpclient/metrics_test.go b/conjure-go-client/httpclient/metrics_test.go index ead24ab5..7883110c 100644 --- a/conjure-go-client/httpclient/metrics_test.go +++ b/conjure-go-client/httpclient/metrics_test.go @@ -123,7 +123,7 @@ func TestRoundTripperWithMetrics(t *testing.T) { httpclient.WithHTTPTimeout(5*time.Second), httpclient.WithServiceName("my-service"), httpclient.WithMetrics(tagsProviders...), - httpclient.WithBaseURL(serverURLstr)) + httpclient.WithBaseURLs([]string{serverURLstr})) require.NoError(t, err) rpcMethodName, expectedMethodNameTag := getRpcNameAndExpectedTag() @@ -212,17 +212,17 @@ func TestMetricsMiddleware_HTTPClient(t *testing.T) { } func TestMetricsMiddleware_ClientTimeout(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { time.Sleep(time.Second) w.WriteHeader(200) })) - defer server.Close() + defer srv.Close() rootRegistry := metrics.NewRootMetricsRegistry() ctx := metrics.WithRegistry(context.Background(), rootRegistry) client, err := httpclient.NewClient( - httpclient.WithBaseURL(server.URL), + httpclient.WithBaseURLs([]string{srv.URL}), httpclient.WithTLSInsecureSkipVerify(), httpclient.WithServiceName("test-service"), httpclient.WithHTTPTimeout(time.Millisecond), @@ -251,10 +251,10 @@ func TestMetricsMiddleware_ClientTimeout(t *testing.T) { } func TestMetricsMiddleware_ContextCanceled(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(200) })) - defer server.Close() + defer srv.Close() rootRegistry := metrics.NewRootMetricsRegistry() ctx := metrics.WithRegistry(context.Background(), rootRegistry) @@ -262,7 +262,7 @@ func TestMetricsMiddleware_ContextCanceled(t *testing.T) { cancel() client, err := httpclient.NewClient( - httpclient.WithBaseURL(server.URL), + httpclient.WithBaseURLs([]string{srv.URL}), httpclient.WithTLSInsecureSkipVerify(), httpclient.WithServiceName("test-service"), httpclient.WithMetrics()) @@ -383,15 +383,15 @@ func TestMetricsMiddleware_InFlightRequests(t *testing.T) { ctx := metrics.WithRegistry(context.Background(), rootRegistry) serviceNameTag := metrics.MustNewTag("service-name", "test-service") - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { clientMetric := rootRegistry.Counter(httpclient.MetricRequestInFlight, serviceNameTag).Count() assert.Equal(t, int64(1), clientMetric, "%s should be nonzero during a request", httpclient.MetricRequestInFlight) w.WriteHeader(200) })) - defer server.Close() + defer srv.Close() client, err := httpclient.NewClient( - httpclient.WithBaseURL(server.URL), + httpclient.WithBaseURLs([]string{srv.URL}), httpclient.WithServiceName("test-service"), httpclient.WithMetrics()) require.NoError(t, err) diff --git a/conjure-go-client/httpclient/recovery_test.go b/conjure-go-client/httpclient/recovery_test.go index c67dad85..d3b556eb 100644 --- a/conjure-go-client/httpclient/recovery_test.go +++ b/conjure-go-client/httpclient/recovery_test.go @@ -33,7 +33,7 @@ func TestRecoveryMiddleware(t *testing.T) { defer server.Close() client, err := httpclient.NewClient( - httpclient.WithBaseURL(server.URL), + httpclient.WithBaseURLs([]string{server.URL}), httpclient.WithMiddleware(panicMiddleware{err: helloErr}), ) require.NoError(t, err) diff --git a/conjure-go-client/httpclient/trace_test.go b/conjure-go-client/httpclient/trace_test.go index ce37a34b..6d390a13 100644 --- a/conjure-go-client/httpclient/trace_test.go +++ b/conjure-go-client/httpclient/trace_test.go @@ -88,7 +88,7 @@ func TestTracing(t *testing.T) { })) defer server.Close() - client, err := httpclient.NewClient(httpclient.WithBaseURL(server.URL)) + client, err := httpclient.NewClient(httpclient.WithBaseURLs([]string{server.URL})) require.NoError(t, err) resp, err := client.Do(ctx, testCase.requestParams...) From f7a07327ec00c6fdafba5867a6a25e5b1d949506 Mon Sep 17 00:00:00 2001 From: Brad Moylan Date: Fri, 27 Sep 2024 10:53:15 -0700 Subject: [PATCH 4/5] comment --- conjure-go-client/httpclient/client_builder.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/conjure-go-client/httpclient/client_builder.go b/conjure-go-client/httpclient/client_builder.go index f14a776a..d1434732 100644 --- a/conjure-go-client/httpclient/client_builder.go +++ b/conjure-go-client/httpclient/client_builder.go @@ -47,6 +47,9 @@ const ( ) var ( + // ErrEmptyURIs is returned when the client expects to have base URIs configured to make requests, but the URIs are empty. + // This check occurs in two places: when the client is constructed and when a request is executed. + // To avoid the construction validation, use WithAllowCreateWithEmptyURIs(). ErrEmptyURIs = fmt.Errorf("httpclient URLs must not be empty") ) From 047327dadb3a5b4ebeb9a469bd831152957cba46 Mon Sep 17 00:00:00 2001 From: Brad Moylan Date: Fri, 27 Sep 2024 10:55:39 -0700 Subject: [PATCH 5/5] param --- conjure-go-client/httpclient/client.go | 3 ++- conjure-go-client/httpclient/client_builder.go | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/conjure-go-client/httpclient/client.go b/conjure-go-client/httpclient/client.go index fe3d6076..10deea2e 100644 --- a/conjure-go-client/httpclient/client.go +++ b/conjure-go-client/httpclient/client.go @@ -50,6 +50,7 @@ type Client interface { } type clientImpl struct { + serviceName refreshable.String client RefreshableHTTPClient middlewares []Middleware errorDecoderMiddleware Middleware @@ -84,7 +85,7 @@ func (c *clientImpl) Delete(ctx context.Context, params ...RequestParam) (*http. func (c *clientImpl) Do(ctx context.Context, params ...RequestParam) (*http.Response, error) { uris := c.uriScorer.CurrentURIScoringMiddleware().GetURIsInOrderOfIncreasingScore() if len(uris) == 0 { - return nil, werror.WrapWithContextParams(ctx, ErrEmptyURIs, "") + return nil, werror.WrapWithContextParams(ctx, ErrEmptyURIs, "", werror.SafeParam("serviceName", c.serviceName.CurrentString())) } attempts := 2 * len(uris) diff --git a/conjure-go-client/httpclient/client_builder.go b/conjure-go-client/httpclient/client_builder.go index d1434732..91db6014 100644 --- a/conjure-go-client/httpclient/client_builder.go +++ b/conjure-go-client/httpclient/client_builder.go @@ -178,6 +178,7 @@ func newClient(ctx context.Context, b *clientBuilder, params ...ClientParam) (Cl return b.URIScorerBuilder(uris) }) return &clientImpl{ + serviceName: b.HTTP.ServiceName, client: httpClient, uriScorer: uriScorer, maxAttempts: b.MaxAttempts,