Skip to content

Fix pagination token parsing error#8424

Open
ericokuma wants to merge 1 commit into
mainfrom
cursor/APP-613-fix-pagination-token-parsing-error-acbd
Open

Fix pagination token parsing error#8424
ericokuma wants to merge 1 commit into
mainfrom
cursor/APP-613-fix-pagination-token-parsing-error-acbd

Conversation

@ericokuma

@ericokuma ericokuma commented Dec 1, 2025

Copy link
Copy Markdown
Contributor

Fixes APP-613

The rill project partitions command's --page-token failed because the ExecutedOn timestamp was not correctly marshaled into the pagination token, leading to a Time.UnmarshalJSON error.

This PR updates the pagination token generation to correctly encode the ExecutedOn timestamp (with a zero-time fallback for nil values) alongside the partition key. A new helper function modelPartitionPageToken encapsulates this logic, and dedicated unit tests ensure proper token creation and decoding for both present and nil ExecutedOn values.

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

Linear Issue: APP-613

Open in Cursor Open in Web


Note

Fixes page token generation for model partitions by encoding ExecutedOn with key, refines pagination limit usage, and adds unit tests.

  • Runtime server:
    • Pagination:
      • Generate NextPageToken using new modelPartitionPageToken, encoding ExecutedOn (with zero-time fallback) and partition Key via pagination.MarshalPageToken.
      • Refactor page size handling to use pageLimit and update next-page condition to len(partitions) == pageLimit && > 0.
    • Tests:
      • Add controller_pagination_test.go with cases for present and nil ExecutedOn, verifying token marshal/unmarshal of time and key.

Written by Cursor Bugbot for commit f1b581d. This will update automatically on new commits. Configure here.

Co-authored-by: eric.okuma <eric.okuma@rilldata.com>
@cursor

cursor Bot commented Dec 1, 2025

Copy link
Copy Markdown

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
Learn more about Cursor Agents

@ericokuma ericokuma marked this pull request as ready for review December 5, 2025 01:20

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is being reviewed by Cursor Bugbot

Details

You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

executedOn = *partition.ExecutedOn
}
return pagination.MarshalPageToken(executedOn, partition.Key)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Pagination fails for pending partitions with NULL ExecutedOn

When paginating through pending partitions (where ExecutedOn is NULL), modelPartitionPageToken encodes a zero time. On the next page request, this zero time is passed as BeforeExecutedOn, causing the SQL query to include both executed_on IS NULL (from WherePending) and comparison conditions like executed_on < zero_time. Since SQL NULL values don't compare equal to or less than any actual value, these conditions are mutually exclusive and the query returns zero results. The pagination effectively breaks on the second page for pending partitions.

Fix in Cursor Fix in Web

@ericpgreen2 ericpgreen2 removed their request for review December 5, 2025 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants