[10.x] Prevent DB Cache::get() occur race condition (#49031)

* [10.x] Prevent Cache::get() occur race condition * Fix test * Add Cache Get and ForgetIfExpired integration * Use Cache::put() to store expired cache * Fix test * Add Cache Get and ForgetIfExpired integration test * Test Use Cache::put() to store expired cache * Fix wrong test statement * Fix test * Fix test * [10.x] Fix DatabaseStore `add()` failed due to incorrect key * Style fix * formatting --------- Co-authored-by: Taylor Otwell <[email protected]>
laravel · Nov 17, 2023 · 708a90b · 708a90b
1 parent 59c509b
commit 708a90b
Show file tree

Hide file tree

Showing 4 changed files with 115 additions and 10 deletions.
diff --git a/src/Illuminate/Cache/DatabaseStore.php b/src/Illuminate/Cache/DatabaseStore.php
@@ -115,7 +115,7 @@ public function get($key)
         // item from the cache. Then we will return a null value since the cache is
         // expired. We will use "Carbon" to make this comparison with the column.
         if ($this->currentTime() >= $cache->expiration) {
-            $this->forget($key);
+            $this->forgetIfExpired($key);
 
             return;
         }
@@ -150,14 +150,14 @@ public function put($key, $value, $seconds)
      */
     public function add($key, $value, $seconds)
     {
-        $key = $this->prefix.$key;
-        $value = $this->serialize($value);
-        $expiration = $this->getTime() + $seconds;
-
         if (! is_null($this->get($key))) {
             return false;
         }
 
+        $key = $this->prefix.$key;
+        $value = $this->serialize($value);
+        $expiration = $this->getTime() + $seconds;
+
         $doesntSupportInsertOrIgnore = [SqlServerConnection::class];
 
         if (! in_array(get_class($this->getConnection()), $doesntSupportInsertOrIgnore)) {
@@ -316,6 +316,22 @@ public function forget($key)
         return true;
     }
 
+    /**
+     * Remove an item from the cache if it is expired.
+     *
+     * @param  string  $key
+     * @return bool
+     */
+    public function forgetIfExpired($key)
+    {
+        $this->table()
+            ->where('key', '=', $this->prefix.$key)
+            ->where('expiration', '<=', $this->getTime())
+            ->delete();
+
+        return true;
+    }
+
     /**
      * Remove all items from the cache.
      *

diff --git a/src/Illuminate/Support/Number.php b/src/Illuminate/Support/Number.php
@@ -117,7 +117,7 @@ public static function forHumans(int|float $number, int $precision = 0)
         $displayExponent = $numberExponent - ($numberExponent % 3);
         $number /= pow(10, $displayExponent);
 
-        return trim(sprintf('%s %s', number_format($number, $precision), $units[$displayExponent]));
+        return trim(sprintf('%s %s', number_format($number, $precision), $units[$displayExponent] ?? ''));
     }
 
     /**

diff --git a/tests/Cache/CacheDatabaseStoreTest.php b/tests/Cache/CacheDatabaseStoreTest.php
@@ -30,12 +30,12 @@ public function testNullIsReturnedWhenItemNotFound()
 
     public function testNullIsReturnedAndItemDeletedWhenItemIsExpired()
     {
-        $store = $this->getMockBuilder(DatabaseStore::class)->onlyMethods(['forget'])->setConstructorArgs($this->getMocks())->getMock();
+        $store = $this->getMockBuilder(DatabaseStore::class)->onlyMethods(['forgetIfExpired'])->setConstructorArgs($this->getMocks())->getMock();
         $table = m::mock(stdClass::class);
         $store->getConnection()->shouldReceive('table')->once()->with('table')->andReturn($table);
         $table->shouldReceive('where')->once()->with('key', '=', 'prefixfoo')->andReturn($table);
         $table->shouldReceive('first')->once()->andReturn((object) ['expiration' => 1]);
-        $store->expects($this->once())->method('forget')->with($this->equalTo('foo'))->willReturn(null);
+        $store->expects($this->once())->method('forgetIfExpired')->with($this->equalTo('foo'))->willReturn(null);
 
         $this->assertNull($store->get('foo'));
     }

diff --git a/tests/Integration/Database/DatabaseCacheStoreTest.php b/tests/Integration/Database/DatabaseCacheStoreTest.php
@@ -2,6 +2,7 @@
 
 namespace Illuminate\Tests\Integration\Database;
 
+use Illuminate\Support\Carbon;
 use Illuminate\Support\Facades\Cache;
 use Illuminate\Support\Facades\DB;
 use Orchestra\Testbench\Attributes\WithMigration;
@@ -18,6 +19,15 @@ public function testValueCanStoreNewCache()
         $this->assertSame('bar', $store->get('foo'));
     }
 
+    public function testPutOperationShouldNotStoreExpired()
+    {
+        $store = $this->getStore();
+
+        $store->put('foo', 'bar', 0);
+
+        $this->assertDatabaseMissing($this->getCacheTableName(), ['key' => $this->withCachePrefix('foo')]);
+    }
+
     public function testValueCanUpdateExistCache()
     {
         $store = $this->getStore();
@@ -41,6 +51,16 @@ public function testValueCanUpdateExistCacheInTransaction()
         $this->assertSame('new-bar', $store->get('foo'));
     }
 
+    public function testAddOperationShouldNotStoreExpired()
+    {
+        $store = $this->getStore();
+
+        $result = $store->add('foo', 'bar', 0);
+
+        $this->assertFalse($result);
+        $this->assertDatabaseMissing($this->getCacheTableName(), ['key' => $this->withCachePrefix('foo')]);
+    }
+
     public function testAddOperationCanStoreNewCache()
     {
         $store = $this->getStore();
@@ -80,7 +100,7 @@ public function testAddOperationCanUpdateIfCacheExpired()
     {
         $store = $this->getStore();
 
-        $store->add('foo', 'bar', 0);
+        $this->insertToCacheTable('foo', 'bar', 0);
         $result = $store->add('foo', 'new-bar', 60);
 
         $this->assertTrue($result);
@@ -91,7 +111,7 @@ public function testAddOperationCanUpdateIfCacheExpiredInTransaction()
     {
         $store = $this->getStore();
 
-        $store->add('foo', 'bar', 0);
+        $this->insertToCacheTable('foo', 'bar', 0);
 
         DB::beginTransaction();
         $result = $store->add('foo', 'new-bar', 60);
@@ -101,8 +121,77 @@ public function testAddOperationCanUpdateIfCacheExpiredInTransaction()
         $this->assertSame('new-bar', $store->get('foo'));
     }
 
+    public function testGetOperationReturnNullIfExpired()
+    {
+        $store = $this->getStore();
+
+        $this->insertToCacheTable('foo', 'bar', 0);
+
+        $result = $store->get('foo');
+
+        $this->assertNull($result);
+    }
+
+    public function testGetOperationCanDeleteExpired()
+    {
+        $store = $this->getStore();
+
+        $this->insertToCacheTable('foo', 'bar', 0);
+
+        $store->get('foo');
+
+        $this->assertDatabaseMissing($this->getCacheTableName(), ['key' => $this->withCachePrefix('foo')]);
+    }
+
+    public function testForgetIfExpiredOperationCanDeleteExpired()
+    {
+        $store = $this->getStore();
+
+        $this->insertToCacheTable('foo', 'bar', 0);
+
+        $store->forgetIfExpired('foo');
+
+        $this->assertDatabaseMissing($this->getCacheTableName(), ['key' => $this->withCachePrefix('foo')]);
+    }
+
+    public function testForgetIfExpiredOperationShouldNotDeleteUnExpired()
+    {
+        $store = $this->getStore();
+
+        $store->put('foo', 'bar', 60);
+
+        $store->forgetIfExpired('foo');
+
+        $this->assertDatabaseHas($this->getCacheTableName(), ['key' => $this->withCachePrefix('foo')]);
+    }
+
+    /**
+     * @return \Illuminate\Cache\DatabaseStore
+     */
     protected function getStore()
     {
         return Cache::store('database');
     }
+
+    protected function getCacheTableName()
+    {
+        return config('cache.stores.database.table');
+    }
+
+    protected function withCachePrefix(string $key)
+    {
+        return config('cache.prefix').$key;
+    }
+
+    protected function insertToCacheTable(string $key, $value, $ttl = 60)
+    {
+        DB::table($this->getCacheTableName())
+            ->insert(
+                [
+                    'key' => $this->withCachePrefix($key),
+                    'value' => $value,
+                    'expiration' => Carbon::now()->addSeconds($ttl)->getTimestamp(),
+                ]
+            );
+    }
 }