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

Add db paginate #689

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Add db paginate #689

wants to merge 22 commits into from

Conversation

james-owen
Copy link
Member

@james-owen james-owen commented Dec 3, 2020

Sets us up for pagination to list endpoints like /_components/instances

  • Calls db.createReadStream() which has been modified in the storage module, for list requests
  • Adds prev and size query params to list requests
  • Checks whether prev matches up with where it's being requested, ie. a request to nymag.com/_components/ad/instances should be for something likenymag.com/_components/ad/instances/aaa, and drops prev if it's invalid, errors if it's not
  • Checks whether size is a number and within the configured maximum, errors if it's not a number

Requires clay/amphora-storage-postgres#64 for pagination.

Copy link
Contributor

@mattoberle mattoberle left a comment

Choose a reason for hiding this comment

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

Let's take some time to chat about CLAY_STORAGE_PAGE_SIZE, CLAY_STORAGE_MAX_PAGE, and .paginate.

I think this is really close, but we need to hammer out:

  1. Compatibility with other amphora storage modules.
  2. The best place to set / validate MAX_PAGE_SIZE and DEFAULT_PAGE_SIZE.
  3. The best naming convention for those two settings.

I know you mentioned moving the settings into Amphora which I think is a decent option (if we could leave it up to the storage method to implement/use the additional previous and size opts).

test/fixtures/mocks/storage.js Outdated Show resolved Hide resolved
lib/responses.js Outdated Show resolved Hide resolved
lib/responses.js Outdated Show resolved Hide resolved
lib/responses.js Outdated Show resolved Hide resolved
Copy link
Contributor

@mattoberle mattoberle left a comment

Choose a reason for hiding this comment

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

👍

We talked about one additional test case for CLAY_STORAGE_DEFAULT_PAGE_SIZE=undefined and CLAY_STORAGE_MAX_PAGE_SIZE=int and then this seems good to go to me!

@james-owen james-owen requested review from amirandalibi and removed request for jpope19 July 26, 2022 17:09
Copy link
Member

@jjpaulino jjpaulino left a comment

Choose a reason for hiding this comment

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

🍕

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.

3 participants