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

930 Introduced V4 DB endpoints with support for cursor pagination. #4110

Merged
merged 12 commits into from
Jun 27, 2024

Conversation

albertisfu
Copy link
Contributor

@albertisfu albertisfu commented Jun 14, 2024

This PR introduces V4 of the API for DB endpoints and adds cursor pagination for sorting keys that support custom pagination according to issue #930.

To avoid duplicating Routes and Viewsets, cursor pagination support was added to the DEFAULT_PAGINATION_CLASS, which I renamed to VersionBasedPagination. This class determines when to apply cursor pagination or page number pagination based on the API version and the ordering keys that support cursor pagination.

For V3 endpoints, everything remains the same. They will always use page number pagination, and the deep pagination limits are unchanged. For V4 endpoints, cursor pagination applies only to supported ordering keys, and if an ordering key is not supported, it uses page number pagination with the same deep pagination limits as in V3.

The default cursor pagination ordering key is defined within each ViewSet as:
ordering = "-id"
This matches the same default ordering key (when order_by is not provided) as in V3, since most of them use id and when a different sorting key is used, it is a datetime field, which is also supported for cursor pagination.

Other supported cursor pagination ordering keys are also defined in each ViewSet as needed in other_cursor_ordering_keys, for instance:

other_cursor_ordering_keys = [
        "id",
        "date_created",
        "-date_created",
        "date_modified",
        "-date_modified",
    ]

These additional ordering keys matches the same available ordering keys for each ViewSet in V3 that can support cursor pagination.
Most of the endpoints supports the following cursor pagination ordering keys:

 [
        "-id"
        "id",
        "date_created",
        "-date_created",
        "date_modified",
        "-date_modified",
]

Endpoints with differences are:

SearchAlertViewSet
default ordering: -date_created
other_cursor_ordering_keys = ["date_created"]

DocketAlertViewSet
default ordering: -date_created
other_cursor_ordering_keys = ["date_created"]

DocketTagViewSet
default ordering: -id
other_cursor_ordering_keys = ["id"]

SourceViewSet
default ordering: -id
other_cursor_ordering_keys = ["id", "date_modified", "-date_modified"]

PacerFetchRequestViewSet
default ordering: -id
other_cursor_ordering_keys = [
"id",
"date_created",
"-date_created",
"date_modified",
"-date_modified",
"date_completed",
"-date_completed",
]

OpinionsCitedViewSet
default ordering: -id
other_cursor_ordering_keys = ["id"]

There are some endpoints that doesn't support cursor pagination at all.

CourtViewSet: This Viewset directly uses the PageNumberPagination pagination_class so deep pagination is possible, considering there is a limited number of courts, this is not an issue and Cursor pagination is not required either.
PacerDocIdLookupViewSet: This is the recap-query endpoint used in the extension. I asked Eduardo and told me that it just return a limited number of items so cursor pagination is not required.
memberships: This endpoint only allow POST, so pagination is not required.
citation-lookup: This endpoint is limited to return up to 250 items and also cursor pagination is not possible since the response is not directly returned from DB.

  • In brief, every endpoint that supports cursor pagination will use a cursor if the order_by key is not provided (default ordering key like -id ) or if the provided key supports cursor pagination.

  • To avoid raising an unhandled error when a user changes the ordering key and uses a cursor created for a different ordering, I added a check that compares the current cursor position (ordering type) and the requested ordering key type (e.g., from id to date_created). If they are the same type, the request is allowed; otherwise, a 404 status code and "Invalid cursor" message are returned. The original behavior in this scenario resulted in a 500 error due to the cursor being incompatible with the newly requested sorting key.

  • There can be inconsistencies in the results if the user changes the ordering key to one of the same type. For instance, if the cursor was originally requested for id and the ordering key is changed to -id without clearing the cursor, the request will work, but the user may see inconsistencies in the results, such as duplicated items. This issue is unavoidable because the cursor does not store the sorting key used directly, so making direct comparison is not possible. This could be documented, advising that before performing a request with a new ordering key when using cursor pagination, the previous cursor should be removed from the request.

  • Additionally, it is possible for a user to start paginating using a cursor and then switch to an ordering field that does not support cursor pagination. In this case, the cursor parameter will be ignored (if it is still in the request), and the page request will start from page 1. Similarly, if the user starts with page number pagination and then switches to a cursor-supported ordering key, it will start from the first cursor page, and the page parameter will be ignored.

    This can be an issue if we want to support both cursor pagination and page number pagination for keys that currently only support cursor pagination, but for now, this is not a problem.

Let me know what do you think.

cl/api/pagination.py Fixed Show fixed Hide fixed
@albertisfu albertisfu force-pushed the 930-v4-search-api-cursor-pagination-db-endpoints branch from ca67bdb to 03ac230 Compare June 14, 2024 20:19
@albertisfu albertisfu marked this pull request as ready for review June 14, 2024 23:29
@albertisfu albertisfu requested a review from mlissner June 14, 2024 23:29
Copy link
Member

@mlissner mlissner left a comment

Choose a reason for hiding this comment

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

Looks good. A couple little comments, and I haven't reviewed the tests, but it looks like a good solution to the goal.

Can you make a note somewhere in the code about what it will take to remove v3 in the future, so that you'll have an easier time of it when we finally deprecate v3?

cl/api/pagination.py Outdated Show resolved Hide resolved
cl/audio/api_views.py Outdated Show resolved Hide resolved
@albertisfu
Copy link
Contributor Author

Thanks! I've applied the suggestions above.

Can you make a note somewhere in the code about what it will take to remove v3 in the future, so that you'll have an easier time of it when we finally deprecate v3?

Sure, I added the note in cl.api.urls.

@mlissner
Copy link
Member

All yours, Eduardo. Thanks Alberto!

@ERosendo
Copy link
Contributor

LGTM. We can merge the PR after addressing my question about the date_completed field.

@albertisfu
Copy link
Contributor Author

I've addressed the issue pointed out by @ERosendo and improved the tests so similar errors can be caught in the future.
I think this ready for merge @mlissner

Copy link
Member

@mlissner mlissner left a comment

Choose a reason for hiding this comment

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

LGTM. Let's do this.

@mlissner mlissner enabled auto-merge June 27, 2024 15:39
@mlissner
Copy link
Member

Auto-merge set.

@mlissner mlissner merged commit 2c8c581 into main Jun 27, 2024
13 checks passed
@mlissner mlissner deleted the 930-v4-search-api-cursor-pagination-db-endpoints branch June 27, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants