From 771579496fc1fe676de23aa199338388ab6bbab2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 26 Aug 2016 15:04:30 +0000 Subject: [PATCH] Make logic for streaming VCS stuff directly to Files more reusable Summary: Ref T11524. Ref T10423. Earlier, I converted `diffusion.filecontentquery` to put the actual file content in Files, then return a PHID for the file, instead of trying to send the content over Conduit. In T11524, we have a similar set of problems with diffs that contain non-UTF8 data (and, in T10423, diffs that are simply enormous). I want to provide an API method to do the same sort of thing with diff output (like from `git diff`), so we call the method, it shoves the data in Files, and then we go pull it out of Files. To support this, take the "shove the output of a Future into Files" logic and put it in a new base `FileFuture` query. This will let me make `RawDiffQuery` share the logic more easily. Test Plan: Browsed Diffusion, ran `diffusion.filecontentquery` to fetch file content. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10423, T11524 Differential Revision: https://secure.phabricator.com/D16458 --- src/__phutil_library_map__.php | 4 +++- src/applications/diffusion/conduit/DiffusionFileContentQueryConduitAPIMethod.php | 40 +++------------------------------------- src/applications/diffusion/query/DiffusionFileFutureQuery.php | 153 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php | 92 ++------------------------------------------------------------------------------------------ src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php | 5 ----- src/applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php | 5 ----- src/applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php | 5 ----- 7 file(s) changed, 161 insertion(s)(+), 143 deletion(s)(-) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -656,6 +656,7 @@ 'DiffusionExternalSymbolsSource' => 'applications/diffusion/symbol/DiffusionExternalSymbolsSource.php', 'DiffusionFileContentQuery' => 'applications/diffusion/query/filecontent/DiffusionFileContentQuery.php', 'DiffusionFileContentQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionFileContentQueryConduitAPIMethod.php', + 'DiffusionFileFutureQuery' => 'applications/diffusion/query/DiffusionFileFutureQuery.php', 'DiffusionFindSymbolsConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionFindSymbolsConduitAPIMethod.php', 'DiffusionGetLintMessagesConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionGetLintMessagesConduitAPIMethod.php', 'DiffusionGetRecentCommitsByPathConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionGetRecentCommitsByPathConduitAPIMethod.php', @@ -5149,8 +5150,9 @@ 'DiffusionExternalController' => 'DiffusionController', 'DiffusionExternalSymbolQuery' => 'Phobject', 'DiffusionExternalSymbolsSource' => 'Phobject', - 'DiffusionFileContentQuery' => 'DiffusionQuery', + 'DiffusionFileContentQuery' => 'DiffusionFileFutureQuery', 'DiffusionFileContentQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', + 'DiffusionFileFutureQuery' => 'DiffusionQuery', 'DiffusionFindSymbolsConduitAPIMethod' => 'DiffusionConduitAPIMethod', 'DiffusionGetLintMessagesConduitAPIMethod' => 'DiffusionConduitAPIMethod', 'DiffusionGetRecentCommitsByPathConduitAPIMethod' => 'DiffusionConduitAPIMethod', diff --git a/src/applications/diffusion/conduit/DiffusionFileContentQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionFileContentQueryConduitAPIMethod.php --- a/src/applications/diffusion/conduit/DiffusionFileContentQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionFileContentQueryConduitAPIMethod.php @@ -19,48 +19,14 @@ return array( 'path' => 'required string', 'commit' => 'required string', - 'timeout' => 'optional int', - 'byteLimit' => 'optional int', - ); + ) + DiffusionFileFutureQuery::getConduitParameters(); } protected function getResult(ConduitAPIRequest $request) { $drequest = $this->getDiffusionRequest(); - $file_query = DiffusionFileContentQuery::newFromDiffusionRequest($drequest); - - $timeout = $request->getValue('timeout'); - if ($timeout) { - $file_query->setTimeout($timeout); - } - - $byte_limit = $request->getValue('byteLimit'); - if ($byte_limit) { - $file_query->setByteLimit($byte_limit); - } - - $file = $file_query->execute(); - - $too_slow = (bool)$file_query->getExceededTimeLimit(); - $too_huge = (bool)$file_query->getExceededByteLimit(); - - $file_phid = null; - if (!$too_slow && !$too_huge) { - $repository = $drequest->getRepository(); - $repository_phid = $repository->getPHID(); - - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - $file->attachToObject($repository_phid); - unset($unguarded); - - $file_phid = $file->getPHID(); - } - - return array( - 'tooSlow' => $too_slow, - 'tooHuge' => $too_huge, - 'filePHID' => $file_phid, - ); + return DiffusionFileContentQuery::newFromDiffusionRequest($drequest) + ->respondToConduitRequest($request); } } diff --git a/src/applications/diffusion/query/DiffusionFileFutureQuery.php b/src/applications/diffusion/query/DiffusionFileFutureQuery.php new file mode 100644 --- /dev/null +++ b/src/applications/diffusion/query/DiffusionFileFutureQuery.php @@ -0,0 +1,153 @@ + 'optional int', + 'byteLimit' => 'optional int', + ); + } + + public function setTimeout($timeout) { + $this->timeout = $timeout; + return $this; + } + + public function getTimeout() { + return $this->timeout; + } + + public function setByteLimit($byte_limit) { + $this->byteLimit = $byte_limit; + return $this; + } + + public function getByteLimit() { + return $this->byteLimit; + } + + final public function getExceededByteLimit() { + return $this->didHitByteLimit; + } + + final public function getExceededTimeLimit() { + return $this->didHitTimeLimit; + } + + abstract protected function getFileContentFuture(); + + final public function respondToConduitRequest(ConduitAPIRequest $request) { + $drequest = $this->getRequest(); + + $timeout = $request->getValue('timeout'); + if ($timeout) { + $this->setTimeout($timeout); + } + + $byte_limit = $request->getValue('byteLimit'); + if ($byte_limit) { + $this->setByteLimit($byte_limit); + } + + $file = $this->execute(); + + $too_slow = (bool)$this->getExceededTimeLimit(); + $too_huge = (bool)$this->getExceededByteLimit(); + + $file_phid = null; + if (!$too_slow && !$too_huge) { + $repository = $drequest->getRepository(); + $repository_phid = $repository->getPHID(); + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $file->attachToObject($repository_phid); + unset($unguarded); + + $file_phid = $file->getPHID(); + } + + return array( + 'tooSlow' => $too_slow, + 'tooHuge' => $too_huge, + 'filePHID' => $file_phid, + ); + } + + final public function executeInline() { + $future = $this->getFileContentFuture(); + list($stdout) = $future->resolvex(); + return $future; + } + + final protected function executeQuery() { + $future = $this->newConfiguredFileContentFuture(); + + $drequest = $this->getRequest(); + + $name = basename($drequest->getPath()); + $ttl = PhabricatorTime::getNow() + phutil_units('48 hours in seconds'); + + try { + $threshold = PhabricatorFileStorageEngine::getChunkThreshold(); + $future->setReadBufferSize($threshold); + + $source = id(new PhabricatorExecFutureFileUploadSource()) + ->setName($name) + ->setTTL($ttl) + ->setViewPolicy(PhabricatorPolicies::POLICY_NOONE) + ->setExecFuture($future); + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $file = $source->uploadFile(); + unset($unguarded); + + } catch (CommandException $ex) { + if (!$future->getWasKilledByTimeout()) { + throw $ex; + } + + $this->didHitTimeLimit = true; + $file = null; + } + + $byte_limit = $this->getByteLimit(); + + if ($byte_limit && ($file->getByteSize() > $byte_limit)) { + $this->didHitByteLimit = true; + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + id(new PhabricatorDestructionEngine()) + ->destroyObject($file); + unset($unguarded); + + $file = null; + } + + return $file; + } + + private function newConfiguredFileContentFuture() { + $future = $this->getFileContentFuture(); + + if ($this->getTimeout()) { + $future->setTimeout($this->getTimeout()); + } + + $byte_limit = $this->getByteLimit(); + if ($byte_limit) { + $future->setStdoutSizeLimit($byte_limit + 1); + } + + return $future; + } + + +} diff --git a/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php --- a/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php @@ -1,99 +1,11 @@ timeout = $timeout; - return $this; - } - - public function getTimeout() { - return $this->timeout; - } - - public function setByteLimit($byte_limit) { - $this->byteLimit = $byte_limit; - return $this; - } - - public function getByteLimit() { - return $this->byteLimit; - } +abstract class DiffusionFileContentQuery + extends DiffusionFileFutureQuery { final public static function newFromDiffusionRequest( DiffusionRequest $request) { return parent::newQueryObject(__CLASS__, $request); - } - - final public function getExceededByteLimit() { - return $this->didHitByteLimit; - } - - final public function getExceededTimeLimit() { - return $this->didHitTimeLimit; - } - - abstract protected function getFileContentFuture(); - abstract protected function resolveFileContentFuture(Future $future); - - final protected function executeQuery() { - $future = $this->getFileContentFuture(); - - if ($this->getTimeout()) { - $future->setTimeout($this->getTimeout()); - } - - $byte_limit = $this->getByteLimit(); - if ($byte_limit) { - $future->setStdoutSizeLimit($byte_limit + 1); - } - - $drequest = $this->getRequest(); - - $name = basename($drequest->getPath()); - $ttl = PhabricatorTime::getNow() + phutil_units('48 hours in seconds'); - - try { - $threshold = PhabricatorFileStorageEngine::getChunkThreshold(); - $future->setReadBufferSize($threshold); - - $source = id(new PhabricatorExecFutureFileUploadSource()) - ->setName($name) - ->setTTL($ttl) - ->setViewPolicy(PhabricatorPolicies::POLICY_NOONE) - ->setExecFuture($future); - - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - $file = $source->uploadFile(); - unset($unguarded); - - } catch (CommandException $ex) { - if (!$future->getWasKilledByTimeout()) { - throw $ex; - } - - $this->didHitTimeLimit = true; - $file = null; - } - - if ($byte_limit && ($file->getByteSize() > $byte_limit)) { - $this->didHitByteLimit = true; - - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - id(new PhabricatorDestructionEngine()) - ->destroyObject($file); - unset($unguarded); - - $file = null; - } - - return $file; } } diff --git a/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php --- a/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php @@ -15,9 +15,4 @@ $path); } - protected function resolveFileContentFuture(Future $future) { - list($corpus) = $future->resolvex(); - return $corpus; - } - } diff --git a/src/applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php --- a/src/applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php @@ -16,9 +16,4 @@ $path); } - protected function resolveFileContentFuture(Future $future) { - list($corpus) = $future->resolvex(); - return $corpus; - } - } diff --git a/src/applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php --- a/src/applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php @@ -14,9 +14,4 @@ $repository->getSubversionPathURI($path, $commit)); } - protected function resolveFileContentFuture(Future $future) { - list($corpus) = $future->resolvex(); - return $corpus; - } - } -- tangled.sh