Fix #14074 - Paginate not working #14080
Conversation
|
Someone is attempting to deploy a commit to the Pipedreamers Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
WalkthroughThe changes update several modules in the NocoDB components, primarily focusing on version increments for various actions and sources. A significant modification is made to the pagination logic in Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you so much for submitting this! We've added it to our backlog to review, and our team has been notified. |
|
Thanks for submitting this PR! When we review PRs, we follow the Pipedream component guidelines. If you're not familiar, here's a quick checklist:
|
![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
🧹 Outside diff range and nitpick comments (2)
components/nocodb/nocodb.app.mjs (2)
125-126: Approve the change to offset-based pagination, but suggest improvements.The switch from page-based to offset-based pagination is a good improvement that allows for more flexible data retrieval. However, there are a few points to consider:
The hard-coded
limitof 1000 might not be suitable for all use cases or API endpoints. Consider making this configurable, either as a parameter to thepaginatemethod or as a class constant that can be overridden if needed.There's no check to ensure that the
offsetdoesn't exceed the total number of items. Consider adding a check using thepageInfoobject to prevent unnecessary API calls.Consider the following improvements:
async *paginate({ fn, args = {}, max, limit = 1000 }) { let lastPage, count = 0; args.params = { ...args.params, offset: 0, limit }; do { const { list, pageInfo, } = await fn(args); for (const item of list) { yield item; if (max && ++count === max) { return; } } args.params.offset += list.length; lastPage = !pageInfo.isLastPage; } while (lastPage && (!pageInfo.totalItems || args.params.offset < pageInfo.totalItems)); }This refactored version:
- Makes the
limitconfigurable with a default of 1000.- Increments the
offsetby the number of items in each page, not per item.- Adds a check to stop pagination if we've reached the total number of items (assuming
pageInfo.totalItemsexists).
125-139: Summary of pagination changes and their impactThe changes to the
paginatemethod address the pagination issue mentioned in the PR objectives by switching from a page-based to an offset-based approach. This change allows for more flexible data retrieval, especially for large datasets.However, there are a few areas where the implementation can be improved:
- The hard-coded limit of 1000 items per page might not be suitable for all use cases.
- The offset increment logic needs to be adjusted to correctly paginate through the data.
- Additional checks can be added to prevent unnecessary API calls when all data has been retrieved.
These improvements will make the pagination more robust and efficient, ensuring that it works correctly across different scenarios and API endpoints.
Consider the following architectural improvements:
- Make the
limitparameter configurable, allowing it to be set based on the specific requirements of each API endpoint or use case.- Implement a mechanism to track the total number of items (if provided by the API) to optimize the pagination process and prevent unnecessary API calls.
- Add error handling to manage potential issues with the API responses, ensuring that the pagination process is resilient to unexpected data formats or server errors.
| }; | ||
| do { | ||
| const { | ||
| list, pageInfo, | ||
| } = await fn(args); | ||
| for (const item of list) { | ||
| args.params.offset++; |
There was a problem hiding this comment.
Fix the offset increment logic.
The current implementation increments the offset for each item yielded. This is incorrect for pagination purposes, as it will cause the next API call to skip items.
Replace the line:
args.params.offset++;with:
// This line should be moved outside the for loop
args.params.offset += list.length;This change ensures that the offset is incremented by the number of items retrieved in each API call, maintaining correct pagination.
There was a problem hiding this comment.
@coderabbitai Performing args.params.offset++ in the for loop or args.params.offset += list.length outside the loop is the exact same thing.
It is even better to do args.params.offset++ because in case the if (max && ++count === max) condition is met, offset is incremented by the correct number of rows returned.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@coderabbitai Performing args.params.offset++ in the for loop or args.params.offset += list.length outside the loop is the exact same thing.
It is even better to do args.params.offset++ because in case the if (max && ++count === max) condition is met, offset is incremented by the correct number of rows returned.
There was a problem hiding this comment.
@LucBerge, you're right. Incrementing args.params.offset within the loop ensures the offset accurately reflects the number of items processed, especially when a max limit is specified. Thank you for the clarification. I'll note this for future reviews.
✏️ Learnings added
Learnt from: LucBerge
PR: PipedreamHQ/pipedream#14080
File: components/nocodb/nocodb.app.mjs:133-133
Timestamp: 2024-09-25T16:13:11.336Z
Learning: When implementing pagination with an offset, incrementing `args.params.offset` within the loop ensures correct tracking of the offset, particularly when a maximum count limit (`max`) is used.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Hi @LucBerge, would you mind sharing the problem you're facing with the current implementation and steps to reproduce it? To test this, I need to verify that the underlining issue of the current implementation is fixed by your implementation. On another note, I see that your pagination implementation has hard limit is 1000. Is this correct? |
|
Hello @vunguyenhung ,
See #14074
You are correct it is hard coded.
The second value can be changed or removed (default to 25 if missing). I do not know if it is better to have a dedicated field in pipedream for that. Maybe call it In Nocodb UI, the max value for records per page is 100. I am not even sure it correctly retrieves 1000 per page. Maybe it is limited to 100 even if is set 1000. |
|
Okay, I understand your explaination for the limit field. Could you share the isue you faced with the current implementation? |
|
@vunguyenhung Are you able to reproduce the bug ? With the limit field unset:It performs a first paginate request and get the record 1 to 25. The while condition is never met because it is not the last page. So it retrieves the items 1 to 25 again and again and again.... With the limit field set to 30 (for example):Lets imagine I have 42 items (from 1 to 42) and the current page limit is 25, I get the following returned records: BTW, the |
|
Merge on behalf of the user |
|
Hi everyone, all test cases are passed! Ready for release! Test report |
WHY
Fix #14074
Summary by CodeRabbit
New Features
Version Updates
nocodb-add-record: 0.0.3 → 0.0.4nocodb-delete-record: 0.0.3 → 0.0.4nocodb-get-record: 0.0.3 → 0.0.4nocodb-list-records-matching-criteria: 0.0.4 → 0.0.5nocodb-update-record: 0.0.3 → 0.0.4@pipedream/nocodb: 0.0.6 → 0.0.7nocodb-new-record: 0.0.4 → 0.0.5nocodb-updated-record: 0.0.4 → 0.0.5