Start decoupling ApiClient and DatabricksClient. (#1245)

## What changes are proposed in this pull request?

The SDK has two clients which serve very similar purposes: `ApiClient`
and `DatabricksClient`. This PR is a first step to pay to merge these
two client into a single consistent client. Specifically, it brings some
of the functionality from `DatabricksClient` in `ApiClient` in
preparation for a change of code generation logic.

This PR also slightly changes the way the `ApiClient` default parameter
values are set by moving the logic in its constructor.

## How is this tested?

Unit and integration tests.

Author: renaudhartert-db

NEXT_CHANGELOG.md

@@ -6,6 +6,8 @@
 
 ### Bug Fixes
 
+- Default values for `ApiClient` are now properly set in the type constructor rather than in the config constructor. 
+
 ### Documentation
 
 ### Internal Changes

client/client.go

@@ -15,13 +15,13 @@ func New(cfg *config.Config) (*DatabricksClient, error) {
 	if err != nil {
 		return nil, err
 	}
-	client, err := cfg.NewApiClient()
+	clientCfg, err := config.HTTPClientConfigFromConfig(cfg)
 	if err != nil {
 		return nil, err
 	}
 	return &DatabricksClient{
 		Config: cfg,
-		client: client,
+		client: httpclient.NewApiClient(clientCfg),
 	}, nil
 }
 

config/api_client.go

@@ -14,42 +14,44 @@ import (
 	"github.com/databricks/databricks-sdk-go/useragent"
 )
 
-func (c *Config) NewApiClient() (*httpclient.ApiClient, error) {
-	if skippable, ok := c.HTTPTransport.(interface {
+func HTTPClientConfigFromConfig(cfg *Config) (httpclient.ClientConfig, error) {
+	if skippable, ok := cfg.HTTPTransport.(interface {
 		SkipRetryOnIO() bool
 	}); ok && skippable.SkipRetryOnIO() {
-		c.Loaders = []Loader{noopLoader{}}
-		c.Credentials = noopAuth{}
+		cfg.Loaders = []Loader{noopLoader{}}
+		cfg.Credentials = noopAuth{}
 	}
-	err := c.EnsureResolved()
+
+	err := cfg.EnsureResolved()
 	if err != nil {
-		return nil, err
+		return httpclient.ClientConfig{}, err
 	}
-	retryTimeout := time.Duration(orDefault(c.RetryTimeoutSeconds, 300)) * time.Second
-	httpTimeout := time.Duration(orDefault(c.HTTPTimeoutSeconds, 60)) * time.Second
-	return httpclient.NewApiClient(httpclient.ClientConfig{
-		RetryTimeout:       retryTimeout,
-		HTTPTimeout:        httpTimeout,
-		RateLimitPerSecond: orDefault(c.RateLimitPerSecond, 15),
-		DebugHeaders:       c.DebugHeaders,
-		DebugTruncateBytes: c.DebugTruncateBytes,
-		InsecureSkipVerify: c.InsecureSkipVerify,
-		Transport:          c.HTTPTransport,
-		AuthVisitor:        c.Authenticate,
+
+	return httpclient.ClientConfig{
+		AccountID:          cfg.AccountID,
+		Host:               cfg.Host,
+		RetryTimeout:       time.Duration(cfg.RetryTimeoutSeconds) * time.Second,
+		HTTPTimeout:        time.Duration(cfg.HTTPTimeoutSeconds) * time.Second,
+		RateLimitPerSecond: cfg.RateLimitPerSecond,
+		DebugHeaders:       cfg.DebugHeaders,
+		DebugTruncateBytes: cfg.DebugTruncateBytes,
+		InsecureSkipVerify: cfg.InsecureSkipVerify,
+		Transport:          cfg.HTTPTransport,
+		AuthVisitor:        cfg.Authenticate,
 		Visitors: []httpclient.RequestVisitor{
 			func(r *http.Request) error {
 				if r.URL == nil {
 					return fmt.Errorf("no URL found in request")
 				}
-				url, err := url.Parse(c.Host)
+				url, err := url.Parse(cfg.Host)
 				if err != nil {
 					return err
 				}
 				r.URL.Host = url.Host
 				r.URL.Scheme = url.Scheme
 				return nil
 			},
-			authInUserAgentVisitor(c),
+			authInUserAgentVisitor(cfg),
 			func(r *http.Request) error {
 				// Detect if we are running in a CI/CD environment
 				provider := useragent.CiCdProvider()
@@ -74,7 +76,9 @@ func (c *Config) NewApiClient() (*httpclient.ApiClient, error) {
 			},
 		},
 		TransientErrors: []string{
-			"REQUEST_LIMIT_EXCEEDED", // This is temporary workaround for SCIM API returning 500.  Remove when it's fixed
+			// This is temporary workaround for SCIM API returning 500.
+			// TODO: Remove when it's fixed.
+			"REQUEST_LIMIT_EXCEEDED",
 		},
 		ErrorMapper: apierr.GetAPIError,
 		ErrorRetriable: func(ctx context.Context, err error) bool {
@@ -84,14 +88,7 @@ func (c *Config) NewApiClient() (*httpclient.ApiClient, error) {
 			}
 			return false
 		},
-	}), nil
-}
-
-func orDefault(configured, _default int) int {
-	if configured == 0 {
-		return _default
-	}
-	return configured
+	}, nil
 }
 
 // noopLoader skips configuration loading
@@ -108,3 +105,12 @@ func (noopAuth) Configure(context.Context, *Config) (credentials.CredentialsProv
 	visitor := func(r *http.Request) error { return nil }
 	return credentials.CredentialsProviderFn(visitor), nil
 }
+
+// Deprecated: use [HTTPClientConfigFromConfig] with [httpclient.NewApiClient].
+func (c *Config) NewApiClient() (*httpclient.ApiClient, error) {
+	cfg, err := HTTPClientConfigFromConfig(c)
+	if err != nil {
+		return nil, err
+	}
+	return httpclient.NewApiClient(cfg), nil
+}

httpclient/api_client.go

@@ -28,6 +28,9 @@ type ClientConfig struct {
 	AuthVisitor RequestVisitor
 	Visitors    []RequestVisitor
 
+	AccountID string
+	Host      string
+
 	RetryTimeout       time.Duration
 	HTTPTimeout        time.Duration
 	InsecureSkipVerify bool
@@ -42,6 +45,35 @@ type ClientConfig struct {
 	Transport http.RoundTripper
 }
 
+type DoOption struct {
+	in           RequestVisitor
+	out          func(body *common.ResponseWrapper) error
+	body         any
+	contentType  string
+	isAuthOption bool
+	queryParams  map[string]any
+}
+
+type ApiClient struct {
+	config      ClientConfig
+	rateLimiter *rate.Limiter
+	httpClient  *http.Client
+}
+
+// IsAccountClient returns true if the client is configured for Accounts API.
+func (apic *ApiClient) IsAccountClient() bool {
+	if apic.config.AccountID == "" {
+		return false
+	}
+	if strings.HasPrefix(apic.config.Host, "https://accounts.") {
+		return true
+	}
+	if strings.HasPrefix(apic.config.Host, "https://accounts-dod.") {
+		return true
+	}
+	return false
+}
+
 var defaultTransport = makeDefaultTransport()
 
 func makeDefaultTransport() *http.Transport {
@@ -73,10 +105,13 @@ func (cfg ClientConfig) httpTransport() http.RoundTripper {
 }
 
 func NewApiClient(cfg ClientConfig) *ApiClient {
+	// Set defaults for config values that are not set.
 	cfg.HTTPTimeout = time.Duration(orDefault(int(cfg.HTTPTimeout), int(30*time.Second)))
 	cfg.DebugTruncateBytes = orDefault(cfg.DebugTruncateBytes, 96)
 	cfg.RetryTimeout = time.Duration(orDefault(int(cfg.RetryTimeout), int(5*time.Minute)))
-	cfg.HTTPTimeout = time.Duration(orDefault(int(cfg.HTTPTimeout), int(30*time.Second)))
+	cfg.HTTPTimeout = time.Duration(orDefault(int(cfg.HTTPTimeout), int(60*time.Second)))
+	cfg.RateLimitPerSecond = orDefault(cfg.RateLimitPerSecond, 15)
+
 	if cfg.ErrorMapper == nil {
 		// default generic error mapper
 		cfg.ErrorMapper = DefaultErrorMapper
@@ -107,21 +142,6 @@ func NewApiClient(cfg ClientConfig) *ApiClient {
 	}
 }
 
-type ApiClient struct {
-	config      ClientConfig
-	rateLimiter *rate.Limiter
-	httpClient  *http.Client
-}
-
-type DoOption struct {
-	in           RequestVisitor
-	out          func(body *common.ResponseWrapper) error
-	body         any
-	contentType  string
-	isAuthOption bool
-	queryParams  map[string]any
-}
-
 // Do sends an HTTP request against path.
 func (c *ApiClient) Do(ctx context.Context, method, path string, opts ...DoOption) error {
 	var authVisitor RequestVisitor