From d1fb2f7fb96da77f56caa039f69d047355e2a7fa Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 7 Jan 2016 03:50:16 -0800 Subject: [PATCH] Make `diffusion.filecontentquery` return file PHIDs instead of raw content Summary: Fixes T9319. Proxied requests (e.g., in the cluster) for binary files (like images) currently fail because we can not return binary data over Conduit in JSON. Although Conduit will eventually support binary-safe encodings, a cleaner approach to this is just to return a `filePHID` instead of the raw content. This is generally faster and more flexible, and gives us more opportunities to add caching later. After making the call, the client pulls the file data separately. We also no longer need to return a complex data structure because we don't do blame over this call any longer. Test Plan: - Viewed images in Diffusion. - Viewed READMEs in Diffusion. - Used `bin/differential attach-commit rX Dy` to hit attach pathway. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9319 Differential Revision: https://secure.phabricator.com/D14970 --- src/__phutil_library_map__.php | 2 - .../DifferentialDiffExtractionEngine.php | 16 ++- ...fusionFileContentQueryConduitAPIMethod.php | 45 +++++-- .../controller/DiffusionBrowseController.php | 124 +++++++----------- .../controller/DiffusionController.php | 45 +++++++ .../DiffusionRepositoryController.php | 19 +-- .../diffusion/data/DiffusionFileContent.php | 63 --------- .../filecontent/DiffusionFileContentQuery.php | 79 ++++------- .../DiffusionGitFileContentQuery.php | 10 +- .../DiffusionMercurialFileContentQuery.php | 10 +- .../DiffusionSvnFileContentQuery.php | 29 +--- 11 files changed, 186 insertions(+), 256 deletions(-) delete mode 100644 src/applications/diffusion/data/DiffusionFileContent.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9639eef11e..c73cec5965 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -598,7 +598,6 @@ phutil_register_library_map(array( 'DiffusionExternalController' => 'applications/diffusion/controller/DiffusionExternalController.php', 'DiffusionExternalSymbolQuery' => 'applications/diffusion/symbol/DiffusionExternalSymbolQuery.php', 'DiffusionExternalSymbolsSource' => 'applications/diffusion/symbol/DiffusionExternalSymbolsSource.php', - 'DiffusionFileContent' => 'applications/diffusion/data/DiffusionFileContent.php', 'DiffusionFileContentQuery' => 'applications/diffusion/query/filecontent/DiffusionFileContentQuery.php', 'DiffusionFileContentQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionFileContentQueryConduitAPIMethod.php', 'DiffusionFindSymbolsConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionFindSymbolsConduitAPIMethod.php', @@ -4571,7 +4570,6 @@ phutil_register_library_map(array( 'DiffusionExternalController' => 'DiffusionController', 'DiffusionExternalSymbolQuery' => 'Phobject', 'DiffusionExternalSymbolsSource' => 'Phobject', - 'DiffusionFileContent' => 'Phobject', 'DiffusionFileContentQuery' => 'DiffusionQuery', 'DiffusionFileContentQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', 'DiffusionFindSymbolsConduitAPIMethod' => 'DiffusionConduitAPIMethod', diff --git a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php index b6d9d11953..0fc6391987 100644 --- a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php +++ b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php @@ -156,9 +156,21 @@ final class DifferentialDiffExtractionEngine extends Phobject { 'commit' => $identifier, 'path' => $path, )); - $corpus = $response['corpus']; - if ($files[$file_phid]->loadFileData() != $corpus) { + $new_file_phid = $response['filePHID']; + if (!$new_file_phid) { + return true; + } + + $new_file = id(new PhabricatorFileQuery()) + ->setViewer($viewer) + ->withPHIDs(array($new_file_phid)) + ->executeOne(); + if (!$new_file) { + return true; + } + + if ($files[$file_phid]->loadFileData() != $new_file->loadFileData()) { return true; } } else { diff --git a/src/applications/diffusion/conduit/DiffusionFileContentQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionFileContentQueryConduitAPIMethod.php index 73ed4161fe..8088aa66b8 100644 --- a/src/applications/diffusion/conduit/DiffusionFileContentQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionFileContentQueryConduitAPIMethod.php @@ -27,8 +27,7 @@ final class DiffusionFileContentQueryConduitAPIMethod protected function getResult(ConduitAPIRequest $request) { $drequest = $this->getDiffusionRequest(); - $file_query = DiffusionFileContentQuery::newFromDiffusionRequest($drequest) - ->setViewer($request->getUser()); + $file_query = DiffusionFileContentQuery::newFromDiffusionRequest($drequest); $timeout = $request->getValue('timeout'); if ($timeout) { @@ -40,16 +39,44 @@ final class DiffusionFileContentQueryConduitAPIMethod $file_query->setByteLimit($byte_limit); } - $file_content = $file_query->loadFileContent(); + $content = $file_query->execute(); - $text_list = $rev_list = $blame_dict = array(); + $too_slow = (bool)$file_query->getExceededTimeLimit(); + $too_huge = (bool)$file_query->getExceededByteLimit(); - $file_content - ->setBlameDict($blame_dict) - ->setRevList($rev_list) - ->setTextList($text_list); + $file_phid = null; + if (!$too_slow && !$too_huge) { + $file = $this->newFile($drequest, $content); + $file_phid = $file->getPHID(); + } - return $file_content->toDictionary(); + return array( + 'tooSlow' => $too_slow, + 'tooHuge' => $too_huge, + 'filePHID' => $file_phid, + ); + } + + private function newFile(DiffusionRequest $drequest, $content) { + $path = $drequest->getPath(); + $name = basename($path); + + $repository = $drequest->getRepository(); + $repository_phid = $repository->getPHID(); + + $file = PhabricatorFile::buildFromFileDataOrHash( + $content, + array( + 'name' => $name, + 'ttl' => time() + phutil_units('48 hours in seconds'), + 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, + )); + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $file->attachToObject($repository_phid); + unset($unguarded); + + return $file; } } diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index e61f24173a..eef95bb1ee 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -153,44 +153,62 @@ final class DiffusionBrowseController extends DiffusionController { ); } - $file_content = DiffusionFileContent::newFromConduit( - $this->callConduitWithDiffusionRequest( - 'diffusion.filecontentquery', - $params)); - $data = $file_content->getCorpus(); + $response = $this->callConduitWithDiffusionRequest( + 'diffusion.filecontentquery', + $params); - if ($view === 'raw') { - return $this->buildRawResponse($path, $data); - } + $hit_byte_limit = $response['tooHuge']; + $hit_time_limit = $response['tooSlow']; - $this->loadLintMessages(); - $this->coverage = $drequest->loadCoverage(); - - if ($byte_limit && (strlen($data) == $byte_limit)) { + $file_phid = $response['filePHID']; + if ($hit_byte_limit) { $corpus = $this->buildErrorCorpus( pht( 'This file is larger than %s byte(s), and too large to display '. 'in the web UI.', - $byte_limit)); - } else if (ArcanistDiffUtils::isHeuristicBinaryFile($data)) { - $file = $this->loadFileForData($path, $data); - $file_uri = $file->getBestURI(); - - if ($file->isViewableImage()) { - $corpus = $this->buildImageCorpus($file_uri); - } else { - $corpus = $this->buildBinaryCorpus($file_uri, $data); - } + phutil_format_bytes($byte_limit))); + } else if ($hit_time_limit) { + $corpus = $this->buildErrorCorpus( + pht( + 'This file took too long to load from the repository (more than '. + '%s second(s)).', + new PhutilNumber($time_limit))); } else { - // Build the content of the file. - $corpus = $this->buildCorpus( - $show_blame, - $show_color, - $file_content, - $needs_blame, - $drequest, - $path, - $data); + $file = id(new PhabricatorFileQuery()) + ->setViewer($viewer) + ->withPHIDs(array($file_phid)) + ->executeOne(); + if (!$file) { + throw new Exception(pht('Failed to load content file!')); + } + + if ($view === 'raw') { + return $file->getRedirectResponse(); + } + + $data = $file->loadFileData(); + if (ArcanistDiffUtils::isHeuristicBinaryFile($data)) { + $file_uri = $file->getBestURI(); + + if ($file->isViewableImage()) { + $corpus = $this->buildImageCorpus($file_uri); + } else { + $corpus = $this->buildBinaryCorpus($file_uri, $data); + } + } else { + $this->loadLintMessages(); + $this->coverage = $drequest->loadCoverage(); + + // Build the content of the file. + $corpus = $this->buildCorpus( + $show_blame, + $show_color, + $data, + $needs_blame, + $drequest, + $path, + $data); + } } if ($request->isAjax()) { @@ -327,23 +345,7 @@ final class DiffusionBrowseController extends DiffusionController { } $content[] = $this->buildOpenRevisions(); - - - $readme_path = $results->getReadmePath(); - if ($readme_path) { - $readme_content = $this->callConduitWithDiffusionRequest( - 'diffusion.filecontentquery', - array( - 'path' => $readme_path, - 'commit' => $drequest->getStableCommit(), - )); - if ($readme_content) { - $content[] = id(new DiffusionReadmeView()) - ->setUser($this->getViewer()) - ->setPath($readme_path) - ->setContent($readme_content['corpus']); - } - } + $content[] = $this->renderDirectoryReadme($results); $crumbs = $this->buildCrumbs( array( @@ -584,7 +586,7 @@ final class DiffusionBrowseController extends DiffusionController { private function buildCorpus( $show_blame, $show_color, - DiffusionFileContent $file_content, + $file_corpus, $needs_blame, DiffusionRequest $drequest, $path, @@ -594,7 +596,6 @@ final class DiffusionBrowseController extends DiffusionController { $blame_timeout = 15; $blame_failed = false; - $file_corpus = $file_content->getCorpus(); $highlight_limit = DifferentialChangesetParser::HIGHLIGHT_BYTE_LIMIT; $blame_limit = DifferentialChangesetParser::HIGHLIGHT_BYTE_LIMIT; $can_highlight = (strlen($file_corpus) <= $highlight_limit); @@ -1256,28 +1257,6 @@ final class DiffusionBrowseController extends DiffusionController { return $rows; } - private function loadFileForData($path, $data) { - $file = PhabricatorFile::buildFromFileDataOrHash( - $data, - array( - 'name' => basename($path), - 'ttl' => time() + 60 * 60 * 24, - 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, - )); - - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - $file->attachToObject( - $this->getDiffusionRequest()->getRepository()->getPHID()); - unset($unguarded); - - return $file; - } - - private function buildRawResponse($path, $data) { - $file = $this->loadFileForData($path, $data); - return $file->getRedirectResponse(); - } - private function buildImageCorpus($file_uri) { $properties = new PHUIPropertyListView(); @@ -1299,7 +1278,6 @@ final class DiffusionBrowseController extends DiffusionController { } private function buildBinaryCorpus($file_uri, $data) { - $size = new PhutilNumber(strlen($data)); $text = pht('This is a binary file. It is %s byte(s) in length.', $size); $text = id(new PHUIBoxView()) diff --git a/src/applications/diffusion/controller/DiffusionController.php b/src/applications/diffusion/controller/DiffusionController.php index e398a88114..a3c661a26d 100644 --- a/src/applications/diffusion/controller/DiffusionController.php +++ b/src/applications/diffusion/controller/DiffusionController.php @@ -287,4 +287,49 @@ abstract class DiffusionController extends PhabricatorController { ->appendChild($pager); } + protected function renderDirectoryReadme(DiffusionBrowseResultSet $browse) { + $readme_path = $browse->getReadmePath(); + if ($readme_path === null) { + return null; + } + + $drequest = $this->getDiffusionRequest(); + $viewer = $this->getViewer(); + + try { + $result = $this->callConduitWithDiffusionRequest( + 'diffusion.filecontentquery', + array( + 'path' => $readme_path, + 'commit' => $drequest->getStableCommit(), + )); + } catch (Exception $ex) { + return null; + } + + $file_phid = $result['filePHID']; + if (!$file_phid) { + return null; + } + + $file = id(new PhabricatorFileQuery()) + ->setViewer($viewer) + ->withPHIDs(array($file_phid)) + ->executeOne(); + if (!$file) { + return null; + } + + $corpus = $file->loadFileData(); + + if (!strlen($corpus)) { + return null; + } + + return id(new DiffusionReadmeView()) + ->setUser($this->getViewer()) + ->setPath($readme_path) + ->setContent($corpus); + } + } diff --git a/src/applications/diffusion/controller/DiffusionRepositoryController.php b/src/applications/diffusion/controller/DiffusionRepositoryController.php index dc6969b81d..9fc948d19a 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryController.php @@ -161,23 +161,10 @@ final class DiffusionRepositoryController extends DiffusionController { $phids = array_keys($phids); $handles = $this->loadViewerHandles($phids); - $readme = null; if ($browse_results) { - $readme_path = $browse_results->getReadmePath(); - if ($readme_path) { - $readme_content = $this->callConduitWithDiffusionRequest( - 'diffusion.filecontentquery', - array( - 'path' => $readme_path, - 'commit' => $drequest->getStableCommit(), - )); - if ($readme_content) { - $readme = id(new DiffusionReadmeView()) - ->setUser($this->getViewer()) - ->setPath($readme_path) - ->setContent($readme_content['corpus']); - } - } + $readme = $this->renderDirectoryReadme($browse_results); + } else { + $readme = null; } $content[] = $this->buildBrowseTable( diff --git a/src/applications/diffusion/data/DiffusionFileContent.php b/src/applications/diffusion/data/DiffusionFileContent.php deleted file mode 100644 index 1680d1f964..0000000000 --- a/src/applications/diffusion/data/DiffusionFileContent.php +++ /dev/null @@ -1,63 +0,0 @@ -textList = $text_list; - return $this; - } - public function getTextList() { - if (!$this->textList) { - return phutil_split_lines($this->getCorpus(), $retain_ends = false); - } - return $this->textList; - } - - public function setRevList(array $rev_list) { - $this->revList = $rev_list; - return $this; - } - public function getRevList() { - return $this->revList; - } - - public function setBlameDict(array $blame_dict) { - $this->blameDict = $blame_dict; - return $this; - } - public function getBlameDict() { - return $this->blameDict; - } - - public function setCorpus($corpus) { - $this->corpus = $corpus; - return $this; - } - - public function getCorpus() { - return $this->corpus; - } - - public function toDictionary() { - return array( - 'corpus' => $this->getCorpus(), - 'blameDict' => $this->getBlameDict(), - 'revList' => $this->getRevList(), - 'textList' => $this->getTextList(), - ); - } - - public static function newFromConduit(array $dict) { - return id(new DiffusionFileContent()) - ->setCorpus($dict['corpus']) - ->setBlameDict($dict['blameDict']) - ->setRevList($dict['revList']) - ->setTextList($dict['textList']); - } - -} diff --git a/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php index 6a9cf2e021..3ea0e12ba1 100644 --- a/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php @@ -1,18 +1,13 @@ timeout = $timeout; return $this; @@ -31,71 +26,51 @@ abstract class DiffusionFileContentQuery extends DiffusionQuery { return $this->byteLimit; } - public function setViewer(PhabricatorUser $user) { - $this->viewer = $user; - return $this; - } - - public function getViewer() { - return $this->viewer; - } - final public static function newFromDiffusionRequest( DiffusionRequest $request) { return parent::newQueryObject(__CLASS__, $request); } - abstract public function getFileContentFuture(); - abstract protected function executeQueryFromFuture(Future $future); + final public function getExceededByteLimit() { + return $this->didHitByteLimit; + } - final public function loadFileContentFromFuture(Future $future) { + final public function getExceededTimeLimit() { + return $this->didHitTimeLimit; + } - if ($this->timeout) { - $future->setTimeout($this->timeout); + abstract protected function getFileContentFuture(); + abstract protected function resolveFileContentFuture(Future $future); + + final protected function executeQuery() { + $future = $this->getFileContentFuture(); + + if ($this->getTimeout()) { + $future->setTimeout($this->getTimeout()); } - if ($this->getByteLimit()) { - $future->setStdoutSizeLimit($this->getByteLimit()); + $byte_limit = $this->getByteLimit(); + if ($byte_limit) { + $future->setStdoutSizeLimit($byte_limit + 1); } try { - $file_content = $this->executeQueryFromFuture($future); + $file_content = $this->resolveFileContentFuture($future); } catch (CommandException $ex) { if (!$future->getWasKilledByTimeout()) { throw $ex; } - $message = pht( - '', - $this->timeout); - - $file_content = new DiffusionFileContent(); - $file_content->setCorpus($message); + $this->didHitTimeLimit = true; + $file_content = null; } - $this->fileContent = $file_content; - - $repository = $this->getRequest()->getRepository(); - $try_encoding = $repository->getDetail('encoding'); - if ($try_encoding) { - $this->fileContent->setCorpus( - phutil_utf8_convert( - $this->fileContent->getCorpus(), 'UTF-8', $try_encoding)); + if ($byte_limit && (strlen($file_content) > $byte_limit)) { + $this->didHitByteLimit = true; + $file_content = null; } - return $this->fileContent; - } - - final protected function executeQuery() { - return $this->loadFileContentFromFuture($this->getFileContentFuture()); - } - - final public function loadFileContent() { - return $this->executeQuery(); - } - - final public function getRawData() { - return $this->fileContent->getCorpus(); + return $file_content; } } diff --git a/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php index 448d2886dc..a383aee922 100644 --- a/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php @@ -2,7 +2,7 @@ final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery { - public function getFileContentFuture() { + protected function getFileContentFuture() { $drequest = $this->getRequest(); $repository = $drequest->getRepository(); @@ -15,13 +15,9 @@ final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery { $path); } - protected function executeQueryFromFuture(Future $future) { + protected function resolveFileContentFuture(Future $future) { list($corpus) = $future->resolvex(); - - $file_content = new DiffusionFileContent(); - $file_content->setCorpus($corpus); - - return $file_content; + return $corpus; } } diff --git a/src/applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php index ec49414666..539f5752c2 100644 --- a/src/applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php @@ -3,7 +3,7 @@ final class DiffusionMercurialFileContentQuery extends DiffusionFileContentQuery { - public function getFileContentFuture() { + protected function getFileContentFuture() { $drequest = $this->getRequest(); $repository = $drequest->getRepository(); @@ -16,13 +16,9 @@ final class DiffusionMercurialFileContentQuery $path); } - protected function executeQueryFromFuture(Future $future) { + protected function resolveFileContentFuture(Future $future) { list($corpus) = $future->resolvex(); - - $file_content = new DiffusionFileContent(); - $file_content->setCorpus($corpus); - - return $file_content; + return $corpus; } } diff --git a/src/applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php index 4976d7a27b..0f6f30b355 100644 --- a/src/applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php @@ -2,7 +2,7 @@ final class DiffusionSvnFileContentQuery extends DiffusionFileContentQuery { - public function getFileContentFuture() { + protected function getFileContentFuture() { $drequest = $this->getRequest(); $repository = $drequest->getRepository(); @@ -14,30 +14,9 @@ final class DiffusionSvnFileContentQuery extends DiffusionFileContentQuery { $repository->getSubversionPathURI($path, $commit)); } - protected function executeQueryFromFuture(Future $future) { - try { - list($corpus) = $future->resolvex(); - } catch (CommandException $ex) { - $stderr = $ex->getStdErr(); - if (preg_match('/path not found$/', trim($stderr))) { - // TODO: Improve user experience for this. One way to end up here - // is to have the parser behind and look at a file which was recently - // nuked; Diffusion will think it still exists and try to grab content - // at HEAD. - throw new Exception( - pht( - 'Failed to retrieve file content from Subversion. The file may '. - 'have been recently deleted, or the Diffusion cache may be out of '. - 'date.')); - } else { - throw $ex; - } - } - - $file_content = new DiffusionFileContent(); - $file_content->setCorpus($corpus); - - return $file_content; + protected function resolveFileContentFuture(Future $future) { + list($corpus) = $future->resolvex(); + return $corpus; } }