From b7fa55ff939697c633f995a4861e14f27935896c Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 18 Mar 2015 19:06:39 -0700 Subject: [PATCH] Fix improper selection of the chunk engine as a writable engine Summary: Fixes T7621. The engine selection code started out making sense, but didn't make as much sense by the time I was done with it. Specifically, from the vanilla file upload, we may incorrectly try to write directly to the chunk storage engine. This is incorrect, and produces a confusing/bad error. Make chunk storage engines explicit and don't try to do single-file one-shot writes to them. Test Plan: - Tried to upload a large file with vanilla uploader, got better error message. - Uploaded small and large files with drag and drop. - Viewed {nav Files > Help/Options}. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T7621 Differential Revision: https://secure.phabricator.com/D12110 --- .../check/PhabricatorStorageSetupCheck.php | 11 +--- ...atorFilesApplicationStorageEnginePanel.php | 3 +- .../conduit/FileAllocateConduitAPIMethod.php | 50 +++++++------- .../engine/PhabricatorFileStorageEngine.php | 66 ++++++++++++++++--- 4 files changed, 85 insertions(+), 45 deletions(-) diff --git a/src/applications/config/check/PhabricatorStorageSetupCheck.php b/src/applications/config/check/PhabricatorStorageSetupCheck.php index acdcd49306..dbe79c90d6 100644 --- a/src/applications/config/check/PhabricatorStorageSetupCheck.php +++ b/src/applications/config/check/PhabricatorStorageSetupCheck.php @@ -10,15 +10,8 @@ final class PhabricatorStorageSetupCheck extends PhabricatorSetupCheck { * @phutil-external-symbol class PhabricatorStartup */ protected function executeChecks() { - $chunk_engine_active = false; - - $engines = PhabricatorFileStorageEngine::loadWritableEngines(); - foreach ($engines as $engine) { - if ($engine->isChunkEngine()) { - $chunk_engine_active = true; - break; - } - } + $engines = PhabricatorFileStorageEngine::loadWritableChunkEngines(); + $chunk_engine_active = (bool)$engines; if (!$chunk_engine_active) { $doc_href = PhabricatorEnv::getDocLink('Configuring File Storage'); diff --git a/src/applications/files/applicationpanel/PhabricatorFilesApplicationStorageEnginePanel.php b/src/applications/files/applicationpanel/PhabricatorFilesApplicationStorageEnginePanel.php index 0b2999a66d..a174102c18 100644 --- a/src/applications/files/applicationpanel/PhabricatorFilesApplicationStorageEnginePanel.php +++ b/src/applications/files/applicationpanel/PhabricatorFilesApplicationStorageEnginePanel.php @@ -18,6 +18,7 @@ final class PhabricatorFilesApplicationStorageEnginePanel $engines = PhabricatorFileStorageEngine::loadAllEngines(); $writable_engines = PhabricatorFileStorageEngine::loadWritableEngines(); + $chunk_engines = PhabricatorFileStorageEngine::loadWritableChunkEngines(); $yes = pht('Yes'); $no = pht('No'); @@ -44,7 +45,7 @@ final class PhabricatorFilesApplicationStorageEnginePanel $test = $no; } - if (isset($writable_engines[$key])) { + if (isset($writable_engines[$key]) || isset($chunk_engines[$key])) { $rowc[] = 'highlighted'; } else { $rowc[] = null; diff --git a/src/applications/files/conduit/FileAllocateConduitAPIMethod.php b/src/applications/files/conduit/FileAllocateConduitAPIMethod.php index ae6c16ae03..80f1cad7e1 100644 --- a/src/applications/files/conduit/FileAllocateConduitAPIMethod.php +++ b/src/applications/files/conduit/FileAllocateConduitAPIMethod.php @@ -85,39 +85,35 @@ final class FileAllocateConduitAPIMethod ); } + // If there are any non-chunk engines which this file can fit into, + // just tell the client to upload the file. $engines = PhabricatorFileStorageEngine::loadStorageEngines($length); if ($engines) { + return array( + 'upload' => true, + 'filePHID' => null, + ); + } - // Pick the first engine. If the file is small enough to fit into a - // single engine without chunking, this will be a non-chunk engine and - // we'll just tell the client to upload the file. - $engine = head($engines); - if ($engine) { - if (!$engine->isChunkEngine()) { - return array( - 'upload' => true, - 'filePHID' => null, - ); - } + // Otherwise, this is a large file and we want to perform a chunked + // upload if we have a chunk engine available. + $chunk_engines = PhabricatorFileStorageEngine::loadWritableChunkEngines(); + if ($chunk_engines) { + $chunk_properties = $properties; - // Otherwise, this is a large file and we need to perform a chunked - // upload. - - $chunk_properties = $properties; - - if ($hash) { - $chunk_properties += array( - 'chunkedHash' => $chunked_hash, - ); - } - - $file = $engine->allocateChunks($length, $chunk_properties); - - return array( - 'upload' => true, - 'filePHID' => $file->getPHID(), + if ($hash) { + $chunk_properties += array( + 'chunkedHash' => $chunked_hash, ); } + + $chunk_engine = head($chunk_engines); + $file = $chunk_engine->allocateChunks($length, $chunk_properties); + + return array( + 'upload' => true, + 'filePHID' => $file->getPHID(), + ); } // None of the storage engines can accept this file. diff --git a/src/applications/files/engine/PhabricatorFileStorageEngine.php b/src/applications/files/engine/PhabricatorFileStorageEngine.php index 6d0826c95c..be2fb2bbd3 100644 --- a/src/applications/files/engine/PhabricatorFileStorageEngine.php +++ b/src/applications/files/engine/PhabricatorFileStorageEngine.php @@ -12,6 +12,7 @@ * @task construct Constructing an Engine * @task meta Engine Metadata * @task file Managing File Data + * @task load Loading Storage Engines */ abstract class PhabricatorFileStorageEngine { @@ -188,12 +189,17 @@ abstract class PhabricatorFileStorageEngine { abstract public function deleteFile($handle); + +/* -( Loading Storage Engines )-------------------------------------------- */ + + /** * Select viable default storage engines according to configuration. We'll * select the MySQL and Local Disk storage engines if they are configured * to allow a given file. * * @param int File size in bytes. + * @task load */ public static function loadStorageEngines($length) { $engines = self::loadWritableEngines(); @@ -213,6 +219,10 @@ abstract class PhabricatorFileStorageEngine { return $writable; } + + /** + * @task load + */ public static function loadAllEngines() { static $engines; @@ -246,28 +256,72 @@ abstract class PhabricatorFileStorageEngine { return $engines; } - public static function loadWritableEngines() { + + /** + * @task load + */ + private static function loadProductionEngines() { $engines = self::loadAllEngines(); - $writable = array(); + $active = array(); foreach ($engines as $key => $engine) { if ($engine->isTestEngine()) { continue; } + $active[$key] = $engine; + } + + return $active; + } + + + /** + * @task load + */ + public static function loadWritableEngines() { + $engines = self::loadProductionEngines(); + + $writable = array(); + foreach ($engines as $key => $engine) { if (!$engine->canWriteFiles()) { continue; } + if ($engine->isChunkEngine()) { + // Don't select chunk engines as writable. + continue; + } $writable[$key] = $engine; } return $writable; } + /** + * @task load + */ + public static function loadWritableChunkEngines() { + $engines = self::loadProductionEngines(); + + $chunk = array(); + foreach ($engines as $key => $engine) { + if (!$engine->canWriteFiles()) { + continue; + } + if (!$engine->isChunkEngine()) { + continue; + } + $chunk[$key] = $engine; + } + + return $chunk; + } + + /** - * Return the largest file size which can be uploaded without chunking. + * Return the largest file size which can not be uploaded in chunks. * * Files smaller than this will always upload in one request, so clients * can safely skip the allocation step. @@ -275,14 +329,10 @@ abstract class PhabricatorFileStorageEngine { * @return int|null Byte size, or `null` if there is no chunk support. */ public static function getChunkThreshold() { - $engines = self::loadWritableEngines(); + $engines = self::loadWritableChunkEngines(); $min = null; foreach ($engines as $engine) { - if (!$engine->isChunkEngine()) { - continue; - } - if (!$min) { $min = $engine; continue;