Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensure that paging forward and backward yields the same first page #144

Merged
merged 2 commits into from
Feb 17, 2023

Conversation

travis
Copy link
Member

@travis travis commented Feb 17, 2023

My implementation of paging has a seriously confusing bug that I caught while preparing for a demo - if you paged forward and then paged backward, the list you got after paging backward would look like the original list but have its start and end cursors reversed. It would also be in the reverse order. We knew about the reverse ordering but didn't catch its implications for cursors.

This happens because when you passed pre, Dynamo would evaluate and list keys in reverse order.

This all brought me around to the opinion that we should always list keys in the same order, and make sure we swap endCursor and startCursor when pre is passed.

This PR adds tests to ensure that paging forward once and then backward does indeed give the same list of keys, in the same order, with the same startCursor and endCursor.

The power of demos!

My implementation of paging has a seriously confusing bug - if you
paged forward and then paged backward, the list you got after paging
backward would look like the original list but have its start and end cursors reversed.
It would also be in the reverse order. We knew about the reverse ordering
but didn't catch its implications for cursors.

This happens because when you passed `pre`, Dynamo would evaluate and
list keys in reverse order.

This all brought me around to the opinion that we should always list keys
in the same order, and make sure we swap endCursor
and startCursor when `pre` is passed.

This PR adds tests to ensure that paging forward once and then
backward does indeed give the same list of keys, in the same order, with
the same startCursor and endCursor.

The power of demos!
@seed-deploy seed-deploy bot temporarily deployed to pr144 February 17, 2023 07:37 Inactive
@travis travis marked this pull request as ready for review February 17, 2023 07:37
@seed-deploy
Copy link

seed-deploy bot commented Feb 17, 2023

View stack outputs
  • pr144-w3infra-CarparkStack

    Name Value
    BucketName carpark-pr144-0
    Region us-east-2
  • pr144-w3infra-SatnavStack

    Name Value
    BucketName satnav-pr144-0
    Region us-east-2
  • pr144-w3infra-UploadApiStack

    Name Value
    ApiEndpoint https://up5c2ve95a.execute-api.us-east-2.amazonaws.com
    CustomDomain https://pr144.up.web3.storage
  • pr144-w3infra-BusStack

  • pr144-w3infra-ReplicatorStack

  • pr144-w3infra-UcanInvocationStack

  • pr144-w3infra-UploadDbStack

@seed-deploy seed-deploy bot temporarily deployed to pr144 February 17, 2023 07:48 Inactive
@travis
Copy link
Member Author

travis commented Feb 17, 2023

Verified this works in the seed-deploy env, thanks for the quick review @alanshaw! Merging.

@travis travis merged commit 98938b4 into main Feb 17, 2023
@travis travis deleted the fix/paging-cursor-bug branch February 17, 2023 08:01
@heyjay44 heyjay44 mentioned this pull request Feb 17, 2023
83 tasks
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