Skip to content
This repository has been archived by the owner on Sep 2, 2020. It is now read-only.

Pagination #113

Merged
merged 4 commits into from
Jun 13, 2014
Merged

Pagination #113

merged 4 commits into from
Jun 13, 2014

Conversation

ghost
Copy link

@ghost ghost commented Jun 12, 2014

This adds pagination support for all non-singleton GET routes.

This is very basic pagination that simply allows the API consumer to specify a page and count in the querystring in order to limit the results. There is no snapshotting of data, so if deletions or additions occur between calls, the pages may have redundant or insufficient data. Each call to a paginated function also returns a _pageData object in the body, which contains the current page, count per page, and the total number of objects available.

Note that there is no pagination going on yet at a deeper level than simply filtering the results returned (the database is still being queried for the full result set).

@brianloveswords
Copy link
Contributor

streamsql supports pagination out of the box as a pair of options to get

limit and page: How many rows and which page of results to get. Example: {limit: 25, page: 3}

This should probably use that.

@ghost
Copy link
Author

ghost commented Jun 12, 2014

So I was initially going to try that, but I was wary of not returning some sort of "total" in the pagination data, which as far as I can tell isn't provided by the streamsql calls.

I could also add a manual sql query to each of the getAll functions that just gets a total row count, but I wasn't sure if that was reaching a threshold of ugliness that made it all not worth it.

@@ -13,7 +13,7 @@
"dependencies": {
"mysql": "~2.0.0-alpha9",
"restify": "~2.6.1",
"streamsql": "~0.8.0",
"streamsql": "git+https://github.com/brianloveswords/streamsql.git#count-with-pagination",
Copy link
Author

Choose a reason for hiding this comment

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

Need to wait on merging this until this is a proper version. Just using the branch for now for testing purposes.

@ghost
Copy link
Author

ghost commented Jun 12, 2014

Alright, this is now using streamsql's phat pagination. Also added tests to each route that can be paginated.

@ghost
Copy link
Author

ghost commented Jun 12, 2014

Updated package.json to use streamsql's new 0.8.3 version that supports pagination (not strictly necessary as it would fall under ~0.8.0 anyway, but hey).

@brianloveswords
Copy link
Contributor

This is now broken from merging those other PRs, needs rebasing

@ghost
Copy link
Author

ghost commented Jun 12, 2014

Word.

@ghost
Copy link
Author

ghost commented Jun 12, 2014

Alrighty, 'tis rebased.

@@ -0,0 +1,11 @@
module.exports = function sendPaginated(req, res, responseData, total) {
if (req.pageData) {
responseData._pageData = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason for naming the property _pageData? The preceding underscore suggests that it's private, but presumably the consumer would want to use this data, right?

Copy link
Author

Choose a reason for hiding this comment

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

Heh, yeah, this is one of those things that looked more wrong to me the more I worked with it, so I'm happy to change it. My original thought was to do this to avoid conflicting with a "real" data element that happened to be named pageData, since I wasn't too keen on changing the entire structure of our responses by tucking all of our current results inside a "results" field or something. That said, this is obviously a largely theoretical concern, since the likelihood of our responses needing a field called pageData that isn't related to pagination is probably not too high. I'll change these, it's gross.

@cmcavoy
Copy link
Contributor

cmcavoy commented Jun 13, 2014

looks good to me.

cmcavoy added a commit that referenced this pull request Jun 13, 2014
@cmcavoy cmcavoy merged commit 6c1ea77 into master Jun 13, 2014
@cmcavoy cmcavoy deleted the pagination branch June 13, 2014 14:45
@SueSmith
Copy link
Contributor

I'm adding this pagination info to the docs on the relevant routes:

Available request parameters

  • page: - page of results to return
  • count: - count of results to return per page

Hope that sounds right..!

@SueSmith SueSmith mentioned this pull request Jun 13, 2014
@ghost
Copy link
Author

ghost commented Jun 13, 2014

That is indeed correct! Thank you, sue.

@SueSmith
Copy link
Contributor

great thanks @christensenep 😬

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants