improvement: WithAllowCreateWithEmptyURIs() suppresses errors due to …

…empty URIs in NewClient() (#693)

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

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)

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()

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 {

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)
 }
 

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,

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)