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

Pagination Links #1041

Merged
merged 20 commits into from
Aug 22, 2015
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add pagination links automatically
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.
  • Loading branch information
bacarini committed Aug 18, 2015
commit 2c2f948fa0ea5e2fb05940c636c9f078a09a33b6
7 changes: 6 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ If you wish to use a serializer other than the default, you can explicitly pass
render json: @posts, each_serializer: PostPreviewSerializer

# Or, you can explicitly provide the collection serializer as well
render json: @posts, serializer: PaginatedSerializer, each_serializer: PostPreviewSerializer
render json: @posts, serializer: CollectionSerializer, each_serializer: PostPreviewSerializer
```

### Meta
Expand Down Expand Up @@ -272,6 +272,11 @@ And you can change the JSON key that the serializer should use for a particular

The `url` declaration describes which named routes to use while generating URLs
for your JSON. Not every adapter will require URLs.
## Pagination

Pagination links will be included in your response automatically as long as the resource is paginated using [Kaminari](https://github.com/amatsuda/kaminari) or [WillPaginate](https://github.com/mislav/will_paginate) and if you are using a ```JSON-API``` adapter. The others adapters does not have this feature.

For more information about it, please see in our docs [How to add pagination links](https://github.com/rails-api/active_model_serializers/blob/master/docs/howto/add_pagination_links.md)
Copy link
Member

Choose a reason for hiding this comment

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

👍


## Caching

Expand Down
18 changes: 12 additions & 6 deletions docs/howto/add_pagination_links.md
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
# How to add pagination links

If you want pagination links in your response, specify it in the `render`
Pagination links will be included in your response automatically as long as the resource is paginated and if you are using a ```JSON-API``` adapter. The others adapters does not have this feature.

Copy link
Member

Choose a reason for hiding this comment

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

But you could use the PaginationLinks builder with the JSON adapter right? like if I wanted to put links in the 'meta' attribute using a subclass of the json adapter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, PaginationLinks only will be included to JSON-API adapter, but you can still use a custom seriallizer to do so.

Ex.

render json: @posts, serializer: PaginatedSerializer, each_serializer: PostPreviewSerializer
class PaginatedSerializer < ActiveModel::Serializer::ArraySerializer
  def initialize(object, options={})
    meta_key = options[:meta_key] || :meta
    options[meta_key] ||= {}
    options[meta_key] = {
      current_page: object.current_page,
      next_page: object.next_page,
      prev_page: object.prev_page,
      total_pages: object.total_pages,
      total_count: object.total_count
    }
    super(object, options)
  end
end

Copy link
Member

@bf4 bf4 Aug 18, 2015 via email

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is not!
I may do it, but it is another issue, no?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

👍 yup, this would be good to have it, maybe a new article on the docs 😄

```ruby
render json: @posts, pagination: true
```
If you want pagination links in your response, use [Kaminari](https://github.com/amatsuda/kaminari) or [WillPaginate](https://github.com/mislav/will_paginate).

AMS relies on either `Kaminari` or `WillPaginate`. Please install either dependency by adding one of those to your Gemfile.
```ruby
#kaminari example
@posts = Kaminari.paginate_array(Post.all).page(3).per(1)
render json: @posts

