From 34da4ee352333965424c316bb809a974a800776b Mon Sep 17 00:00:00 2001 From: Brad Moylan Date: Fri, 27 Sep 2024 13:37:26 -0700 Subject: [PATCH] improvement: WithAllowCreateWithEmptyURIs() suppresses errors due to empty URIs in NewClient() (#693) --- changelog/@unreleased/pr-693.v2.yml | 6 ++++ conjure-go-client/httpclient/client.go | 3 +- .../httpclient/client_builder.go | 28 +++++++++++---- conjure-go-client/httpclient/client_params.go | 10 ++++++ conjure-go-client/httpclient/client_test.go | 2 +- conjure-go-client/httpclient/config.go | 10 ++---- .../httpclient/config_refreshable_test.go | 36 +++++++++++++++++-- 7 files changed, 77 insertions(+), 18 deletions(-) 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 diff --git a/conjure-go-client/httpclient/client.go b/conjure-go-client/httpclient/client.go index 69df1d16..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.ErrorWithContextParams(ctx, "no base URIs are configured") + 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 e040b775..91db6014 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" @@ -45,12 +46,23 @@ const ( defaultMaxBackoff = 2 * time.Second ) +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") +) + 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 @@ -120,7 +132,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...) @@ -136,7 +148,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.ServiceName.CurrentString())) + return nil, werror.ErrorWithContextParams(ctx, "httpclient URLs must be set in configuration or by constructor param", werror.SafeParam("serviceName", b.HTTP.ServiceName.CurrentString())) + } + if !b.AllowEmptyURIs && len(b.URIs.CurrentStringSlice()) == 0 { + return nil, werror.WrapWithContextParams(ctx, ErrEmptyURIs, "", werror.SafeParam("serviceName", b.HTTP.ServiceName.CurrentString())) } var edm Middleware @@ -163,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, @@ -192,7 +208,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...) @@ -242,18 +258,18 @@ 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 { 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.ServiceName = validParams.ServiceName() b.HTTP.DialerParams = validParams.Dialer() diff --git a/conjure-go-client/httpclient/client_params.go b/conjure-go-client/httpclient/client_params.go index 08261d59..ef3bbeb9 100644 --- a/conjure-go-client/httpclient/client_params.go +++ b/conjure-go-client/httpclient/client_params.go @@ -466,6 +466,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_test.go b/conjure-go-client/httpclient/client_test.go index d16ffeff..e3b838c3 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) } diff --git a/conjure-go-client/httpclient/config.go b/conjure-go-client/httpclient/config.go index 72601e5e..5807b0a1 100644 --- a/conjure-go-client/httpclient/config.go +++ b/conjure-go-client/httpclient/config.go @@ -19,7 +19,7 @@ import ( "context" "io/ioutil" "net/url" - "sort" + "slices" "time" "github.com/palantir/conjure-go-runtime/v2/conjure-go-client/httpclient/internal/refreshingclient" @@ -385,7 +385,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: derefPtr(config.ConnectTimeout, defaultDialTimeout), KeepAlive: derefPtr(config.KeepAlive, defaultKeepAlive), @@ -477,11 +477,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 5cc5fe61..bf0d68c7 100644 --- a/conjure-go-client/httpclient/config_refreshable_test.go +++ b/conjure-go-client/httpclient/config_refreshable_test.go @@ -104,10 +104,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, 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") + + 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)