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

Paginate all SCIM list requests in the SDK #693

Closed
wants to merge 1 commit into from

Conversation

xinjiezhen-db
Copy link

Changes

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

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).
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 passing
  • make fmt applied
  • relevant integration tests applied

@xinjiezhen-db xinjiezhen-db requested review from nfx and mgyucht November 16, 2023 00:14
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1d2ce5a) 15.87% compared to head (018a5a4) 15.87%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #693   +/-   ##
=======================================
  Coverage   15.87%   15.87%           
=======================================
  Files          94       94           
  Lines       13773    13773           
=======================================
  Hits         2186     2186           
  Misses      11402    11402           
  Partials      185      185           

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

@mgyucht
Copy link
Contributor

mgyucht commented Nov 28, 2023

Continuing with this change in #717

@mgyucht mgyucht closed this Nov 28, 2023
github-merge-queue bot pushed a commit that referenced this pull request Nov 28, 2023
## Changes
This PR sets default parameters for the SCIM pagination APIs,
specifically setting StartIndex to 1 (as specified in the [SCIM
RFC](https://datatracker.ietf.org/doc/html/rfc7644#section-3.4.2),
startindex is 1-based index) and Count to 100 to ensure that we paginate
requests to the REST API. This should help reduce load on the SCIM APIs
by decreasing the size of response bodies for users of this SDK.

This change supersedes
#693.

## Tests
<!-- 
How is this tested? Please see the checklist below and also describe any
other relevant tests
-->

- [ ] `make test` passing
- [ ] `make fmt` applied
- [ ] relevant integration tests applied
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