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.

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

Jump to bottom

Paginate all SCIM list requests in the SDK #440

Merged
merged 7 commits into from
Nov 14, 2023
Merged

Paginate all SCIM list requests in the SDK #440

merged 7 commits into from
Nov 14, 2023

Conversation

mgyucht
Copy link
Contributor

@mgyucht mgyucht commented Nov 14, 2023

Changes

This PR incorporates two hard-coded changes for the SCIM API in the Python SDK:

  1. startIndex starts at 1 for SCIM APIs, not 0. However, the existing .Pagination.Increment controls both the start index as well as whether the pagination is per-page or per-resource. Later, we should replace this extension with two independent OpenAPI options: one_indexed (defaulting to false) and pagination_basis (defaulting to resource but can be overridden to page).
  2. If users don't specify a limit, the SDK will include a hard-coded limit of 100 resources per request. We could add this to the OpenAPI spec as an option default_limit, which is useful for any non-paginated APIs that later expose pagination options and allow the SDK to gracefully support those. However, we don't want to encourage folks to use this pattern: all new list APIs are required to be paginated from the start.

Tests

  • make test run locally
  • make fmt applied
  • relevant integration tests applied

@mgyucht mgyucht mentioned this pull request Nov 14, 2023
Closed
3 tasks
@mgyucht mgyucht requested a review from tanmay-db November 14, 2023 10:27
nfx
nfx approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@nfx nfx left a comment

Choose a reason for hiding this comment

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

thank you for adding this. few nits

