Skip to content

Keep the first remaining row when a pagination boundary is deleted #1909

Description

@anxkhn

Users paging a list while its boundary item is deleted or moves out of the query silently miss the adjacent surviving item. The shared page helper also serves background WalkAll and LoadAll callers. Missing rows are omitted from the returned list, not deleted from storage.

At pkg/page/page.go:77:

NewPage drops edges[0] whenever c.Key is non-nil, using edges = edges[1:] without checking that this row matches the cursor. The Tail branch similarly drops the last row after reversal at line 101. Cursor.SQLFragment in pkg/page/cursor.go:83 uses inclusive comparisons against the stored value and ID, so deleting the boundary still returns the next surviving row. The code already compares the first row's CursorKey to c.Key at page.go:83, but only uses that comparison for page metadata, not trimming. This is reachable through TaskService.ListForOrganizationID at pkg/probo/task_service.go:464 and :471, Tasks.LoadByOrganizationID at pkg/coredata/task.go:467, and Task.Delete at pkg/coredata/task.go:718. Root cause: trimming assumes an inclusive SQL boundary necessarily exists in the result set.

Proposed fix

In pkg/page/page.go, compare the inclusive boundary row with the supplied CursorKey before removing it. After optional boundary removal, determine overflow from the remaining count, retain at most c.Size rows, and set the traversal-direction page flag from that overflow for Head and Tail. Keep cursor encoding and SQL inclusivity unchanged. Extend pkg/page/load_all_test.go using the existing store and keyset helpers with TestNewPageMissingBoundary and TestWalkAllDeletedBoundary. Cover existing and absent boundaries, short and full pages, both directions, and identical ordered values with distinct IDs. No call-site refactor is needed.

How to see it

Unambiguous source trace, not executed. Use load_all_test.go's newLoadAllStore with values 0 through 4 and a Head ascending cursor of size 2. The first query returns 0,1,2 and NewPage returns 0,1 with HasNext=true. Retain value 1's CursorKey, delete value 1 from the store, then fetch using keysetPage with that cursor. Inclusive SQL semantics yield 2,3,4. NewPage takes edges[1:] and returns 3,4 with HasNext=false. Expected: 2,3 with HasNext=true, then 4. Mirror the case with Tail and both sort directions. Also test a missing boundary with a single surviving row, which currently returns an empty page.

If this looks right I can push fix/pagination-missing-boundary on anxkhn/probo instead of opening a pull request first.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions