Merge pull request #92 from utopia-php/feat-improve-redis

feat: improve redis
utopia-php · Dec 27, 2024 · bef426a · bef426a
2 parents 54a5835 + 072725d
commit bef426a
Show file tree

Hide file tree

Showing 7 changed files with 70 additions and 136 deletions.
diff --git a/composer.json b/composer.json
@@ -29,5 +29,11 @@
         "phpstan/phpstan": "^1.9",
         "laravel/pint": "1.5.*",
         "phpbench/phpbench": "^1.2"
+    },
+    "config": {
+        "allow-plugins": {
+            "php-http/discovery": true,
+            "tbachert/spi": true
+        }
     }
 }
diff --git a/composer.lock b/composer.lock
diff --git a/docker-compose.yml b/docker-compose.yml
@@ -1,5 +1,3 @@
-version: '3'
-
 services:
   mysql:
     image: mysql:8

diff --git a/phpbench.json b/phpbench.json
@@ -1,6 +1,6 @@
 {
-    "$schema":"vendor/phpbench/phpbench/phpbench.schema.json",
+    "$schema": "vendor/phpbench/phpbench/phpbench.schema.json",
     "runner.bootstrap": "vendor/autoload.php",
-    "runner.path": "tests",
-    "runner.file_pattern": "*Bench.php"
+    "runner.path": "tests/Abuse/Bench",
+    "runner.file_pattern": "*.php"
 }
diff --git a/src/Abuse/Adapters/TimeLimit/Redis.php b/src/Abuse/Adapters/TimeLimit/Redis.php
@@ -13,10 +13,16 @@ class Redis extends TimeLimit
      */
     protected \Redis $redis;
 
+    /**
+     * @var int
+     */
+    protected int $ttl;
+
     public function __construct(string $key, int $limit, int $seconds, \Redis $redis)
     {
         $this->redis = $redis;
         $this->key = $key;
+        $this->ttl = $seconds;
         $now = \time();
         $this->timestamp = (int)($now - ($now % $seconds));
         $this->limit = $limit;
@@ -62,16 +68,13 @@ protected function hit(string $key, int $timestamp): void
             return;
         }
 
-        /** @var string $count */
-        $count = $this->redis->get(self::NAMESPACE . '__'. $key .'__'. $timestamp);
-        if (!$count) {
-            $this->count = 0;
-        } else {
-            $this->count = intval($count);
-        }
+        $key = self::NAMESPACE . '__' . $key . '__' . $timestamp;
+        $this->redis->multi()
+            ->incr($key)
+            ->expire($key, $this->ttl)
+            ->exec();
 
-        $this->redis->incr(self::NAMESPACE . '__'. $key .'__'. $timestamp);
-        $this->count++;
+        $this->count = ($this->count ?? 0) + 1;
     }
 
     /**
@@ -107,32 +110,7 @@ public function getLogs(?int $offset = null, ?int $limit = 25): array
      */
     public function cleanup(int $timestamp): bool
     {
-        $iterator = null;
-        while ($iterator !== 0) {
-            $keys = $this->redis->scan($iterator, self::NAMESPACE . '__*__*', 1000);
-            $keys = $this->filterKeys($keys ? $keys : [], $timestamp);
-            $this->redis->del($keys);
-        }
+        // No need for manual cleanup - Redis TTL handles this automatically
         return true;
     }
-
-    /**
-     * Filter keys
-     *
-     * @param array<string> $keys
-     * @param integer $timestamp
-     * @return array<string>
-     */
-    protected function filterKeys(array $keys, int $timestamp): array
-    {
-        $filteredKeys = [];
-        foreach ($keys as $key) {
-            $parts = explode('__', $key);
-            $keyTimestamp = (int)end($parts); // Assuming the last part is always the timestamp
-            if ($keyTimestamp < $timestamp) {
-                $filteredKeys[] = $key;
-            }
-        }
-        return $filteredKeys;
-    }
 }
diff --git a/src/Abuse/Adapters/TimeLimit/RedisCluster.php b/src/Abuse/Adapters/TimeLimit/RedisCluster.php
@@ -13,10 +13,16 @@ class RedisCluster extends TimeLimit
      */
     protected \RedisCluster $redis;
 
+    /**
+     * @var int
+     */
+    protected int $ttl;
+
     public function __construct(string $key, int $limit, int $seconds, \RedisCluster $redis)
     {
         $this->redis = $redis;
         $this->key = $key;
+        $this->ttl = $seconds;
         $now = \time();
         $this->timestamp = (int)($now - ($now % $seconds));
         $this->limit = $limit;
@@ -63,99 +69,65 @@ protected function hit(string $key, int $timestamp): void
             return;
         }
 
-        /** @var string|false $count */
-        $count = $this->redis->get(self::NAMESPACE . '__'. $key .'__'. $timestamp);
-        if ($count === false) {
-            $this->count = 0;
-        } else {
-            $this->count = intval($count);
-        }
+        $key = self::NAMESPACE . '__'. $key .'__'. $timestamp;
 
