Pagination Links #1041

bacarini · 2015-08-05T18:58:34Z

I would like to discuss on how we should display pagination links on AMS.

I think you can relie on kaminariand will_paginategems to do so. Having said that, I created a class to build links.

related issue

bf4 · 2015-08-06T01:16:12Z

README.md

+If you want pagination links in your response, specify it in the `render`
+
+```ruby
+  render json: @posts, pagination: true


Thanks for this. Did you try subclassing the ArraySerializer? e.g. https://github.com/rails-api/active_model_serializers#2-for-an-array-resource and just setting the meta? Have you seen pagination gems such as https://github.com/davidcelis/api-pagination ?

Great! @bf4 about the meta thing, it would work, but it would not follow json-api conventions.

@bacarini I think the README might not be the best place for describing this feature, mostly because it's to a particular adapter (json-api) and also because would be awesome to have some examples.
We are trying to consolidate our new docs, would be great if instead of adding this to the README you could open a new article there with this PR describing the feature, some examples and some of the gems that ppl can use in order to make it work.

bacarini · 2015-08-06T14:48:23Z

Hi @bf4,

Actually, I got same ideas from api-pagination gem. I used to use it, but It does not apply to JSON-API specification. That gem builds links on headers, like below.

I want to insert pagination into the response body. At least, I understood that from JSON-API. Is it right?

bf4 · 2015-08-06T17:07:05Z

Hmm.. well, it should subclass the ArraySerializer for sure, but besides testing that we can set meta data in a collection, I'm not sure it's the job of this library to expose anything more than the meta option for any pagination libraries to use.

bf4 · 2015-08-06T17:14:22Z

My bad, it should be in a links object, not meta http://jsonapi.org/format/#fetching-pagination

Pagination

A server MAY choose to limit the number of resources returned in a response to a subset ("page") of the whole set available.

A server MAY provide links to traverse a paginated data set ("pagination links").

Pagination links MUST appear in the links object that corresponds to a collection. To paginate the primary data, supply pagination links in the top-level links object. To paginate an included collection returned in a compound document, supply pagination links in the corresponding links object.

The following keys MUST be used for pagination links:

first: the first page of data

last: the last page of data

prev: the previous page of data

next: the next page of data

The page query parameter is reserved for pagination. Servers and clients SHOULD use this key for pagination operations.
For example, a page-based strategy might use query parameters such as page[number] and page[size], an offset-based strategy might use page[offset] and page[limit], while a cursor-based strategy might use page[cursor].

"links": {
  "self": "http://example.com/posts",
  "first": "http://example.com/posts?page[number]=1",
}

bf4 · 2015-08-06T17:15:32Z

I'm happy to pair with you to work on the links object. (We'll need tests, too, and might want to start with a PR to the json-api/example page to make sure we're getting it right)

bacarini · 2015-08-06T17:22:33Z

great... So, I will work on it. Should we edit this page example or create another one?

bf4 · 2015-08-07T16:38:08Z

add more examples to it.. see e.g. json-api/json-api#828 (error objects) which is a result of me working on error objects here in AMS #1004

bf4 · 2015-08-07T16:39:04Z

lib/active_model/serializer/adapter.rb

+
+          include_meta(hash)
+          include_pagination_links(hash) if options && options[:pagination]
+        end


I think these changes should be in the json_api adapter, no/

bf4 · 2015-08-07T16:45:13Z

Don't forget to add tests! They'll be required before this gets merged, so better now than later, right?

bacarini · 2015-08-07T20:06:59Z

Hey @bf4, I've changed a few things according to your comments. I will add more test later on.

Meanwhile, it would be great some reviews on the new code.

tks

joaomdmoura · 2015-08-07T20:21:04Z

lib/active_model/serializer/adapter.rb

-        hash = serializable_hash(options)
-        include_meta(hash) unless self.class == FlattenJson
-        hash
+        serializable_hash(options).tap do |hash|


@joaomdmoura OT: Might it make more sense for the FlattenJson adapter to implement a no-op include_meta so the superclass doesn't need to know about the subclass?

@bf4 I couldn't agree more with you about this approach. I will be happy to work on it, but in a different PR.

Did it in #1048 because

#1048 Merged already.

joaomdmoura · 2015-08-07T20:37:55Z

Great work @bacarini. Btw awesome PR on json-api as well @bf4 !

Don't forget to add tests! They'll be required before this gets merged, so better now than later, right?

👍 Indeed I see you added a test, would be great to have one covering the action_controller flow. You can check how we have being doing it here

bacarini · 2015-08-08T19:32:51Z

@joaomdmoura @bf4, I added test to pagination feature and also included it to /doc.

I've exchanged test/action_controller/json_api_linked_test.rb to a new different directory, test/action_controller/json_api/linked_test.rb. In my opinion, it is clearer and more organized. What do you think guys?

I've also opened a PR on json-api to make sure we're getting it right, as @bf4 said.

enriikke · 2015-08-09T17:17:31Z

This is awesome! Thanks for the work on this @bacarini 💥 👍 😄

I would like to make a suggestion. Sometimes pagination is tied to a search or filtered query. Looking at the docs, the returned pagination links look like ?page=1&per_page=1 so any other parameter that was sent with the request is not returned.

This might be up to individual interpretation but the way the pagination links are returned will require the client to take additional steps before it can use them (i.e. prefixing the api endpoint, adding filters or search query).

I'm suggesting that pagination links should be returned in a way that no additional processing is needed by the client. I'm working on a project using JSON API that needed pagination links and this is similar to how I approached it:

url = request.original_url.sub(/\?.*$/, "")
params = request.query_parameters.merge(page: { number: page, size: size }).to_param

"#{url}?#{params}"

In my case, I'm using a page hash as suggested in this note http://jsonapi.org/format/#fetching-pagination:

Note: JSON API is agnostic about the pagination strategy used by a server. Effective pagination
strategies include (but are not limited to): page-based, offset-based, and cursor-based. The page query
parameter can be used as a basis for any of these strategies. For example, a page-based strategy
might use query parameters such as page[number] and page[size], an offset-based strategy might use
page[offset] and page[limit], while a cursor-based strategy might use page[cursor].

In any case, this is great. I like that this gives the ability to add a root links object without overriding meta. I will be happy to assist in any way I can 🔨

bacarini · 2015-08-10T14:12:38Z

According to a comment on json-api PR, I added complete URIs, not just query strings.

It will accept link/self like this PR or the original request url.

@enriikke, I can see your point, but what do you think to discuss it on json-api PR?

bacarini · 2015-08-10T15:29:02Z

@enriikke may bad, I misunderstood your previous comment.

I added new code to merge with urls parameters that already exists.

enriikke · 2015-08-10T19:41:14Z

Awesome @bacarini I'm looking forward to this PR being merged 👍 😃

Pagination links will be included in your response automatically as long as the resource is paginated using Kaminari or WillPaginate and if you are using a JSON-API adapter. The others adapters does not have this feature.

bacarini · 2015-08-18T22:11:02Z

@bf4 no worries! 😃

I`ve changed a few things, could you review?
tks a lot 👍

