Change from offset & limit to number and size #9207

seanh · 2024-12-12T12:02:15Z

Change the list-memberships API's pagination query params from
page[offset] and page[limit] to page[number] and page[size].

This is a better API design because it doesn't enable the client to send
nonsensical values. For example page[offset]=2 and page[size]=10
is nonsensical because offset isn't a multiple of limit: what would
the offset and limit for the previous page link be?

With page and size this class of nonsensical requests is no longer
possible.

This will make it easier to add first, last, prev and next page links to
the backend's API responses, as was attempted in this PR (not merged):
#9181

If the client supplies a page number that is beyond the end of the
group's list of memberships, rather than an error the server simply
responds with empty data:

{
    "meta": {
        "page": {
            "total": 101
        }
    },
    "data": []
}

The client can use the meta.page.total value to figure out how many
pages there should be and re-render its pagination controls.

(This can happen for example if members leave the group after pagination
controls are rendered: the pagination controls may contain links to
pages that no longer exist.)

Testing

Creating a group with lots of members

Log in as devdata_admin
Create a new group
Create an auth client with:
Authority: localhost
Grant type: client_credentials
Trusted: yes

Run this script to add 100 members to the group:

#!/usr/bin/env python3
import httpx

auth = ("CLIENT_ID", "CLIENT_SECRET")
pubid = "GROUP_PUBID"

for i in range(100):
    httpx.post(
        "http://localhost:5000/api/users",
        auth=auth,
        json={
            "authority": "localhost",
            "username": f"user{i}",
            "email": f"user{i}@example.com",
        },
    )
    httpx.post(
        f"http://localhost:5000/api/groups/{pubid}/members/acct:user{i}@localhost",
        auth=auth,
    )

The script requires httpx so:

python3 -m venv /tmp/venv
/tmp/venv/bin/pip install httpx
/tmp/venv/bin/python script.py

Testing the API

Create an API key then:

$ httpx http://localhost:5000/api/groups/{PUBID}/members --method GET --headers Authorization 'Bearer {APIKEY}' --params page[number] 1 --params page[size] 10

Testing the UI

Go to http://localhost:5000/admin/features and enable the group_members feature flag, then go to the group's edit page and test the pagination controls at the bottom.

seanh · 2024-12-12T12:14:32Z

h/schemas/pagination.py

+        validator=Range(min=1),
+        missing=1,


Starting page number from 1 instead of 0

robertknight · 2024-12-12T13:49:24Z

A related issue: If you are on page 3 with a page size of 20, so the first item has offset 40 and you change the page size to 50, what page do you end up on? There is no 50-item sized page that exactly aligns with offset 40. A possible solution would be to adjust the page number to Math.floor(pageIndex * oldPageSize / newPageSize), so you'll end up on the first page which contains the item that was previously at the top.

robertknight

LGTM. There are a few variable names and comments in the frontend that will need an update.

robertknight · 2024-12-12T13:42:15Z

h/static/scripts/group-forms/components/EditGroupMembersForm.tsx

@@ -191,7 +191,7 @@ export default function EditGroupMembersForm({
  const config = useContext(Config)!;
  const currentUserid = config.context.user.userid;

-  const [pageIndex, setPageIndex] = useState(0);
+  const [pageIndex, setPageIndex] = useState(1);


These should be renamed to pageNumber/setPageNumber to reflect the change to 1-based numbers.

robertknight · 2024-12-12T13:42:18Z

h/static/scripts/group-forms/components/EditGroupMembersForm.tsx

-              currentPage={pageIndex + 1}
-              onChangePage={page => setPageIndex(page - 1)}
+              currentPage={pageIndex}
+              onChangePage={page => setPageIndex(page)}


This can be simplified to onChangePage={setPageNumber} after renaming mentioned above.

robertknight · 2024-12-12T13:43:21Z

h/static/scripts/group-forms/components/EditGroupMembersForm.tsx

-      offset: pageIndex * pageSize,
-      limit: pageSize,
+      pageNumber: pageIndex,
+      pageSize: pageSize,


Options can be simplified to { pageNumber, pageSize, signal: abort.signal } after the renaming mentioned above.

robertknight · 2024-12-12T14:08:46Z

h/static/scripts/group-forms/utils/api.ts

@@ -113,10 +113,10 @@ export type APIOptions = {
  signal?: AbortSignal;

  /** Index of first item to return in paginated APIs. */


This comment needs to be updated to eg. "1-based number of first page to return ..."

Copilot reviewed 5 out of 9 changed files in this pull request and generated no suggestions.

Files not reviewed (4)

h/static/scripts/group-forms/utils/api.ts: Evaluated as low risk
h/static/scripts/group-forms/utils/test/api-test.js: Evaluated as low risk
h/static/scripts/group-forms/components/test/EditGroupMembersForm-test.js: Evaluated as low risk
h/views/api/group_members.py: Evaluated as low risk

Comments skipped due to low confidence (2)

tests/functional/api/groups/members_test.py:100

The use of 'offset' in the 'group_members_service.get_memberships' call should be reviewed to ensure it aligns with the new pagination logic. Consider updating it to use 'pageNumber' and 'pageSize'.

context.group, offset=4, limit=2

tests/functional/api/groups/members_test.py:282

The error message should be updated to reflect the new parameters 'page[number]' and 'page[size]'.

"reason": "page[offset]: -1 is less than minimum value 0\npage[limit]: 0 is less than minimum value 1"

Change the list-memberships API's pagination query params from `page[offset]` and `page[limit]` to `page[number]` and `page[size]`. This is a better API design because it doesn't enable the client to send nonsensical values. For example `page[offset]=2` and `page[size]=10` is nonsensical because `offset` isn't a multiple of `limit`: what would the `offset` and `limit` for the previous page link be? With `page` and `size` this class of nonsensical requests is no longer possible. This will make it easier to add first, last, prev and next page links to the backend's API responses, as was attempted in this PR (not merged): #9181 If the client supplies a page number that is beyond the end of the group's list of memberships, rather than an error the server simply responds with empty date: { "meta": { "page": { "total": 101 } }, "data": [] } The client can use the `meta.page.total` value to figure out how many pages there should be and re-render its pagination controls. (This can happen for example if members leave the group after pagination controls are rendered: the pagination controls may contain links to pages that no longer exist.)

seanh requested a review from robertknight December 12, 2024 12:02

seanh force-pushed the page-and-size branch from 57e56e4 to f5802b1 Compare December 12, 2024 12:10

seanh commented Dec 12, 2024

View reviewed changes

seanh force-pushed the page-and-size branch from f5802b1 to 3192f4e Compare December 12, 2024 12:16

seanh marked this pull request as ready for review December 12, 2024 12:16

robertknight approved these changes Dec 12, 2024

View reviewed changes

seanh requested a review from Copilot December 12, 2024 14:49

Copilot AI reviewed Dec 12, 2024

View reviewed changes

seanh mentioned this pull request Dec 12, 2024

Update docs with recent changes to membership APIs #9210

Merged

seanh force-pushed the page-and-size branch from 3192f4e to d553337 Compare January 7, 2025 17:01

seanh force-pushed the page-and-size branch from d553337 to 801a7a0 Compare January 7, 2025 17:11

seanh merged commit 506e82d into main Jan 7, 2025
10 checks passed

seanh deleted the page-and-size branch January 7, 2025 17:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change from offset & limit to number and size #9207

Change from offset & limit to number and size #9207

seanh commented Dec 12, 2024 •

edited

Loading

seanh Dec 12, 2024

robertknight commented Dec 12, 2024

robertknight left a comment

robertknight Dec 12, 2024

seanh Jan 7, 2025

robertknight Dec 12, 2024

seanh Jan 7, 2025

robertknight Dec 12, 2024

seanh Jan 7, 2025

robertknight Dec 12, 2024

seanh Jan 7, 2025

		@@ -113,10 +113,10 @@ export type APIOptions = {
		signal?: AbortSignal;

		/** Index of first item to return in paginated APIs. */

Change from offset & limit to number and size #9207

Change from offset & limit to number and size #9207

Conversation

seanh commented Dec 12, 2024 • edited Loading

Testing

Creating a group with lots of members

Testing the API

Testing the UI

Choose a reason for hiding this comment

robertknight commented Dec 12, 2024

robertknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh commented Dec 12, 2024 •

edited

Loading