[Fix] Support Query parameters for all HTTP operations (#1124)

## What changes are proposed in this pull request?
Support Query parameters for all HTTP operations. Previously, only
GET/HEAD/DELETE were supported.

## How is this tested?
* Added Unit Tests
* Run Integration Tests

client/client.go

@@ -63,12 +63,13 @@ func (c *DatabricksClient) GetOAuthToken(ctx context.Context, authDetails string
 
 // Do sends an HTTP request against path.
 func (c *DatabricksClient) Do(ctx context.Context, method, path string,
-	headers map[string]string, request, response any,
+	headers map[string]string, queryParams map[string]any, request, response any,
 	visitors ...func(*http.Request) error) error {
 	opts := []httpclient.DoOption{}
 	for _, v := range visitors {
 		opts = append(opts, httpclient.WithRequestVisitor(v))
 	}
+	opts = append(opts, httpclient.WithQueryParameters(queryParams))
 	opts = append(opts, httpclient.WithRequestHeaders(headers))
 	opts = append(opts, httpclient.WithRequestData(request))
 	opts = append(opts, httpclient.WithResponseUnmarshal(response))

client/client_test.go

@@ -43,11 +43,15 @@ func TestSimpleRequestFailsURLError(t *testing.T) {
 		}),
 	})
 	require.NoError(t, err)
-	err = c.Do(context.Background(), "GET", "/a/b", map[string]string{
-		"e": "f",
-	}, map[string]string{
-		"c": "d",
-	}, nil)
+	err = c.Do(
+		context.Background(),
+		"GET",
+		"/a/b",
+		map[string]string{"e": "f"},
+		nil,
+		map[string]string{"c": "d"},
+		nil,
+	)
 	require.EqualError(t, err, `Get "https://some/a/b?c=d": nope`)
 }
 
@@ -66,11 +70,15 @@ func TestSimpleRequestFailsAPIError(t *testing.T) {
 		}),
 	})
 	require.NoError(t, err)
-	err = c.Do(context.Background(), "GET", "/a/b", map[string]string{
-		"e": "f",
-	}, map[string]string{
-		"c": "d",
-	}, nil)
+	err = c.Do(
+		context.Background(),
+		"GET",
+		"/a/b",
+		map[string]string{"e": "f"},
+		nil,
+		map[string]string{"c": "d"},
+		nil,
+	)
 	require.EqualError(t, err, "nope")
 	require.ErrorIs(t, err, apierr.ErrInvalidParameterValue)
 }
@@ -115,11 +123,15 @@ func TestETag(t *testing.T) {
 		}),
 	})
 	require.NoError(t, err)
-	err = c.Do(context.Background(), "GET", "/a/b", map[string]string{
-		"e": "f",
-	}, map[string]string{
-		"c": "d",
-	}, nil)
+	err = c.Do(
+		context.Background(),
+		"GET",
+		"/a/b",
+		map[string]string{"e": "f"},
+		nil,
+		map[string]string{"c": "d"},
+		nil,
+	)
 	details := apierr.GetErrorInfo(err)
 	require.Equal(t, 1, len(details))
 	errorDetails := details[0]
@@ -148,7 +160,52 @@ func TestSimpleRequestSucceeds(t *testing.T) {
 	})
 	require.NoError(t, err)
 	var resp Dummy
-	err = c.Do(context.Background(), "POST", "/c", nil, Dummy{1}, &resp)
+	err = c.Do(
+		context.Background(),
+		"POST",
+		"/c",
+		nil,
+		nil,
+		Dummy{1},
+		&resp,
+	)
+	require.NoError(t, err)
+	require.Equal(t, 2, resp.Foo)
+}
+
+func TestQueryParamsRequestSucceeds(t *testing.T) {
+	type Dummy struct {
+		Foo int `json:"foo"`
+	}
+	c, err := New(&config.Config{
+		Host:       "some",
+		Token:      "token",
+		ConfigFile: "/dev/null",
+		HTTPTransport: hc(func(r *http.Request) (*http.Response, error) {
+			if r.URL.RawQuery != "a=b&c=1" {
+				return nil, fmt.Errorf("unexpected query params: %s", r.URL.RawQuery)
+			}
+			return &http.Response{
+				StatusCode: 200,
+				Body:       io.NopCloser(strings.NewReader(`{"foo": 2}`)),
+				Request:    r,
+			}, nil
+		}),
+	})
+	require.NoError(t, err)
+	var resp Dummy
+	err = c.Do(
+		context.Background(),
+		"POST",
+		"/c",
+		nil,
+		map[string]any{
+			"a": "b",
+			"c": 1,
+		},
+		Dummy{1},
+		&resp,
+	)
 	require.NoError(t, err)
 	require.Equal(t, 2, resp.Foo)
 }
@@ -180,7 +237,15 @@ func TestSimpleRequestRetried(t *testing.T) {
 	})
 	require.NoError(t, err)
 	var resp Dummy
-	err = c.Do(context.Background(), "PATCH", "/a", nil, Dummy{1}, &resp)
+	err = c.Do(
+		context.Background(),
+		"PATCH",
+		"/a",
+		nil,
+		nil,
+		Dummy{1},
+		&resp,
+	)
 	require.NoError(t, err)
 	require.Equal(t, 2, resp.Foo)
 	require.True(t, retried[0], "request was not retried")
@@ -203,7 +268,15 @@ func TestSimpleRequestAPIError(t *testing.T) {
 		}),
 	})
 	require.NoError(t, err)
-	err = c.Do(context.Background(), "PATCH", "/a", nil, map[string]any{}, nil)
+	err = c.Do(
+		context.Background(),
+		"PATCH",
+		"/a",
+		nil,
+		nil,
+		map[string]any{},
+		nil,
+	)
 	var aerr *apierr.APIError
 	require.ErrorAs(t, err, &aerr)
 	require.Equal(t, "NOT_FOUND", aerr.ErrorCode)
@@ -223,7 +296,15 @@ func TestHttpTransport(t *testing.T) {
 	client, err := New(cfg)
 	require.NoError(t, err)
 
-	err = client.Do(context.Background(), "GET", "/a", nil, nil, bytes.Buffer{})
+	err = client.Do(
+		context.Background(),
+		"GET",
+		"/a",
+		nil,
+		nil,
+		nil,
+		bytes.Buffer{},
+	)
 	require.NoError(t, err)
 	require.True(t, calledMock)
 }
@@ -249,9 +330,25 @@ func TestDoRemovesDoubleSlashesFromFilesAPI(t *testing.T) {
 		}),
 	})
 	require.NoError(t, err)
-	err = c.Do(context.Background(), "GET", "/api/2.0/fs/files//Volumes/abc/def/ghi", nil, map[string]any{}, nil)
+	err = c.Do(
+		context.Background(),
+		"GET",
+		"/api/2.0/fs/files//Volumes/abc/def/ghi",
+		nil,
+		nil,
+		map[string]any{},
+		nil,
+	)
 	require.NoError(t, err)
-	err = c.Do(context.Background(), "GET", "/api/2.0/anotherservice//test", nil, map[string]any{}, nil)
+	err = c.Do(
+		context.Background(),
+		"GET",
+		"/api/2.0/anotherservice//test",
+		nil,
+		nil,
+		map[string]any{},
+		nil,
+	)
 	require.NoError(t, err)
 }
 
@@ -340,7 +437,15 @@ func captureUserAgent(t *testing.T) string {
 	})
 	require.NoError(t, err)
 
-	err = c.Do(context.Background(), "GET", "/a", nil, nil, nil)
+	err = c.Do(
+		context.Background(),
+		"GET",
+		"/a",
+		nil,
+		nil,
+		nil,
+		nil,
+	)
 	require.NoError(t, err)
 
 	return userAgent
@@ -450,7 +555,15 @@ func testNonJSONResponseIncludedInError(t *testing.T, statusCode int, status, er
 	})
 	require.NoError(t, err)
 	var m map[string]string
-	err = c.Do(context.Background(), "GET", "/a", nil, nil, &m)
+	err = c.Do(
+		context.Background(),
+		"GET",
+		"/a",
+		nil,
+		nil,
+		nil,
+		&m,
+	)
 	require.EqualError(t, err, errorMessage)
 }
 
@@ -477,6 +590,14 @@ func TestRetryOn503(t *testing.T) {
 		}),
 	})
 	require.NoError(t, err)
-	err = c.Do(context.Background(), "GET", "/a/b", nil, map[string]any{}, nil)
+	err = c.Do(
+		context.Background(),
+		"GET",
+		"/a/b",
+		nil,
+		nil,
+		map[string]any{},
+		nil,
+	)
 	require.NoError(t, err)
 }

httpclient/api_client.go

@@ -111,13 +111,21 @@ type DoOption struct {
 	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
+	var explicitQueryParams map[string]any
 	visitors := c.config.Visitors[:]
 	for _, o := range opts {
+		if o.queryParams != nil {
+			if explicitQueryParams != nil {
+				return fmt.Errorf("only one set of query params is allowed")
+			}
+			explicitQueryParams = o.queryParams
+		}
 		if o.in == nil {
 			continue
 		}
@@ -150,7 +158,7 @@ func (c *ApiClient) Do(ctx context.Context, method, path string, opts ...DoOptio
 		data = o.body
 		contentType = o.contentType
 	}
-	requestBody, err := makeRequestBody(method, &path, data, contentType)
+	requestBody, err := makeRequestBody(method, &path, data, contentType, explicitQueryParams)
 	if err != nil {
 		return fmt.Errorf("request marshal: %w", err)
 	}

httpclient/api_client_test.go

@@ -67,6 +67,24 @@ func TestSimpleRequestFailsURLError(t *testing.T) {
 	require.EqualError(t, err, `Get "/a/b?c=d": nope`)
 }
 
+func TestQueryParameters(t *testing.T) {
+	c := NewApiClient(ClientConfig{
+		RetryTimeout: 1 * time.Millisecond,
+		Transport: hc(func(r *http.Request) (*http.Response, error) {
+			require.Equal(t, "POST", r.Method)
+			require.Equal(t, "/a/b", r.URL.Path)
+			require.Equal(t, "c=d&e=1", r.URL.RawQuery)
+			return nil, fmt.Errorf("nope")
+		}),
+	})
+	err := c.Do(context.Background(), "POST", "/a/b",
+		WithQueryParameters(map[string]any{
+			"c": "d",
+			"e": 1,
+		}))
+	require.EqualError(t, err, `Post "/a/b?c=d&e=1": nope`)
+}
+
 func TestSimpleRequestFailsAPIError(t *testing.T) {
 	c := NewApiClient(ClientConfig{
 		Transport: hc(func(r *http.Request) (*http.Response, error) {

httpclient/request.go

@@ -73,6 +73,18 @@ func WithRequestData(body any) DoOption {
 	}
 }
 
+// WithQueryParameters takes a map and sends it as query string for non GET/DELETE/HEAD calls.
+// This is ignored for GET/DELETE/HEAD calls, as the query parameters are serialized from the body instead.
+//
+// Experimental: this method may eventually be split into more granular options.
+func WithQueryParameters(queryParams map[string]any) DoOption {
+	// refactor this, so that we split JSON/query string serialization and make
+	// separate request visitors internally.
+	return DoOption{
+		queryParams: queryParams,
+	}
+}
+
 // WithUrlEncodedData takes either a struct instance, map, string, bytes, or io.Reader plus
 // a content type, and sends it either as query string for GET and DELETE calls, or as request body
 // for POST, PUT, and PATCH calls. The content type is set to "application/x-www-form-urlencoded"
@@ -148,24 +160,41 @@ func EncodeMultiSegmentPathParameter(p string) string {
 	return b.String()
 }
 
-func makeRequestBody(method string, requestURL *string, data interface{}, contentType string) (common.RequestBody, error) {
-	if data == nil {
+// We used to not send any query parameters for non GET/DELETE/HEAD requests.
+// Moreover, serialization for query paramters in GET/DELETE/HEAD requests depends on the `url` tag.
+// This tag is wrongly generated and fixing it will have an unknown inpact on the SDK.
+// So:
+// * GET/DELETE/HEAD requests are sent with query parameters serialized from data using the `url` tag as before (no change).
+// * The rest of the requests are sent with query parameters serialized from explicitQueryParams, which does not use the `url` tag.
+// TODO: For SDK-Mod, refactor this and remove the `url` tag completely.
+func makeRequestBody(method string, requestURL *string, data interface{}, contentType string, explicitQueryParams map[string]any) (common.RequestBody, error) {
+	if data == nil && len(explicitQueryParams) == 0 {
 		return common.RequestBody{}, nil
 	}
-	if method == "GET" || method == "DELETE" || method == "HEAD" {
-		qs, err := makeQueryString(data)
-		if err != nil {
-			return common.RequestBody{}, err
+	if data != nil {
+		if method == "GET" || method == "DELETE" || method == "HEAD" {
+			qs, err := makeQueryString(data)
+			if err != nil {
+				return common.RequestBody{}, err
+			}
+			*requestURL += "?" + qs
+			return common.NewRequestBody([]byte{})
+		}
+		if contentType == UrlEncodedContentType {
+			qs, err := makeQueryString(data)
+			if err != nil {
+				return common.RequestBody{}, err
+			}
+			return common.NewRequestBody(qs)
 		}
-		*requestURL += "?" + qs
-		return common.NewRequestBody([]byte{})
 	}
-	if contentType == UrlEncodedContentType {
-		qs, err := makeQueryString(data)
+	if len(explicitQueryParams) > 0 {
+		qs, err := makeQueryString(explicitQueryParams)
 		if err != nil {
 			return common.RequestBody{}, err
 		}
-		return common.NewRequestBody(qs)
+		*requestURL += "?" + qs
+		return common.NewRequestBody(data)
 	}
 	return common.NewRequestBody(data)
 }