fix: make page[] param wrap consistent

.gitignore

@@ -24,3 +24,10 @@ Gemfile.lock
 *.gemfile.lock
 
 /coverage
+
+# IntellJ/RubyMine
+*.iml
+.idea/
+
+# asdf config
+.tool-versions

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- [#348](https://github.com/JsonApiClient/json_api_client/pull/348) - add NestedParamPaginator to address inconsistency in handling of pagination query string params (issue [#347](https://github.com/JsonApiClient/json_api_client/issues/347)).
+
 ## 1.12.2
 
 - [#350](https://github.com/JsonApiClient/json_api_client/pull/350) - fix resource including with blank `relationships` response data

README.md

@@ -599,6 +599,20 @@ class MyApi::Base < JsonApiClient::Resource
 end
 ```
 
+### NestedParamPaginator
+
+The current default `JsonApiClient::Paginating::Paginator` is inconsistent in the way it handles pagination query string params. The JSON:API spec seems to favor a nested structure like `page[page]=1&page[per_page]=10`. However, the default paginator doesn't always return or enforce that nested structure. See issue [#347](https://github.com/JsonApiClient/json_api_client/issues/347) for more details.
+
+If you'd like a more consistent pagination query string param behavior, assign the `JsonApiClient::Paginating::NestedParamPaginator` to your resource as a custom paginator, like so:
+
+```ruby
+class Order < JsonApiClient::Resource
+  self.paginator = JsonApiClient::Paginating::NestedParamPaginator
+end
+```
+
+You can also extend `NestedParamPaginator` in your custom paginators or assign the `page_param` or `per_page_param` as with the default version above.
+
 ### Custom type
 
 If your model must be named differently from classified type of resource you can easily customize it.

lib/json_api_client/paginating.rb

@@ -1,5 +1,6 @@
 module JsonApiClient
   module Paginating
     autoload :Paginator, 'json_api_client/paginating/paginator'
+    autoload :NestedParamPaginator, 'json_api_client/paginating/nested_param_paginator'
   end
-end
+end

lib/json_api_client/paginating/nested_param_paginator.rb

@@ -0,0 +1,140 @@
+module JsonApiClient
+  module Paginating
+    # An alternate, more consistent Paginator that always wraps
+    # pagination query string params in a top-level wrapper_name,
+    # e.g. page[offset]=2, page[limit]=10.
+    class NestedParamPaginator
+      DEFAULT_WRAPPER_NAME = "page".freeze
+      DEFAULT_PAGE_PARAM = "page".freeze
+      DEFAULT_PER_PAGE_PARAM = "per_page".freeze
+
+      # Define class accessors as methods to enforce standard way
+      # of defining pagination related query string params.
+      class << self
+
+        def wrapper_name
+          @_wrapper_name ||= DEFAULT_WRAPPER_NAME
+        end
+
+        def wrapper_name=(param = DEFAULT_WRAPPER_NAME)
+          raise ArgumentError, "don't wrap wrapper_name" unless valid_param?(param)
+
+          @_wrapper_name = param.to_s
+        end
+
+        def page_param
+          @_page_param ||= DEFAULT_PAGE_PARAM
+          "#{wrapper_name}[#{@_page_param}]"
+        end
+
+        def page_param=(param = DEFAULT_PAGE_PARAM)
+          raise ArgumentError, "don't wrap page_param" unless valid_param?(param)
+
+          @_page_param = param.to_s
+        end
+
+        def per_page_param
+          @_per_page_param ||= DEFAULT_PER_PAGE_PARAM
+          "#{wrapper_name}[#{@_per_page_param}]"
+        end
+
+        def per_page_param=(param = DEFAULT_PER_PAGE_PARAM)
+          raise ArgumentError, "don't wrap per_page_param" unless valid_param?(param)
+
+          @_per_page_param = param
+        end
+
+        private
+
+        def valid_param?(param)
+          !(param.nil? || param.to_s.include?("[") || param.to_s.include?("]"))
+        end
+
+      end
+
+      attr_reader :params, :result_set, :links
+
+      def initialize(result_set, data)
+        @params = params_for_uri(result_set.uri)
+        @result_set = result_set
+        @links = data["links"]
+      end
+
+      def next
+        result_set.links.fetch_link("next")
+      end
+
+      def prev
+        result_set.links.fetch_link("prev")
+      end
+
+      def first
+        result_set.links.fetch_link("first")
+      end
+
+      def last
+        result_set.links.fetch_link("last")
+      end
+
+      def total_pages
+        if links["last"]
+          uri = result_set.links.link_url_for("last")
+          last_params = params_for_uri(uri)
+          last_params.fetch(page_param, &method(:current_page)).to_i
+        else
+          current_page
+        end
+      end
+
+      # this is an estimate, not necessarily an exact count
+      def total_entries
+        per_page * total_pages
+      end
+      def total_count; total_entries; end
+
+      def offset
+        per_page * (current_page - 1)
+      end
+
+      def per_page
+        params.fetch(per_page_param) do
+          result_set.length
+        end.to_i
+      end
+
+      def current_page
+        params.fetch(page_param, 1).to_i
+      end
+
+      def out_of_bounds?
+        current_page > total_pages
+      end
+
+      def previous_page
+        current_page > 1 ? (current_page - 1) : nil
+      end
+
+      def next_page
+        current_page < total_pages ? (current_page + 1) : nil
+      end
+
+      def page_param
+        self.class.page_param
+      end
+
+      def per_page_param
+        self.class.per_page_param
+      end
+
+      alias limit_value per_page
+
+      protected
+
+      def params_for_uri(uri)
+        return {} unless uri
+        uri = Addressable::URI.parse(uri)
+        ( uri.query_values || {} ).with_indifferent_access
+      end
+    end
+  end
+end

lib/json_api_client/query/builder.rb

@@ -146,7 +146,13 @@ def primary_key_params
       end
 
       def pagination_params
-        @pagination_params.empty? ? {} : {page: @pagination_params}
+        if klass.paginator.ancestors.include?(Paginating::Paginator)
+          # Original Paginator inconsistently wraps pagination params here. Keeping
+          # default behavior for now so as not to break backward compatibility.
+          @pagination_params.empty? ? {} : {page: @pagination_params}
+        else
+          @pagination_params
+        end
       end
 
       def includes_params

test/unit/nested_param_paginator_test.rb

@@ -0,0 +1,82 @@
+require 'test_helper'
+
+class NestedParamPaginatorTest < MiniTest::Test
+
+  class Book < JsonApiClient::Resource
+    self.site = "http://example.com/"
+  end
+
+  def setup
+    @nested_param_paginator = JsonApiClient::Paginating::NestedParamPaginator
+    @default_paginator = JsonApiClient::Paginating::Paginator
+    Article.paginator = @nested_param_paginator
+  end
+
+  def teardown
+    @nested_param_paginator.page_param = @nested_param_paginator::DEFAULT_PAGE_PARAM
+    @nested_param_paginator.per_page_param = @nested_param_paginator::DEFAULT_PER_PAGE_PARAM
+    Article.paginator = @default_paginator
+  end
+
+  def test_default_page_params_wrapped_consistently
+    assert_equal "page[page]", @nested_param_paginator.page_param
+    assert_equal "page[per_page]", @nested_param_paginator.per_page_param
+  end
+
+  def test_custom_page_params_wrapped_consistently
+    @nested_param_paginator.page_param = "offset"
+    @nested_param_paginator.per_page_param = "limit"
+    assert_equal "page[offset]", @nested_param_paginator.page_param
+    assert_equal "page[limit]", @nested_param_paginator.per_page_param
+  end
+
+  def test_custom_page_param_does_not_allow_double_wrap
+    assert_raises ArgumentError do
+      @nested_param_paginator.page_param = "page[number]"
+    end
+  end
+
+  def test_custom_per_page_param_does_not_allow_double_wrap
+    assert_raises ArgumentError do
+      @nested_param_paginator.per_page_param = "page[size]"
+    end
+  end
+
+  def test_pagination_params_total_calculations
+    @nested_param_paginator.page_param = "number"
+    @nested_param_paginator.per_page_param = "size"
+    stub_request(:get, "http://example.com/articles?page[number]=1&page[size]=2")
+        .to_return(headers: {content_type: "application/vnd.api+json"}, body: {
+            data: [{
+                       type: "articles",
+                       id: "1",
+                       attributes: {
+                           title: "JSON API paints my bikeshed!"
+                       }
+                   },
+                   {
+                       type: "articles",
+                       id: "2",
+                       attributes: {
+                           title: "json_api_client counts pages correctly"
+                       }
+                   }],
+            links: {
+                self:  "http://example.com/articles?page[number]=1&page[size]=2",
+                next:  "http://example.com/articles?page[number]=2&page[size]=2",
+                prev:  nil,
+                first: "http://example.com/articles?page[number]=1&page[size]=2",
+                last:  "http://example.com/articles?page[number]=4&page[size]=2"
+            }
+        }.to_json)
+
+    articles = Article.paginate(page: 1, per_page: 2).to_a
+    assert_equal 1, articles.current_page
+    assert_equal 4, articles.total_pages
+    assert_equal 8, articles.total_entries
+  ensure
+    @nested_param_paginator.page_param = "page"
+    @nested_param_paginator.per_page_param = "per_page"
+  end
+
+end