refactor: make attempt method return early if all requests have been …

…exhausted

src/http_limiter.ts

@@ -141,9 +141,12 @@ export class HttpLimiter<KnownStores extends Record<string, LimiterManagerStoreF
     const limiterResponse = await limiter.get(key)
 
     /**
-     * Abort when user has exhausted all the requests
+     * Abort when user has exhausted all the requests.
+     *
+     * We still run the "consume" method when remaining count is zero, since
+     * that will trigger the block logic on the key
      */
-    if (limiterResponse && limiterResponse.remaining < 0) {
+    if (limiterResponse && limiterResponse.consumed > limiterResponse.limit) {
       debug('requests exhausted for key "%s"', key)
       const error = new E_TOO_MANY_REQUESTS(limiterResponse)
       this.#exceptionModifier(error)

src/limiter.ts

@@ -72,6 +72,18 @@ export class Limiter implements LimiterStoreContract {
    * callback.
    */
   async attempt<T>(key: string | number, callback: () => T | Promise<T>): Promise<T | undefined> {
+    /**
+     * Return early when remaining requests are less than
+     * zero.
+     *
+     * We still run the "consume" method when remaining count is zero, since
+     * that will trigger the block logic on the key
+     */
+    const response = await this.get(key)
+    if (response && response.consumed > response.limit) {
+      return
+    }
+
     try {
       await this.consume(key)
       return callback()

tests/limiter.spec.ts

@@ -113,27 +113,49 @@ test.group('Limiter', () => {
 
     const limiter = new Limiter(store)
 
-    assert.isUndefined(
-      await limiter.attempt('ip_localhost', () => {
-        executionStack.push('executed 1')
-      })
-    )
-    assert.isUndefined(
-      await limiter.attempt('ip_localhost', () => {
-        executionStack.push('executed 2')
-      })
-    )
-    assert.isUndefined(
-      await limiter.attempt('ip_localhost', () => {
-        executionStack.push('executed 3')
-      })
-    )
-    assert.isUndefined(
-      await limiter.attempt('ip_localhost', () => {
-        executionStack.push('executed 4')
-      })
-    )
+    await limiter.attempt('ip_localhost', () => {
+      executionStack.push('executed 1')
+    })
+    await limiter.attempt('ip_localhost', () => {
+      executionStack.push('executed 2')
+    })
+    await limiter.attempt('ip_localhost', () => {
+      executionStack.push('executed 3')
+    })
+    await limiter.attempt('ip_localhost', () => {
+      executionStack.push('executed 4')
+    })
+
+    assert.deepEqual(executionStack, ['executed 1', 'executed 2'])
+    assert.equal(await limiter.remaining('ip_localhost'), 0)
+  })
+
+  test('block key when trying to attempt after exhausting all requests', async ({ assert }) => {
+    const executionStack: string[] = []
+    const redis = createRedis(['rlflx:ip_localhost']).connection()
+    const store = new LimiterRedisStore(redis, {
+      duration: '1 minute',
+      requests: 2,
+      blockDuration: '30 mins',
+    })
+
+    const limiter = new Limiter(store)
+
+    await limiter.attempt('ip_localhost', () => {
+      executionStack.push('executed 1')
+    })
+    await limiter.attempt('ip_localhost', () => {
+      executionStack.push('executed 2')
+    })
+    await limiter.attempt('ip_localhost', () => {
+      executionStack.push('executed 3')
+    })
+    await limiter.attempt('ip_localhost', () => {
+      executionStack.push('executed 4')
+    })
+
     assert.deepEqual(executionStack, ['executed 1', 'executed 2'])
+    assert.closeTo(await limiter.availableIn('ip_localhost'), 30 * 60, 5)
   })
 
   test('get seconds left until the key will be available for new request', async ({ assert }) => {

tests/stores/redis.spec.ts

@@ -184,6 +184,25 @@ test.group('Limiter redis store | wrapper | get', () => {
     const response = await store.get('ip_localhost')
     assert.isNull(response)
   })
+
+  test('get response for negative points', async ({ assert }) => {
+    const redis = createRedis(['rlflx:ip_localhost']).connection()
+    const store = new LimiterRedisStore(redis, {
+      duration: '1 minute',
+      requests: 1,
+    })
+
+    await store.consume('ip_localhost')
+    await assert.rejects(() => store.consume('ip_localhost'))
+    const response = await store.get('ip_localhost')
+    assert.instanceOf(response, LimiterResponse)
+    assert.containsSubset(response!.toJSON(), {
+      limit: 1,
+      remaining: 0,
+      consumed: 2,
+    })
+    assert.closeTo(response!.availableIn, 60, 5)
+  })
 })
 
 test.group('Limiter redis store | wrapper | set', () => {

tests/throttle_middleware.spec.ts

@@ -40,7 +40,7 @@ test.group('Throttle middleware', () => {
     assert.isTrue(nextCalled)
   })
 
-  test('do not call next when key is blocked', async ({ assert }) => {
+  test('do not call next when key has exhausted all requests', async ({ assert }) => {
     let nextCalled: boolean = false
 
     const redis = createRedis(['rlflx:api_1']).connection()
@@ -73,6 +73,40 @@ test.group('Throttle middleware', () => {
     }
   })
 
+  test('block key when requests are made even after rate limited', async ({ assert }) => {
+    let nextCalled: boolean = false
+
+    const redis = createRedis(['rlflx:api_1']).connection()
+    const limiterManager = new LimiterManager({
+      default: 'redis',
+      stores: {
+        redis: (options) => new LimiterRedisStore(redis, options),
+      },
+    })
+
+    const apiLimiter = limiterManager.define('api', () => {
+      return limiterManager.allowRequests(1).every('1 minute').usingKey(1).blockFor('30 mins')
+    })
+
+    /**
+     * This will consume all the requests the
+     * key has
+     */
+    await limiterManager.use({ duration: 60, requests: 1 }).consume('api_1')
+
+    const ctx = new HttpContextFactory().create()
+
+    try {
+      await apiLimiter(ctx, () => {
+        nextCalled = true
+      })
+    } catch (error) {
+      assert.equal(error.message, 'Too many requests')
+      assert.closeTo(error.response.availableIn, 30 * 60, 5)
+      assert.isFalse(nextCalled)
+    }
+  })
+
   test('do not throttle request when no limiter is used', async ({ assert }) => {
     let nextCalled: boolean = false