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

Add context with rate limit struct for Octokit::TooManyRequests and Octokit:: AbuseDetected errors #1270

Merged

Conversation

jatindhankhar
Copy link
Contributor

@jatindhankhar jatindhankhar commented Jun 30, 2020

This adds the context attribute to Octokit:: Error for much smoother handling of exception like Octokit::TooManyRequests and Octokit:: AbuseDetected, so that clients consuming them can use the values specified for better retries.

Can be consumed like this

 begin
  octokit_client.get('resource')
rescue Octokit::TooManyRequests => rate_limited_error
  puts "Retry in #{rate_limited_errro.retry_in} seconds"
 # Do fancy sutff
end 

Specifically useful when using with background processors like sidekiq

class PollingWorker
  include Sidekiq::Worker

  sidekiq_retry_in do |attempt, exception|
    case exception
    when Octokit::TooManyRequests
      exception.context.resets_in
    when Octokit::AbuseDetected
      exception.context.resets_in
    end
  end

  def perform
    # Do Polling work
    poller = Octokit::Client.new
    data = poller.fetch('/resource')
  end
end
 

The idea is to have meaningful context for all types of errors depending upon the type of error.

@jatindhankhar jatindhankhar changed the title [WIP] Add context with rate limit struct for Forbidden errors Add context with rate limit struct for Forbidden errors Jul 3, 2020
@jatindhankhar jatindhankhar marked this pull request as ready for review July 3, 2020 18:02
@jatindhankhar jatindhankhar changed the title Add context with rate limit struct for Forbidden errors Add context with rate limit struct for Octokit::TooManyRequests and Octokit:: AbuseDetected errors Jul 3, 2020
Copy link
Member

@tarebyte tarebyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Just a small change request.

lib/octokit/error.rb Outdated Show resolved Hide resolved
@jatindhankhar jatindhankhar requested a review from tarebyte July 8, 2020 04:53
@tarebyte tarebyte merged commit dbea2c6 into octokit:4-stable Jul 9, 2020
@jatindhankhar
Copy link
Contributor Author

jatindhankhar commented Jul 9, 2020

Thanks @tarebyte for merging this 😄

I have another proposition that I have been using for my personal projects and work, custom pagination for Octokit

Let me know if this also seems worth implementing, I will open relevant issues and PR.

Pagination methods

def should_fetch_more?(page_limit, page_iterated)
    page_limit.nil? || page_iterated < page_limit.to_i
  end

  def can_fetch_more?(last_response)
    last_response.rels[:next].present? && @octokit_client.rate_limit.remaining > 0
  end

  def iterate_pagination(opts = {}, result)
    page_limit = opts[:page_limit]
    page_iterated = 1
    last_response = @octokit_client.last_response
    record_limit = opts[:item_limit]
    while should_fetch_more?(page_limit, page_iterated) && can_fetch_more?(last_response)
      last_response = last_response.rels[:next].get
      if result.is_a?(Sawyer::Resource)
        result.items += result.is_a?(Array) ? last_response.data : last_response.data.items
      else
        result += result.is_a?(Array) ? last_response.data : last_response.data.items
      end

      page_iterated += 1
      return limit_result_count(result, record_limit.to_i) if record_limit && result.count >= record_limit.to_i
    end
    record_limit ? limit_result_count(result, record_limit.to_i) : result
  end

so any utility function can be passed as proc to execute

def execute(opts = {}, &block)
    iterate_pagination(opts, yield)
  end
def find_users(search_text = '', opts = {})
    execute(opts) { @octokit_client.search_users(search_text).items }
end

and can be used as

find_users("user_name", {sort: :indexed, order: :desc, page_limit: 1})

we can clean internal keys as well

 INTERNAL_OPTS_KEYS = [:page_limit, :item_limit]

def clean_opts(opts)
    opts.except(*INTERNAL_OPTS_KEYS)
 end

for methods that are parsed by octokit for equivalent parameters

@tarebyte
Copy link
Member

tarebyte commented Jul 9, 2020

@jatindhankhar mind opening another issue? It's hard to track requests at the end of a merged PR.

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