Skip to content

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.

@james-owen james-owen requested a review from mattoberle December 7, 2020 22:23
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).

@james-owen james-owen requested a review from mattoberle December 9, 2020 20:57
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.

4 participants