From aa4adf3ab870919e109544a8f7c96cd702754965 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 13 Mar 2015 11:30:24 -0700 Subject: [PATCH] Add support for partially uploaded files Summary: Ref T7149. This flags allocated but incomplete files and doesn't explode when trying to download them. Files are marked complete when the last chunk is uploaded. I added a key on `` so we can show you a list of partially uploaded files and prompt you to resume them at some point down the road. Test Plan: Massaged debugging settings and uploaded README.md very slowly in 32b chunks. Saw the file lose its "Partial" flag when the last chunk finished. Reviewers: btrahan Reviewed By: btrahan Subscribers: joshuaspence, epriestley Maniphest Tasks: T7149 Differential Revision: https://secure.phabricator.com/D12063 --- .../sql/autopatches/20150312.filechunk.2.sql | 2 + .../sql/autopatches/20150312.filechunk.3.sql | 2 + .../conduit/FileAllocateConduitAPIMethod.php | 6 +- .../files/conduit/FileConduitAPIMethod.php | 11 +++ .../FileUploadChunkConduitAPIMethod.php | 9 +- .../PhabricatorFileDataController.php | 97 +++++++++++-------- .../PhabricatorFileInfoController.php | 26 ++++- .../files/query/PhabricatorFileChunkQuery.php | 18 ++++ .../files/storage/PhabricatorFile.php | 23 +++-- 9 files changed, 136 insertions(+), 58 deletions(-) create mode 100644 resources/sql/autopatches/20150312.filechunk.2.sql create mode 100644 resources/sql/autopatches/20150312.filechunk.3.sql diff --git a/resources/sql/autopatches/20150312.filechunk.2.sql b/resources/sql/autopatches/20150312.filechunk.2.sql new file mode 100644 index 0000000000..a6bb5bf8ad --- /dev/null +++ b/resources/sql/autopatches/20150312.filechunk.2.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_file.file + ADD isPartial BOOL NOT NULL DEFAULT 0; diff --git a/resources/sql/autopatches/20150312.filechunk.3.sql b/resources/sql/autopatches/20150312.filechunk.3.sql new file mode 100644 index 0000000000..82032692f8 --- /dev/null +++ b/resources/sql/autopatches/20150312.filechunk.3.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_file.file + ADD KEY `key_partial` (authorPHID, isPartial); diff --git a/src/applications/files/conduit/FileAllocateConduitAPIMethod.php b/src/applications/files/conduit/FileAllocateConduitAPIMethod.php index 65d2602287..00e31b5ed1 100644 --- a/src/applications/files/conduit/FileAllocateConduitAPIMethod.php +++ b/src/applications/files/conduit/FileAllocateConduitAPIMethod.php @@ -53,7 +53,7 @@ final class FileAllocateConduitAPIMethod $hash, $properties); - if ($file && !$force_chunking) { + if ($file) { return array( 'upload' => false, 'filePHID' => $file->getPHID(), @@ -70,7 +70,7 @@ final class FileAllocateConduitAPIMethod if ($file) { return array( - 'upload' => $file->isPartial(), + 'upload' => (bool)$file->getIsPartial(), 'filePHID' => $file->getPHID(), ); } @@ -103,7 +103,7 @@ final class FileAllocateConduitAPIMethod // Otherwise, this is a large file and we need to perform a chunked // upload. - $chunk_properties = array(); + $chunk_properties = $properties; if ($hash) { $chunk_properties += array( diff --git a/src/applications/files/conduit/FileConduitAPIMethod.php b/src/applications/files/conduit/FileConduitAPIMethod.php index bdcae6de91..2419303218 100644 --- a/src/applications/files/conduit/FileConduitAPIMethod.php +++ b/src/applications/files/conduit/FileConduitAPIMethod.php @@ -89,6 +89,16 @@ abstract class FileConduitAPIMethod extends ConduitAPIMethod { return $data; } + protected function loadAnyMissingChunk( + PhabricatorUser $viewer, + PhabricatorFile $file) { + + return $this->newChunkQuery($viewer, $file) + ->withIsComplete(false) + ->setLimit(1) + ->execute(); + } + private function newChunkQuery( PhabricatorUser $viewer, PhabricatorFile $file) { @@ -106,4 +116,5 @@ abstract class FileConduitAPIMethod extends ConduitAPIMethod { ->withChunkHandles(array($file->getStorageHandle())); } + } diff --git a/src/applications/files/conduit/FileUploadChunkConduitAPIMethod.php b/src/applications/files/conduit/FileUploadChunkConduitAPIMethod.php index 25dc115698..c26ab42124 100644 --- a/src/applications/files/conduit/FileUploadChunkConduitAPIMethod.php +++ b/src/applications/files/conduit/FileUploadChunkConduitAPIMethod.php @@ -57,16 +57,19 @@ final class FileUploadChunkConduitAPIMethod // NOTE: These files have a view policy which prevents normal access. They // are only accessed through the storage engine. - $file = PhabricatorFile::newFromFileData( + $chunk_data = PhabricatorFile::newFromFileData( $data, array( 'name' => $file->getMonogram().'.chunk-'.$chunk->getID(), 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, )); - $chunk->setDataFilePHID($file->getPHID())->save(); + $chunk->setDataFilePHID($chunk_data->getPHID())->save(); - // TODO: If all chunks are up, mark the file as complete. + $missing = $this->loadAnyMissingChunk($viewer, $file); + if (!$missing) { + $file->setIsPartial(0)->save(); + } return null; } diff --git a/src/applications/files/controller/PhabricatorFileDataController.php b/src/applications/files/controller/PhabricatorFileDataController.php index 9049a45c4c..6d793fa7b3 100644 --- a/src/applications/files/controller/PhabricatorFileDataController.php +++ b/src/applications/files/controller/PhabricatorFileDataController.php @@ -5,6 +5,7 @@ final class PhabricatorFileDataController extends PhabricatorFileController { private $phid; private $key; private $token; + private $file; public function willProcessRequest(array $data) { $this->phid = $data['phid']; @@ -16,20 +17,9 @@ final class PhabricatorFileDataController extends PhabricatorFileController { return false; } - protected function checkFileAndToken($file) { - if (!$file) { - return new Aphront404Response(); - } - - if (!$file->validateSecretKey($this->key)) { - return new Aphront403Response(); - } - - return null; - } - public function processRequest() { $request = $this->getRequest(); + $viewer = $this->getViewer(); $alt = PhabricatorEnv::getEnvConfig('security.alternate-file-domain'); $base_uri = PhabricatorEnv::getEnvConfig('phabricator.base-uri'); @@ -44,32 +34,22 @@ final class PhabricatorFileDataController extends PhabricatorFileController { // Alternate files domain isn't configured or it's set // to the same as the default domain - // load the file with permissions checks; - $file = id(new PhabricatorFileQuery()) - ->setViewer($request->getUser()) - ->withPHIDs(array($this->phid)) - ->executeOne(); - - $error_response = $this->checkFileAndToken($file); - if ($error_response) { - return $error_response; + $response = $this->loadFile($viewer); + if ($response) { + return $response; } + $file = $this->getFile(); // when the file is not CDNable, don't allow cache $cache_response = $file->getCanCDN(); } else if ($req_domain != $alt_domain) { // Alternate domain is configured but this request isn't using it - // load the file with permissions checks; - $file = id(new PhabricatorFileQuery()) - ->setViewer($request->getUser()) - ->withPHIDs(array($this->phid)) - ->executeOne(); - - $error_response = $this->checkFileAndToken($file); - if ($error_response) { - return $error_response; + $response = $this->loadFile($viewer); + if ($response) { + return $response; } + $file = $this->getFile(); // if the user can see the file, generate a token; // redirect to the alt domain with the token; @@ -82,18 +62,15 @@ final class PhabricatorFileDataController extends PhabricatorFileController { ->setURI($token_uri); } else { - // We are using the alternate domain + // We are using the alternate domain. We don't have authentication + // on this domain, so we bypass policy checks when loading the file. - // load the file, bypassing permission checks; - $file = id(new PhabricatorFileQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withPHIDs(array($this->phid)) - ->executeOne(); - - $error_response = $this->checkFileAndToken($file); - if ($error_response) { - return $error_response; + $bypass_policies = PhabricatorUser::getOmnipotentUser(); + $response = $this->loadFile($bypass_policies); + if ($response) { + return $response; } + $file = $this->getFile(); $acquire_token_uri = id(new PhutilURI($file->getViewURI())) ->setDomain($main_domain); @@ -205,4 +182,44 @@ final class PhabricatorFileDataController extends PhabricatorFileController { return $uri; } + private function loadFile(PhabricatorUser $viewer) { + $file = id(new PhabricatorFileQuery()) + ->setViewer($viewer) + ->withPHIDs(array($this->phid)) + ->executeOne(); + + if (!$file) { + return new Aphront404Response(); + } + + if (!$file->validateSecretKey($this->key)) { + return new Aphront403Response(); + } + + if ($file->getIsPartial()) { + // We may be on the CDN domain, so we need to use a fully-qualified URI + // here to make sure we end up back on the main domain. + $info_uri = PhabricatorEnv::getURI($file->getInfoURI()); + + return $this->newDialog() + ->setTitle(pht('Partial Upload')) + ->appendParagraph( + pht( + 'This file has only been partially uploaded. It must be '. + 'uploaded completely before you can download it.')) + ->addCancelButton($info_uri); + } + + $this->file = $file; + + return null; + } + + private function getFile() { + if (!$this->file) { + throw new Exception(pht('Call loadFile() before getFile()!')); + } + return $this->file; + } + } diff --git a/src/applications/files/controller/PhabricatorFileInfoController.php b/src/applications/files/controller/PhabricatorFileInfoController.php index 58f315f067..27a67005d9 100644 --- a/src/applications/files/controller/PhabricatorFileInfoController.php +++ b/src/applications/files/controller/PhabricatorFileInfoController.php @@ -52,11 +52,21 @@ final class PhabricatorFileInfoController extends PhabricatorFileController { $ttl = $file->getTTL(); if ($ttl !== null) { $ttl_tag = id(new PHUITagView()) - ->setType(PHUITagView::TYPE_OBJECT) + ->setType(PHUITagView::TYPE_STATE) + ->setBackgroundColor(PHUITagView::COLOR_YELLOW) ->setName(pht('Temporary')); $header->addTag($ttl_tag); } + $partial = $file->getIsPartial(); + if ($partial) { + $partial_tag = id(new PHUITagView()) + ->setType(PHUITagView::TYPE_STATE) + ->setBackgroundColor(PHUITagView::COLOR_ORANGE) + ->setName(pht('Partial Upload')); + $header->addTag($partial_tag); + } + $actions = $this->buildActionView($file); $timeline = $this->buildTransactionView($file); $crumbs = $this->buildApplicationCrumbs(); @@ -126,21 +136,27 @@ final class PhabricatorFileInfoController extends PhabricatorFileController { ->setObjectURI($this->getRequest()->getRequestURI()) ->setObject($file); + $can_download = !$file->getIsPartial(); + if ($file->isViewableInBrowser()) { $view->addAction( id(new PhabricatorActionView()) ->setName(pht('View File')) ->setIcon('fa-file-o') - ->setHref($file->getViewURI())); + ->setHref($file->getViewURI()) + ->setDisabled(!$can_download) + ->setWorkflow(!$can_download)); } else { $view->addAction( id(new PhabricatorActionView()) ->setUser($viewer) - ->setRenderAsForm(true) - ->setDownload(true) + ->setRenderAsForm($can_download) + ->setDownload($can_download) ->setName(pht('Download File')) ->setIcon('fa-download') - ->setHref($file->getViewURI())); + ->setHref($file->getViewURI()) + ->setDisabled(!$can_download) + ->setWorkflow(!$can_download)); } $view->addAction( diff --git a/src/applications/files/query/PhabricatorFileChunkQuery.php b/src/applications/files/query/PhabricatorFileChunkQuery.php index e701c779ce..7c2a961fd0 100644 --- a/src/applications/files/query/PhabricatorFileChunkQuery.php +++ b/src/applications/files/query/PhabricatorFileChunkQuery.php @@ -6,6 +6,7 @@ final class PhabricatorFileChunkQuery private $chunkHandles; private $rangeStart; private $rangeEnd; + private $isComplete; private $needDataFiles; public function withChunkHandles(array $handles) { @@ -19,6 +20,11 @@ final class PhabricatorFileChunkQuery return $this; } + public function withIsComplete($complete) { + $this->isComplete = $complete; + return $this; + } + public function needDataFiles($need) { $this->needDataFiles = $need; return $this; @@ -104,6 +110,18 @@ final class PhabricatorFileChunkQuery $this->rangeEnd); } + if ($this->isComplete !== null) { + if ($this->isComplete) { + $where[] = qsprintf( + $conn_r, + 'dataFilePHID IS NOT NULL'); + } else { + $where[] = qsprintf( + $conn_r, + 'dataFilePHID IS NULL'); + } + } + $where[] = $this->buildPagingClause($conn_r); return $this->formatWhereClause($where); diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index 4b73934df5..b75df473a1 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -51,6 +51,7 @@ final class PhabricatorFile extends PhabricatorFileDAO protected $ttl; protected $isExplicitUpload = 1; protected $viewPolicy = PhabricatorPolicies::POLICY_USER; + protected $isPartial = 0; private $objects = self::ATTACHABLE; private $objectPHIDs = self::ATTACHABLE; @@ -67,6 +68,7 @@ final class PhabricatorFile extends PhabricatorFileDAO return id(new PhabricatorFile()) ->setViewPolicy($view_policy) + ->setIsPartial(0) ->attachOriginalFile(null) ->attachObjects(array()) ->attachObjectPHIDs(array()); @@ -91,6 +93,7 @@ final class PhabricatorFile extends PhabricatorFileDAO 'ttl' => 'epoch?', 'isExplicitUpload' => 'bool?', 'mailKey' => 'bytes20', + 'isPartial' => 'bool', ), self::CONFIG_KEY_SCHEMA => array( 'key_phid' => null, @@ -110,6 +113,9 @@ final class PhabricatorFile extends PhabricatorFileDAO 'key_dateCreated' => array( 'columns' => array('dateCreated'), ), + 'key_partial' => array( + 'columns' => array('authorPHID', 'isPartial'), + ), ), ) + parent::getConfiguration(); } @@ -294,6 +300,7 @@ final class PhabricatorFile extends PhabricatorFileDAO $file->setStorageEngine($engine->getEngineIdentifier()); $file->setStorageHandle(PhabricatorFileChunk::newChunkHandle()); $file->setStorageFormat(self::STORAGE_FORMAT_RAW); + $file->setIsPartial(1); $file->readPropertiesFromParameters($params); @@ -628,9 +635,16 @@ final class PhabricatorFile extends PhabricatorFileDAO } $parts[] = $name; - $path = implode('/', $parts); + $path = '/'.implode('/', $parts); - return PhabricatorEnv::getCDNURI($path); + // If this file is only partially uploaded, we're just going to return a + // local URI to make sure that Ajax works, since the page is inevitably + // going to give us an error back. + if ($this->getIsPartial()) { + return PhabricatorEnv::getURI($path); + } else { + return PhabricatorEnv::getCDNURI($path); + } } /** @@ -1170,11 +1184,6 @@ final class PhabricatorFile extends PhabricatorFileDAO ->setURI($uri); } - public function isPartial() { - // TODO: Placeholder for resumable uploads. - return false; - } - /* -( PhabricatorApplicationTransactionInterface )------------------------- */