bf4 · 2015-08-19T01:37:16Z

lib/active_model/serializer/adapter.rb

-        hash
+        serializable_hash(options).tap do |hash|
+          include_meta(hash)
+        end


This should go away when you rebase, no?

bacarini · 2015-08-19T14:21:47Z

@joaomdmoura and @bf4 updated docs as suggested.

joaomdmoura · 2015-08-19T18:58:41Z

test/action_controller/json_api/pagination_test.rb

+          assert_equal expected_links, response['links']
+        end
+
+        def test_render_only_prev_and_first_pagination_links_with_additional_params


joaomdmoura · 2015-08-19T19:01:38Z

LGTM 👍 only this still open.
@bf4 I know you are deeply involved in the review of this one so I'll also wit a LGTM from you before merge.

bf4 · 2015-08-20T01:20:47Z

docs/howto/add_pagination_links.md

+}
+```
+
+AMS relies on either [Kaminari](https://github.com/amatsuda/kaminari) or [WillPaginate](https://github.com/mislav/will_paginate). Please install either dependency by adding one of those to your Gemfile.


nitpick: AMS relies on either Kaminari or WillPaginate for pagination.

alternative to consider: AMS pagination relies on a paginated collection with the methods current_page, total_pages, and size, such as are supported by both Kaminari or WillPaginate.

(links left out for ease of me typing :)

bf4 · 2015-08-20T01:47:57Z

@joaomdmoura looks good to me. The interface is solid and the tests cover the usage well. I'd like to iterate on it a bit, but we can do that in another PR. @bacarini Has put in a lot of work and it will be the basis for more like it, I am sure! 👍 all around!

bf4 · 2015-08-20T01:49:33Z

I don't think it necessarily needs to be rebased since I think the commits tell a good story.

bf4 · 2015-08-20T01:58:23Z

After this is merged, I think #1018 will need to be repurposed to handle the non-pagination link scenarios and how they may interact with pagination links.

kalys · 2015-08-22T06:53:46Z

@bf4 Hi. When are you going to merge this PR?

joaomdmoura · 2015-08-22T16:38:19Z

Awesome work @bacarini . I'me merging this.

Adding pagination links

bf4 · 2015-08-23T01:05:35Z

@kalys I don't have commit :) Thanks @joaomdmoura for stewarding this and @bacarini for all the work!

bacarini · 2015-08-24T13:17:54Z

That is great!!!
Tks @bf4 and @joaomdmoura. 👍

I am glad we could make it through. 😃

leandrocp · 2015-08-25T12:54:12Z

Great work @bacarini 🍻

bacarini · 2015-08-25T13:04:31Z

cheers mate 🍻 😄

bacarini mentioned this pull request Aug 5, 2015

Pagination Links on AMS #1040

Closed

bf4 reviewed Aug 6, 2015
View reviewed changes

leandrocp mentioned this pull request Aug 6, 2015

Adds support for top-level links objects #1018

Merged

bf4 reviewed Aug 7, 2015
View reviewed changes

bacarini force-pushed the master branch from 80f6562 to 36d2140 Compare August 7, 2015 20:02

joaomdmoura reviewed Aug 7, 2015
View reviewed changes

bacarini force-pushed the master branch from db8ae19 to 7433b9d Compare August 8, 2015 19:26

bacarini changed the title ~~WIP - Pagination Links~~ Pagination Links Aug 8, 2015

bacarini mentioned this pull request Aug 8, 2015

add example of pagination links json-api/json-api#834

Merged

bacarini force-pushed the master branch 2 times, most recently from 018b71c to 9dbf7c3 Compare August 10, 2015 14:22

bacarini added 2 commits August 18, 2015 19:04

Add pagination links automatically

2c2f948

Pagination links will be included in your response automatically as long as the resource is paginated using Kaminari or WillPaginate and if you are using a JSON-API adapter. The others adapters does not have this feature.

add test to prev and first with additional params

5031eb9

bacarini force-pushed the master branch from f2fa434 to 83f9e74 Compare August 18, 2015 22:05

bf4 reviewed Aug 19, 2015
View reviewed changes

send whole request context to model serializer

01eab3b

bacarini force-pushed the master branch from 83f9e74 to 01eab3b Compare August 19, 2015 13:49

bacarini added 2 commits August 19, 2015 11:09

add more documentation to pagination links

f85027e

improvements on how to get self link on pagination class

3c3578a

joaomdmoura reviewed Aug 19, 2015
View reviewed changes

bf4 reviewed Aug 20, 2015
View reviewed changes

bacarini added 2 commits August 20, 2015 11:31

add kaminari and will_paginate examples

b73ffe2

test pagination links the way the controller does

d50262e

bacarini mentioned this pull request Aug 20, 2015

Minimal config for make works with kaminari bacarini/jbuilder_pagination#1

Closed

4 tasks

joaomdmoura added a commit that referenced this pull request Aug 22, 2015

Merge pull request #1041 from bacarini/master

87c47f8

Adding pagination links

joaomdmoura merged commit 87c47f8 into rails-api:master Aug 22, 2015

bf4 mentioned this pull request Jan 29, 2016

Document JSON API implementation defs and progress in class #1482

Merged

Pagination Links #1041

Pagination Links #1041

Conversation

bacarini commented Aug 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bacarini commented Aug 6, 2015

bf4 commented Aug 6, 2015

bf4 commented Aug 6, 2015

bf4 commented Aug 6, 2015

bacarini commented Aug 6, 2015

bf4 commented Aug 7, 2015

Choose a reason for hiding this comment

bf4 commented Aug 7, 2015

bacarini commented Aug 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaomdmoura commented Aug 7, 2015

bacarini commented Aug 8, 2015

enriikke commented Aug 9, 2015

bacarini commented Aug 10, 2015

bacarini commented Aug 10, 2015

enriikke commented Aug 10, 2015

bacarini commented Aug 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bacarini commented Aug 19, 2015

Choose a reason for hiding this comment

joaomdmoura commented Aug 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Aug 20, 2015

bf4 commented Aug 20, 2015

bf4 commented Aug 20, 2015

kalys commented Aug 22, 2015

joaomdmoura commented Aug 22, 2015

bf4 commented Aug 23, 2015

bacarini commented Aug 24, 2015

leandrocp commented Aug 25, 2015

bacarini commented Aug 25, 2015