reactphp · WyriHaximus · Jun 25, 2018 · Jun 24, 2018
diff --git a/composer.json b/composer.json
@@ -5,7 +5,7 @@
     "license": "MIT",
     "require": {
         "php": ">=5.3.0",
-        "react/cache": "~0.4.0|~0.3.0",
+        "react/cache": "^0.5 || ^0.4 || ^0.3",
         "react/event-loop": "^1.0 || ^0.5 || ^0.4 || ^0.3.5",
         "react/promise": "^2.1 || ^1.2.1",
         "react/promise-timer": "^1.2",

diff --git a/src/Query/RecordCache.php b/src/Query/RecordCache.php
@@ -6,7 +6,11 @@
 use React\Dns\Model\Message;
 use React\Dns\Model\Record;
 use React\Promise;
+use React\Promise\PromiseInterface;
 
+/**
+ * Wraps an underlying cache interface and exposes only cached DNS data
+ */
 class RecordCache
 {
     private $cache;
@@ -17,6 +21,13 @@ public function __construct(CacheInterface $cache)
         $this->cache = $cache;
     }
 
+    /**
+     * Looks up the cache if there's a cached answer for the given query
+     *
+     * @param Query $query
+     * @return PromiseInterface Promise<Record[],mixed> resolves with array of Record objects on sucess
+     *     or rejects with mixed values when query is not cached already.
+     */
     public function lookup(Query $query)
     {
         $id = $this->serializeQueryToIdentity($query);
@@ -26,8 +37,17 @@ public function lookup(Query $query)
         return $this->cache
             ->get($id)
             ->then(function ($value) use ($query, $expiredAt) {
+                // cache 0.5+ resolves with null on cache miss, return explicit cache miss here
+                if ($value === null) {
+                    return Promise\reject();
+                }
+
+                /* @var $recordBag RecordBag */
                 $recordBag = unserialize($value);
 
+                // reject this cache hit if the query was started before the time we expired the cache?
+                // todo: this is a legacy left over, this value is never actually set, so this never applies.
+                // todo: this should probably validate the cache time instead.
                 if (null !== $expiredAt && $expiredAt <= $query->currentTime) {
                     return Promise\reject();
                 }
@@ -36,13 +56,26 @@ public function lookup(Query $query)
             });
     }
 
+    /**
+     * Stores all records from this response message in the cache
+     *
+     * @param int     $currentTime
+     * @param Message $message
+     * @uses self::storeRecord()
+     */
     public function storeResponseMessage($currentTime, Message $message)
     {
         foreach ($message->answers as $record) {
             $this->storeRecord($currentTime, $record);
         }
     }
 
+    /**
+     * Stores a single record from a response message in the cache
+     *
+     * @param int    $currentTime
+     * @param Record $record
+     */
     public function storeRecord($currentTime, Record $record)
     {
         $id = $this->serializeRecordToIdentity($record);
@@ -53,13 +86,21 @@ public function storeRecord($currentTime, Record $record)
             ->get($id)
             ->then(
                 function ($value) {
+                    if ($value === null) {
+                        // cache 0.5+ cache miss resolves with null, return empty bag here
+                        return new RecordBag();
+                    }
+
+                    // reuse existing bag on cache hit to append new record to it
                     return unserialize($value);
                 },
                 function ($e) {
+                    // legacy cache < 0.5 cache miss rejects promise, return empty bag here
                     return new RecordBag();
                 }
             )
-            ->then(function ($recordBag) use ($id, $currentTime, $record, $cache) {
+            ->then(function (RecordBag $recordBag) use ($id, $currentTime, $record, $cache) {
+                // add a record to the existing (possibly empty) record bag and save to cache
                 $recordBag->set($currentTime, $record);
                 $cache->set($id, serialize($recordBag));
             });

diff --git a/tests/Query/RecordCacheTest.php b/tests/Query/RecordCacheTest.php
@@ -9,23 +9,93 @@
 use React\Dns\Query\RecordCache;
 use React\Dns\Query\Query;
 use React\Promise\PromiseInterface;
+use React\Promise\Promise;
 
 class RecordCacheTest extends TestCase
 {
     /**
-    * @covers React\Dns\Query\RecordCache
-    * @test
-    */
-    public function lookupOnEmptyCacheShouldReturnNull()
+     * @covers React\Dns\Query\RecordCache
+     * @test
+     */
+    public function lookupOnNewCacheMissShouldReturnNull()
     {
         $query = new Query('igor.io', Message::TYPE_A, Message::CLASS_IN, 1345656451);
 
-        $cache = new RecordCache(new ArrayCache());
+        $base = $this->getMockBuilder('React\Cache\CacheInterface')->getMock();
+        $base->expects($this->once())->method('get')->willReturn(\React\Promise\resolve(null));
+
+        $cache = new RecordCache($base);
+        $promise = $cache->lookup($query);
+
+        $this->assertInstanceOf('React\Promise\RejectedPromise', $promise);
+    }
+
+    /**
+     * @covers React\Dns\Query\RecordCache
+     * @test
+     */
+    public function lookupOnLegacyCacheMissShouldReturnNull()
+    {
+        $query = new Query('igor.io', Message::TYPE_A, Message::CLASS_IN, 1345656451);
+
+        $base = $this->getMockBuilder('React\Cache\CacheInterface')->getMock();
+        $base->expects($this->once())->method('get')->willReturn(\React\Promise\reject());
+
+        $cache = new RecordCache($base);
         $promise = $cache->lookup($query);
 
         $this->assertInstanceOf('React\Promise\RejectedPromise', $promise);
     }
 
+    /**
+     * @covers React\Dns\Query\RecordCache
+     * @test
+     */
+    public function storeRecordPendingCacheDoesNotSetCache()
+    {
+        $query = new Query('igor.io', Message::TYPE_A, Message::CLASS_IN, 1345656451);
+        $pending = new Promise(function () { });
+
+        $base = $this->getMockBuilder('React\Cache\CacheInterface')->getMock();
+        $base->expects($this->once())->method('get')->willReturn($pending);
+        $base->expects($this->never())->method('set');
+
+        $cache = new RecordCache($base);
+        $cache->storeRecord($query->currentTime, new Record('igor.io', Message::TYPE_A, Message::CLASS_IN, 3600, '178.79.169.131'));
+    }
+
+    /**
+     * @covers React\Dns\Query\RecordCache
+     * @test
+     */
+    public function storeRecordOnNewCacheMissSetsCache()
+    {
+        $query = new Query('igor.io', Message::TYPE_A, Message::CLASS_IN, 1345656451);
+
+        $base = $this->getMockBuilder('React\Cache\CacheInterface')->getMock();
+        $base->expects($this->once())->method('get')->willReturn(\React\Promise\resolve(null));
+        $base->expects($this->once())->method('set')->with($this->isType('string'), $this->isType('string'));
+
+        $cache = new RecordCache($base);
+        $cache->storeRecord($query->currentTime, new Record('igor.io', Message::TYPE_A, Message::CLASS_IN, 3600, '178.79.169.131'));
+    }
+
+    /**
+     * @covers React\Dns\Query\RecordCache
+     * @test
+     */
+    public function storeRecordOnOldCacheMissSetsCache()
+    {
+        $query = new Query('igor.io', Message::TYPE_A, Message::CLASS_IN, 1345656451);
+
+        $base = $this->getMockBuilder('React\Cache\CacheInterface')->getMock();
+        $base->expects($this->once())->method('get')->willReturn(\React\Promise\reject());
+        $base->expects($this->once())->method('set')->with($this->isType('string'), $this->isType('string'));
+
+        $cache = new RecordCache($base);
+        $cache->storeRecord($query->currentTime, new Record('igor.io', Message::TYPE_A, Message::CLASS_IN, 3600, '178.79.169.131'));
+    }
+
     /**
     * @covers React\Dns\Query\RecordCache
     * @test