Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 20, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For external clients (like Terraform), the migration path is to pass nil for GET/HEAD/DELETE requests and to explicitly pass the query parameters here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the normal path.
However, since you have integration tests for each resource in terraform, it would also be possible to pass the query parameters always and validate that the resource has not changed.

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))
Expand Down
169 changes: 145 additions & 24 deletions client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`)
}

Expand All @@ -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)
}
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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")
Expand All @@ -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)
Expand All @@ -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)
}
Expand All @@ -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)
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}

Expand All @@ -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)
}
10 changes: 9 additions & 1 deletion httpclient/api_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
}
Expand Down
18 changes: 18 additions & 0 deletions httpclient/api_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
51 changes: 40 additions & 11 deletions httpclient/request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
}
Loading
Loading