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

hectorcast-db · 2025-01-17T10:29:12Z

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

renaudhartert-db · 2025-01-17T11:09:42Z

client/client_test.go

+	},
+		nil,
+		map[string]string{
+			"c": "d",
+		}, nil)


This formatting is hard to read and not consistent, let's do something slightly more idiomatic:

err = c.Do( context.Background(), "GET", "/a/b", map[string]string{"e": "f"}, nil, map[string]string{"c": "d"}, nil, )

Also applies to other tests.

renaudhartert-db · 2025-01-17T13:00:40Z

client/client_test.go

+	},
+		nil,
+		map[string]string{
+			"c": "d",


Can we also cover cases where the value is not a string?

github-actions · 2025-01-17T13:34:12Z

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/sdk-go

Inputs:

PR number: 1124
Commit SHA: 279f0c5d9373a312931a58a4138b47bd08db93c9

Checks will be approved automatically on success.

mgyucht · 2025-01-17T15:32:34Z

httpclient/request_test.go

@@ -141,3 +141,26 @@ func TestEncodeMultiSegmentPathParameter(t *testing.T) {
 	// # and ? should be encoded.
 	assert.Equal(t, "a%23b%3Fc", EncodeMultiSegmentPathParameter("a#b?c"))
 }
+
+func TestMakeRequestBodyExplicitQueryParams(t *testing.T) {
+	type x struct {


To be clear, the first scenario should not happen, IIUC. For GET/HEAD/DELETE requests, extra query parameters are never passed in.

Could the second scenario happen, where the request contains url tags that are overridden by the explicit query parameters?

To be clear, the first scenario should not happen, IIUC. For GET/HEAD/DELETE requests, extra query parameters are never passed in.

Correct.

Could the second scenario happen, where the request contains url tags that are overridden by the explicit query parameters?

The logic to add the params is the same as before (just the source has changed). I am not aware of having a path with url tags in the spec.

As I think about it, I think this can only happen for APIs using google.api.http.body. Protobuf-based APIs that don't use this send all parameters in either the path or body. We could see if there are any OpenAPIs that set x-databricks-body-field and have parameters with "in": "query", since they might have the same behavior.

mgyucht · 2025-01-17T15:36:03Z

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,


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?

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.

## Changes Upgrade Go SDK to 0.56.0 Relevant changes: - Support Query parameters for all HTTP operations (databricks/databricks-sdk-go#1124).

## What changes are proposed in this pull request? #1124 fixed the handling of APIs that use query parameters and a body simultaneously. After that change, query parameters are always sent, even if set to the zero value. This PR addresses this, using the same logic for query parameters as for fields in the body: if a field is set to its zero value, it will be included in the query parameter if it is present in ForceSendFields. ## How is this tested? This behavior is dependend upon in the Terraform provider. I'll be using it in databricks/terraform-provider-databricks#4430 and verifying that there is no behavior change in the generated request. When using this PR in Terraform, tests asserting that the path doesn't include query parameters pass.

hectorcast-db temporarily deployed to test-trigger-is January 17, 2025 10:29 — with GitHub Actions Inactive

hectorcast-db temporarily deployed to test-trigger-is January 17, 2025 10:30 — with GitHub Actions Inactive

hectorcast-db requested review from mgyucht and renaudhartert-db January 17, 2025 10:36

renaudhartert-db reviewed Jan 17, 2025

View reviewed changes

[Fix] Support Query parameters for POST/PATCH operations

279f0c5

hectorcast-db force-pushed the hectorcast-db/fix-query branch from 8972af3 to 279f0c5 Compare January 17, 2025 13:33

hectorcast-db temporarily deployed to test-trigger-is January 17, 2025 13:34 — with GitHub Actions Inactive

hectorcast-db temporarily deployed to test-trigger-is January 17, 2025 13:35 — with GitHub Actions Inactive

mgyucht approved these changes Jan 17, 2025

View reviewed changes

hectorcast-db added this pull request to the merge queue Jan 20, 2025

Merged via the queue into main with commit e079db9 Jan 20, 2025
15 checks passed

hectorcast-db deleted the hectorcast-db/fix-query branch January 20, 2025 15:14

mgyucht mentioned this pull request Jan 22, 2025

[Release] Release v0.56.0 #1134

Merged

mgyucht mentioned this pull request Jan 23, 2025

[Fix] Do not send query parameters when set to zero value #1136

Merged

andrewnester mentioned this pull request Jan 23, 2025

Upgrade Go SDK to 0.56.0 databricks/cli#2214

Merged

github-merge-queue bot pushed a commit to databricks/cli that referenced this pull request Jan 23, 2025

Upgrade Go SDK to 0.56.0 (#2214)

798189e

## Changes Upgrade Go SDK to 0.56.0 Relevant changes: - Support Query parameters for all HTTP operations (databricks/databricks-sdk-go#1124).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

hectorcast-db commented Jan 17, 2025

Uh oh!

renaudhartert-db Jan 17, 2025

Uh oh!

renaudhartert-db Jan 17, 2025

Uh oh!

github-actions bot commented Jan 17, 2025

Uh oh!

mgyucht Jan 17, 2025

Uh oh!

hectorcast-db Jan 20, 2025 •

edited

Loading

Uh oh!

mgyucht Jan 20, 2025

Uh oh!

mgyucht Jan 17, 2025

Uh oh!

hectorcast-db Jan 20, 2025

Uh oh!

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

hectorcast-db commented Jan 17, 2025

What changes are proposed in this pull request?

How is this tested?

Uh oh!

renaudhartert-db Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

renaudhartert-db Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 17, 2025

Uh oh!

mgyucht Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

hectorcast-db Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgyucht Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

mgyucht Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

hectorcast-db Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hectorcast-db Jan 20, 2025 •

edited

Loading