Implements relative cursor pagination. #594

garethson · 2019-07-04T17:50:53Z

Implements a convenient API for using our new relative cursor pagination, which has been partially released as of API version 2019-07.

Rather than providing an automatic way to iterate through all records in the query, we're adding the following methods to any model collection:

next_page?
previous_page?
fetch_next_page
fetch_previous_page

This allows you to perform the navigation however you see fit. Here is an example of using the API to iterate and process a list of all pending orders:

  orders = ShopifyAPI::Order.find(:all, params: { financial_status: 'pending' })
  process_orders(orders)
  while orders.next_page?
    orders = orders.fetch_next_page
    process_orders(orders)
  end

Currently making all endpoints available on the unstable API, and we can merge this when all endpoints have relative cursor pagination available in Shopify.

Tested:

products = ShopifyAPI::Product.where(limit: 3)
=> [1933186826296, 1933188005944, 1933188563000]

products.previous_page?
=> false

products.next_page?
=> true

products = products.fetch_next_page
=> [1933189120056, 1933200949304, 1970112430136]

products.previous_page?
=> true

products.next_page?
=> true

products = products.fetch_previous_page
=> [1933186826296, 1933188005944, 1933188563000]

garethson · 2019-07-04T18:23:01Z

lib/shopify_api/collection_pagination.rb

+    def fetch_page(page_info)
+      return [] unless page_info
+
+      resource_class.where(original_params.merge(page_info: next_page_info))


We could do where(page_info: next_page_info)but we're supporting old versions of ActiveResource, and where was added only in later versions.

garethson · 2019-07-04T18:30:56Z

lib/shopify_api/collection_pagination.rb

+
+    def fetch_next_page
+      fetch_page(next_page_info)
+    end


Thought about it, but decided to not memoize the next/previous page. If you're calling fetch, just have it fetch.

kmcphillips

Approach is good! A few small questions. Thanks for the great tests.

kmcphillips · 2019-07-04T18:24:22Z

lib/shopify_api/collection_pagination.rb

+
+      return nil unless link_header.present?
+
+      CGI.parse(link_header.url.query)["page_info"][0]


Good place to dig here, as this will raises if page_info key is missing or not an array. Do we support ruby below 2.3 on this gem?

s.required_ruby_version = ">= 2.4"

kmcphillips · 2019-07-05T20:21:12Z

lib/shopify_api/collection_pagination.rb

