Change from offset & limit to number and size

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.)

h/schemas/pagination.py

@@ -2,15 +2,15 @@
 
 
 class PaginationQueryParamsSchema(Schema):
-    offset = SchemaNode(
+    page = SchemaNode(
         Integer(),
-        name="page[offset]",
-        validator=Range(min=0),
-        missing=0,
+        name="page[number]",
+        validator=Range(min=1),
+        missing=1,
     )
-    limit = SchemaNode(
+    size = SchemaNode(
         Integer(),
-        name="page[limit]",
+        name="page[size]",
         validator=Range(min=1, max=100),
         missing=20,
     )

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

@@ -84,17 +84,17 @@ async function fetchMembers(
   currentUserid: string,
   options: {
     signal: AbortSignal;
-    offset: number;
-    limit: number;
+    pageNumber: number;
+    pageSize: number;
   },
 ): Promise<{ total: number; members: MemberRow[] }> {
-  const { offset, limit, signal } = options;
+  const { pageNumber, pageSize, signal } = options;
   const { url, method, headers } = api;
   const { meta, data }: GroupMembersResponse = await callAPI(url, {
     headers,
-    limit,
+    pageSize,
     method,
-    offset,
+    pageNumber,
     signal,
   });
 
@@ -191,7 +191,7 @@ export default function EditGroupMembersForm({
   const config = useContext(Config)!;
   const currentUserid = config.context.user.userid;
 
-  const [pageIndex, setPageIndex] = useState(0);
+  const [pageNumber, setPageNumber] = useState(1);
   const [totalMembers, setTotalMembers] = useState<number | null>(null);
   const totalPages =
     totalMembers !== null ? Math.ceil(totalMembers / pageSize) : null;
@@ -216,8 +216,8 @@ export default function EditGroupMembersForm({
     const abort = new AbortController();
     setErrorMessage(null);
     fetchMembers(config.api.readGroupMembers, currentUserid, {
-      offset: pageIndex * pageSize,
-      limit: pageSize,
+      pageNumber,
+      pageSize,
       signal: abort.signal,
     })
       .then(({ total, members }) => {
@@ -230,7 +230,7 @@ export default function EditGroupMembersForm({
     return () => {
       abort.abort();
     };
-  }, [config.api.readGroupMembers, currentUserid, pageIndex, setError]);
+  }, [config.api.readGroupMembers, currentUserid, pageNumber, setError]);
 
   const columns: TableColumn<MemberRow>[] = [
     {
@@ -399,8 +399,8 @@ export default function EditGroupMembersForm({
         {typeof totalPages === 'number' && totalPages > 1 && (
           <div className="mt-4 flex justify-center">
             <Pagination
-              currentPage={pageIndex + 1}
-              onChangePage={page => setPageIndex(page - 1)}
+              currentPage={pageNumber}
+              onChangePage={setPageNumber}
               totalPages={totalPages}
             />
           </div>

h/static/scripts/group-forms/components/test/EditGroupMembersForm-test.js

@@ -307,8 +307,8 @@ describe('EditGroupMembersForm', () => {
       fakeCallAPI,
       '/api/groups/1234/members',
       sinon.match({
-        offset: pageSize,
-        limit: pageSize,
+        pageNumber: 2,
+        pageSize: pageSize,
       }),
     );
 

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

@@ -112,11 +112,11 @@ export type APIOptions = {
   headers?: Record<PropertyKey, unknown>;
   signal?: AbortSignal;
 
-  /** Index of first item to return in paginated APIs. */
-  offset?: number;
+  /** 1-based number of first page to return in paginated APIs. */
+  pageNumber?: number;
 
   /** Maximum number of items to return in response for a paginated API. */
-  limit?: number;
+  pageSize?: number;
 };
 
 /** Make an API call and return the parsed JSON body or throw APIError. */
@@ -125,9 +125,9 @@ export async function callAPI<R = unknown>(
   {
     headers = {},
     json = null,
-    limit,
+    pageSize,
     method = 'GET',
-    offset,
+    pageNumber,
     signal,
   }: APIOptions = {},
 ): Promise<R> {
@@ -145,11 +145,11 @@ export async function callAPI<R = unknown>(
   }
 
   const queryParams: Record<string, string | number> = {};
-  if (typeof offset === 'number') {
-    queryParams['page[offset]'] = offset;
+  if (typeof pageNumber === 'number') {
+    queryParams['page[number]'] = pageNumber;
   }
-  if (typeof limit === 'number') {
-    queryParams['page[limit]'] = limit;
+  if (typeof pageSize === 'number') {
+    queryParams['page[size]'] = pageSize;
   }
 
   const requestURL = new URL(url);

h/static/scripts/group-forms/utils/test/api-test.js

@@ -126,14 +126,14 @@ describe('callAPI', () => {
 
   it('adds pagination params to URL', async () => {
     const paginatedURL = new URL(url);
-    const offset = 5;
-    const limit = 10;
-    paginatedURL.searchParams.set('page[offset]', offset);
-    paginatedURL.searchParams.set('page[limit]', limit);
+    const pageNumber = 5;
+    const pageSize = 10;
+    paginatedURL.searchParams.set('page[number]', pageNumber);
+    paginatedURL.searchParams.set('page[size]', pageSize);
     const response = new Response(JSON.stringify({}), { status: 200 });
     fakeFetch.resolves(response);
 
-    await callAPI(url, { offset, limit });
+    await callAPI(url, { pageNumber, pageSize });
 
     assert.calledWith(fakeFetch, paginatedURL.toString());
   });

h/views/api/group_members.py

@@ -31,7 +31,7 @@
 }
 
 
-@api_config(request_param=not_("page[offset]"), **LIST_MEMBERS_API_CONFIG)
+@api_config(request_param=not_("page[number]"), **LIST_MEMBERS_API_CONFIG)
 def list_members_legacy(context: GroupContext, request):
     """Legacy version of the list-members API, maintained for backwards-compatibility."""
 
@@ -55,14 +55,16 @@ def list_members_legacy(context: GroupContext, request):
     ]
 
 
-@api_config(request_param="page[offset]", **LIST_MEMBERS_API_CONFIG)
+@api_config(request_param="page[number]", **LIST_MEMBERS_API_CONFIG)
 def list_members(context: GroupContext, request):
     group = context.group
     group_members_service = request.find_service(name="group_members")
 
     params = validate_query_params(PaginationQueryParamsSchema(), request.params)
-    offset = params["page[offset]"]
-    limit = params["page[limit]"]
+    page_number = params["page[number]"]
+    page_size = params["page[size]"]
+    offset = page_size * (page_number - 1)
+    limit = page_size
 
     total = group_members_service.count_memberships(group)
     memberships = group_members_service.get_memberships(

tests/functional/api/groups/members_test.py

@@ -130,20 +130,20 @@ def test_it_returns_list_of_members_for_restricted_group_without_auth(
                     created=datetime(1970, 1, 1, 0, 0, second),
                     updated=datetime(1970, 1, 2, 0, 0, second),
                 )
-                for second, user in enumerate(factories.User.create_batch(size=4))
+                for second, user in enumerate(factories.User.create_batch(size=9))
             ]
         )
         db_session.commit()
 
         res = app.get(
             "/api/groups/{pubid}/members".format(pubid=group.pubid),
-            params={"page[offset]": 1, "page[limit]": 2},
+            params={"page[number]": 2, "page[size]": 3},
             headers={"User-Agent": "test_user_agent", "Referer": "test_referer"},
         )
 
         assert res.status_code == 200
         assert res.json == {
-            "meta": {"page": {"total": 4}},
+            "meta": {"page": {"total": 9}},
             "data": [
                 {
                     "authority": membership.group.authority,
@@ -155,7 +155,7 @@ def test_it_returns_list_of_members_for_restricted_group_without_auth(
                     "created": f"1970-01-01T00:00:{second:02}.000000+00:00",
                     "updated": f"1970-01-02T00:00:{second:02}.000000+00:00",
                 }
-                for second, membership in list(enumerate(group.memberships))[1:3]
+                for second, membership in list(enumerate(group.memberships))[3:6]
             ],
         }
 
@@ -183,7 +183,7 @@ def test_it_returns_list_of_members_if_user_has_access_to_private_group(
 
         res = app.get(
             "/api/groups/{pubid}/members".format(pubid=group.pubid),
-            params={"page[offset]": 0},
+            params={"page[number]": 1},
             headers=token_authorization_header(token),
         )
 
@@ -214,6 +214,30 @@ def test_it_returns_list_of_members_if_user_has_access_to_private_group(
             ],
         }
 
+    def test_it_returns_empty_list_if_page_number_beyond_last_page(
+        self, app, factories, db_session
+    ):
+        group = factories.RestrictedGroup(
+            memberships=[
+                GroupMembership(
+                    user=user,
+                    created=datetime(1970, 1, 1, 0, 0, second),
+                    updated=datetime(1970, 1, 2, 0, 0, second),
+                )
+                for second, user in enumerate(factories.User.create_batch(size=2))
+            ]
+        )
+        db_session.commit()
+
+        res = app.get(
+            "/api/groups/{pubid}/members".format(pubid=group.pubid),
+            params={"page[number]": 2, "page[size]": 10},
+            headers={"User-Agent": "test_user_agent", "Referer": "test_referer"},
+        )
+
+        assert res.json["meta"]["page"]["total"] == 2
+        assert res.json["data"] == []
+
     def test_it_returns_404_if_user_does_not_have_read_access_to_group(
         self, app, db_session, factories
     ):
@@ -222,7 +246,7 @@ def test_it_returns_404_if_user_does_not_have_read_access_to_group(
 
         res = app.get(
             "/api/groups/{pubid}/members".format(pubid=group.pubid),
-            params={"page[offset]": 0},
+            params={"page[number]": 1},
             headers=token_authorization_header(factories.DeveloperToken()),
             expect_errors=True,
         )
@@ -232,12 +256,12 @@ def test_it_returns_404_if_user_does_not_have_read_access_to_group(
     def test_it_returns_empty_list_if_no_members_in_group(self, app):
         res = app.get(
             "/api/groups/__world__/members",
-            params={"page[offset]": 0},
+            params={"page[number]": 1},
         )
 
         assert res.json == {"meta": {"page": {"total": 0}}, "data": []}
 
-    def test_it_returns_an_error_if_offset_and_limit_are_invalid(
+    def test_it_returns_an_error_if_number_and_size_are_invalid(
         self, app, db_session, factories
     ):
         group = factories.Group()
@@ -248,14 +272,14 @@ def test_it_returns_an_error_if_offset_and_limit_are_invalid(
 
         res = app.get(
             "/api/groups/{pubid}/members".format(pubid=group.pubid),
-            params={"page[offset]": -1, "page[limit]": 0},
+            params={"page[number]": 0, "page[size]": 0},
             headers=token_authorization_header(token),
             expect_errors=True,
         )
 
         assert res.status_code == 400
         assert res.json == {
-            "reason": "page[offset]: -1 is less than minimum value 0\npage[limit]: 0 is less than minimum value 1",
+            "reason": "page[number]: 0 is less than minimum value 1\npage[size]: 0 is less than minimum value 1",
             "status": "failure",
         }
 

tests/unit/h/schemas/pagination_test.py

@@ -10,10 +10,10 @@ class TestPaginationQueryParamsSchema:
     @pytest.mark.parametrize(
         "input_,output",
         [
-            ({}, {"page[offset]": 0, "page[limit]": 20}),
+            ({}, {"page[number]": 1, "page[size]": 20}),
             (
-                {"page[offset]": 150, "page[limit]": 50},
-                {"page[offset]": 150, "page[limit]": 50},
+                {"page[number]": 150, "page[size]": 50},
+                {"page[number]": 150, "page[size]": 50},
             ),
         ],
     )
@@ -26,17 +26,17 @@ def test_valid(self, schema, input_, output):
         "input_,message",
         [
             (
-                {"page[offset]": -1},
-                r"^page\[offset\]: -1 is less than minimum value 0$",
+                {"page[number]": 0},
+                r"^page\[number\]: 0 is less than minimum value 1$",
             ),
-            ({"page[limit]": 0}, r"^page\[limit\]: 0 is less than minimum value 1$"),
+            ({"page[size]": 0}, r"^page\[size\]: 0 is less than minimum value 1$"),
             (
-                {"page[limit]": 101},
-                r"^page\[limit\]: 101 is greater than maximum value 100$",
+                {"page[size]": 101},
+                r"^page\[size\]: 101 is greater than maximum value 100$",
             ),
             (
-                {"page[offset]": "foo", "page[limit]": "bar"},
-                r'^page\[offset\]: "foo" is not a number\npage\[limit\]: "bar" is not a number$',
+                {"page[number]": "foo", "page[size]": "bar"},
+                r'^page\[number\]: "foo" is not a number\npage\[size\]: "bar" is not a number$',
             ),
         ],
     )

tests/unit/h/views/api/group_members_test.py

@@ -20,7 +20,7 @@
 from h.views.api.exceptions import PayloadError
 
 
-class TestListMembersLegacyLegacy:
+class TestListMembersLegacy:
     def test_it(
         self,
         context,
@@ -80,8 +80,8 @@ def test_it(
         validate_query_params,
     ):
         pyramid_request.params = validate_query_params.return_value = {
-            "page[offset]": sentinel.offset,
-            "page[limit]": sentinel.limit,
+            "page[number]": 3,
+            "page[size]": 2,
         }
         group_members_service.get_memberships.return_value = [
             sentinel.membership_1,
@@ -104,7 +104,7 @@ def test_it(
         )
         group_members_service.count_memberships.assert_called_once_with(context.group)
         group_members_service.get_memberships.assert_called_once_with(
-            context.group, offset=sentinel.offset, limit=sentinel.limit
+            context.group, offset=4, limit=2
         )
         assert GroupMembershipJSONPresenter.call_args_list == [
             call(pyramid_request, sentinel.membership_1),