TooManyRequests not handled by desk gem about desk

[alexanderdean]

Whoops, should have dug into the code:

65da365#diff-00b86054467328d3459932d83679b66bR49

[chriswarren]

Ah, great. Thanks for updating. Sorry that I hadn't had a chance to check it out yet.

[alexanderdean]

Actually - I had it opposite! config.use_max_requests = true enforces the maximum requests in the Desk gem; I thought it did the opposite. So reopening as I think this is an issue in the default max requests retry functionality...

[alexanderdean]

Hmm, maybe there isn't any back-off-and-retry-if-too-many-requests code. It would be cool to add support for this. What do you think Chris?

[chriswarren]

Yeah, that would be great to have; feel free to give it a shot.

[alexanderdean]

Okay giving this a go now...

[alexanderdean]

Right I have done some digging...

The existing gem has:

      when 420
        raise Desk::EnhanceYourCalm.new(error_message(env), env[:response_headers])

Which is confusing, because 420 is not listed as a Desk.com response code:

http://dev.desk.com/API/using-the-api/#status-codes

Also I don't understand this:

  class EnhanceYourCalm < Error
    # The number of seconds your application should wait before requesting date from the Search API again
    #
    # @see http://dev.desk.com/pages/rate-limiting
    def retry_after
      @http_headers.values_at('retry-after', 'Retry-After').detect {|value| value }.to_i
    end
  end

Again, this Retry-After header is not mentioned anywhere in the Desk.com documentation...

Is this all an artifact from an earlier version of Desk.com, or maybe a copy-paste error from an old Twitter API client?

Another thing:

    def ratelimit_limit
      @http_headers.values_at('x-ratelimit-limit', 'X-RateLimit-Limit').detect {|value| value }.to_i
    end

    def ratelimit_remaining
      @http_headers.values_at('x-ratelimit-limit', 'X-RateLimit-Limit').detect {|value| value }.to_i
    end

I think this is a copy-paste error - last strings should be 'x-ratelimit-remaining', 'X-RateLimit-Remaining'.

Confusingly, you do have an exception TooManyRequests:

  # Raised when Desk max_requests is reached and use_max_requests is set to true
  class TooManyRequests < StandardError; end

but this is an internal error, thrown when the client determines that it thinks Desk.com itself would start throwing TooManyRequests errors (which are uncaught).

As an internal error, TooManyRequests inherits from StandardError, so does not have access to the very helpful ratelimit_... arguments.

@chriswarren what do you suggest we do? My suggestion would be:

• Fix the mistakes in ratelimit_remaining
• Remove EnhanceYourCalm and stop catching 420
• Rename the current TooManyRequests to TooManyRequestsClient
• Create a new TooManyRequestsApi and throw it on 429

Please advise...

[alexanderdean]

It looks like the Zendesk API does support Retry-After, but not the Desk.com API:

https://support.zendesk.com/entries/35459738-Best-practices-for-avoiding-rate-limiting

[colinc]

Sorry I'm late to the party. Some of the code is a hold over from the Desk.com (formerly Assistly) API v1 and a little bit is a hold over from the Twitter gem. The EnhanceYourCalm comes from that Twitter holdover (from the pre-v1.1 API). Since Desk is now using the 429 response code (as indicated on the page you linked to above), it may be as simple as changing the "when 420" line to "when 429".

I know the code used to work and still passes the tests so it's likely just that they changed status response codes.

[colinc]

Also, if you look here: http://dev.desk.com/API/using-the-api/#rate-limits

It looks like, instead of using the 'Retry-after' header, we should now be using the 'X-Rate-Limit-Reset' header.

And for the ratelimit_limit and ratelimit_remaining functions we should have a dash between "rate" and "limit" since they're:

X-Rate-Limit-Limit
X-Rate-Limit-Remaining

Those 2 changes and the one above should resolve the problem. @alexanderdean if you want to make the changes and test them then that would be awesome.

[alexanderdean]

Hi @colinc - it sounds like we're on the same page. I will put something together per the above...