diff --git a/.gitignore b/.gitignore index 7374a56d..6efdc659 100644 --- a/.gitignore +++ b/.gitignore @@ -24,3 +24,10 @@ Gemfile.lock *.gemfile.lock /coverage + +# IntellJ/RubyMine +*.iml +.idea/ + +# asdf config +.tool-versions diff --git a/CHANGELOG.md b/CHANGELOG.md index e94256fe..7c58fa0c 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index a21e6284..a3ad7a1c 100644 --- a/README.md +++ b/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. diff --git a/lib/json_api_client/paginating.rb b/lib/json_api_client/paginating.rb index 9443fff8..f49563a2 100644 --- a/lib/json_api_client/paginating.rb +++ b/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 \ No newline at end of file +end diff --git a/lib/json_api_client/paginating/nested_param_paginator.rb b/lib/json_api_client/paginating/nested_param_paginator.rb new file mode 100644 index 00000000..27e5ca4e --- /dev/null +++ b/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 diff --git a/lib/json_api_client/query/builder.rb b/lib/json_api_client/query/builder.rb index 8e82e77f..7799e67c 100644 --- a/lib/json_api_client/query/builder.rb +++ b/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 diff --git a/test/unit/nested_param_paginator_test.rb b/test/unit/nested_param_paginator_test.rb new file mode 100644 index 00000000..f6e788a5 --- /dev/null +++ b/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 diff --git a/test/unit/nested_param_paginator_top_level_links_test.rb b/test/unit/nested_param_paginator_top_level_links_test.rb new file mode 100644 index 00000000..faad42e6 --- /dev/null +++ b/test/unit/nested_param_paginator_top_level_links_test.rb @@ -0,0 +1,282 @@ +require 'test_helper' + +# Copied from TopLevelLinksTest and modified to have consistent +# pagination params using the new NestedParamPaginator +class NestedParamPaginatorTopLevelLinksTest < MiniTest::Test + + 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_can_parse_global_links + stub_request(:get, "http://example.com/articles/1") + .to_return(headers: {content_type: "application/vnd.api+json"}, body: { + data: { + type: "articles", + id: "1", + attributes: { + title: "JSON API paints my bikeshed!" + } + }, + links: { + self: "http://example.com/articles/1", + related: "http://example.com/articles/1/related" + } + }.to_json) + stub_request(:get, "http://example.com/articles/1/related") + .to_return(headers: {content_type: "application/vnd.api+json"}, body: { + data: { + type: "article-image", + id: "14", + image: "http://foo.com/cat.png" + } + }.to_json) + + articles = Article.find(1) + links = articles.links + assert links + assert links.respond_to?(:related), "ResultSet links should respond to related" + + related = links.related + assert related.is_a?(JsonApiClient::ResultSet), "expected related link to return another ResultSet" + end + + def test_can_parse_pagination_links + stub_request(:get, "http://example.com/articles") + .to_return(headers: {content_type: "application/vnd.api+json"}, body: { + data: [{ + type: "articles", + id: "1", + attributes: { + title: "JSON API paints my bikeshed!" + } + }], + links: { + self: "http://example.com/articles", + next: "http://example.com/articles?page[page]=2", + prev: nil, + first: "http://example.com/articles", + last: "http://example.com/articles?page[page]=6" + } + }.to_json) + stub_request(:get, "http://example.com/articles?page[page]=2") + .to_return(headers: {content_type: "application/vnd.api+json"}, body: { + data: [{ + type: "articles", + id: "2", + attributes: { + title: "This is tha BOMB" + } + }], + links: { + self: "http://example.com/articles?page[page]=2", + next: "http://example.com/articles?page[page]=3", + prev: "http://example.com/articles", + first: "http://example.com/articles", + last: "http://example.com/articles?page[page]=6" + } + }.to_json) + + assert_pagination + end + + def test_can_parse_pagination_links_with_custom_config + @nested_param_paginator.page_param = "number" + + stub_request(:get, "http://example.com/articles") + .to_return(headers: {content_type: "application/vnd.api+json"}, body: { + data: [{ + type: "articles", + id: "1", + attributes: { + title: "JSON API paints my bikeshed!" + } + }], + links: { + self: "http://example.com/articles", + next: "http://example.com/articles?#{{page: {number: 2}}.to_query}", + prev: nil, + first: "http://example.com/articles", + last: "http://example.com/articles?#{{page: {number: 6}}.to_query}" + } + }.to_json) + stub_request(:get, "http://example.com/articles?#{{page: {number: 2}}.to_query}") + .to_return(headers: {content_type: "application/vnd.api+json"}, body: { + data: [{ + type: "articles", + id: "2", + attributes: { + title: "This is tha BOMB" + } + }], + links: { + self: "http://example.com/articles?#{{page: {number: 2}}.to_query}", + next: "http://example.com/articles?#{{page: {number: 3}}.to_query}", + prev: "http://example.com/articles", + first: "http://example.com/articles", + last: "http://example.com/articles?#{{page: {number: 6}}.to_query}" + } + }.to_json) + + assert_pagination + end + + def test_can_parse_pagination_links_when_no_next_page + stub_request(:get, "http://example.com/articles") + .to_return(headers: {content_type: "application/vnd.api+json"}, body: { + data: [{ + type: "articles", + id: "1", + attributes: { + title: "JSON API paints my bikeshed!" + } + }], + links: { + self: "http://example.com/articles", + prev: nil, + first: "http://example.com/articles", + last: "http://example.com/articles?page[page]=1" + } + }.to_json) + + assert_pagination_when_no_next_page + end + + def test_can_parse_complex_pagination_links + stub_request(:get, "http://example.com/articles") + .to_return(headers: {content_type: "application/vnd.api+json"}, body: { + data: [{ + type: "articles", + id: "1", + attributes: { + title: "JSON API paints my bikeshed!" + } + }], + links: { + self: { + href: "http://example.com/articles", + meta: {} + }, + next: { + href: "http://example.com/articles?page[page]=2", + meta: {} + }, + prev: nil, + first: { + href: "http://example.com/articles", + meta: {} + }, + last: { + href: "http://example.com/articles?page[page]=6", + meta: {} + } + } + }.to_json) + stub_request(:get, "http://example.com/articles?page[page]=2") + .to_return(headers: {content_type: "application/vnd.api+json"}, body: { + data: [{ + type: "articles", + id: "2", + attributes: { + title: "This is tha BOMB" + } + }], + links: { + self: { + href: "http://example.com/articles?page[page]=2", + meta: {} + }, + next: { + href: "http://example.com/articles?page[page]=3", + meta: {} + }, + prev: { + href: "http://example.com/articles", + meta: {} + }, + first: { + href: "http://example.com/articles", + meta: {} + }, + last: { + href: "http://example.com/articles?page[page]=6", + meta: {} + } + } + }.to_json) + + assert_pagination + end + + private + + def assert_pagination + articles = Article.all + + # test kaminari pagination params + assert_equal 1, articles.current_page + assert_equal 1, articles.per_page + assert_equal 6, articles.total_pages + assert_equal 0, articles.offset + assert_equal 6, articles.total_entries + assert_equal 1, articles.limit_value + assert_equal 2, articles.next_page + assert_equal 1, articles.per_page + assert_equal false, articles.out_of_bounds? + + # test browsing to next page + pages = articles.pages + assert pages.respond_to?(:next) + assert pages.respond_to?(:prev) + assert pages.respond_to?(:last) + assert pages.respond_to?(:first) + + page2 = articles.pages.next + assert page2.is_a?(JsonApiClient::ResultSet) + assert_equal 1, page2.length + article = page2.first + assert_equal "2", article.id + assert_equal "This is tha BOMB", article.title + + # test browsing to the previous page + page1 = page2.pages.prev + assert page1.is_a?(JsonApiClient::ResultSet) + assert_equal 1, page1.length + article = page1.first + assert_equal "1", article.id + assert_equal "JSON API paints my bikeshed!", article.title + end + + def assert_pagination_when_no_next_page + articles = Article.all + + # test kaminari pagination params + assert_equal 1, articles.current_page + assert_equal 1, articles.per_page + assert_equal 1, articles.total_pages + assert_equal 0, articles.offset + assert_equal 1, articles.total_entries + assert_equal 1, articles.limit_value + assert_nil articles.next_page + assert_equal 1, articles.per_page + assert_equal false, articles.out_of_bounds? + + # test browsing to next page + pages = articles.pages + assert pages.respond_to?(:next) + assert pages.respond_to?(:prev) + assert pages.respond_to?(:last) + assert pages.respond_to?(:first) + + page2 = articles.pages.next + assert page2.nil? + end +end