reverted the revert, but added an option to restore old behavior

Author: mackuba

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 The "really niche bugfix" edition:
 
+* don't stop fetching in `fetch_all` if an empty page is returned but the cursor is not nil; it's technically allowed for the server to return an empty page but still have more data to send, e.g. if the service does some filtering at the last step and all records happen to be filtered out
+  - unfortunately, this causes problems with some specific endpoints which incorrectly return a cursor when reaching the end of data, so you can restore the previous behavior by setting the `stop_fetch_on_empty_page` option
 * in `post_request`, don't set Content-Type to "application/json" if the data sent is a string or nil (it might cause an error in some cases, like when uploading some binary content)
 * handle the (somewhat theoretical but possible) case where an access token is not a JWT but just some opaque blob – in that case, Minisky will now not throw an error trying to parse it, but just treat it as "unknown" and will not try to refresh it
   - note: at the moment Minisky will not catch the "token expired" error and refresh the token automatically in such scenario

lib/minisky/requests.rb

@@ -50,6 +50,16 @@ module Requests
     #
     attr_accessor :default_progress
 
+    # By default, when `fetch_all` receives a response with an empty page with no records but
+    # which includes a cursor for the next page, it keeps fetching until it receives a response
+    # with null cursor. This is more technically correct, but can cause problems with some
+    # non-compliant APIs, so you can set this option to stop fetching when an empty page
+    # is received.
+    #
+    # @return [Boolean]
+    #
+    attr_accessor :stop_fetch_on_empty_page
+
     attr_writer :send_auth_headers
     attr_writer :auto_manage_tokens
 
@@ -285,10 +295,7 @@ def fetch_all(method, params = nil, auth: default_auth_mode,
         params[:cursor] = cursor
         pages += 1
 
-        # TODO: don't break if records.empty? but cursor != nil
-        # but currently this causes problems with some Bluesky endpoints like getBookmarks
-
-        break if !cursor || records.empty? || pages == max_pages
+        break if !cursor || pages == max_pages || (stop_fetch_on_empty_page && records.empty?)
         break if break_when && records.any? { |x| break_when.call(x) }
       end
 

spec/shared/ex_fetch_all.rb

@@ -218,6 +218,21 @@
         result = subject.fetch_all('com.example.service.fetchAll', field: 'feed')
         result.should == ['one', 'two', 'three', 'six']
       end
+
+      context 'with stop_fetch_on_empty_page option set to true' do
+        before do
+          subject.stop_fetch_on_empty_page = true
+        end
+
+        it 'should stop at the first empty page' do
+          result = subject.fetch_all('com.example.service.fetchAll', field: 'feed')
+          result.should == ['one', 'two', 'three']
+
+          WebMock.should have_requested(:get, @stubbed_urls[0]).once
+          WebMock.should have_requested(:get, @stubbed_urls[1]).once
+          WebMock.should_not have_requested(:get, @stubbed_urls[2])
+        end
+      end
     end
 
     context 'when field is not passed' do