From 62da8f7243a8ac1bd79a076c516aac4532d9c3b8 Mon Sep 17 00:00:00 2001 From: Petr Levtonov Date: Mon, 27 Jan 2025 13:41:26 +0100 Subject: [PATCH 1/5] Properly resolve RateLimiter not working while redis serialization or compression is enabled --- src/Illuminate/Cache/RateLimiter.php | 28 +++++++++++++-- src/Illuminate/Cache/RedisStore.php | 4 --- .../Redis/Connections/PacksPhpRedisValues.php | 35 +++++++++++++++++++ tests/Cache/RedisCacheIntegrationTest.php | 17 +++++++++ tests/Integration/Queue/RateLimitedTest.php | 1 + 5 files changed, 78 insertions(+), 7 deletions(-) diff --git a/src/Illuminate/Cache/RateLimiter.php b/src/Illuminate/Cache/RateLimiter.php index 8d8afe30a14c..a834a0a12447 100644 --- a/src/Illuminate/Cache/RateLimiter.php +++ b/src/Illuminate/Cache/RateLimiter.php @@ -4,6 +4,7 @@ use Closure; use Illuminate\Contracts\Cache\Repository as Cache; +use Illuminate\Redis\Connections\PhpRedisConnection; use Illuminate\Support\Collection; use Illuminate\Support\InteractsWithTime; @@ -165,12 +166,12 @@ public function increment($key, $decaySeconds = 60, $amount = 1) $key.':timer', $this->availableAt($decaySeconds), $decaySeconds ); - $added = $this->cache->add($key, 0, $decaySeconds); + $added = $this->runClean(fn (): mixed => $this->cache->add($key, 0, $decaySeconds)); $hits = (int) $this->cache->increment($key, $amount); if (! $added && $hits == 1) { - $this->cache->put($key, 1, $decaySeconds); + $this->runClean(fn (): mixed => $this->cache->put($key, 1, $decaySeconds)); } return $hits; @@ -199,7 +200,7 @@ public function attempts($key) { $key = $this->cleanRateLimiterKey($key); - return $this->cache->get($key, 0); + return $this->runClean(fn (): mixed => $this->cache->get($key, 0)); } /** @@ -282,6 +283,27 @@ public function cleanRateLimiterKey($key) return preg_replace('/&([a-z])[a-z]+;/i', '$1', htmlentities($key)); } + /** + * Allow the rate limiter to run a callback against a clean repository. + * + * For example when using RedisStore, the serializer and compressor can be + * disabled during the callback execution. + */ + protected function runClean(callable $callback): mixed + { + $store = $this->cache->getStore(); + if (! $store instanceof RedisStore) { + return $callback(); + } + + $connection = $store->connection(); + if (! $connection instanceof PhpRedisConnection) { + return $callback(); + } + + return $connection->runClean($callback); + } + /** * Resolve the rate limiter name. * diff --git a/src/Illuminate/Cache/RedisStore.php b/src/Illuminate/Cache/RedisStore.php index ebdd0febbd82..39f1a0777ea0 100755 --- a/src/Illuminate/Cache/RedisStore.php +++ b/src/Illuminate/Cache/RedisStore.php @@ -432,10 +432,6 @@ public function setPrefix($prefix) protected function pack($value, $connection) { if ($connection instanceof PhpRedisConnection) { - if ($this->shouldBeStoredWithoutSerialization($value)) { - return $value; - } - if ($connection->serialized()) { return $connection->pack([$value])[0]; } diff --git a/src/Illuminate/Redis/Connections/PacksPhpRedisValues.php b/src/Illuminate/Redis/Connections/PacksPhpRedisValues.php index cc7f31094883..d733d45e7fa7 100644 --- a/src/Illuminate/Redis/Connections/PacksPhpRedisValues.php +++ b/src/Illuminate/Redis/Connections/PacksPhpRedisValues.php @@ -137,6 +137,41 @@ public function lz4Compressed(): bool $this->client->getOption(Redis::OPT_COMPRESSION) === Redis::COMPRESSION_LZ4; } + /** + * Run a callback without serialization or compression enabled. Useful when + * performing operations like increment/decrement that should not be + * serialized or compressed on client side. + */ + public function runClean(callable $callback): mixed + { + /** @var \Redis|\RedisCluster $client */ + $client = $this->client; + + $oldSerializer = null; + if ($this->serialized()) { + $oldSerializer = $client->getOption($client::OPT_SERIALIZER); + $client->setOption($client::OPT_SERIALIZER, $client::SERIALIZER_NONE); + } + + $oldCompressor = null; + if ($this->compressed()) { + $oldCompressor = $client->getOption($client::OPT_COMPRESSION); + $client->setOption($client::OPT_COMPRESSION, $client::COMPRESSION_NONE); + } + + try { + return $callback(); + } finally { + if ($oldSerializer !== null) { + $client->setOption($client::OPT_SERIALIZER, $oldSerializer); + } + + if ($oldCompressor !== null) { + $client->setOption($client::OPT_COMPRESSION, $oldCompressor); + } + } + } + /** * Determine if the current PhpRedis extension version supports packing. * diff --git a/tests/Cache/RedisCacheIntegrationTest.php b/tests/Cache/RedisCacheIntegrationTest.php index ff99dd0bfd81..ea3262003f8d 100644 --- a/tests/Cache/RedisCacheIntegrationTest.php +++ b/tests/Cache/RedisCacheIntegrationTest.php @@ -2,6 +2,7 @@ namespace Illuminate\Tests\Cache; +use Illuminate\Cache\RateLimiter; use Illuminate\Cache\RedisStore; use Illuminate\Cache\Repository; use Illuminate\Foundation\Testing\Concerns\InteractsWithRedis; @@ -37,6 +38,22 @@ public function testRedisCacheAddTwice($driver) $this->assertGreaterThan(3500, $this->redis[$driver]->connection()->ttl('k')); } + /** + * @param string $driver + */ + #[DataProvider('redisDriverProvider')] + public function testRedisCacheRateLimiter($driver) + { + $store = new RedisStore($this->redis[$driver]); + $repository = new Repository($store); + $rateLimiter = new RateLimiter($repository); + + $this->assertFalse($rateLimiter->tooManyAttempts('key', 1)); + $this->assertEquals(1, $rateLimiter->hit('key', 60)); + $this->assertTrue($rateLimiter->tooManyAttempts('key', 1)); + $this->assertFalse($rateLimiter->tooManyAttempts('key', 2)); + } + /** * Breaking change. * diff --git a/tests/Integration/Queue/RateLimitedTest.php b/tests/Integration/Queue/RateLimitedTest.php index de6e0e0f785a..334ec657536e 100644 --- a/tests/Integration/Queue/RateLimitedTest.php +++ b/tests/Integration/Queue/RateLimitedTest.php @@ -63,6 +63,7 @@ public function testRateLimitedJobsAreNotExecutedOnLimitReached2() $cache->shouldReceive('add')->andReturn(true, true); $cache->shouldReceive('increment')->andReturn(1); $cache->shouldReceive('has')->andReturn(true); + $cache->shouldReceive('getStore')->andReturn(new ArrayStore); $rateLimiter = new RateLimiter($cache); $this->app->instance(RateLimiter::class, $rateLimiter); From 6a6d17698d376309bc9d15195899252292f0b182 Mon Sep 17 00:00:00 2001 From: Petr Levtonov Date: Mon, 27 Jan 2025 14:29:33 +0100 Subject: [PATCH 2/5] Fix code style --- tests/Cache/RedisCacheIntegrationTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Cache/RedisCacheIntegrationTest.php b/tests/Cache/RedisCacheIntegrationTest.php index ea3262003f8d..57c6362b84d8 100644 --- a/tests/Cache/RedisCacheIntegrationTest.php +++ b/tests/Cache/RedisCacheIntegrationTest.php @@ -38,7 +38,7 @@ public function testRedisCacheAddTwice($driver) $this->assertGreaterThan(3500, $this->redis[$driver]->connection()->ttl('k')); } - /** + /** * @param string $driver */ #[DataProvider('redisDriverProvider')] From 7d8bcf8041dcf4fcd1ca7bffee4ce14d63ff6c3f Mon Sep 17 00:00:00 2001 From: Petr Levtonov Date: Mon, 27 Jan 2025 14:37:24 +0100 Subject: [PATCH 3/5] Fix test --- tests/Cache/CacheRateLimiterTest.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/Cache/CacheRateLimiterTest.php b/tests/Cache/CacheRateLimiterTest.php index b1cbec87598a..a4e7d5e2a099 100644 --- a/tests/Cache/CacheRateLimiterTest.php +++ b/tests/Cache/CacheRateLimiterTest.php @@ -2,6 +2,7 @@ namespace Illuminate\Tests\Cache; +use Illuminate\Cache\ArrayStore; use Illuminate\Cache\RateLimiter; use Illuminate\Contracts\Cache\Repository as Cache; use Mockery as m; @@ -20,6 +21,7 @@ public function testTooManyAttemptsReturnTrueIfAlreadyLockedOut() $cache->shouldReceive('get')->once()->with('key', 0)->andReturn(1); $cache->shouldReceive('has')->once()->with('key:timer')->andReturn(true); $cache->shouldReceive('add')->never(); + $cache->shouldReceive('getStore')->andReturn(new ArrayStore); $rateLimiter = new RateLimiter($cache); $this->assertTrue($rateLimiter->tooManyAttempts('key', 1)); @@ -31,6 +33,7 @@ public function testHitProperlyIncrementsAttemptCount() $cache->shouldReceive('add')->once()->with('key:timer', m::type('int'), 1)->andReturn(true); $cache->shouldReceive('add')->once()->with('key', 0, 1)->andReturn(true); $cache->shouldReceive('increment')->once()->with('key', 1)->andReturn(1); + $cache->shouldReceive('getStore')->andReturn(new ArrayStore); $rateLimiter = new RateLimiter($cache); $rateLimiter->hit('key', 1); @@ -42,6 +45,7 @@ public function testIncrementProperlyIncrementsAttemptCount() $cache->shouldReceive('add')->once()->with('key:timer', m::type('int'), 1)->andReturn(true); $cache->shouldReceive('add')->once()->with('key', 0, 1)->andReturn(true); $cache->shouldReceive('increment')->once()->with('key', 5)->andReturn(5); + $cache->shouldReceive('getStore')->andReturn(new ArrayStore); $rateLimiter = new RateLimiter($cache); $rateLimiter->increment('key', 1, 5); @@ -53,6 +57,7 @@ public function testDecrementProperlyDecrementsAttemptCount() $cache->shouldReceive('add')->once()->with('key:timer', m::type('int'), 1)->andReturn(true); $cache->shouldReceive('add')->once()->with('key', 0, 1)->andReturn(true); $cache->shouldReceive('increment')->once()->with('key', -5)->andReturn(-5); + $cache->shouldReceive('getStore')->andReturn(new ArrayStore); $rateLimiter = new RateLimiter($cache); $rateLimiter->decrement('key', 1, 5); @@ -65,6 +70,7 @@ public function testHitHasNoMemoryLeak() $cache->shouldReceive('add')->once()->with('key', 0, 1)->andReturn(false); $cache->shouldReceive('increment')->once()->with('key', 1)->andReturn(1); $cache->shouldReceive('put')->once()->with('key', 1, 1); + $cache->shouldReceive('getStore')->andReturn(new ArrayStore); $rateLimiter = new RateLimiter($cache); $rateLimiter->hit('key', 1); @@ -74,6 +80,7 @@ public function testRetriesLeftReturnsCorrectCount() { $cache = m::mock(Cache::class); $cache->shouldReceive('get')->once()->with('key', 0)->andReturn(3); + $cache->shouldReceive('getStore')->andReturn(new ArrayStore); $rateLimiter = new RateLimiter($cache); $this->assertEquals(2, $rateLimiter->retriesLeft('key', 5)); @@ -84,6 +91,7 @@ public function testClearClearsTheCacheKeys() $cache = m::mock(Cache::class); $cache->shouldReceive('forget')->once()->with('key'); $cache->shouldReceive('forget')->once()->with('key:timer'); + $cache->shouldReceive('getStore')->andReturn(new ArrayStore); $rateLimiter = new RateLimiter($cache); $rateLimiter->clear('key'); @@ -93,6 +101,7 @@ public function testAvailableInReturnsPositiveValues() { $cache = m::mock(Cache::class); $cache->shouldReceive('get')->andReturn(now()->subSeconds(60)->getTimestamp(), null); + $cache->shouldReceive('getStore')->andReturn(new ArrayStore); $rateLimiter = new RateLimiter($cache); $this->assertTrue($rateLimiter->availableIn('key:timer') >= 0); @@ -106,6 +115,7 @@ public function testAttemptsCallbackReturnsTrue() $cache->shouldReceive('add')->once()->with('key:timer', m::type('int'), 1); $cache->shouldReceive('add')->once()->with('key', 0, 1)->andReturns(1); $cache->shouldReceive('increment')->once()->with('key', 1)->andReturn(1); + $cache->shouldReceive('getStore')->andReturn(new ArrayStore); $executed = false; @@ -124,6 +134,7 @@ public function testAttemptsCallbackReturnsCallbackReturn() $cache->shouldReceive('add')->times(6)->with('key:timer', m::type('int'), 1); $cache->shouldReceive('add')->times(6)->with('key', 0, 1)->andReturns(1); $cache->shouldReceive('increment')->times(6)->with('key', 1)->andReturn(1); + $cache->shouldReceive('getStore')->andReturn(new ArrayStore); $rateLimiter = new RateLimiter($cache); @@ -157,6 +168,7 @@ public function testAttemptsCallbackReturnsFalse() $cache = m::mock(Cache::class); $cache->shouldReceive('get')->once()->with('key', 0)->andReturn(2); $cache->shouldReceive('has')->once()->with('key:timer')->andReturn(true); + $cache->shouldReceive('getStore')->andReturn(new ArrayStore); $executed = false; @@ -174,6 +186,7 @@ public function testKeysAreSanitizedFromUnicodeCharacters() $cache->shouldReceive('get')->once()->with('john', 0)->andReturn(1); $cache->shouldReceive('has')->once()->with('john:timer')->andReturn(true); $cache->shouldReceive('add')->never(); + $cache->shouldReceive('getStore')->andReturn(new ArrayStore); $rateLimiter = new RateLimiter($cache); $this->assertTrue($rateLimiter->tooManyAttempts('jôhn', 1)); @@ -190,6 +203,7 @@ public function testKeyIsSanitizedOnlyOnce() $cache->shouldReceive('get')->once()->with($cleanedKey, 0)->andReturn(1); $cache->shouldReceive('has')->once()->with("$cleanedKey:timer")->andReturn(true); $cache->shouldReceive('add')->never(); + $cache->shouldReceive('getStore')->andReturn(new ArrayStore); $this->assertTrue($rateLimiter->tooManyAttempts($key, 1)); } From 58fe652c10a793132de9a71756371eae55fc6176 Mon Sep 17 00:00:00 2001 From: Petr Levtonov Date: Mon, 27 Jan 2025 14:41:24 +0100 Subject: [PATCH 4/5] Skip outdated test --- tests/Integration/Cache/RedisStoreTest.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/Integration/Cache/RedisStoreTest.php b/tests/Integration/Cache/RedisStoreTest.php index 25ee5ccbe721..af731995b7de 100644 --- a/tests/Integration/Cache/RedisStoreTest.php +++ b/tests/Integration/Cache/RedisStoreTest.php @@ -252,6 +252,8 @@ public function testPutManyCallsPutWhenClustered() public function testIncrementWithSerializationEnabled() { + $this->markTestSkipped('Test makes no sense anymore. Application must explicitly wrap such code in runClean() when used with serialization/compression enabled.'); + /** @var \Illuminate\Cache\RedisStore $store */ $store = Cache::store('redis'); /** @var \Redis $client */ From cf87fa4fdbab998e1c5be6f3b8f3bd3543838ac1 Mon Sep 17 00:00:00 2001 From: Taylor Otwell Date: Mon, 27 Jan 2025 16:44:01 -0600 Subject: [PATCH 5/5] formatting --- src/Illuminate/Cache/RateLimiter.php | 22 +++--- .../Redis/Connections/PacksPhpRedisValues.php | 72 ++++++++++--------- 2 files changed, 51 insertions(+), 43 deletions(-) diff --git a/src/Illuminate/Cache/RateLimiter.php b/src/Illuminate/Cache/RateLimiter.php index a834a0a12447..12f76fee6e2a 100644 --- a/src/Illuminate/Cache/RateLimiter.php +++ b/src/Illuminate/Cache/RateLimiter.php @@ -166,12 +166,16 @@ public function increment($key, $decaySeconds = 60, $amount = 1) $key.':timer', $this->availableAt($decaySeconds), $decaySeconds ); - $added = $this->runClean(fn (): mixed => $this->cache->add($key, 0, $decaySeconds)); + $added = $this->withoutSerializationOrCompression( + fn () => $this->cache->add($key, 0, $decaySeconds) + ); $hits = (int) $this->cache->increment($key, $amount); if (! $added && $hits == 1) { - $this->runClean(fn (): mixed => $this->cache->put($key, 1, $decaySeconds)); + $this->withoutSerializationOrCompression( + fn () => $this->cache->put($key, 1, $decaySeconds) + ); } return $hits; @@ -200,7 +204,7 @@ public function attempts($key) { $key = $this->cleanRateLimiterKey($key); - return $this->runClean(fn (): mixed => $this->cache->get($key, 0)); + return $this->withoutSerializationOrCompression(fn () => $this->cache->get($key, 0)); } /** @@ -284,24 +288,26 @@ public function cleanRateLimiterKey($key) } /** - * Allow the rate limiter to run a callback against a clean repository. + * Execute the given callback without serialization or compression when applicable. * - * For example when using RedisStore, the serializer and compressor can be - * disabled during the callback execution. + * @param callable $callback + * @return mixed */ - protected function runClean(callable $callback): mixed + protected function withoutSerializationOrCompression(callable $callback) { $store = $this->cache->getStore(); + if (! $store instanceof RedisStore) { return $callback(); } $connection = $store->connection(); + if (! $connection instanceof PhpRedisConnection) { return $callback(); } - return $connection->runClean($callback); + return $connection->withoutSerializationOrCompression($callback); } /** diff --git a/src/Illuminate/Redis/Connections/PacksPhpRedisValues.php b/src/Illuminate/Redis/Connections/PacksPhpRedisValues.php index d733d45e7fa7..526a764ede89 100644 --- a/src/Illuminate/Redis/Connections/PacksPhpRedisValues.php +++ b/src/Illuminate/Redis/Connections/PacksPhpRedisValues.php @@ -82,6 +82,43 @@ public function pack(array $values): array return array_map($processor, $values); } + /** + * Execute the given callback without serialization or compression when applicable. + * + * @param callable $callback + * @return mixed + */ + public function withoutSerializationOrCompression(callable $callback) + { + $client = $this->client; + + $oldSerializer = null; + + if ($this->serialized()) { + $oldSerializer = $client->getOption($client::OPT_SERIALIZER); + $client->setOption($client::OPT_SERIALIZER, $client::SERIALIZER_NONE); + } + + $oldCompressor = null; + + if ($this->compressed()) { + $oldCompressor = $client->getOption($client::OPT_COMPRESSION); + $client->setOption($client::OPT_COMPRESSION, $client::COMPRESSION_NONE); + } + + try { + return $callback(); + } finally { + if ($oldSerializer !== null) { + $client->setOption($client::OPT_SERIALIZER, $oldSerializer); + } + + if ($oldCompressor !== null) { + $client->setOption($client::OPT_COMPRESSION, $oldCompressor); + } + } + } + /** * Determine if serialization is enabled. * @@ -137,41 +174,6 @@ public function lz4Compressed(): bool $this->client->getOption(Redis::OPT_COMPRESSION) === Redis::COMPRESSION_LZ4; } - /** - * Run a callback without serialization or compression enabled. Useful when - * performing operations like increment/decrement that should not be - * serialized or compressed on client side. - */ - public function runClean(callable $callback): mixed - { - /** @var \Redis|\RedisCluster $client */ - $client = $this->client; - - $oldSerializer = null; - if ($this->serialized()) { - $oldSerializer = $client->getOption($client::OPT_SERIALIZER); - $client->setOption($client::OPT_SERIALIZER, $client::SERIALIZER_NONE); - } - - $oldCompressor = null; - if ($this->compressed()) { - $oldCompressor = $client->getOption($client::OPT_COMPRESSION); - $client->setOption($client::OPT_COMPRESSION, $client::COMPRESSION_NONE); - } - - try { - return $callback(); - } finally { - if ($oldSerializer !== null) { - $client->setOption($client::OPT_SERIALIZER, $oldSerializer); - } - - if ($oldCompressor !== null) { - $client->setOption($client::OPT_COMPRESSION, $oldCompressor); - } - } - } - /** * Determine if the current PhpRedis extension version supports packing. *