Pagination links will only be included in your response if you are using a ```JSON-API``` adapter, the others adapters doesn't have this feature.
#will_paginate example
@posts = Post.all.paginate(page: 3, per_page: 1)
render json: @posts
```
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

@bacarini would you mind updating the will_paginate example to use the 'newer, recommended' syntax that doesn't require the deprecated finders?

@posts = Post.page(3).per_page(1)

Or even

@array = Post.all; @posts = WillPaginate::Collection.create(page = 1, WillPaginate.per_page, @array.size) do |pager| pager.replace @array[pager.offset, pager.per_page].to_a end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, @bf4 tks for the review.

I believe that, using WillPaginate::Collection we are overcomplicated it, since will_paginate already do it for us.
http://www.rubydoc.info/github/mislav/will_paginate/WillPaginate/Collection

Array.class_eval do
  def paginate(page = 1, per_page = 15)
    WillPaginate::Collection.create(page, per_page, size) do |pager|
      pager.replace self[pager.offset, pager.per_page].to_a
    end
  end
end

What do you think doing this way?

Kaminari examples
#array
@posts = Kaminari.paginate_array([1, 2, 3]).page(3).per(1)
render json: @posts

#active_record
@posts = Post.page(3).per(1)
render json: @posts
WillPaginate examples
#array
@posts = [1,2,3].paginate(page: 3, per_page: 1)
render json: @posts

#active_record
@posts = Post.page(3).per_page(1)
render json: @posts

Copy link
Member

@bf4 bf4 Aug 20, 2015 via email

Choose a reason for hiding this comment

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


```ruby
ActiveModel::Serializer.config.adapter = :json_api
Expand Down Expand Up @@ -38,3 +42,5 @@ ex:
}
}
```

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.
Copy link
Member

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

7 changes: 4 additions & 3 deletions lib/action_controller/serialization.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,10 @@ def get_serializer(resource, options = {})
"Please pass 'adapter: false' or see ActiveSupport::SerializableResource#serialize"
options[:adapter] = false
end
if options[:pagination]
options[:original_url] = original_url
options[:query_parameters] = query_parameters
if resource.respond_to?(:current_page) && resource.respond_to?(:total_pages)
options[:pagination] = {}
options[:pagination][:original_url] = original_url
options[:pagination][:query_parameters] = query_parameters
end
Copy link
Member

Choose a reason for hiding this comment

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

Just thinking about this part of the pagination interface and how someone might want to generate this outside of a controller

might we want to instead set these as:

options[:pagination][:original_url] ||= original_url
options[:pagination][:query_parameters] ||= query_parameters

that way options[:pagination] is still truthy and its options are scoped within it... and is also passed to the adapter as such

On the other hand, since original url is really a fallback for options[:links][:self], maybe this should be
options[:links][:self] ||= original_url (which, now that I think about it, we probably only want to strip the params off the original url for building the non-self links, right?)

Another alternative is to pass render json: @object, links: { pagination: true } and we just check if options[:links].delete(:pagination) is true. That way, there's one less option being passed around, and pagination piggy backs on links, which it has to do anyways.

I rearranged your code as below while thinking this through. Hope it's helpful.

FIRST_PAGE = 1

def options
  @options ||=
    begin
      {
        pagination: {
          original_url: request.original_url,
          query_parameters: request.query_parameters
        }
      }
    end
end

def links
  @links ||= options[:links] || {}
end

def build_pagination_links(collection)
  pagination_links = {}
  return pagination_links if collection.total_pages == FIRST_PAGE


  original_url = options[:pagination][:original_url]
  url_base = original_url[/\A[^?]+/]
  query_parameters = options[:query_parameters].dup || original_url[/\?.*/] || {}

  current_links = links
  self_link = current_links.fetch(:self) { original_url }

  collection_size            = collection.size
  collection_current_page    = collection.current_page
  collection_total_pages     = collection.total_pages


  params = query_parameters.merge!(page: {})
  link_template = "#{url_base}?%s"
  # only add first/previous page links when not on the first page (i.e. we can go back)
  if collection_current_page != FIRST_PAGE
    pagination_links[:first] = link_template % params[:page].merge(size: collection_size, number: FIRST_PAGE).to_query
    pagination_links[:prev]  = link_template % params[:page].merge(size: collection_size, number: (collection_current_page - FIRST_PAGE)).to_query
  end

  # only add next/last page links when not on the last page (i.e. we can go forward)
  if collection_current_page != collection_total_pages
    pagination_links[:next] = link_template % params[:page].merge(size: collection_size, number: (collection_current_page + FIRST_PAGE)).to_query
    pagination_links[:last] = link_template % params[:page].merge(size: collection_size, number: collection_total_pages).to_query
  end
  pagination_links
end

links.update(build_pagination_links(paginated_resource))

Copy link
Member

Choose a reason for hiding this comment

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

options[:pagination][:original_url] ||= original_url
options[:pagination][:query_parameters] ||= query_parameters

👍

I'm also in favor or building the self link based on this.
It seems to me that some of the changes proposed here are implemented already, right @bacarini?

Copy link
Member

Choose a reason for hiding this comment

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

could we maybe instead pass in a context so that serialization isn't as tightly coupled to the controller?

e.g. in the controller, just

options.fetch(:context) { options[:context] = request }
ActiveModel::SerializableResource.serialize(resource, options) do |serializable_resource|

Then in the ArraySerializer

class ArraySerializer
   def object # Same interface as Serializer. The instance_variable_get(:@resources) in the code is an interface smell
     @resource
   end
end

Then in the JsonApi adapter

  def serializable_hash(options = nil)
          options ||= {}
          if serializer.respond_to?(:each)
            serializer.each do |s|
               # code
            end
            add_links(options) # add links when we have a collection serializer
          end

        private

        def add_links(options)
          links = @hash[:links] ||= {} # no reason for them to be nil or false
          @hash[:links].update(JsonApi::PaginationLinks.new(object, links).serializable_hash(options))
        end
end

Then in the PaginationLinks builder

attr_reader :collection
def initialize(collection, links)
  @collection = collection
  @links = links
end
def serializable_hash(options)
  pagination_links = {}
  return pagination_links if !collection.respond_to?(:current_page) && !collection.respond_to?(:total_pages)
  return pagination_links if collection.total_pages == FIRST_PAGE

  context = options.fetch(:context) # not sure what the desired behavior is here. If context (the controller.request) is required, I shouldn't be putting it in 'options'. The idea, though, is that we only need to get the current url etc from the context when pagination.  No need to do it in the controller when passing it in, I don't think.
  original_url = context.original_url
  url_base = original_url[/\A[^?]+/]
  query_parameters = context.query_parameters.dup || original_url[/\?.*/].to_query || {} # this line is broken, but it's the general idea...

  current_links = @links
  self_link = current_links.fetch(:self) { original_url }

  collection_size            = collection.size
  collection_current_page    = collection.current_page
  collection_total_pages     = collection.total_pages


  params = query_parameters.merge!(page: {})
  link_template = "#{url_base}?%s"
  # only add first/previous page links when not on the first page (i.e. we can go back)
  if collection_current_page != FIRST_PAGE
    pagination_links[:first] = link_template % params[:page].merge(size: collection_size, number: FIRST_PAGE).to_query # confirm that the url this generates has params like `first: "page[size]=1&page[number]=10"`
    pagination_links[:prev]  = link_template % params[:page].merge(size: collection_size, number: (collection_current_page - FIRST_PAGE)).to_query
  end

  # only add next/last page links when not on the last page (i.e. we can go forward)
  if collection_current_page != collection_total_pages
    pagination_links[:next] = link_template % params[:page].merge(size: collection_size, number: (collection_current_page + FIRST_PAGE)).to_query
    pagination_links[:last] = link_template % params[:page].merge(size: collection_size, number: collection_total_pages).to_query
  end
  pagination_links
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @bf4 , Is the idea to send the whole request through objects?
We are using just original_url in case of it is not already defined in options[:links][:self] and query_parameters, I really don't think it is good idea, but may have an issue that I am seeing that you are.

It might be an option if this feature will be approved.

Copy link
Member

Choose a reason for hiding this comment

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

@bacarini Yeah, the idea is that since we only need to get the current url etc from the context when paginating, instead of manipulating the request object in the controller, and pass in two pieces of generated data via options, we can pass in the context (that is, the request object) and only use it when paginating. The details of it being an options param I don't totally like, but it is a pattern used in Rails to have an evaluation context, which I do like a bit better for letting moving the responsibility of understanding the context from the controller to the Link Paginator. Also, it adds less methods to the controller.

That make sense?

Copy link
Member

Choose a reason for hiding this comment

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

There's a bunch wrong in the specifics of the code above, I totally admit. I was more interested in the flow.. but e.g.

def serializable_hash(options)
  pagination_links = {}

is wrong. those options passed in to serializable_hash should be the pagination_links options. The thing that has the context should be passed into the initializer, I think.

i.e.

# the context is the request object from the controller and it needs to have original_url and query_parameters
def initialize(collection, links, context)
  @collection = collection
  @links = links
  @context = context
end
def serializable_hash(pagination_links = nil)
  pagination_links ||= {}

ActiveModel::SerializableResource.serialize(resource, options) do |serializable_resource|
if serializable_resource.serializer?
Expand Down
2 changes: 1 addition & 1 deletion lib/active_model/serializable_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
module ActiveModel
class SerializableResource

ADAPTER_OPTION_KEYS = Set.new([:include, :fields, :adapter, :pagination])
ADAPTER_OPTION_KEYS = Set.new([:include, :fields, :adapter])

def initialize(resource, options = {})
@resource = resource
Expand Down
2 changes: 1 addition & 1 deletion lib/active_model/serializer/adapter/json_api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ def add_resource_relationships(attrs, serializer, options = {})
def add_links(options)
links = @hash.fetch(:links) { {} }
resources = serializer.instance_variable_get(:@resource)
@hash[:links] = add_pagination_links(links, resources, options) if @options[:pagination]
@hash[:links] = add_pagination_links(links, resources, options) if options[:pagination]
end

def add_pagination_links(links, resources, options)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def initialize(collection)

def serializable_hash(options = {})
pages_from.each_with_object({}) do |(key, value), hash|
query_parameters = options.fetch(:query_parameters) { {} }
query_parameters = options[:pagination].fetch(:query_parameters) { {} }
params = query_parameters.merge(page: { number: value, size: collection.size }).to_query

hash[key] = "#{url(options)}?#{params}"
Expand Down Expand Up @@ -51,7 +51,7 @@ def raise_unless_any_gem_installed

def url(options)
self_link = options.fetch(:links) {{}}
self_link.fetch(:self) {} ? options[:links][:self] : options[:original_url]
self_link.fetch(:self) {} ? options[:links][:self] : options[:pagination][:original_url]
end
end
end
Expand Down
17 changes: 4 additions & 13 deletions test/action_controller/json_api/pagination_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,16 @@ def using_will_paginate
end

def render_pagination_using_kaminari
render json: using_kaminari, adapter: :json_api, pagination: true
render json: using_kaminari, adapter: :json_api
end

def render_pagination_using_will_paginate
render json: using_will_paginate, adapter: :json_api, pagination: true
render json: using_will_paginate, adapter: :json_api
end

def render_array_without_pagination_links
render json: using_will_paginate, adapter: :json_api, pagination: false
end

def render_array_omitting_pagination_options
render json: using_kaminari, adapter: :json_api
setup
render json: @array, adapter: :json_api
end
end

Expand Down Expand Up @@ -104,12 +101,6 @@ def test_array_without_pagination_links
response = JSON.parse(@response.body)
refute response.key? 'links'
end

def test_array_omitting_pagination_options
get :render_array_omitting_pagination_options, page: { number: 2, size: 1 }
response = JSON.parse(@response.body)
refute response.key? 'links'
end
end
end
end
Expand Down
42 changes: 22 additions & 20 deletions test/adapter/json_api/pagination_links_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,11 @@ def using_will_paginate
end

def data
{
data: [{
id:"2",
type:"profiles",
attributes:{
name:"Name 2",
description:"Description 2"
}
}]
{ data:[
{ id:"1", type:"profiles", attributes:{name:"Name 1", description:"Description 1" } },
{ id:"2", type:"profiles", attributes:{name:"Name 2", description:"Description 2" } },
{ id:"3", type:"profiles", attributes:{name:"Name 3", description:"Description 3" } }
]
}
end

Expand All @@ -56,42 +52,48 @@ def expected_response_without_pagination_links
end

def expected_response_with_pagination_links
data.merge links
{}.tap do |hash|
hash[:data] = [data.values.flatten.second]
hash.merge! links
end
Copy link
Member

Choose a reason for hiding this comment

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

overall, I'm happy with this PR and I think it's ok to merge. I'd like to change this to something like

expected_response_without_pagination_links.merge(links)

Copy link
Member

Choose a reason for hiding this comment

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

which I would submit in a follow-up pr to keep this moving

end

def expected_response_with_pagination_links_and_additional_params
new_links = links[:links].each_with_object({}) {|(key, value), hash| hash[key] = "#{value}&teste=teste" }
data.merge links: new_links
{}.tap do |hash|
hash[:data] = [data.values.flatten.second]
hash.merge! links: new_links
end
end

def test_pagination_links_using_kaminari
serializer = ArraySerializer.new(using_kaminari)
adapter = ActiveModel::Serializer::Adapter::JsonApi.new(serializer, pagination: true)
adapter = ActiveModel::Serializer::Adapter::JsonApi.new(serializer)
Copy link
Member

Choose a reason for hiding this comment

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

See #1018 (comment) re: using the SerializableResource interface, tl;dr

 def load_adapter(paginated_collection, options)
        options = options.merge(adapter: :json_api)
        ActiveModel::SerializableResource.new(paginated_collection, options)
      end

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! 👍 😄


assert_equal expected_response_with_pagination_links,
adapter.serializable_hash(original_url: "http://example.com")
adapter.serializable_hash(pagination: { original_url: "http://example.com" })
end

def test_pagination_links_using_will_paginate
serializer = ArraySerializer.new(using_will_paginate)
adapter = ActiveModel::Serializer::Adapter::JsonApi.new(serializer, pagination: true)
adapter = ActiveModel::Serializer::Adapter::JsonApi.new(serializer)

assert_equal expected_response_with_pagination_links,
adapter.serializable_hash(original_url: "http://example.com")
adapter.serializable_hash(pagination: { original_url: "http://example.com" })
end

def test_pagination_links_with_additional_params
serializer = ArraySerializer.new(using_will_paginate)
adapter = ActiveModel::Serializer::Adapter::JsonApi.new(serializer, pagination: true)
adapter = ActiveModel::Serializer::Adapter::JsonApi.new(serializer)
assert_equal expected_response_with_pagination_links_and_additional_params,
adapter.serializable_hash(original_url: "http://example.com",
query_parameters: { teste: "teste"})
adapter.serializable_hash(pagination: { original_url: "http://example.com",
query_parameters: { teste: "teste"}})

end

def test_not_showing_pagination_links
serializer = ArraySerializer.new(using_will_paginate)
adapter = ActiveModel::Serializer::Adapter::JsonApi.new(serializer, pagination: false)
serializer = ArraySerializer.new(@array)
adapter = ActiveModel::Serializer::Adapter::JsonApi.new(serializer)

assert_equal expected_response_without_pagination_links, adapter.serializable_hash
end
Expand Down