Update hashing algorithm

venveo · Jun 16, 2022 · 1bfc5f0 · 1bfc5f0
1 parent 24c43e5
commit 1bfc5f0
Show file tree

Hide file tree

Showing 5 changed files with 48 additions and 28 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,7 @@
 ### Change
 - Compress now requires Craft 4
 - Compress now requires PHP 8.0.1
+- Hashing mechanism now accounts for dateUpdated and sorts records. All zip files will need to be regenerated.
 
 ## 1.0.3 - 2019-11-22
 ### Fixed

diff --git a/src/controllers/CompressController.php b/src/controllers/CompressController.php
@@ -10,14 +10,13 @@
 
 namespace venveo\compress\controllers;
 
-use Craft;
-use craft\elements\Asset;
 use craft\helpers\DateTimeHelper;
 use craft\web\Controller;
 use venveo\compress\Compress as Plugin;
 use venveo\compress\errors\CompressException;
 use venveo\compress\models\Archive as ArchiveModel;
 use venveo\compress\records\Archive as ArchiveRecord;
+use yii\base\InvalidConfigException;
 
 /**
  * Class CompressController
@@ -30,13 +29,15 @@ class CompressController extends Controller
     /**
      * Gets a direct link to the asset
      * @param $uid
-     * @return \craft\web\Response|string|\yii\console\Response
+     * @return \yii\web\Response
      * @throws CompressException
+     * @throws InvalidConfigException
      */
     public function actionGetLink($uid)
     {
+        /** @var ArchiveRecord $record */
         $record = ArchiveRecord::find()->where(['=', 'uid', $uid])->one();
-        if (!$record instanceof ArchiveRecord) {
+        if (!$record) {
             return \Craft::$app->response->setStatusCode(404, 'Archive could not be found');
         }
 
@@ -45,9 +46,14 @@ public function actionGetLink($uid)
             $archiveModel = ArchiveModel::hydrateFromRecord($record);
             // It's possible for an asset ID to exist, but getAsset to return false on soft-deleted assets
             if ($archiveModel->getAsset()) {
-                $record->dateLastAccessed = DateTimeHelper::currentUTCDateTime();
-                $record->save();
-                return \Craft::$app->response->redirect($archiveModel->getAsset()->getUrl());
+                $assetUrl = $archiveModel->getAsset()->getUrl();
+                if ($assetUrl) {
+                    $record->dateLastAccessed = DateTimeHelper::currentUTCDateTime();
+                    $record->save();
+                    return \Craft::$app->response->redirect($archiveModel->getAsset()->getUrl());
+                }
+
+                return \Craft::$app->response->setStatusCode(404, 'Could not produce zip file URL.');
             }
         }
 
@@ -69,7 +75,7 @@ public function actionGetLink($uid)
         } catch (\Exception $e) {
             \Craft::error('Archive could not be generated: ' . $e->getMessage(), __METHOD__);
             \Craft::error($e->getTraceAsString(), __METHOD__);
-            throw new CompressException('Archive could not be generated: '. $e->getMessage());
+            throw new CompressException('Archive could not be generated: ' . $e->getMessage());
         }
     }
 }
diff --git a/src/jobs/CreateArchive.php b/src/jobs/CreateArchive.php
@@ -39,13 +39,13 @@ public function execute($queue): void
         // If it's not in the cache, we'll assume it got completed on-demand
         if (!\Craft::$app->cache->get($this->cacheKey)) {
             Craft::info('Archive already completed');
-            return true;
+            return;
         }
 
         $archiveRecord = ArchiveRecord::find()->where(['=', 'uid', $this->archiveUid])->one();
         if (!$archiveRecord instanceof ArchiveRecord) {
             Craft::error('Archive was deleted before it could be created');
-            return false;
+            return;
         }
 
         try {
@@ -58,7 +58,7 @@ public function execute($queue): void
             \Craft::$app->cache->delete($this->cacheKey);
             \Craft::$app->cache->delete($this->cacheKey . ':jobId');
             // Go ahead and blow up
-            return false;
+            return;
         }
         \Craft::$app->cache->delete($this->cacheKey);
         \Craft::$app->cache->delete($this->cacheKey . ':jobId');

diff --git a/src/models/Archive.php b/src/models/Archive.php
@@ -21,19 +21,22 @@
  * @author    Venveo
  * @package   Compress
  * @since     1.0.0
+ *
+ * @property-read mixed $contents
+ * @property-read null|string $lazyLink
  */
 class Archive extends Model
 {
-    public $id;
-    public $uid;
-    public $assetId;
-    public $hash;
+    public ?int $id = null;
+    public ?string $uid = null;
+    public ?int $assetId = null;
+    public ?string $hash = null;
 
     public ?DateTime $dateUpdated;
     public ?DateTime $dateCreated;
     public ?DateTime $dateLastAccessed = null;
 
-    public $asset;
+    public ?Asset $asset = null;
 
 
     /**
@@ -53,26 +56,33 @@ public static function hydrateFromRecord(ActiveRecord $record, Asset $asset = nu
     /**
      * @return Asset|null
      */
-    public function getAsset()
+    public function getAsset($siteId = null): ?Asset
     {
         if (!$this->assetId) {
             return null;
         }
         if ($this->asset instanceof Asset) {
             return $this->asset;
         }
+        if (!$siteId) {
+            $siteId = \Craft::$app->sites?->currentSite?->id;
+        }
 
-        $this->asset = \Craft::$app->assets->getAssetById($this->assetId);
+        $this->asset = \Craft::$app->assets->getAssetById($this->assetId, $siteId);
         return $this->asset;
     }
 
     /**
      * @return string|null
      */
-    public function getLazyLink()
+    public function getLazyLink(): ?string
     {
         if ($this->asset instanceof Asset) {
-            return $this->asset->getUrl();
+            // Ensure we _can_ get a url for the asset
+            $assetUrl = $this->asset->getUrl();
+            if($assetUrl) {
+                return $assetUrl;
+            }
         }
         return UrlHelper::actionUrl('compress/compress/get-link', ['uid' => $this->uid]);
     }

diff --git a/src/services/Compress.php b/src/services/Compress.php
@@ -57,7 +57,7 @@ public function getArchiveModelForQuery($query, $lazy = false, $filename = null)
         // Get the assets and create a unique hash to represent them
         if ($query instanceof AssetQuery) {
             $assets = $query->all();
-        } elseif (is_array($query)) {
+        } elseif ($query instanceof \ArrayAccess) {
             $assets = $query;
         } else {
             Craft::error('Unexpected input provided for asset query', __METHOD__);
@@ -122,7 +122,7 @@ public function createArchiveRecord($assets, $archiveAsset = null)
      * @throws \craft\errors\VolumeException
      * @throws \Exception
      */
-    public function createArchiveAsset(ArchiveRecord $archiveRecord)
+    public function createArchiveAsset(ArchiveRecord $archiveRecord): ?ArchiveModel
     {
         $uuid = StringHelper::UUID();
         $fileAssetRecords = $archiveRecord->fileAssets;
@@ -219,7 +219,6 @@ private function createArchiveRecords($zippedAssets, $asset, $archiveRecord = nu
         $archiveRecord = $event->archiveRecord;
 
         $rows = [];
-        /** @var Asset $zippedAsset */
         foreach ($zippedAssets as $zippedAsset) {
             $rows[] = [
                 $archiveRecord->id,
@@ -248,9 +247,13 @@ private function getHashForAssets($assets): string
     {
         $ids = [];
         foreach ($assets as $asset) {
-            $ids[] = [$asset->id];
+            $updatedAt = $asset->dateUpdated->getTimestamp();
+            $key = $asset->id . ':' . $updatedAt;
+            $ids[] = $key;
         }
-        return md5(\GuzzleHttp\json_encode($ids));
+        sort($ids);
+        $hashKey = implode('', $ids);
+        return md5($hashKey);
     }
 
     /**
@@ -334,7 +337,7 @@ public function getArchiveContents(ArchiveModel $archive): AssetQuery
      * @param null $limit
      * @return array
      */
-    public function getArchives($offset = 0, $limit = null): array
+    public function getArchives(?int $offset = 0, ?int $limit = null): array
     {
         $records = ArchiveRecord::find();
         if ($offset) {
@@ -352,12 +355,12 @@ public function getArchives($offset = 0, $limit = null): array
     }
 
     /**
-     * Get an archive model from it's record's UID
+     * Get an archive model from its record's UID
      *
      * @param $uid
      * @return ArchiveModel|null
      */
-    public function getArchiveModelByUID($uid)
+    public function getArchiveModelByUID($uid): ?ArchiveModel
     {
         $record = ArchiveRecord::find()->where(['=', 'uid', $uid])->one();
         if (!$record instanceof ArchiveRecord) {