Fix threshold behavior to allow N requests instead of N-1

[jonathanJ7]

Summary

This pull request fixes the core rate limiting bug where threshold: N was incorrectly blocking requests on the Nth attempt instead of allowing N requests before blocking the (N+1)th request. This change aligns the actual implementation behavior with the original README documentation and user expectations (check this PR for more context).

Background / Problem

The current implementation has a logical flaw in the rate limiting logic that contradicts the intuitive expectations. Here's what was happening:

Current (Buggy) Behavior

• With threshold: 1, the very first request gets blocked
• With threshold: 5, only 4 requests are allowed before the 5th is blocked
• With threshold: 15, only 14 requests are allowed before the 15th is blocked

Expected Behavior

Users naturally expect threshold: N to mean "allow N requests per interval."

Root Cause Analysis

The bug is in /lib/graph_attack/rate_limit.rb in the calls_exceeded_on_query? method:

# BUGGY CODE (lines 29-32)
def calls_exceeded_on_query?(rate_limited_field)
  with_redis_client do |redis_client|
    rate_limit = Ratelimit.new(rate_limited_field, redis: redis_client)
    rate_limit.add(key)              # ❌ INCREMENT FIRST
    rate_limit.exceeded?(key, threshold: threshold, interval: interval)  # ❌ CHECK AFTER
  end
end

The problem: increment first, check second. This means:

1. Request arrives
2. Counter increments from 0 → 1
3. Check: "Is 1 ≥ threshold(1)?" → Yes → Block request

Solution

Fixed the order of operations to check first, increment second:

# FIXED CODE
def calls_exceeded_on_query?(rate_limited_field)
  with_redis_client do |redis_client|
    rate_limit = Ratelimit.new(rate_limited_field, redis: redis_client)
    if rate_limit.exceeded?(key, threshold: threshold, interval: interval)  # ✅ CHECK FIRST
      true  # Already exceeded, block this request
    else
      rate_limit.add(key)  # ✅ INCREMENT ONLY IF NOT EXCEEDED
      false  # Allow this request
    end
  end
end

Now the logic works correctly:

1. Request arrives
2. Check: "Is current count ≥ threshold?" → No → Allow request & increment counter
3. Next request: Check: "Is current count ≥ threshold?" → Yes → Block request

Testing

Added Explicit Test Cases for the Bug Fix

Documentation Updates

Updated /README.md to be crystal clear about the behavior:

Before:

"This would block requests starting from the 15th attempt within a 60-second window by the same IP address. That is, if 15 requests are made, the first 14 will succeed and the 15th will fail."

After:

"This would allow 15 requests per minute by the same IP address, blocking the 16th and subsequent requests within that 60-second window."

[sunny]

Perfect, this makes much more sense. Thank you very much for the detailed description and changes. ❤️

[sunny]

Released in 2.4.1 🚀