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

[SDK 2665] Update endpoint methods to support 'from' and 'take' checkpoint pagination parameters, where appropriate #278

Merged
merged 12 commits into from
Jul 27, 2021

Conversation

evansims
Copy link
Member

@evansims evansims commented Jul 20, 2021

Changes

This PR updates the following methods to support checkpoint pagination API2 parameters:

  • Roles::list_users()
  • Organizations::all_organizations()
  • Organizations::all_organization_members()

References

  • Please see the internal Jira tickets SDK-2666 and related epic SDK-2653 for details on this initiative.

Testing

  • This change adds unit test coverage
  • This change adds integration test coverage
  • This change has been tested on the latest version of the platform/language or why not

Checklist

@evansims evansims marked this pull request as ready for review July 20, 2021 17:15
@evansims evansims requested a review from a team as a code owner July 20, 2021 17:15
@evansims evansims added the review:tiny Tiny review label Jul 20, 2021
@jimmyjames jimmyjames requested a review from lbalmaceda July 21, 2021 19:02
Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

I'm seeing now that page/per_page could mean the same as from/take. Have you tried what happens if a call that was using the former now uses the latter? Because if that's the case, all these methods could be simplified in either of these two ways:

  • duplicated method signatures, each using its variant.
  • keep the old signature, but when building the request, mapping the old variant to the new one.

Might be worth checking with the team if the old properties are deprecated now that the endpoint supports "checkpoint" pagination.

auth0/v3/management/organizations.py Outdated Show resolved Hide resolved
auth0/v3/management/organizations.py Outdated Show resolved Hide resolved
auth0/v3/management/organizations.py Show resolved Hide resolved
auth0/v3/management/organizations.py Outdated Show resolved Hide resolved
auth0/v3/management/organizations.py Outdated Show resolved Hide resolved
auth0/v3/management/roles.py Outdated Show resolved Hide resolved
auth0/v3/management/roles.py Outdated Show resolved Hide resolved
Co-authored-by: Luciano Balmaceda <[email protected]>
@evansims
Copy link
Member Author

Hey @lbalmaceda 👋 Thanks, I've merged those suggestions.

I'm seeing now that page/per_page could mean the same as from/take.

It's true per_page and take mean effectively the same thing, and each is separately required to activate their respective pagination styles. page and from are unique: page is just an incrementing int for the next page of results, whereas from is (usually) an API-provided UUID to retrieve the next list of results (although /logs treats this slightly differently.) You make the initial request without a from, retrieve the value of next returned from the API response, and make your subsequent request with that value as the from.

This isn't something that could be mapped in the way suggested, unfortunately.

As far as splitting up the endpoint methods for checkpoint pagination variants, we can certainly do that. Personally, this feels like something that should just be handled with method arguments to me — but I'm happy to implement this however you think best suits the SDK.

Have you tried what happens if a call that was using the former now uses the latter?

Checkpoint pagination always takes precedence by the API in handling requests; it falls back to basic pagination when take is missing.

Might be worth checking with the team if the old properties are deprecated now that the endpoint supports "checkpoint" pagination.

No indication from anyone I've spoken with that basic pagination is going anywhere. Checkpoint pagination just addresses the problem of API dataset changes being applied between client API requests, causing a shift in results, which can lead to gaps in data on the client's end.

@evansims evansims merged commit efc398a into master Jul 27, 2021
@evansims evansims deleted the feature/checkpoint-pagination/sdk-2665 branch July 27, 2021 13:15
@lbalmaceda lbalmaceda added this to the v3-Next milestone Jul 27, 2021
@lbalmaceda lbalmaceda modified the milestones: v3-Next, 3.17.0 Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants