Merge pull request #337 from adrhumphreys/bugfix/invalid-cache-betwee…

…n-servers

Use a timestamp in the hash cache

src/Dev/Tasks/VersionedFilesMigrationTask.php

@@ -29,5 +29,4 @@ public function run($request)
         $migrator = VersionedFilesMigrator::create($strategy, ASSETS_PATH, true);
         $migrator->migrate();
     }
-
 }

src/Dev/VersionedFilesMigrator.php

@@ -191,5 +191,4 @@ public function getLog()
     {
         return $this->log;
     }
-
 }

src/Storage/Sha1FileHashingService.php

@@ -3,15 +3,14 @@
 namespace SilverStripe\Assets\Storage;
 
 use InvalidArgumentException;
-use League\Flysystem\FileNotFoundException;
 use League\Flysystem\Filesystem;
 use League\Flysystem\Util;
 use Psr\SimpleCache\CacheInterface;
-use SilverStripe\Assets\Flysystem\FlysystemAssetStore;
 use SilverStripe\Core\Config\Configurable;
 use SilverStripe\Core\Flushable;
 use SilverStripe\Core\Injector\Injectable;
 use SilverStripe\Core\Injector\Injector;
+use SilverStripe\ORM\FieldType\DBDatetime;
 
 /**
  * Utility for computing and comparing unique file hash. All `$fs` parameters can either be:
@@ -45,7 +44,7 @@ class Sha1FileHashingService implements FileHashingService, Flushable
 
     /** @var Filesystem[] */
     private $filesystems;
-    
+
     public function computeFromStream($stream)
     {
         Util::rewindStream($stream);
@@ -120,7 +119,7 @@ public function computeFromFile($fileID, $fs)
         $fs = $this->getFilesystem($fs);
         $stream = $fs->readStream($fileID);
         $hash = $this->computeFromStream($stream);
-        
+
         $this->set($fileID, $fs, $hash);
 
         return $hash;
@@ -179,10 +178,23 @@ private function getCache()
     private function buildCacheKey($fileID, $fs)
     {
         $fsID = $this->getFilesystemKey($fs);
-
         return base64_encode(sprintf('%s://%s', $fsID, $fileID));
     }
 
+    /**
+     * Return the current timestamp for the provided file or the current time if the file doesn't exists.
+     * @param string $fileID
+     * @param string|Filesystem $fs
+     * @return string
+     */
+    private function getTimestamp($fileID, $fs)
+    {
+        $filesystem = $this->getFilesystem($fs);
+        return $filesystem->has($fileID) ?
+            $filesystem->getTimestamp($fileID) :
+            DBDatetime::now()->getTimestamp();
+    }
+
     public function invalidate($fileID, $fs)
     {
         $key = $this->buildCacheKey($fileID, $fs);
@@ -200,7 +212,14 @@ public function get($fileID, $fs)
     {
         if ($this->isCached()) {
             $key = $this->buildCacheKey($fileID, $fs);
-            return $this->getCache()->get($key, false);
+            $value = $this->getCache()->get($key, false);
+            if ($value) {
+                list($timestamp, $hash) = $value;
+                if ($timestamp === $this->getTimestamp($fileID, $fs)) {
+                    // Only return the cached hash if the cached timestamp matches the timestamp of the physical file
+                    return $hash;
+                }
+            }
         }
         return false;
     }
@@ -210,7 +229,13 @@ public function set($fileID, $fs, $hash)
     {
         if ($this->isCached()) {
             $key = $this->buildCacheKey($fileID, $fs);
-            $this->getCache()->set($key, $hash);
+            // We store the file's timestamp in the cache so we can compare, that way we can know if some outside
+            // process has touched the file
+            $value = [
+                $this->getTimestamp($fileID, $fs),
+                $hash
+            ];
+            $this->getCache()->set($key, $value);
         }
     }
 

tests/php/Flysystem/FlysystemAssetStoreTest.php

@@ -50,7 +50,7 @@ protected function setUp()
             ->getMock();
 
         $this->publicFilesystem = $this->getMockBuilder(Filesystem::class)
-            ->setMethods(['has', 'read', 'readStream'])
+            ->setMethods(['has', 'read', 'readStream', 'getTimestamp'])
             ->setConstructorArgs([$this->publicAdapter])
             ->getMock();
 
@@ -59,7 +59,7 @@ protected function setUp()
             ->getMock();
 
         $this->protectedFilesystem = $this->getMockBuilder(Filesystem::class)
-            ->setMethods(['has', 'read', 'readStream'])
+            ->setMethods(['has', 'read', 'readStream', 'getTimestamp'])
             ->setConstructorArgs([$this->protectedAdapter])
             ->getMock();
 

tests/php/Storage/Sha1FileHashingServiceTest.php

@@ -54,6 +54,7 @@ public function setUp()
 
         $this->publicFs->write($this->fileID, $this->publicContent);
         $this->protectedFs->write($this->fileID, $this->protectedContent);
+        Sha1FileHashingService::flush();
     }
 
     public function tearDown()
@@ -78,7 +79,7 @@ public function testComputeFromStream()
         }
     }
 
-    public function testcomputeFromFile()
+    public function testComputeFromFile()
     {
         $service = new Sha1FileHashingService();
         $service->disableCache();
@@ -137,6 +138,25 @@ public function testComputeWithCache()
         $this->assertEquals($hash, $service->computeFromFile('missing-file.text', AssetStore::VISIBILITY_PUBLIC));
     }
 
+    public function testComputeTouchFile()
+    {
+        $service = new Sha1FileHashingService();
+        $service->enableCache();
+
+        $this->assertEquals($this->publicHash, $service->computeFromFile($this->fileID, $this->publicFs));
+
+        // Our timestamp is accruate to the second, we need to wait a bit to make sure our new timestamp won't be in
+        // the same second as the old one.
+        sleep(2);
+        $this->publicFs->update($this->fileID, $this->protectedContent);
+
+        $this->assertEquals(
+            $this->protectedHash,
+            $service->computeFromFile($this->fileID, $this->publicFs),
+            'When a file is touched by an outside process, existing value for that file are invalidated'
+        );
+    }
+
     public function testCompare()
     {
         $service = new Sha1FileHashingService();