-        $this->redis->incr(self::NAMESPACE . '__'. $key .'__'. $timestamp);
-        $this->count++;
+        $this->redis->multi();
+        $this->redis->incr($key);
+        $this->redis->expire($key, $this->ttl);
+        $this->redis->exec();
+
+        $this->count = ($this->count ?? 0) + 1;
     }
 
     /**
-     * Get abuse logs
-     *
-     * Return logs with an offset and limit
+     * Get abuse logs with proper cursor-based pagination
      *
      * @param int|null $offset
      * @param int|null $limit
      * @return array<string, mixed>
      */
     public function getLogs(?int $offset = 0, ?int $limit = 25): array
     {
-        // TODO limit potential is SCAN but needs cursor no offset
-        $keys = $this->scan(self::NAMESPACE . '__*', $offset, $limit);
-        if ($keys === false) {
-            return [];
+        $offset = $offset ?? 0;
+        $limit = $limit ?? 25;
+        $matches = [];
+        $pattern = self::NAMESPACE . '__*';
+
+        // Get all keys from each master
+        foreach ($this->redis->_masters() as $master) {
+            $cursor = null;
+            do {
+                /** @phpstan-ignore-next-line */
+                $keys = $this->redis->scan($cursor, $master, $pattern, 100);
+                if ($keys !== false) {
+                    $matches = array_merge($matches, $keys);
+                }
+            } while ($cursor > 0 && count($matches) < $offset + $limit);
         }
 
-        $logs = [];
-        foreach ($keys as $key) {
-            $logs[$key] = $this->redis->get($key);
+        // Sort to ensure consistent ordering
+        sort($matches);
+
+        // Apply offset and limit
+        $matches = array_slice($matches, $offset, $limit);
+
+        if (empty($matches)) {
+            return [];
         }
-        return $logs;
+
+        // Batch fetch values using mget
+        $values = $this->redis->mget($matches);
+        return array_combine($matches, $values);
     }
 
     /**
-     * Delete all logs older than $timestamp
+     * No need for manual cleanup - using Redis TTL
      *
      * @param int $timestamp
      * @return bool
      */
     public function cleanup(int $timestamp): bool
     {
-        $keys = $this->scan(self::NAMESPACE . '__*__*');
-        $keys = $this->filterKeys($keys ? $keys : [], $timestamp);
-        /** @phpstan-ignore-next-line */
-        $this->redis->del($keys);
         return true;
     }
-
-    /**
-     * Filter keys
-     *
-     * @param array<string> $keys
-     * @param integer $timestamp
-     * @return array<string>
-     */
-    protected function filterKeys(array $keys, int $timestamp): array
-    {
-        $filteredKeys = [];
-        foreach ($keys as $key) {
-            $parts = explode('__', $key);
-            $keyTimestamp = (int)end($parts); // Assuming the last part is always the timestamp
-            if ($keyTimestamp < $timestamp) {
-                $filteredKeys[] = $key;
-            }
-        }
-        return $filteredKeys;
-    }
-
-    /**
-     * Scan keys across all masters in the Redis cluster
-     *
-     * @param string $pattern Pattern to match keys
-     * @param int|null $cursor Reference to the cursor for scanning
-     * @param int|null $count Number of keys to return per iteration
-     * @return array<string>|false
-     */
-    protected function scan(string $pattern, ?int &$cursor = null, ?int $count = 1000): array|false
-    {
-        $matches = [];
-        foreach ($this->redis->_masters() as $master) {
-            $cursor = null;
-            do {
-                /** @phpstan-ignore-next-line */
-                $keys = $this->redis->scan($cursor, $master, $pattern, $count);
-                if ($keys !== false) {
-                    $matches = array_merge($matches, $keys);
-                }
-            } while ($cursor > 0);
-        }
-
-        return empty($matches) ? false : $matches;
-    }
 }
diff --git a/tests/Abuse/Base.php b/tests/Abuse/Base.php
@@ -86,26 +86,6 @@ public function testLimitReset(): void
         $this->assertEquals($abuse->check(), false);
     }
 
-    /**
-     * Test logs are deleted after cleanup
-     */
-    public function testCleanup(): void
-    {
-        $adapter = $this->getAdapter('', 1, 1);
-        $abuse = new Abuse($adapter);
-
-        $logs = $abuse->getLogs(0, 100);
-        $this->assertEquals(6, \count($logs)); // 6 keys are created in the test
-
-        sleep(1);
-
-        $status = $abuse->cleanup(time());
-        $this->assertEquals($status, true);
-
-        $logs = $abuse->getLogs(0, 100);
-        $this->assertEquals(0, \count($logs));
-    }
-
     /**
      * Verify that the time format is correct
      */