MPT-20733 add offset pagination support#193
Open
svazquezco wants to merge 1 commit into
Open
Conversation
svazquezco
requested review from
d3rky and
ruben-sebrango
and removed request for
a team
📝 WalkthroughWalkthroughThis PR migrates pagination across the SDK from page-based ( ChangesPagination migration from page-based to offset/limit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/sdk_usage/api.md`:
- Around line 141-145: The example JSON uses "$meta" but the non-paginated
response serializer APIResponse.ok(...) emits "meta"; update the docs example to
show "meta" (without the dollar sign) so it matches APIResponse.ok(...) and the
tests, preventing SDK consumers from being misled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b2ea9996-26ba-420a-ac3b-2f08e4bb7220
📒 Files selected for processing (16)
docs/sdk_usage/api.mdmock_app/api/api_routes.pymock_app/api/routes/api.pymock_app/docs/example.httpmock_app/mocks/api_service.pymock_app/sync/agreements.pympt_extension_sdk/api/pagination.pympt_extension_sdk/api/responses.pympt_extension_sdk/services/mpt_api_service/agreement.pympt_extension_sdk/services/mpt_api_service/base.pympt_extension_sdk/services/mpt_api_service/product.pytests/api/builders/test_api.pytests/api/test_pagination.pytests/api/test_responses.pytests/services/mpt_api_service/test_base.pytests/services/mpt_api_service/test_product.py
svazquezco
force-pushed
the
codex/MPT-20733-pagination
branch
from
4e0ddea to
753e667
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/api/test_pagination.py (1)
27-31: 💤 Low valueConsider adding tests for invalid
limitstring and negative values.The
offsetvalidation tests cover"abc"and"-1", but similar cases forlimitare not tested. Since_parse_limitdelegates to_parse_non_negative_int, these code paths should also be exercised.📝 Suggested additional test cases
`@pytest.mark.parametrize`("query", [{"limit": "abc"}, {"limit": "-1"}]) def test_pagination_validates_limit_invalid_values(query): with pytest.raises(ValidationError) as error_info: Pagination.from_query(query) assert error_info.value.errors[0].pointer == "`#/limit`"Also applies to: 34-39
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/api/test_pagination.py` around lines 27 - 31, Add tests that cover invalid string and negative values for limit: create a parameterized test (similar to the offset tests) that calls Pagination.from_query with {"limit": "abc"} and {"limit": "-1"}, expects a ValidationError, and asserts the error pointer equals "`#/limit`"; this exercises Pagination.from_query and the _parse_limit -> _parse_non_negative_int path and mirrors the existing offset test pattern (use ValidationError and the same assertion style as test_pagination_validates_limit).mpt_extension_sdk/services/mpt_api_service/base.py (1)
37-38: ⚡ Quick winConsider validating offset and limit parameters.
The method accepts
offsetandlimitwithout validating that they are non-negative. Negative values could cause unexpected API behavior or errors.🛡️ Proposed validation
async def _paginate( self, collection: Any, model: type[Model], *, offset: int = 0, limit: int = 100, ) -> PaginatedCollection[Model]: """Fetch and serialize one page from a Marketplace collection.""" + if offset < 0: + raise ValueError("offset must be non-negative") + if limit < 1: + raise ValueError("limit must be positive") page = await collection.fetch_page(offset=offset, limit=limit)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mpt_extension_sdk/services/mpt_api_service/base.py` around lines 37 - 38, The method accepting offset: int = 0, limit: int = 100 must validate that offset and limit are non-negative; add a guard at the start of the function (checking the offset and limit parameters) and raise a clear ValueError (or return a controlled error) if offset < 0 or limit < 0, and optionally clamp limit to a maximum if needed; update any callers or tests of this function to expect the validation. Reference the offset and limit parameters in the method signature and implement the check near the top of the function body.tests/services/mpt_api_service/test_base.py (1)
43-61: ⚡ Quick winAdd test coverage for fallback behavior when pagination metadata is absent.
The current test only covers the happy path where
page.meta.paginationis present. The_paginatemethod has fallback logic (lines 44-47 inbase.py) for whenpaginationisNone, but this path is untested.📋 Suggested test case
def test_paginate_without_meta_uses_fallbacks(mocker): """Test _paginate when page.meta is None.""" collection = mocker.Mock(spec=["fetch_page"]) collection.fetch_page = mocker.AsyncMock( return_value=FakePage(["one", "two"], meta=None) ) mocker.patch.object( FakeModel, "from_payload", autospec=True, side_effect=[{"payload": "one"}, {"payload": "two"}], ) service = FakeService(mocker.Mock(spec=AsyncMPTClient)) result = asyncio.run(service._paginate(collection, FakeModel, offset=5, limit=3)) assert result.offset == 5 # Falls back to input parameter assert result.limit == 3 # Falls back to input parameter assert result.total == 2 # Falls back to len(page) assert result.resources == [{"payload": "one"}, {"payload": "two"}]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/services/mpt_api_service/test_base.py` around lines 43 - 61, Add a test that covers the fallback branch of FakeService._paginate when page.meta is None: create a collection mock with collection.fetch_page returning FakePage(["one","two"], meta=None), patch FakeModel.from_payload to return [{"payload":"one"}, {"payload":"two"}], instantiate FakeService with a mocked AsyncMPTClient, call service._paginate(collection, FakeModel, offset=5, limit=3) and assert result.offset == 5, result.limit == 3, result.total == 2 (len(page)), and result.resources == [{"payload":"one"}, {"payload":"two"}]; name the test e.g. test_paginate_without_meta_uses_fallbacks and use asyncio.run to execute the async call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mock_app/sync/agreements.py`:
- Around line 12-13: The current sync only fetches the first page because it
calls ctx.mpt_api_service.agreements.get_all(limit=5) once and iterates
page.resources; change the logic to iterate through all paginated pages returned
by get_all: repeatedly fetch subsequent pages (using the pagination mechanism
provided by the service such as page.next(), page.next_page(), or passing
page.next_cursor to get_all) and process each page.resources until no more pages
remain, so all agreements are synced rather than only the first page.
---
Nitpick comments:
In `@mpt_extension_sdk/services/mpt_api_service/base.py`:
- Around line 37-38: The method accepting offset: int = 0, limit: int = 100 must
validate that offset and limit are non-negative; add a guard at the start of the
function (checking the offset and limit parameters) and raise a clear ValueError
(or return a controlled error) if offset < 0 or limit < 0, and optionally clamp
limit to a maximum if needed; update any callers or tests of this function to
expect the validation. Reference the offset and limit parameters in the method
signature and implement the check near the top of the function body.
In `@tests/api/test_pagination.py`:
- Around line 27-31: Add tests that cover invalid string and negative values for
limit: create a parameterized test (similar to the offset tests) that calls
Pagination.from_query with {"limit": "abc"} and {"limit": "-1"}, expects a
ValidationError, and asserts the error pointer equals "`#/limit`"; this exercises
Pagination.from_query and the _parse_limit -> _parse_non_negative_int path and
mirrors the existing offset test pattern (use ValidationError and the same
assertion style as test_pagination_validates_limit).
In `@tests/services/mpt_api_service/test_base.py`:
- Around line 43-61: Add a test that covers the fallback branch of
FakeService._paginate when page.meta is None: create a collection mock with
collection.fetch_page returning FakePage(["one","two"], meta=None), patch
FakeModel.from_payload to return [{"payload":"one"}, {"payload":"two"}],
instantiate FakeService with a mocked AsyncMPTClient, call
service._paginate(collection, FakeModel, offset=5, limit=3) and assert
result.offset == 5, result.limit == 3, result.total == 2 (len(page)), and
result.resources == [{"payload":"one"}, {"payload":"two"}]; name the test e.g.
test_paginate_without_meta_uses_fallbacks and use asyncio.run to execute the
async call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 274783b8-25b4-4cd1-974d-3b3505b99678
📒 Files selected for processing (16)
docs/sdk_usage/api.mdmock_app/api/api_routes.pymock_app/api/routes/api.pymock_app/docs/example.httpmock_app/mocks/api_service.pymock_app/sync/agreements.pympt_extension_sdk/api/pagination.pympt_extension_sdk/api/responses.pympt_extension_sdk/services/mpt_api_service/agreement.pympt_extension_sdk/services/mpt_api_service/base.pympt_extension_sdk/services/mpt_api_service/product.pytests/api/builders/test_api.pytests/api/test_pagination.pytests/api/test_responses.pytests/services/mpt_api_service/test_base.pytests/services/mpt_api_service/test_product.py
✅ Files skipped from review due to trivial changes (3)
- docs/sdk_usage/api.md
- mock_app/docs/example.http
- tests/api/test_responses.py
svazquezco
force-pushed
the
codex/MPT-20733-pagination
branch
from
753e667 to
69c18be
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/services/mpt_api_service/test_product.py (1)
34-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a regression test for IDs above one-page size.
Current coverage only validates a small list (
limit=2). Please add a case for a largeitem_idsinput to prevent regressions around pagination caps/single-page truncation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/services/mpt_api_service/test_product.py` around lines 34 - 50, The test currently only covers a tiny list (limit=2) and misses the multi-page/pagination edge; add a new test in tests/services/mpt_api_service/test_product.py that calls product_item_service.get_product_one_time_items_by_ids with a large item_ids list larger than the page size to exercise pagination and prevent single-page truncation. In that test, mock items_client.filter to return a collection, patch service._paginate (or simulate multiple pages) to return a mock that yields combined resources across pages (e.g., ["item-1", ..., "item-N"]), assert the final result contains all items, and verify _paginate was awaited with the filtered_collection and ProductItem and with an appropriate limit equal to the large list length (or the expected page cap) and items_client.filter was called once with the expected filter argument.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mpt_extension_sdk/services/mpt_api_service/base.py`:
- Around line 42-47: The fallback for total in the PaginatedCollection return is
incorrect when pagination metadata is missing: instead of total=len(page) it
should account for the current offset so downstream pagination remains
consistent; update the total expression in the PaginatedCollection construction
(the return in base.py that references page.meta.pagination,
PaginatedCollection, model.from_payload, and the limit/offset variables) to use
pagination.total if present otherwise offset + len(page).
In `@mpt_extension_sdk/services/mpt_api_service/product.py`:
- Around line 25-30: The current call to _paginate(...) passes
limit=len(item_ids), which forces fetching a single oversized page and can
exceed API pagination caps or miss items; remove the fixed limit and let
_paginate iterate pages (i.e., call
self._paginate(self._client.catalog.items.filter(query), ProductItem) without
limit) or implement safe batching by chunking item_ids and invoking the
filter/_paginate per chunk; update the call site in product.py (the call that
builds page = await self._paginate(...)) and ensure the resulting page.resources
aggregation collects results across pages/chunks to return all matching
ProductItem objects.
---
Outside diff comments:
In `@tests/services/mpt_api_service/test_product.py`:
- Around line 34-50: The test currently only covers a tiny list (limit=2) and
misses the multi-page/pagination edge; add a new test in
tests/services/mpt_api_service/test_product.py that calls
product_item_service.get_product_one_time_items_by_ids with a large item_ids
list larger than the page size to exercise pagination and prevent single-page
truncation. In that test, mock items_client.filter to return a collection, patch
service._paginate (or simulate multiple pages) to return a mock that yields
combined resources across pages (e.g., ["item-1", ..., "item-N"]), assert the
final result contains all items, and verify _paginate was awaited with the
filtered_collection and ProductItem and with an appropriate limit equal to the
large list length (or the expected page cap) and items_client.filter was called
once with the expected filter argument.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 00aba654-07dc-4fad-8929-ab4ac9e7c19f
📒 Files selected for processing (17)
docs/sdk_usage/api.mdmock_app/api/api_routes.pymock_app/api/routes/api.pymock_app/docs/example.httpmock_app/mocks/api_service.pymock_app/sync/agreements.pympt_extension_sdk/api/builders/execution.pympt_extension_sdk/api/pagination.pympt_extension_sdk/api/responses.pympt_extension_sdk/services/mpt_api_service/agreement.pympt_extension_sdk/services/mpt_api_service/base.pympt_extension_sdk/services/mpt_api_service/product.pytests/api/builders/test_api.pytests/api/test_pagination.pytests/api/test_responses.pytests/services/mpt_api_service/test_base.pytests/services/mpt_api_service/test_product.py
✅ Files skipped from review due to trivial changes (2)
- mock_app/docs/example.http
- docs/sdk_usage/api.md
Comment on lines
+42
to
+47
| pagination = page.meta.pagination if page.meta else None | ||
| return PaginatedCollection( | ||
| limit=pagination.limit if pagination else limit, | ||
| offset=pagination.offset if pagination else offset, | ||
| resources=[model.from_payload(element) for element in page], | ||
| total=pagination.total if pagination else len(page), |
There was a problem hiding this comment.
Fix fallback total to stay consistent with non-zero offsets.
When pagination metadata is absent, total=len(page) under-reports the total for offset > 0. That can break downstream pagination loops relying on offset/total consistency.
Suggested fix
- pagination = page.meta.pagination if page.meta else None
+ meta = getattr(page, "meta", None)
+ pagination = getattr(meta, "pagination", None)
+ resources = [model.from_payload(element) for element in page]
return PaginatedCollection(
limit=pagination.limit if pagination else limit,
offset=pagination.offset if pagination else offset,
- resources=[model.from_payload(element) for element in page],
- total=pagination.total if pagination else len(page),
+ resources=resources,
+ total=pagination.total if pagination else offset + len(resources),
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mpt_extension_sdk/services/mpt_api_service/base.py` around lines 42 - 47, The
fallback for total in the PaginatedCollection return is incorrect when
pagination metadata is missing: instead of total=len(page) it should account for
the current offset so downstream pagination remains consistent; update the total
expression in the PaginatedCollection construction (the return in base.py that
references page.meta.pagination, PaginatedCollection, model.from_payload, and
the limit/offset variables) to use pagination.total if present otherwise offset
+ len(page).
Comment on lines
+25
to
+30
| page = await self._paginate( | ||
| self._client.catalog.items.filter(query), | ||
| ProductItem, | ||
| limit=len(item_ids), | ||
| ) | ||
| return page.resources |
There was a problem hiding this comment.
limit=len(item_ids) can break large-ID requests.
This sends an unbounded limit and fetches only one page. For large item_ids, this can exceed API pagination caps and/or miss items beyond a single page.
Suggested direction
- page = await self._paginate(
- self._client.catalog.items.filter(query),
- ProductItem,
- limit=len(item_ids),
- )
- return page.resources
+ max_page_size = 100
+ resources: list[ProductItem] = []
+ for start in range(0, len(item_ids), max_page_size):
+ chunk_ids = item_ids[start : start + max_page_size]
+ chunk_query = (
+ RQLQuery(product__id=product_id)
+ & RQLQuery().id.in_(chunk_ids) # type: ignore[arg-type]
+ & RQLQuery().n("terms.period").eq("one-time")
+ )
+ page = await self._paginate(
+ self._client.catalog.items.filter(chunk_query),
+ ProductItem,
+ limit=len(chunk_ids),
+ )
+ resources.extend(page.resources)
+ return resources🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mpt_extension_sdk/services/mpt_api_service/product.py` around lines 25 - 30,
The current call to _paginate(...) passes limit=len(item_ids), which forces
fetching a single oversized page and can exceed API pagination caps or miss
items; remove the fixed limit and let _paginate iterate pages (i.e., call
self._paginate(self._client.catalog.items.filter(query), ProductItem) without
limit) or implement safe batching by chunking item_ids and invoking the
filter/_paginate per chunk; update the call site in product.py (the call that
builds page = await self._paginate(...)) and ensure the resulting page.resources
aggregation collects results across pages/chunks to return all matching
ProductItem objects.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI-generated PR — Please review carefully.
Summary
PaginatedCollectionandBaseService._paginate.Validation
make checkCloses MPT-20733
#/offsetand#/limitand allow limit=0