stas/jsonapi.rb

`jsonapi_pagination_meta` produces wrong values if called after calling `jsonapi_paginate`

lpagliari opened this issue ยท 4 comments

When using both jsonapi_paginate and jsonapi_pagination_meta to return pagination info, the order when each one is called matters.

Expected Behavior

Calling jsonapi_pagination_meta after calling jsonapi_paginate would produce the same results as calling them on the reverse order.

Actual Behavior

jsonapi_pagination_meta is returning the wrong data if it is called after jsonapi_paginate.

Steps to Reproduce the Problem

  1. Create 6 records of a model;
  2. On the controller, build your result by calling jsonapi_pagination_meta after jsonapi_paginate. For example:
    MySerializer.new(
      objects,
      links: jsonapi_pagination(objects),
      meta: jsonapi_pagination_meta(objects),
    )
    
  3. Call the index URL with params ?page[size]=2&page[number]=1;
  4. The result is:
    "meta": {
        "current": 3,
        "first": 1,
        "prev": 2
    },
    "links": {
        "self": "(...)?page[size]=2&page[number]=1",
        "current": "(...)?filter=&page[number]=1&page[size]=2&sort=",
        "next": "(...)?filter=&page[number]=2&page[size]=2&sort=",
        "last": "(...)?filter=&page[number]=3&page[size]=2&sort="
    }
    
    instead of:
    "meta": {
        "current": 1,
        "first": 2,
        "prev": 3
    },
    "links": {
        "self": "(...)?page[size]=2&page[number]=1",
        "current": "(...)?filter=&page[number]=1&page[size]=2&sort=",
        "next": "(...)?filter=&page[number]=2&page[size]=2&sort=",
        "last": "(...)?filter=&page[number]=3&page[size]=2&sort="
    }
    

Note that the links are correct, the error is on meta.

More Details

The reason seems to be on the fact that we are changing the original page number param on jsonapi_paginate, so when we call jsonapi_pagination_meta the original page number is changed, thus returning the wrong meta info:

def jsonapi_pagination(resources)
  (...)
  pagination.each do |page_name, number|
    original_params[:page][:number] = number # <== here is the issue.
                                             # original_params != params, *but* original_params[:page] == params[:page]
    links[page_name] = original_url + CGI.unescape(
      original_params.to_query
    )
  end

  links
end

The fix seems to be simple: just use a different object for the params. This means simply cloning original_params[:page]:

         *request.path_parameters.keys.map(&:to_s)
       ).to_unsafe_h.with_indifferent_access
 
-      original_params[:page] ||= {}
+      original_params[:page] = original_params[:page].clone || {}
       original_url = request.base_url + request.path + '?'
 
       pagination.each do |page_name, number|

or marshaling the entire params (to be safer):

       original_params = params.except(
         *request.path_parameters.keys.map(&:to_s)
       ).to_unsafe_h.with_indifferent_access
+      original_params = Marshal.load(Marshal.dump(original_params))
 
       original_params[:page] ||= {}
       original_url = request.base_url + request.path + '?'
stas commented

Thanks Luiza! I'll take a look ๐Ÿ™‡โ€โ™‚๏ธ

stas commented

@lpagliari I'll be releasing the fix. Thanks for reporting this.

On another note, based on your example, please consider using the API to handle the pagination. Particularly, you don't need to pass over the links and meta to the serializer if you're using the module as part of your controller.

If you're not using Rails, I'd like to learn more about your setup to see if we can provide a similar to Rails integration.

The reasoning behind this is that the serialization library interface should be considered an internal API (I might switch it to something a bit more maintained)...

Wishing you a great end of the week!

Hi, @stas, thanks for the quick response. I'm using this gem with Grape (I work with Banduk, who recently had submitted a PR related to that), so I'm not using the controller API this gem provides.

BTW great job, this gem is excellent! ๐Ÿ‘

stas commented

I see, thanks @lpagliari

Let me know if you find other things we can improve ๐Ÿ™‡โ€โ™‚๏ธ