Handle nested query parameters in ApiClient.withQuery #125
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tanmay-db
approved these changes
Jul 26, 2023
Comment on lines
+74
to
+92
| private static final List<Class<?>> primitiveTypes = | ||
| Arrays.asList( | ||
| boolean.class, | ||
| Boolean.class, | ||
| byte.class, | ||
| Byte.class, | ||
| char.class, | ||
| Character.class, | ||
| short.class, | ||
| Short.class, | ||
| int.class, | ||
| Integer.class, | ||
| long.class, | ||
| Long.class, | ||
| float.class, | ||
| Float.class, | ||
| double.class, | ||
| Double.class, | ||
| String.class); |
There was a problem hiding this comment.
We can probably reuse this in future, it might be good to have this in a separate file.
There was a problem hiding this comment.
If we need to reuse it, we can make it public at some point. It's hard to figure out what class/file this would exist in that would be useful in other contexts, so I would rather leave it as is until we have two or three other things that it makes sense to group this with.
Comment on lines
43
to
48
| List<String> values = query.get(key); | ||
| if (values == null) { | ||
| values = new ArrayList<>(); | ||
| } | ||
| values.add(value); | ||
| query.put(key, values); |
There was a problem hiding this comment.
Should we handle the use case for when key exists?
There was a problem hiding this comment.
if the key exists, values is not null, so we just skip the if block. Note that I've refactored this, as the put at the end is only needed in the case that values is null.
Comment on lines
+74
to
+76
| if (currentPage == null) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Curious -- why wasn't this required before?
There was a problem hiding this comment.
This is kind of a separate bugfix, but I think that the query history API does not include the "results" field at all if there are no results. This is handled in flipNextPage line 63, but not in hasNext().
mgyucht
added a commit
that referenced
this pull request
Jul 27, 2023
* Handle nested query parameters in ApiClient.withQuery ([#125](#125)). This fixes issues with filters when listing query history. API Changes: * Changed `create()` method for `accountClient.metastoreAssignments()` service to no longer return `com.databricks.sdk.service.catalog.CreateMetastoreAssignmentsResponseItemList` class. * Added `connectionName` field for `com.databricks.sdk.service.catalog.CreateCatalog`. * Added `accessPoint` field for `com.databricks.sdk.service.catalog.CreateExternalLocation`. * Added `encryptionDetails` field for `com.databricks.sdk.service.catalog.CreateExternalLocation`. * Removed `com.databricks.sdk.service.catalog.CreateMetastoreAssignmentsResponseItem` class. * Added `accessPoint` field for `com.databricks.sdk.service.catalog.ExternalLocationInfo`. * Added `encryptionDetails` field for `com.databricks.sdk.service.catalog.ExternalLocationInfo`. * Added `accessPoint` field for `com.databricks.sdk.service.catalog.TableInfo`. * Added `encryptionDetails` field for `com.databricks.sdk.service.catalog.TableInfo`. * Added `accessPoint` field for `com.databricks.sdk.service.catalog.UpdateExternalLocation`. * Added `encryptionDetails` field for `com.databricks.sdk.service.catalog.UpdateExternalLocation`. * Added `accessPoint` field for `com.databricks.sdk.service.catalog.VolumeInfo`. * Added `encryptionDetails` field for `com.databricks.sdk.service.catalog.VolumeInfo`. * Added `com.databricks.sdk.service.catalog.EncryptionDetails` class. * Added `com.databricks.sdk.service.catalog.SseEncryptionDetails` class. * Added `com.databricks.sdk.service.catalog.SseEncryptionDetailsAlgorithm` class. * Added `accountClient.networkPolicy()` service. * Added `com.databricks.sdk.service.settings.AccountNetworkPolicyMessage` class. * Added `com.databricks.sdk.service.settings.DeleteAccountNetworkPolicyRequest` class. * Added `com.databricks.sdk.service.settings.DeleteAccountNetworkPolicyResponse` class. * Added `com.databricks.sdk.service.settings.ReadAccountNetworkPolicyRequest` class. * Added `com.databricks.sdk.service.settings.UpdateAccountNetworkPolicyRequest` class. OpenAPI SHA: fbdd0fa3e83fed2c798a58d376529bdb1285b915, Date: 2023-07-26
Merged
mgyucht
added a commit
that referenced
this pull request
Jul 27, 2023
* Handled nested query parameters in ApiClient.withQuery ([#125](#125)). This fixes issues with filters when listing query history. API Changes: * Changed `create()` method for `accountClient.metastoreAssignments()` service to no longer return `com.databricks.sdk.service.catalog.CreateMetastoreAssignmentsResponseItemList` class. * Added `connectionName` field for `com.databricks.sdk.service.catalog.CreateCatalog`. * Added `accessPoint` field for `com.databricks.sdk.service.catalog.CreateExternalLocation`. * Added `encryptionDetails` field for `com.databricks.sdk.service.catalog.CreateExternalLocation`. * Removed `com.databricks.sdk.service.catalog.CreateMetastoreAssignmentsResponseItem` class. * Added `accessPoint` field for `com.databricks.sdk.service.catalog.ExternalLocationInfo`. * Added `encryptionDetails` field for `com.databricks.sdk.service.catalog.ExternalLocationInfo`. * Added `accessPoint` field for `com.databricks.sdk.service.catalog.TableInfo`. * Added `encryptionDetails` field for `com.databricks.sdk.service.catalog.TableInfo`. * Added `accessPoint` field for `com.databricks.sdk.service.catalog.UpdateExternalLocation`. * Added `encryptionDetails` field for `com.databricks.sdk.service.catalog.UpdateExternalLocation`. * Added `accessPoint` field for `com.databricks.sdk.service.catalog.VolumeInfo`. * Added `encryptionDetails` field for `com.databricks.sdk.service.catalog.VolumeInfo`. * Added `com.databricks.sdk.service.catalog.EncryptionDetails` class. * Added `com.databricks.sdk.service.catalog.SseEncryptionDetails` class. * Added `com.databricks.sdk.service.catalog.SseEncryptionDetailsAlgorithm` class. * Added `accountClient.networkPolicy()` service. * Added `com.databricks.sdk.service.settings.AccountNetworkPolicyMessage` class. * Added `com.databricks.sdk.service.settings.DeleteAccountNetworkPolicyRequest` class. * Added `com.databricks.sdk.service.settings.DeleteAccountNetworkPolicyResponse` class. * Added `com.databricks.sdk.service.settings.ReadAccountNetworkPolicyRequest` class. * Added `com.databricks.sdk.service.settings.UpdateAccountNetworkPolicyRequest` class. OpenAPI SHA: fbdd0fa3e83fed2c798a58d376529bdb1285b915, Date: 2023-07-26 --------- Co-authored-by: Tanmay Rustagi <88379306+tanmay-db@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
becomes
For this to be compatible with the requests library we use today, we need to recursively compute the path of each field in the request object that is annotated with the
QueryParamannotation, then serialize the value according to Jackson.As part of this, I've also generalized the
Requestclass to support repeated query parameter values for lists (see the added integration test).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.
Tests
Added an integration test for SQL Query History API.