+
+    def pagination_link_headers
+      @pagination_link_headers ||= ShopifyAPI::PaginationLinkHeaders.new(
+        ShopifyAPI::Base.connection.response["Link"]


I don't think I understand why we get a response off the base connection here.

Does that mean this gem is not threadsafe?

ActiveResource appears to be threadsafe, and sets the connection class to be threadsafe.

We have a few places where we reference the base connection for response attributes. Here and here.

This was added in AR 4.1, and we only support >= 4.1

Of note, is that headers is explicitly handled as threadsafe in this gem, but that was done before the threadsafe stuff was added. I believe this can be removed, which I'll look into after this.

lib/shopify_api/pagination_link_headers.rb

kmcphillips

All makes sense to me. Thanks for doing this.

DrewMartin · 2019-07-16T19:50:34Z

Should we update the readme?

DrewMartin · 2019-07-16T22:15:24Z

lib/shopify_api/collection_pagination.rb

+    def fetch_page(page_info)
+      return [] unless page_info
+
+      resource_class.where(original_params.merge(page_info: page_info))


If your first request had the param order: 'title asc' then this would merge page_info to that? Because that would make a bad URL. If we tried to request a page against Shopify with the original params + page_info it will fail if there are filters or sort orders in there.

We're probably better off just parsing out all the params out of the next/previous url and using them directly

Discussed offline, I'm going to take that approach, parse the next/previous page and use those for params.

DrewMartin · 2019-07-17T16:43:39Z

test/pagination_test.rb

@@ -9,14 +9,12 @@ def setup
    @next_page_info = "eyJkaXJlY3Rpb24iOiJuZXh0IiwibGFzdF9pZCI6NDQwMDg5NDIzLCJsYXN0X3ZhbHVlIjoiNDQwMDg5NDIzIn0%3D"
    @previous_page_info = "eyJsYXN0X2lkIjoxMDg4MjgzMDksImxhc3RfdmFsdWUiOiIxMDg4MjgzMDkiLCJkaXJlY3Rpb24iOiJuZXh0In0%3D"

-    @next_link_header = "<https://this-is-my-test-shop.myshopify.com/admin/api/unstable/orders.json?page_info=#{@next_page_info}>; rel=\"next\""
-    @previous_link_header = "<https://this-is-my-test-shop.myshopify.com/admin/api/unstable/orders.json?page_info=#{@previous_page_info}>; rel=\"previous\""
+    @next_link_header = "<https://this-is-my-test-shop.myshopify.com/admin/api/unstable/orders.json?page_info=#{@next_page_info}>; rel=\"next\">"


I don't think this is right. The chevrons enclose the URL, not the whole tuple

Suggested change

@next_link_header = "<https://this-is-my-test-shop.myshopify.com/admin/api/unstable/orders.json?page_info=#{@next_page_info}>; rel=\"next\">"

@next_link_header = "<https://this-is-my-test-shop.myshopify.com/admin/api/unstable/orders.json?page_info=#{@next_page_info}>; rel=\"next\""

@DrewMartin Fixed up for both.

DrewMartin · 2019-07-17T16:51:08Z

test/pagination_test.rb

+    @previous_page_info = "eyJsYXN0X2lkIjoxMDg4MjgzMDksImxhc3RfdmFsdWUiOiIxMDg4MjgzMDkiLCJkaXJlY3Rpb24iOiJuZXh0In0%3D"
+
+    @next_link_header = "<https://this-is-my-test-shop.myshopify.com/admin/api/unstable/orders.json?page_info=#{@next_page_info}>; rel=\"next\">"
+    @previous_link_header = "<https://this-is-my-test-shop.myshopify.com/admin/api/unstable/orders.json?page_info=#{@previous_page_info}>; rel=\"previous\">"


Suggested change

@previous_link_header = "<https://this-is-my-test-shop.myshopify.com/admin/api/unstable/orders.json?page_info=#{@previous_page_info}>; rel=\"previous\">"

@previous_link_header = "<https://this-is-my-test-shop.myshopify.com/admin/api/unstable/orders.json?page_info=#{@previous_page_info}>; rel=\"previous\""

DrewMartin · 2019-07-17T16:51:18Z

test/pagination_test.rb

+
+  test "uses all passed in querystring parameters" do
+    params = "page_info=#{@next_page_info}&limit=50&fields=#{CGI.escape('id,created_at')}"
+    @next_link_header = "<https://this-is-my-test-shop.myshopify.com/admin/api/unstable/orders.json?#{params}>; rel=\"next\">"


Suggested change

@next_link_header = "<https://this-is-my-test-shop.myshopify.com/admin/api/unstable/orders.json?#{params}>; rel=\"next\">"

@next_link_header = "<https://this-is-my-test-shop.myshopify.com/admin/api/unstable/orders.json?#{params}>; rel=\"next\""

garethson · 2019-07-17T17:02:47Z

I've updated this to, instead of using original_params, simply passing in all the params returned via the next/previous link headers.

Tested this here, with products with order, as well as products with fields,limit,order.

Tested with order (which is not yet converted).

Finally: Tested with events with filter,limit,order.

README.md

ignacio-chiazzo

Code LGTM, just two questions

ignacio-chiazzo · 2019-07-17T18:17:10Z

lib/shopify_api/pagination_link_headers.rb

+        url = parts[0][/<(.*)>/, 1]
+        rel = parts[1][/rel="(.*)"/, 1]&.to_sym
+
+        url = URI.parse(url)


can you add validations for url + rel?

ignacio-chiazzo · 2019-07-17T18:37:40Z

lib/active_resource/collection_ext.rb

+  class Collection
+    prepend ShopifyAPI::CollectionPagination
+  end
+end


as discussed IRL do we have to solve the problem of querying the next/prev link after doing another query?, e.g.

page1 = Produdcts.find(:all) page2 = Produdcts.find(:all) page2.fetch_next_page page1.next_page page1.previous_page

Updated to parse the headers on collection initialization, vs lazily evaluating them. Also added a test for that case. The collection objects don't have access to the actual response, so we have to rely on ShopifyAPI::Base.connection.response["Link"], which is always for the last made request.

tanema

This looks good, do you have plans for disabling page based pagination in the future like raising an error if they pass in a page param? I just worry that there are old tutorials out there that will be used and the debelopers will be confused why they are not getting more items (because the page param is ignored)

garethson · 2019-07-17T21:01:14Z

@tanema Thanks. Regarding disabling the page param, the API currently raises a fairly descriptive error with a link to the docs if a page param is passed in for a version of the API that isn't supported.

Having a client side error raise would be helpful and would probably catch errors earlier for developers, rather than seeing it in production. I can look into adding this after this is merged.

dominiquesr · 2019-07-29T13:57:54Z

❤️ It's happening! This is going to have a huge impact on adoption!

isaacbowen · 2019-08-05T15:37:46Z

🙏

garethson requested a review from a team as a code owner July 4, 2019 17:50

garethson force-pushed the relative-cursor-pagination branch 3 times, most recently from 950c67f to 9d6464b Compare July 4, 2019 18:16

garethson commented Jul 4, 2019

View reviewed changes

garethson force-pushed the relative-cursor-pagination branch from 9d6464b to 7ac6eca Compare July 4, 2019 18:24

garethson commented Jul 4, 2019

View reviewed changes

garethson force-pushed the relative-cursor-pagination branch 2 times, most recently from 20af685 to 6c45cd0 Compare July 5, 2019 14:39

garethson changed the title ~~WIP - Implements relative cursor pagination.~~ Implements relative cursor pagination. Jul 5, 2019

garethson force-pushed the relative-cursor-pagination branch from 6c45cd0 to 6600fae Compare July 5, 2019 15:02

kmcphillips reviewed Jul 5, 2019

View reviewed changes

garethson force-pushed the relative-cursor-pagination branch from 6600fae to f2c05be Compare July 8, 2019 19:19

kmcphillips approved these changes Jul 8, 2019

View reviewed changes

DrewMartin reviewed Jul 16, 2019

View reviewed changes

garethson force-pushed the relative-cursor-pagination branch from ef49dad to 688d3da Compare July 17, 2019 16:41

DrewMartin suggested changes Jul 17, 2019

View reviewed changes

DrewMartin reviewed Jul 17, 2019

View reviewed changes

garethson force-pushed the relative-cursor-pagination branch 2 times, most recently from 65ab254 to 42770b8 Compare July 17, 2019 16:54

DrewMartin approved these changes Jul 17, 2019

View reviewed changes

garethson force-pushed the relative-cursor-pagination branch from 22c5ce0 to 8f5ae0a Compare July 17, 2019 17:58

DrewMartin reviewed Jul 17, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

ignacio-chiazzo reviewed Jul 17, 2019

View reviewed changes

garethson force-pushed the relative-cursor-pagination branch 3 times, most recently from 7e411e9 to dc1d360 Compare July 17, 2019 18:53

tanema approved these changes Jul 17, 2019

View reviewed changes

Implements relative cursor pagination.

f16e479

garethson force-pushed the relative-cursor-pagination branch from dc1d360 to f16e479 Compare July 17, 2019 21:04

garethson merged commit 51984e8 into master Aug 8, 2019

garethson deleted the relative-cursor-pagination branch August 8, 2019 16:26

garethson mentioned this pull request Aug 8, 2019

Adds a 2019-10 prerelease API version #599

Merged

jtgrenz mentioned this pull request Aug 12, 2019

subclass collection and don't override AR collection #602

Merged

jiri-hacmac mentioned this pull request Aug 13, 2019

API Version 2019-07 pagination #595

Closed

garethson mentioned this pull request Aug 13, 2019

Change cursor pagination API version to 2019-10 #604

Merged

DrewMartin mentioned this pull request Aug 27, 2019

Allow cursor pagination in 2019-07 release for select resources #611

Merged

DrewMartin temporarily deployed to rubygems August 29, 2019 22:00 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements relative cursor pagination. #594

Implements relative cursor pagination. #594

garethson commented Jul 4, 2019 •

edited

garethson Jul 4, 2019

garethson Jul 4, 2019

kmcphillips left a comment

kmcphillips Jul 4, 2019

garethson Jul 5, 2019

kmcphillips Jul 5, 2019

garethson Jul 5, 2019

kmcphillips left a comment

DrewMartin commented Jul 16, 2019

DrewMartin Jul 16, 2019

garethson Jul 17, 2019

DrewMartin Jul 17, 2019

DrewMartin Jul 17, 2019

garethson Jul 17, 2019

DrewMartin Jul 17, 2019

DrewMartin Jul 17, 2019

garethson commented Jul 17, 2019

ignacio-chiazzo left a comment

ignacio-chiazzo Jul 17, 2019

ignacio-chiazzo Jul 17, 2019

garethson Jul 17, 2019

tanema left a comment

garethson commented Jul 17, 2019

dominiquesr commented Jul 29, 2019

isaacbowen commented Aug 5, 2019


		return nil unless link_header.present?

		CGI.parse(link_header.url.query)["page_info"][0]

	@next_link_header = "<https://this-is-my-test-shop.myshopify.com/admin/api/unstable/orders.json?page_info=#{@next_page_info}>; rel=\"next\">"
	@next_link_header = "<https://this-is-my-test-shop.myshopify.com/admin/api/unstable/orders.json?page_info=#{@next_page_info}>; rel=\"next\""

	@previous_link_header = "<https://this-is-my-test-shop.myshopify.com/admin/api/unstable/orders.json?page_info=#{@previous_page_info}>; rel=\"previous\">"
	@previous_link_header = "<https://this-is-my-test-shop.myshopify.com/admin/api/unstable/orders.json?page_info=#{@previous_page_info}>; rel=\"previous\""

Implements relative cursor pagination. #594

Implements relative cursor pagination. #594

Conversation

garethson commented Jul 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmcphillips left a 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

Choose a reason for hiding this comment

kmcphillips left a comment

Choose a reason for hiding this comment

DrewMartin commented Jul 16, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garethson commented Jul 17, 2019

ignacio-chiazzo left a 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

tanema left a comment

Choose a reason for hiding this comment

garethson commented Jul 17, 2019

dominiquesr commented Jul 29, 2019

isaacbowen commented Aug 5, 2019

garethson commented Jul 4, 2019 •

edited