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-2664] Add support for checkpoint pagination #362

Merged
merged 3 commits into from
Jul 26, 2021

Conversation

jimmyjames
Copy link
Contributor

Changes

This change adds checkpoint pagination support to the following APIs:

  • OrganizationEntity#list(PageFilter filter)
  • OrganizationEntity#getMembers(String orgId, PageFilter filter)
  • RolesEntity#listUsers(String roleId, PageFilter filter)

While the methods listed above are not changed directly, the following changes enable supporting checkpoint pagination:

  • Added new methods withFrom(String from) and withTake(int take) to PageFilter to support the from and take query parameters.
  • Added a new field to the Page POJO for the next field that may be returned when using checkpoint pagination, along with a new constructor that accepts the next parameter.
  • Added a new method to PageDeserializer to create a Page with the next parameter. Unlike the other methods in this class, this new method is not abstract, as doing so would require all subclasses to implement this method, and would be a breaking change. Instead, the new method delegates to the existing method to create the Page, and any subclasses that need to support checkpoint pagination will need to override this method to construct the Page with the next field.

References

In addition to SDK-2664, the following API2 endpoints can be consulted for documentation on the checkpoint pagination support they offer:

Testing

In addition to the unit tests included, manual testing was also performed with the changes.

@jimmyjames jimmyjames added this to the v1-Next milestone Jul 21, 2021
@jimmyjames jimmyjames force-pushed the add-checkpoint-pagination-support branch from 2c8d90e to 25bfbbe Compare July 21, 2021 21:19
@jimmyjames jimmyjames closed this Jul 21, 2021
@jimmyjames jimmyjames reopened this Jul 21, 2021
@jimmyjames jimmyjames force-pushed the add-checkpoint-pagination-support branch from 25bfbbe to e425ba0 Compare July 21, 2021 22:09
@jimmyjames jimmyjames marked this pull request as ready for review July 21, 2021 22:16
@jimmyjames jimmyjames requested a review from a team as a code owner July 21, 2021 22:16
@@ -23,4 +23,8 @@ public MembersPage(List<Member> items) {
public MembersPage(Integer start, Integer length, Integer total, Integer limit, List<Member> items) {
super(start, length, total, limit, items);
}

public MembersPage(Integer start, Integer length, Integer total, Integer limit, String next, List<Member> items) {
Copy link
Contributor

Choose a reason for hiding this comment

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

who could be using the old constructors today? should we deprecate them, now that we have these? (separate PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The deserializers call these constructors in their overridden createPage implementations. It probably is worth deprecating these like you said. And also worth considering if there are refactorings we could do (likely in a new major) to make the paging deserialization more future-proof to these types of changes. I can deprecate the old constructors in a follow-up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that would be great.

@jimmyjames jimmyjames merged commit 20a9f52 into master Jul 26, 2021
@jimmyjames jimmyjames modified the milestones: v1-Next, 1.33.0 Jul 26, 2021
@jimmyjames jimmyjames mentioned this pull request Jul 26, 2021
@jimmyjames jimmyjames deleted the add-checkpoint-pagination-support branch August 10, 2021 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants