Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't raise KeyError when trying to navigate to nil next_page #203

Merged
merged 1 commit into from
Sep 13, 2016

Conversation

KevinBongart
Copy link
Contributor

I'm having trouble looping through paginated resources because the last page doesn't have a next link, and instead of resource.pages.next returning nil, it raises a KeyError.

resources = Api::Resource.paginated(per_page: 10)

while resources.present?
  resources.map do |resource|
    puts resouce.id
  end

  resources = begin
    resources.pages.next
  rescue KeyError
    nil
  end
end

This PR attempts to fix it, but there are many questions raised from investigating:

  1. Is it the responsibility of the gem or the user to handle the last page? Users currently have to rescue a KeyError exception, which isn't explicitly related to a missing resource.
  2. If the gem can make it easier for the user, should it return nil? []? an empty Resource object?
  3. What about the previous page on the very first page of a resource?

Or… am I iterating through pages the wrong way? Happy to complete this PR or add more documentation either way!

/cc @chingor13 @bvandenbos @sharshenov @reidab @dplummer

@KevinBongart
Copy link
Contributor Author

KevinBongart commented Aug 27, 2016

I ended up implementing a enumerator on top of the pagination system, based on @rossta's work (https://rossta.net/blog/paginated-resources-in-ruby.html)

class JsonApiClientEnumerator
  include Enumerable

  def initialize(resource, per_page: nil)
    @resource = resource.paginate(per_page: per_page)
    @paginator = nil
    @collection = []
  end

  def each(start = 0)
    return to_enum(:each) unless block_given?

    @collection[start..-1].each do |element|
      yield(element)
    end

    unless @last_response_empty
      start = [@collection.size, start].max
      fetch_next_page
      each(start, &Proc.new)
    end
  end

  private

  def fetch_next_page
    result_set = next_page_result_set

    if result_set.present?
      @collection += result_set
    else
      @last_response_empty = true
    end
  end

  def next_page_result_set
    @paginator = if @paginator.blank?
                   # first iteration: get the pages from the resource
                   @resource.pages
                 else
                   # next iterations: get the pages from the paginator object
                   @paginator.next.pages
                 end

    @paginator.result_set
  rescue KeyError
    # https://github.com/chingor13/json_api_client/pull/203
    []
  end
end

It can be used to lazily iterate through a paginated resource:

JsonApiClientEnumerator.new(CoolDog).each do |cool_dog|
  puts cool_dog.name
end

And customize the pagination limit:

JsonApiClientEnumerator.new(CoolDog, per_page: 5)

@KevinBongart
Copy link
Contributor Author

@chingor13 Would it be useful if I added this enumerator class to json_api_client?

@chingor13
Copy link
Collaborator

I feel like fixing this bug when looping is good enough for now. I think returning nil is fine for now. If you return an empty result set, you may still have to add a check for an empty result set or loop indefinitely.

@chingor13 chingor13 merged commit 2275617 into JsonApiClient:master Sep 13, 2016
@KevinBongart KevinBongart deleted the next_page branch September 13, 2016 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants