[Fixes #283] Provide epoch_time that was used for the request in the data set

[doliveirakn]

Aiming to solve #283

Add an reader for the epoch_time variable in the cache so that it can also be returned in the data from the throttle.

This is allows access to the same time that the cache uses for the count. This can be important for clients that want to provide rate limit information for well-behaved clients

[lmansur]

Hi @doliveirakn, thank you for implementing this PR.

Could you please write a test case for the issue you reported? This ensures the Gem is actually fixing your exact problem, helps document this specific use case and catches any future regression.

[doliveirakn]

@lmansur I thought about a test case for the specific issue that was reported and I don't see a good way to write a spec for that.

The reason being is that the race condition comes from the fact that the time used in the cache is not exposed.

In the issue (#283) I have code that looks like this:

class FooMiddleware
  def initialize(app)
    @app = app
  end

  def call(env)
    status, headers, body = @app.call(env)
    now = Time.now
    match_data = env['rack.attack.match_data']
    if match_data
      headers.merge!({
        'X-RateLimit-Limit' => match_data[:limit].to_s,
        'X-RateLimit-Remaining' => match_data[:limit] - match_data[:count],
        'X-RateLimit-Reset' => (now + (match_data[:period] - now.to_i % match_data[:period])).to_s
      })
    end
    [status, headers, body]
  end
end

This PR as it stands does not fix the problem. However it does let code be written like:

class FooMiddleware
  def initialize(app)
    @app = app
  end

  def call(env)
    status, headers, body = @app.call(env)
    match_data = env['rack.attack.match_data']
    now = match_data[:epoch_time]
    if match_data
      headers.merge!({
        'X-RateLimit-Limit' => match_data[:limit].to_s,
        'X-RateLimit-Remaining' => match_data[:limit] - match_data[:count],
        'X-RateLimit-Reset' => (now + (match_data[:period] - now % match_data[:period])).to_s
      })
    end
    [status, headers, body]
  end
end

The tests now assert that the epoch_time is now available in the match_data. So long as that contract is upheld (and it is now included in the X-RateLimit headers for well-behaved clients section of the Readme) users should be able to avoid this race condition in the future. I believe that to be sufficient. if you don't, would you be able to point to see what kind of test you would like to see and I will happily add it.

[lmansur]

I think you are right, the gem shouldn't really care about the user's implementation.

PR looks fine for me. 👍

[grzuy]

Thank you for your contribution!

[grzuy]

I wonder whether making the epoch time an argument to the #count method might end up feeling a bit cleaner... Instead of keeping the last epoch time in the cache.

Not a blocker for merging though, just a consideration for a possible future refactor :-)