tests/integration/test_iam.py Outdated
Comment on lines 19 to 21
("/api/2.0/preview/scim/v2/Users", lambda w: w.users.list(count=1)),
("/api/2.0/preview/scim/v2/Groups", lambda w: w.groups.list(count=1)),
("/api/2.0/preview/scim/v2/ServicePrincipals", lambda w: w.service_principals.list(count=1)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
("/api/2.0/preview/scim/v2/Users", lambda w: w.users.list(count=1)),
("/api/2.0/preview/scim/v2/Groups", lambda w: w.groups.list(count=1)),
("/api/2.0/preview/scim/v2/ServicePrincipals", lambda w: w.service_principals.list(count=1)),
("/api/2.0/preview/scim/v2/Users", lambda w: w.users.list(count=10)),
("/api/2.0/preview/scim/v2/Groups", lambda w: w.groups.list(count=10)),
("/api/2.0/preview/scim/v2/ServicePrincipals", lambda w: w.service_principals.list(count=10)),

i think count=1 would make integration tests too slow...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this, but I wanted to make sure that we actually exercised the pagination flows. This test took around 15 seconds for all test cases combined.

tests/integration/test_iam.py Outdated


@pytest.mark.parametrize("path,call", [
# there are ~7k users in our aws prod account
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# there are ~7k users in our aws prod account

i don't think it should be in public code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

excellent point

tests/integration/test_iam.py Outdated
# there are ~7k users in our aws prod account
("/api/2.0/accounts/%s/scim/v2/Users", lambda a: a.users.list(count=1000)),
("/api/2.0/accounts/%s/scim/v2/Groups", lambda a: a.groups.list(count=1)),
# there are ~3k service principals in our aws prod account
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# there are ~3k service principals in our aws prod account

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent point

tanmay-db
tanmay-db approved these changes Nov 14, 2023
View reviewed changes
databricks/sdk/service/iam.py
@@ -1182,7 +1182,8 @@ def list(self,

# deduplicate items that may have been added during iteration
seen = set()
query['startIndex'] = 0
query['startIndex'] = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this because the first item starts from number 1 in query? https://docs.databricks.com/api/workspace/groups/list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's right.

@mgyucht mgyucht enabled auto-merge November 14, 2023 11:37
@mgyucht mgyucht added this pull request to the merge queue Nov 14, 2023
Merged via the queue into main with commit 9ba48cc Nov 14, 2023
7 checks passed
@mgyucht mgyucht deleted the feature/scim-pagination branch November 14, 2023 11:40
mgyucht added a commit that referenced this pull request Nov 14, 2023
@mgyucht
Release v0.13.0
9747f59 
* Introduce more specific exceptions, like `NotFound`, `AlreadyExists`, `BadRequest`, `PermissionDenied`, `InternalError`, and others ([#376](#376)). This makes it easier to handle errors thrown by the Databricks API. Instead of catching `DatabricksError` and checking the error_code field, you can catch one of these subtypes of `DatabricksError`, which is more ergonomic and removes the need to rethrow exceptions that you don't want to catch. For example:
```python
try:
  return (self._ws
    .permissions
    .get(object_type, object_id))
except DatabricksError as e:
  if e.error_code in [
    "RESOURCE_DOES_NOT_EXIST",
    "RESOURCE_NOT_FOUND",
    "PERMISSION_DENIED",
    "FEATURE_DISABLED",
    "BAD_REQUEST"]:
    logger.warning(...)
    return None
  raise RetryableError(...) from e
```
can be replaced with
```python
try:
  return (self._ws
    .permissions
    .get(object_type, object_id))
except PermissionDenied, FeatureDisabled:
  logger.warning(...)
  return None
except NotFound:
  raise RetryableError(...)
```
* Paginate all SCIM list requests in the SDK ([#440](#440)). This change ensures that SCIM list() APIs use a default limit of 100 resources, leveraging SCIM's offset + limit pagination to batch requests to the Databricks API.
* Added taskValues support in remoteDbUtils ([#406](#406)).
* Added more detailed error message on default credentials not found error ([#419](#419)).
* Request management token via Azure CLI only for Service Principals and not human users ([#408](#408)).

API Changes:

 * Fixed `create()` method for [w.functions](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/functions.html) workspace-level service and corresponding `databricks.sdk.service.catalog.CreateFunction` and `databricks.sdk.service.catalog.FunctionInfo` dataclasses.
 * Changed `create()` method for [w.metastores](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/metastores.html) workspace-level service with new required argument order.
 * Changed `storage_root` field for `databricks.sdk.service.catalog.CreateMetastore` to be optional.
 * Added `skip_validation` field for `databricks.sdk.service.catalog.UpdateExternalLocation`.
 * Added `libraries` field for `databricks.sdk.service.compute.CreatePolicy`, `databricks.sdk.service.compute.EditPolicy` and `databricks.sdk.service.compute.Policy`.
 * Added `init_scripts` field for `databricks.sdk.service.compute.EventDetails`.
 * Added `file` field for `databricks.sdk.service.compute.InitScriptInfo`.
 * Added `zone_id` field for `databricks.sdk.service.compute.InstancePoolGcpAttributes`.
 * Added several dataclasses related to init scripts.
 * Added `databricks.sdk.service.compute.LocalFileInfo` dataclass.
 * Replaced `ui_state` field with `edit_mode` for `databricks.sdk.service.jobs.CreateJob` and `databricks.sdk.service.jobs.JobSettings`.
 * Replaced `databricks.sdk.service.jobs.CreateJobUiState` dataclass with `databricks.sdk.service.jobs.CreateJobEditMode`.
 * Added `include_resolved_values` field for `databricks.sdk.service.jobs.GetRunRequest`.
 * Replaced `databricks.sdk.service.jobs.JobSettingsUiState` dataclass with `databricks.sdk.service.jobs.JobSettingsEditMode`.
 * Removed [a.o_auth_enrollment](https://databricks-sdk-py.readthedocs.io/en/latest/account/o_auth_enrollment.html) account-level service. This was only used to aid in OAuth enablement during the public preview of OAuth. OAuth is now enabled for all AWS E2 accounts, so usage of this API is no longer needed.
 * Added `network_connectivity_config_id` field for `databricks.sdk.service.provisioning.UpdateWorkspaceRequest`.
 * Added [a.network_connectivity](https://databricks-sdk-py.readthedocs.io/en/latest/account/network_connectivity.html) account-level service.
 * Added `string_shared_as` field for `databricks.sdk.service.sharing.SharedDataObject`.

Internal changes:

* Added regression question to issue template ([#414](#414)).
* Made test_auth no longer fail if you have a default profile setup ([#426](#426)).

OpenAPI SHA: d136ad0541f036372601bad9a4382db06c3c912d, Date: 2023-11-14
@mgyucht mgyucht mentioned this pull request Nov 14, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Nov 14, 2023
@mgyucht
Release v0.13.0 (#442)
86ca043 
* Introduce more specific exceptions, like `NotFound`, `AlreadyExists`,
`BadRequest`, `PermissionDenied`, `InternalError`, and others
([#376](#376)). This
makes it easier to handle errors thrown by the Databricks API. Instead
of catching `DatabricksError` and checking the error_code field, you can
catch one of these subtypes of `DatabricksError`, which is more
ergonomic and removes the need to rethrow exceptions that you don't want
to catch. For example:
```python
try:
  return (self._ws
    .permissions
    .get(object_type, object_id))
except DatabricksError as e:
  if e.error_code in [
    "RESOURCE_DOES_NOT_EXIST",
    "RESOURCE_NOT_FOUND",
    "PERMISSION_DENIED",
    "FEATURE_DISABLED",
    "BAD_REQUEST"]:
    logger.warning(...)
    return None
  raise RetryableError(...) from e
```
can be replaced with
```python
try:
  return (self._ws
    .permissions
    .get(object_type, object_id))
except PermissionDenied, FeatureDisabled:
  logger.warning(...)
  return None
except NotFound:
  raise RetryableError(...)
```
* Paginate all SCIM list requests in the SDK
([#440](#440)). This
change ensures that SCIM list() APIs use a default limit of 100
resources, leveraging SCIM's offset + limit pagination to batch requests
to the Databricks API.
* Added taskValues support in remoteDbUtils
([#406](#406)).
* Added more detailed error message on default credentials not found
error
([#419](#419)).
* Request management token via Azure CLI only for Service Principals and
not human users
([#408](#408)).

API Changes:

* Fixed `create()` method for
[w.functions](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/functions.html)
workspace-level service and corresponding
`databricks.sdk.service.catalog.CreateFunction` and
`databricks.sdk.service.catalog.FunctionInfo` dataclasses.
* Changed `create()` method for
[w.metastores](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/metastores.html)
workspace-level service with new required argument order.
* Changed `storage_root` field for
`databricks.sdk.service.catalog.CreateMetastore` to be optional.
* Added `skip_validation` field for
`databricks.sdk.service.catalog.UpdateExternalLocation`.
* Added `libraries` field for
`databricks.sdk.service.compute.CreatePolicy`,
`databricks.sdk.service.compute.EditPolicy` and
`databricks.sdk.service.compute.Policy`.
* Added `init_scripts` field for
`databricks.sdk.service.compute.EventDetails`.
* Added `file` field for
`databricks.sdk.service.compute.InitScriptInfo`.
* Added `zone_id` field for
`databricks.sdk.service.compute.InstancePoolGcpAttributes`.
 * Added several dataclasses related to init scripts.
 * Added `databricks.sdk.service.compute.LocalFileInfo` dataclass.
* Replaced `ui_state` field with `edit_mode` for
`databricks.sdk.service.jobs.CreateJob` and
`databricks.sdk.service.jobs.JobSettings`.
* Replaced `databricks.sdk.service.jobs.CreateJobUiState` dataclass with
`databricks.sdk.service.jobs.CreateJobEditMode`.
* Added `include_resolved_values` field for
`databricks.sdk.service.jobs.GetRunRequest`.
* Replaced `databricks.sdk.service.jobs.JobSettingsUiState` dataclass
with `databricks.sdk.service.jobs.JobSettingsEditMode`.
* Removed
[a.o_auth_enrollment](https://databricks-sdk-py.readthedocs.io/en/latest/account/o_auth_enrollment.html)
account-level service. This was only used to aid in OAuth enablement
during the public preview of OAuth. OAuth is now enabled for all AWS E2
accounts, so usage of this API is no longer needed.
* Added `network_connectivity_config_id` field for
`databricks.sdk.service.provisioning.UpdateWorkspaceRequest`.
* Added
[a.network_connectivity](https://databricks-sdk-py.readthedocs.io/en/latest/account/network_connectivity.html)
account-level service.
* Added `string_shared_as` field for
`databricks.sdk.service.sharing.SharedDataObject`.

Internal changes:

* Added regression question to issue template
([#414](#414)).
* Made test_auth no longer fail if you have a default profile setup
([#426](#426)).

OpenAPI SHA: d136ad0541f036372601bad9a4382db06c3c912d, Date: 2023-11-14
@williamdphillips
Copy link

williamdphillips commented Nov 22, 2023 •
edited
Loading

Hi @mgyucht & @nfx, maybe I am missing something here but I'm no longer able to query users due to hardcoding of startIndex.

I have some code that removes inactive users on workspaces, so it iterates over all users. Previously I was using startIndex and count to get 100 users at a time, then query the next 100 users. Now, when I try to query the next 100 users it still shows the first 100 due to startIndex being hardcoded.

Is this change working as intended?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tanmay-db tanmay-db tanmay-db approved these changes

@nfx nfx nfx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mgyucht @williamdphillips @nfx @tanmay-db @xinjiezhen-db