Skip to content
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.

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

Handle nested query parameters in Client.attempt #559

Merged
merged 4 commits into from
Jul 27, 2023

Conversation

mgyucht
Copy link
Contributor

@mgyucht mgyucht commented Jul 25, 2023

Changes

The Query History list API filter_by query parameter is modeled by a dictionary, rather than a primitive type, defining the allowed filters including query_start_time_range, statuses, user_ids, and warehouse_ids. To be compatible with gRPC transcoding, query parameters modeled by message types as opposed to primitives need to be separated into one query parameter per nested field, where the key is the path to that query parameter. For example:

ListQueryHistoryRequest{
  FilterBy: QueryFilter{
    UserIds: []int{123, 456}
  }
}

becomes

filter_by.user_ids=123&filter_by.user_ids=456

go-querystring already flattens this but to a different form:

filter_by[user_ids]=123&filter_by[user_ids]=456

To fix this, we replace [ with . and ] with empty string.

This resolves one of the problems from databricks/databricks-sdk-py#99. The issue with the conflict between filter_by and next_page_token will be resolved by the backend service.

Separately, there is an underlying issue affecting the Go SDK specifically, as we delegate query parameter serialization to the go-querystring library. This library looks at the url struct tag to name the query parameters appropriately. However, schemas defined in Components in OpenAPI are initially cached with IsQuery for all fields = false. As we parse operations, we incrementally discover which entities are present in query parameters, and we need to update the underlying types recursively.

Lastly, this includes a bugfix to support the newest version of Delve.

Tests

Added an integration test for ListQueryHistory.

  • make test passing
  • make fmt applied
  • relevant integration tests applied

@@ -154,7 +154,7 @@ func skipf(t *testing.T) func(format string, args ...any) {
// detects if test is run from "debug test" feature in VSCode
func isInDebug() bool {
ex, _ := os.Executable()
return path.Base(ex) == "__debug_bin"
return strings.HasPrefix(path.Base(ex), "__debug_bin")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small bugfix to support newest version of Delve (see databricks/terraform-provider-databricks#2497)

@mgyucht mgyucht requested a review from tanmay-db July 25, 2023 10:58
@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2023

Codecov Report

Patch coverage: 88.23% and project coverage change: +0.10% 🎉

Comparison is base (7d56e87) 18.62% compared to head (9d9f7d3) 18.73%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #559      +/-   ##
==========================================
+ Coverage   18.62%   18.73%   +0.10%     
==========================================
  Files          85       85              
  Lines        9373     9388      +15     
==========================================
+ Hits         1746     1759      +13     
- Misses       7476     7477       +1     
- Partials      151      152       +1     
Files Changed Coverage Δ
service/iam/model.go 0.00% <ø> (ø)
service/sql/model.go 0.00% <ø> (ø)
openapi/code/entity.go 28.40% <60.00%> (+1.90%) ⬆️
client/client.go 79.72% <100.00%> (+0.37%) ⬆️
openapi/code/service.go 46.51% <100.00%> (+1.93%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mgyucht mgyucht merged commit cf4ac71 into main Jul 27, 2023
3 checks passed
@mgyucht mgyucht deleted the serialize-nested-query-params branch July 27, 2023 06:26
@mgyucht mgyucht mentioned this pull request Jul 27, 2023
mgyucht added a commit that referenced this pull request Jul 27, 2023
* Handle nested query parameters in Client.attempt ([#559](#559)).
* Support x-databricks-path-style overrides at the operation level ([#562](#562)).
mgyucht added a commit that referenced this pull request Jul 27, 2023
* Handled nested query parameters in Client.attempt
([#559](#559)).
* Supported x-databricks-path-style overrides at the operation level
([#562](#562)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants