Add information about all rate limits in TooManyRequests error

[GCorbel]

Some endpoints have different kind of limits as DMs and Likes.

I found no official list of headers, but I saw x-user-limit-24hour-limit and x-app-limit-24hour-limit.

For example, when the 15 minutes and 24 hours limits of requests allowed "POST likes" are reached, x-rate-limit-reset gives the next reset for the 15 minutes limit. The reset time for the 24 hours limit is displayed in x-user-limit-24hour-reset. Every request performed before the time in x-user-limit-24hour-reset will fail.

I added the code to have the information about all limits and changed retry_after to return the time that we can send requests again.

This script :

begin
  body = {
    "tweet_id": "<id>"
  }
  client.post("users/<user id>/likes", JSON.dump(body))
rescue X::TooManyRequests => e
  pp e.overall_limits
  pp e.next_available_at
end

Return this value :

{"rate-limit"=>{"limit"=>50, "remaining"=>22, "reset_at"=>2023-11-28 17:05:49 -0500}, "user-limit-24hour"=>{"limit"=>1000, "remaining"=>0, "reset_at"=>2023-11-29 13:59:38 -0500}}
2023-11-29 13:59:38 -0500

[GCorbel]

I don't really like what this method looks like. Le me know if you have a better idea.

[GCorbel]

I refactored to find limits by using the regex /x-(.*-limit.*)-limit/

[GCorbel]

Every request before retry_after are supposed to fail.

[GCorbel]

This takes the max time of reached limits.

[sferik]

Hi @GCorbel,

Thank you for submitting this pull request!

I think it makes sense to expose all of the API’s rate limit header values as methods on X::TooManyRequests. I was previously unaware that these additional rate limit headers you’ve discovered existed and I am still unable to find comprehensive documentation of these headers.

As for your specific implementation, it appears that several linter checks are failing. Some of these issues can be corrected automatically with the following command:

bundle exec rubocop --autocorrect

That’s a good place to start, but still leaves 10 issues that need to be corrected manually. The two big ones are related to the length and complexity of the all_limits methods, both of which are too high. I’d suggest breaking that into individual methods—one for each limit.

The remaining linter complaints are about the tests, but those should be simplified when all_limits is broken into smaller methods. It might make sense to create a new too_many_requests_test.rb file separate from error_test.rb, since this is already the most complex error class and becoming even more so.

Mutation tests are also failing, but don’t worry about fixing those until after you make the other changes to the code/tests.

Thanks again for your patch!

[GCorbel]

Hello, I will fix Rubocop issue. I suppose this have to be added in the "contributing" section in the readme.

[GCorbel]

I tried to refactor tests with the following code. This way, there is no need to define Net::HTTPTooManyRequests in specs and all limits are visible in every test.

diff --git a/test/x/too_many_requests_test.rb b/test/x/too_many_requests_test.rb
index c7406ff..b967d86 100644
--- a/test/x/too_many_requests_test.rb
+++ b/test/x/too_many_requests_test.rb
@@ -4,80 +4,81 @@ module X
   class TooManyRequestsTest < Minitest::Test
     cover TooManyRequests
 
+    def add_limit(name, limits)
+      @response["x-#{name}-limit"] = limits[:limit].to_s
+      @response["x-#{name}-remaining"] = limits[:remaining].to_s
+      @response["x-#{name}-reset"] = limits[:reset_at].to_i.to_s
+    end
+
     def setup
       Time.stub :now, Time.utc(1983, 11, 24) do
-        response = Net::HTTPTooManyRequests.new("1.1", 429, "Too Many Requests")
-        response["x-rate-limit-limit"] = "100"
-        response["x-rate-limit-remaining"] = "0"
-        response["x-rate-limit-reset"] = (Time.now + 60).to_i.to_s
-
-        response["x-app-limit-24hour-limit"] = "200"
-        response["x-app-limit-24hour-remaining"] = "199"
-        response["x-app-limit-24hour-reset"] = (Time.now + 200).to_i.to_s
-
-        response["x-user-limit-24hour-limit"] = "300"
-        response["x-user-limit-24hour-remaining"] = "299"
-        response["x-user-limit-24hour-reset"] = (Time.now + 300).to_i.to_s
-
-        @exception = TooManyRequests.new(response: response)
+        @response = Net::HTTPTooManyRequests.new("1.1", 429, "Too Many Requests")
+        @exception = TooManyRequests.new(response: @response)
       end
     end
 
     def test_initialize_with_empty_response
-      response = Net::HTTPTooManyRequests.new("1.1", 429, "Too Many Requests")
-      exception = TooManyRequests.new(response: response)
-
-      assert_equal 0, exception.limit
-      assert_equal 0, exception.remaining
-      assert_equal Time.at(0).utc, exception.reset_at
-      assert_equal 0, exception.reset_in
+      assert_equal 0, @exception.limit
+      assert_equal 0, @exception.remaining
+      assert_equal Time.at(0).utc, @exception.reset_at
+      assert_equal 0, @exception.reset_in
       assert_equal "Too Many Requests", @exception.message
     end
 
     def test_limit
+      add_limit("rate-limit", limit: 100, remaining: 0, reset_at: Time.now + 60)
+
       assert_equal 100, @exception.limit
     end
 
     def test_limit_with_header
-      response = Net::HTTPTooManyRequests.new("1.1", 429, "Too Many Requests")
-      response["x-rate-limit-limit"] = "100"
-      exception = TooManyRequests.new(response: response)
+      add_limit("rate-limit", limit: 100, remaining: 0, reset_at: Time.now + 60)
 
-      assert_equal 100, exception.limit
+      assert_equal 100, @exception.limit
     end
 
     def test_remaining
+      add_limit("rate-limit", limit: 100, remaining: 0, reset_at: Time.now + 60)
+
       assert_equal 0, @exception.remaining
     end
 
     def test_remaining_with_header
-      response = Net::HTTPTooManyRequests.new("1.1", 429, "Too Many Requests")
-      response["x-rate-limit-remaining"] = "5"
-      exception = TooManyRequests.new(response: response)
+      add_limit("rate-limit", limit: 100, remaining: 5, reset_at: Time.now + 60)
 
-      assert_equal 5, exception.remaining
+      assert_equal 5, @exception.remaining
     end
 
     def test_reset_at
       Time.stub :now, Time.utc(1983, 11, 24) do
+        add_limit("rate-limit", limit: 100, remaining: 0, reset_at: Time.now + 60)
+
         assert_equal Time.now + 60, @exception.reset_at
       end
     end
 
     def test_reset_in
       Time.stub :now, Time.utc(1983, 11, 24) do
+        add_limit("rate-limit", limit: 100, remaining: 0, reset_at: Time.now + 60)
+
         assert_equal 60, @exception.reset_in
       end
     end
 
     def test_retry_after
       Time.stub :now, Time.utc(1983, 11, 24) do
+        add_limit("rate-limit", limit: 100, remaining: 0, reset_at: Time.now + 60)
+
         assert_equal 60, @exception.retry_after
       end
     end
 
     def test_all_limits
       Time.stub :now, Time.utc(1983, 11, 24) do
+        add_limit("rate-limit", limit: 100, remaining: 0, reset_at: Time.now + 60)
+        add_limit("app-limit-24hour", limit: 200, remaining: 199, reset_at: Time.now + 200)
+        add_limit("user-limit-24hour", limit: 300, remaining: 299, reset_at: Time.now + 300)
+
         excepted = {
           "rate-limit" => {
             "limit" => 100,
@@ -102,7 +103,9 @@ module X
 
     def test_next_available_at
       Time.stub :now, Time.utc(1983, 11, 24) do
-        @exception.response["x-app-limit-24hour-remaining"] = "0"
+        add_limit("rate-limit", limit: 100, remaining: 0, reset_at: Time.now + 60)
+        add_limit("app-limit-24hour", limit: 200, remaining: 0, reset_at: Time.now + 200)
+        add_limit("user-limit-24hour", limit: 300, remaining: 299, reset_at: Time.now + 300)
 
         assert_equal Time.at(Time.now.to_i + 200), @exception.next_available_at
       end
@@ -110,26 +113,24 @@ module X
 
     def test_next_available_in
       Time.stub :now, Time.utc(1983, 11, 24) do
-        @exception.response["x-app-limit-24hour-remaining"] = "0"
+        add_limit("rate-limit", limit: 100, remaining: 0, reset_at: Time.now + 60)
+        add_limit("app-limit-24hour", limit: 200, remaining: 0, reset_at: Time.now + 200)
+        add_limit("user-limit-24hour", limit: 300, remaining: 299, reset_at: Time.now + 300)
 
         assert_equal 200, @exception.next_available_in
       end
     end
 
     def test_reset_in_minimum_value
-      response = Net::HTTPTooManyRequests.new("1.1", 429, "Too Many Requests")
-      response["x-rate-limit-reset"] = (Time.now - 60).to_i.to_s
-      exception = TooManyRequests.new(response: response)
+      add_limit("rate-limit", limit: 100, remaining: 0, reset_at: Time.now - 60)
 
-      assert_equal 0, exception.reset_in
+      assert_equal 0, @exception.reset_in
     end
 
     def test_reset_in_ceil
-      response = Net::HTTPTooManyRequests.new("1.1", 429, "Too Many Requests")
-      response["x-rate-limit-reset"] = (Time.now + 61).to_i.to_s
-      exception = TooManyRequests.new(response: response)
+      add_limit("rate-limit", limit: 100, remaining: 0, reset_at: Time.now + 61)
 
-      assert_equal 61, exception.reset_in
+      assert_equal 61, @exception.reset_in
     end
   end
 end

I'm not sure if it's really an improvement because it's a little bit verbose. Let me know what you think.

[sferik]

I’m not a fan of the add_limit helper method. It doesn’t seem like it simplifies very much, it just adds a layer of indirection.

[GCorbel]

I agree. I first did :

    def setup
      Time.stub :now, Time.utc(1983, 11, 24) do
        response = Net::HTTPTooManyRequests.new("1.1", 429, "Too Many Requests")
        response["x-rate-limit-limit"] = "100"
        response["x-rate-limit-remaining"] = "0"
        response["x-rate-limit-reset"] = (Time.now + 60).to_i.to_s

        response["x-app-limit-24hour-limit"] = "200"
        response["x-app-limit-24hour-remaining"] = "199"
        response["x-app-limit-24hour-reset"] = (Time.now + 200).to_i.to_s

        response["x-user-limit-24hour-limit"] = "300"
        response["x-user-limit-24hour-remaining"] = "299"
        response["x-user-limit-24hour-reset"] = (Time.now + 300).to_i.to_s

        @exception = TooManyRequests.new(response: response)
      end
    end

But Rubocop said that the method is too long.

[sferik]

I would refactor it into smaller methods. Something like this:

def setup
  Time.stub :now, Time.utc(1983, 11, 24) do
    response = Net::HTTPTooManyRequests.new("1.1", 429, "Too Many Requests")
    setup_rate_limit!(response)
    setup_app_limit!(response)
    setup_user_limit!(response)
    @exception = TooManyRequests.new(response: response)
  end
end

def setup_rate_limit!(response)
  response["x-rate-limit-limit"] = "100"
  response["x-rate-limit-remaining"] = "0"
  response["x-rate-limit-reset"] = (Time.now + 60).to_i.to_s
end

def setup_app_limit!(response)
  response["x-app-limit-24hour-limit"] = "200"
  response["x-app-limit-24hour-remaining"] = "199"
  response["x-app-limit-24hour-reset"] = (Time.now + 200).to_i.to_s
end

def setup_user_limit!(response)
  response["x-user-limit-24hour-limit"] = "300"
  response["x-user-limit-24hour-remaining"] = "299"
  response["x-user-limit-24hour-reset"] = (Time.now + 300).to_i.to_s
end

[GCorbel]

I see there is some test for the class TooManyRequests in test/x/error_text.rb and in this file. Some looks like duplicates. Do I miss something ?

Otherwise, I spotted some potential things to change. I prefer to note them so they may be changed in another MR and you are aware of this.

---

Those tests look very similar to me :

    def test_limit
      assert_equal 100, @exception.limit
    end

    def test_limit_with_header
      response = Net::HTTPTooManyRequests.new("1.1", 429, "Too Many Requests")
      response["x-rate-limit-limit"] = "100"
      exception = TooManyRequests.new(response: response)

      assert_equal 100, exception.limit
    end

2 tests are needed ?

---

Here, there are assertions on exception and @exception :

    def test_initialize_with_empty_response
      response = Net::HTTPTooManyRequests.new("1.1", 429, "Too Many Requests")
      exception = TooManyRequests.new(response: response)

      assert_equal 0, exception.limit
      assert_equal 0, exception.remaining
      assert_equal Time.at(0).utc, exception.reset_at
      assert_equal 0, exception.reset_in
      assert_equal "Too Many Requests", @exception.message
    end

---

Having to repeat Time.stub :now, Time.utc(1983, 11, 24) is annoying. I see no way to do it simply and globally without adding a gem.

[sferik]

2 tests are needed ?

I agree these tests look redundant and can be removed/combined, as long as mutation coverage doesn’t drop below 100%.

[sferik]

Having to repeat Time.stub :now, Time.utc(1983, 11, 24) is annoying. I see no way to do it simply and globally without adding a gem.

I previously used the timecop gem for this but removed it in this refactoring of the errors, since I didn’t find the added dependency to be worth the cost vs. using Time.stub. If you would like to propose using another gem for this, feel free to do so, but please put it in a separate pull request, so that decision can be evaluated independently of your other proposed changes.

[GCorbel]

I first wrote :

     excepted = {
          "rate-limit" => {
            "limit" => 100,
            "remaining" => 0,
            "reset_at" => Time.at(Time.now.to_i + 60)
          },
          "app-limit-24hour" => {
            "limit" => 200,
            "remaining" => 199,
            "reset_at" => Time.at(Time.now.to_i + 200)
          },
          "user-limit-24hour" => {
            "limit" => 300,
            "remaining" => 299,
            "reset_at" => Time.at(Time.now.to_i + 300)
          }
        }

But rubocop complained about the method length. To me, it's a bit harder to read in one line. Let me know what you think.

[sferik]

To me, this is an indication that the all_limits method is too complex. I would probably factor out a X::RateLimit object that has limit, remaining, reset_at, and reset_in instance methods. Then the X::TooManyRequests error class could define an all_limits instance method (which I would rename to limits) that returns an array of up to three of these objects, depending on which headers are present in the response. Then the limit, remaining, reset_at, and reset_in instance methods on X::TooManyRequests could use this array to return the right value.

[GCorbel]

I give a try.

[sferik]

Here is a rough version of what I had in mind. Note: I haven’t actually run or tested this code, so there might be bugs!

module X
  class RateLimit
    attr_reader :response, :type

    TYPES = ["rate-limit", "app-limit-24hour", "user-limit-24hour"]

    def initialize(response, type)
      @response = response
      @type = type
    end

    def limit
      response["x-#{type}-limit"].to_i
    end

    def remaining
      response["x-#{type}-remaining"].to_i
    end

    def reset_at
      Time.at(response["x-#{type}-reset"].to_i)
    end

    def reset_in
      [(reset_at - Time.now).ceil, 0].max
    end

    alias_method :retry_after, :reset_in
  end
end

module X
  class TooManyRequests < ClientError
    def limits
      RateLimit::TYPES.filter_map do |type|
        RateLimit.new(response, type) if response["x-#{type}-limit"]
      end
    end

    def reset_at
      limits.select { |limit| limit.remaining.zero? }.map(&:reset_at).max
    end

    def reset_in
      [(reset_at - Time.now).ceil, 0].max
    end

    alias_method :retry_after, :reset_in
  end
end

[GCorbel]

["rate-limit", "app-limit-24hour", "user-limit-24hour"] are the limit I saw, but I wouldn't be surprised if there are some others. This is why I like response.to_hash.keys.filter_map { |k| k.match(/x-(.*-limit.*)-(\w+)/)&.captures&.uniq&.first }.

I will propose some changes.

[sferik]

Hmmm, my inclination is to be more explicit. I wouldn’t want this error-handling code to break if the API starts returning another header that accidentally matches the regular expression. If we discover more rate limit types, we can always add them. That seems a bit safer.

[GCorbel]

I applied comments. That's much better.

I still have rbs file to change.Is there a way to generate it ?

[sferik]

Yes, you can use typeprof to generate RBS, but the definitions are not always as precise as they should be.

[GCorbel]

I fixed the RBS file.

[GCorbel]

Mutation tests are also failing, but don’t worry about fixing those until after you make the other changes to the code/tests.

Running bundle exec rake mutant give :

Can not validate opensource repository license.
Licensed:
github.com/rubygems/gems
github.com/sferik/x-ruby
Present:
github.com/gcorbel/x-ruby
rake aborted!
Mutant task failed
/home/dougui/workspace/dimelo/x-ruby/Rakefile:28:in `block in <top (required)>'
/home/dougui/.rbenv/versions/3.1.2/bin/bundle:25:in `load'
/home/dougui/.rbenv/versions/3.1.2/bin/bundle:25:in `<main>'
Tasks: TOP => mutant
(See full trace by running task with --trace)

I didn't really try to fix the error.

EDIT

I understand mutant is a commercial software and I have no license.

[sferik]

Hello, I will fix Rubocop issue. I suppose this have to be added in the "contributing" section in the readme.

Yes, please feel free to update the Contributing section in the README instructions to be clearer.

[sferik]

I believe _v should removed, since you call keys before filter_map, so there are only keys (no values) mapped from the hash.

[GCorbel]

Right, I changed that.

[sferik]

I understand mutant is a commercial software and I have no license.

It is commercial software but it’s free for open source projects, like this one.

@mbj How do other open source projects allow contributors to run mutant on their forks?

[GCorbel]

FTR: When the 24 hours limit is reached, the 15 minutes may have remaining calls. This is correctly handled my what I did.

With the code I gave in #17 (comment), I had this result :

pp e.all_limits
# => {"rate-limit"=>{"limit"=>50, "remaining"=>48, "reset_at"=>2023-11-29 15:54:35 -0500},
    "user-limit-24hour"=>{"limit"=>1000, "remaining"=>0, "reset_at"=>2023-11-30 14:51:14 -0500}}

pp e.next_available_at
# => 2023-11-30 14:51:14 -0500

[GCorbel]

I kept a hash instead of an Array as you suggested. This allows finding a specific limit.

[sferik]

Thinking some more about this, I’m tempted to only return rate limits with zero remaining requests here. Including others may cause confusion about the cause of the error. In that case, it would be unusual for there to be more than one element returned.

This class should also define a singular rate_limit instance method, which returns the rate limit with the farthest reset time.

Making these changes would dramatically simplify the reset_at definition.

[GCorbel]

I did changes.

[mbj]

@mbj How do other open source projects allow contributors to run mutant on their forks?

Ask the contributor to add the official remote to the git remotes. git remote add upstream https://github.com/sferik/x-ruby will do it so the contributor can locally work with mutant.

If any upstream matches the authorized one the license check passes.

[sferik]

@mbj Thank you for explaining!

@GCorbel This patch is coming along quite nicely. I just left another comment but I think it’s almost done. After addressing that comment, the final step is to get mutation coverage back up to 100%, which you should be able to do now. Thanks so much for all your work on this patch!

[GCorbel]

Rubocop and ruby standard aren't agreed here.

Rubocop gives :

lib/x/errors/too_many_requests.rb:12:9: C: [Correctable] Layout/MultilineMethodCallIndentation: Align .select with .to_h on line 11.
        .select { |_, limit| limit.remaining.zero? }
        ^^^^^^^

If I correct, standard gives :

standard: Use Ruby Standard Style (https://github.com/standardrb/standard)
  lib/x/errors/too_many_requests.rb:12:35: Layout/MultilineMethodCallIndentation: Use 2 (not 28) spaces for indenting an expression in an
 assignment spanning multiple lines.

I prefer the "standard" version

[sferik]

When Standard and RuboCop defaults conflict, I always go with Standard Ruby. Feel free to edit .rubocop.yml to make it consistent with Standard Ruby, so the linter check passes.

[GCorbel]

I succeeded to run mutant and fixed all issues (it was tough). I'm satisfied of the result.

Linter fail because there is a conflict between standard and rubocop.

[mbj]

I succeeded to run mutant and fixed all issues (it was tough).

@GCorbel Curious as mutants docs suck, did it make sense to you?

And now my explanation (I read the mutant commit with a positive smile): Mutant tries to "reduce what the code does semantically" and make the tests proof: hey I actually need that extra bit. And often times this also involves not calling #to_i as it accepts so many more formats than for example Integer. If you only need decimals, why would we than call a method that can also eat octals/hexadecimals etc.

The key to pass mutant is NOT to write more tests, but actually make your code ONLY do what the tests ask for not more. Mutation testing is much more an automated code review technique than a metric.

And each of the semantic reductions mutant does get reported as a diff that can be read like:

hey if I apply this diff, your tests still pass. You author can you check if the code is better now? Or do we loose something imporant? If the code is better just use it, else add a test that proofs to me we actually need that extra semantics that got removed.

[sferik]

@GCorbel This is looking very good! Before I merge, I was wondering if you wanted to do any of the following:

• Add something about RuboCop in the “Contributing” section of the README
• Remove redundant tests
• Edit .rubocop.yml to make it consistent with Standard Ruby to make the linter check pass
• Squash your commits into a single commit that applies cleanly

I still think we should be more explicit about specifying rate limit types vs. inferring them using a regular expression. I will probably make that change before releasing this code, but I can do that after merging. I’m also tempted to make the TooManyRequests#rate_limits method return an array instead of a hash. Someone can always select a particular limit if they wish, but that’s not really a core use-case for this code.

[sferik]

@GCorbel Here’s a patch you can apply to get the linter to pass:

diff --git a/.rubocop.yml b/.rubocop.yml
index 28bcde4..800fd5d 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -31,6 +31,11 @@ Layout/HashAlignment:
   EnforcedColonStyle: key
   EnforcedLastArgumentHashStyle: always_inspect
 
+Layout/MultilineMethodCallIndentation:
+  Enabled: true
+  EnforcedStyle: indented
+  IndentationWidth: ~
+
 Layout/ParameterAlignment:
   Enabled: true
   EnforcedStyle: with_fixed_indentation

[GCorbel]

@GCorbel Curious as mutants docs suck, did it make sense to you?

It was not obvious at all to understand what mutant does.

And now my explanation (I read the mutant commit with a positive smile): Mutant tries to "reduce what the code does semantically" and make the tests proof: hey I actually need that extra bit. And often times this also involves not calling #to_i as it accepts so many more formats than for example Integer. If you only need decimals, why would we than call a method that can also eat octals/hexadecimals etc.

I had to change response.fetch("x-#{type}-limit").to_i to Integer(response.fetch("x-#{type}-limit")). As you said, using Integer limit possibilities, but I still prefer using #to_i. It seems to be very specific, and using Integer is a bit harder to read. Furthermore, it doesn't seem to follow "the ruby way".

It's a bit the same for :

   def reset_in
      [(reset_at - Time.now).ceil, 0].max
    end

Here, I guess ceil is used just because mutant complained that there is no test with a float, but we don't really care to round this value, and adding a test for that is not very useful.

[GCorbel]

@GCorbel This is looking very good! Before I merge, I was wondering if you wanted to do any of the following:

* Add something about RuboCop in the “Contributing” section of the `README`

* Remove redundant tests

* Edit `.rubocop.yml` to make it consistent with Standard Ruby to make the linter check pass

* Squash your commits into a single commit that applies cleanly

I did. I think you can squash commits when you merge the PR with GitHub.

I still think we should be more explicit about specifying rate limit types vs. inferring them using a regular expression. I will probably make that change before releasing this code, but I can do that after merging. I’m also tempted to make the TooManyRequests#rate_limits method return an array instead of a hash. Someone can always select a particular limit if they wish, but that’s not really a core use-case for this code.

I changed TooManyRequests#rate_limits to return an array. I'm not sure about how many kinds of limits there are, and there is no doc. To me, we need to know every limit there is to hard code them.

[sferik]

Looks good! Thanks again for this contribution to the library!

[GCorbel]

Thanks. I enjoyed to work with you, on this PR.

[sferik]

Thanks again for your contribution! I just released version 0.13.0, which includes this change, and posted about it.