fix(cli): support recursive fetching for paginated datasources #366
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
@storyblok/astro
storyblok
@storyblok/eslint-config
@storyblok/js
storyblok-js-client
@storyblok/management-api-client
@storyblok/nuxt
@storyblok/react
@storyblok/region-helper
@storyblok/richtext
@storyblok/svelte
@storyblok/vue
commit: |
| const total = Number(response.headers.get('total')) || 0; | ||
| const fetchedEntries = data?.datasource_entries || []; | ||
| const allEntries = [...collectedEntries, ...fetchedEntries]; | ||
| if (allEntries.length < total && fetchedEntries.length > 0) { |
There was a problem hiding this comment.
Bug: Silent pagination halt on missing total header
If the API response doesn't include a valid 'total' header, the pagination logic will fail silently. Line 32 sets total to 0 when the header is missing or invalid (Number(response.headers.get('total')) || 0). This causes the condition on line 35 (allEntries.length < total) to always be false (since any array length is >= 0), preventing recursive pagination even when more pages exist. The function will only return the first page of results instead of fetching all pages.
alexjoverm
left a comment
alexjoverm
left a comment
There was a problem hiding this comment.
Thanks @dipankarmaikap! The code looks great to me. As we discussed, it's only missing an automated test to validate that pagination is applied automatically when results are more than what a page allows
There was a problem hiding this comment.
LGTM (only test missing as @alexjoverm mentioned)
| if (!totalHeader || Number.isNaN(total)) { | ||
| // No valid 'total' header — assume not paginated, return current page only | ||
| return extractDataFunction(data); | ||
| } |
There was a problem hiding this comment.
Bug: Preserve Collected Items on Missing Header Error
When the 'total' header is missing or invalid, the function returns only the current page's data via extractDataFunction(data), completely ignoring the collectedItems parameter. This causes data loss if pagination was working on previous pages but the header is missing on a subsequent page. The function should return [...collectedItems, ...extractDataFunction(data)] to preserve previously collected items.
alexjoverm
left a comment
alexjoverm
left a comment
There was a problem hiding this comment.
@dipankarmaikap LGTM! I added a suggestion for a small refactor, as I see per_page being unnecessarily reimplemented in a few places.
If you prefer to tackle it in separate PR, is also fine. Up to you if you want to merge it right away, or tackle it first
| updated_at: '2021-08-09T12:00:00Z', | ||
| }, | ||
| ]; | ||
| /** |
….yaml Co-authored-by: Alex Jover <[email protected]>
| const entries = await fetchDatasourceEntries(spaceId, ds.id); | ||
| return { ...ds, entries }; | ||
| }) || [], | ||
| }), |
There was a problem hiding this comment.
Bug: Pagination missing in datasource search function
The fetchDatasource function doesn't use pagination when searching for a datasource by name. If the target datasource exists but isn't in the first page of results, it won't be found. This function should use fetchAllPages like fetchDatasources does to ensure all pages are searched.
3 participants
This PR enhances the
fetchDatasourceEntriesfunction to work recursively, enabling it to fetch all datasource entries across multiple pages until all results are retrieved.Note
Introduces a generic pagination helper and applies it to fetch all datasources and their entries across pages, updates tests and mocks for pagination, and aligns OpenAPI specs to use shared page/per_page params.
fetchAllPageshelper and use it infetchDatasourcesandfetchDatasourceEntriesto retrieve all pages viapagequery andtotalheader.datasources/{space}structure when saving consolidated or separate files.packages/cli/src/commands/datasources/pull/actions.test.ts):datasource.mock.json(51 datasources) and add paginated MSW handlers.?page=1).packages/cli/src/commands/datasources/pull/datasource.mock.jsonwith paginated datasources and entries.../shared/pagination.yaml#/pageand/per_pageinassets,components,stories,internal_tags, and add todatasourceslist endpoint.Written by Cursor Bugbot for commit b31e50b. This will update automatically on new commits. Configure here.
Closes: #224, #247