From 21e80a635d798c5be2c6e5c272497b3170c1e079 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 12 Apr 2020 14:45:21 -0700 Subject: [PATCH] Upgrade "arc download" to Toolsets Summary: Ref T13490. This is mostly straightforward. - Drop "--show" in favor of "--as -". - Drop support for 4+ year old "file.info" API. - Use modern stream-to-disk support so we get a real progress bar and don't need to buffer files into memory. Test Plan: Downloaded various files, including large files. Maniphest Tasks: T13490 Differential Revision: https://secure.phabricator.com/D21097 --- src/__phutil_library_map__.php | 13 +- src/ref/file/ArcanistFileRef.php | 52 +++ .../file/ArcanistFileSymbolHardpointQuery.php | 90 +++++ src/ref/file/ArcanistFileSymbolRef.php | 47 +++ .../file/ArcanistFileSymbolRefInspector.php | 22 ++ src/ref/symbol/ArcanistSymbolEngine.php | 11 + src/runtime/ArcanistRuntime.php | 57 ++- src/workflow/ArcanistDownloadWorkflow.php | 334 +++++++----------- 8 files changed, 397 insertions(+), 229 deletions(-) create mode 100644 src/ref/file/ArcanistFileRef.php create mode 100644 src/ref/file/ArcanistFileSymbolHardpointQuery.php create mode 100644 src/ref/file/ArcanistFileSymbolRef.php create mode 100644 src/ref/file/ArcanistFileSymbolRefInspector.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index e9e19fdf..3873124c 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -190,6 +190,10 @@ phutil_register_library_map(array( 'ArcanistFeatureWorkflow' => 'workflow/ArcanistFeatureWorkflow.php', 'ArcanistFileConfigurationSource' => 'config/source/ArcanistFileConfigurationSource.php', 'ArcanistFileDataRef' => 'upload/ArcanistFileDataRef.php', + 'ArcanistFileRef' => 'ref/file/ArcanistFileRef.php', + 'ArcanistFileSymbolHardpointQuery' => 'ref/file/ArcanistFileSymbolHardpointQuery.php', + 'ArcanistFileSymbolRef' => 'ref/file/ArcanistFileSymbolRef.php', + 'ArcanistFileSymbolRefInspector' => 'ref/file/ArcanistFileSymbolRefInspector.php', 'ArcanistFileUploader' => 'upload/ArcanistFileUploader.php', 'ArcanistFilenameLinter' => 'lint/linter/ArcanistFilenameLinter.php', 'ArcanistFilenameLinterTestCase' => 'lint/linter/__tests__/ArcanistFilenameLinterTestCase.php', @@ -1149,7 +1153,7 @@ phutil_register_library_map(array( ), 'ArcanistDoubleQuoteXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistDoubleQuoteXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', - 'ArcanistDownloadWorkflow' => 'ArcanistWorkflow', + 'ArcanistDownloadWorkflow' => 'ArcanistArcWorkflow', 'ArcanistDuplicateKeysInArrayXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistDuplicateKeysInArrayXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistDuplicateSwitchCaseXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', @@ -1173,6 +1177,13 @@ phutil_register_library_map(array( 'ArcanistFeatureWorkflow' => 'ArcanistFeatureBaseWorkflow', 'ArcanistFileConfigurationSource' => 'ArcanistFilesystemConfigurationSource', 'ArcanistFileDataRef' => 'Phobject', + 'ArcanistFileRef' => array( + 'ArcanistRef', + 'ArcanistDisplayRefInterface', + ), + 'ArcanistFileSymbolHardpointQuery' => 'ArcanistRuntimeHardpointQuery', + 'ArcanistFileSymbolRef' => 'ArcanistSymbolRef', + 'ArcanistFileSymbolRefInspector' => 'ArcanistRefInspector', 'ArcanistFileUploader' => 'Phobject', 'ArcanistFilenameLinter' => 'ArcanistLinter', 'ArcanistFilenameLinterTestCase' => 'ArcanistLinterTestCase', diff --git a/src/ref/file/ArcanistFileRef.php b/src/ref/file/ArcanistFileRef.php new file mode 100644 index 00000000..de588bd0 --- /dev/null +++ b/src/ref/file/ArcanistFileRef.php @@ -0,0 +1,52 @@ +getMonogram()); + } + + public static function newFromConduit(array $parameters) { + $ref = new self(); + $ref->parameters = $parameters; + return $ref; + } + + public function getID() { + return idx($this->parameters, 'id'); + } + + public function getPHID() { + return idx($this->parameters, 'phid'); + } + + public function getName() { + return idxv($this->parameters, array('fields', 'name')); + } + + public function getDataURI() { + return idxv($this->parameters, array('fields', 'dataURI')); + } + + public function getSize() { + return idxv($this->parameters, array('fields', 'size')); + } + + public function getMonogram() { + return 'F'.$this->getID(); + } + + public function getDisplayRefObjectName() { + return $this->getMonogram(); + } + + public function getDisplayRefTitle() { + return $this->getName(); + } + +} diff --git a/src/ref/file/ArcanistFileSymbolHardpointQuery.php b/src/ref/file/ArcanistFileSymbolHardpointQuery.php new file mode 100644 index 00000000..4f03f91c --- /dev/null +++ b/src/ref/file/ArcanistFileSymbolHardpointQuery.php @@ -0,0 +1,90 @@ + $ref) { + switch ($ref->getSymbolType()) { + case ArcanistFileSymbolRef::TYPE_ID: + $id_map[$key] = $ref->getSymbol(); + break; + case ArcanistFileSymbolRef::TYPE_PHID: + $phid_map[$key] = $ref->getSymbol(); + break; + } + } + + $futures = array(); + + if ($id_map) { + $id_future = $this->newConduitSearch( + 'file.search', + array( + 'ids' => array_values(array_fuse($id_map)), + )); + + $futures[] = $id_future; + } else { + $id_future = null; + } + + if ($phid_map) { + $phid_future = $this->newConduitSearch( + 'file.search', + array( + 'phids' => array_values(array_fuse($phid_map)), + )); + + $futures[] = $phid_future; + } else { + $phid_future = null; + } + + yield $this->yieldFutures($futures); + + $result_map = array(); + + if ($id_future) { + $id_results = $id_future->resolve(); + $id_results = ipull($id_results, null, 'id'); + + foreach ($id_map as $key => $id) { + $result_map[$key] = idx($id_results, $id); + } + } + + if ($phid_future) { + $phid_results = $phid_future->resolve(); + $phid_results = ipull($phid_results, null, 'phid'); + + foreach ($phid_map as $key => $phid) { + $result_map[$key] = idx($phid_results, $phid); + } + } + + foreach ($result_map as $key => $raw_result) { + if ($raw_result === null) { + continue; + } + + $result_map[$key] = ArcanistFileRef::newFromConduit($raw_result); + } + + yield $this->yieldMap($result_map); + } + +} diff --git a/src/ref/file/ArcanistFileSymbolRef.php b/src/ref/file/ArcanistFileSymbolRef.php new file mode 100644 index 00000000..c3c4fedd --- /dev/null +++ b/src/ref/file/ArcanistFileSymbolRef.php @@ -0,0 +1,47 @@ +getSymbol()); + } + + protected function newCacheKeyParts() { + return array( + sprintf('type(%s)', $this->type), + ); + } + + public function getSymbolType() { + return $this->type; + } + + protected function resolveSymbol($symbol) { + $matches = null; + + $is_id = preg_match('/^[Ff]?([1-9]\d*)\z/', $symbol, $matches); + if ($is_id) { + $this->type = self::TYPE_ID; + return (int)$matches[1]; + } + + $is_phid = preg_match('/^PHID-FILE-\S+\z/', $symbol, $matches); + if ($is_phid) { + $this->type = self::TYPE_PHID; + return $matches[0]; + } + + throw new PhutilArgumentUsageException( + pht( + 'The format of file symbol "%s" is unrecognized. Expected a '. + 'monogram like "F123", or an ID like "123", or a file PHID.', + $symbol)); + } + +} diff --git a/src/ref/file/ArcanistFileSymbolRefInspector.php b/src/ref/file/ArcanistFileSymbolRefInspector.php new file mode 100644 index 00000000..1f42d33c --- /dev/null +++ b/src/ref/file/ArcanistFileSymbolRefInspector.php @@ -0,0 +1,22 @@ +setSymbol($argv[0]); + } + +} diff --git a/src/ref/symbol/ArcanistSymbolEngine.php b/src/ref/symbol/ArcanistSymbolEngine.php index e5a3c60f..526908ff 100644 --- a/src/ref/symbol/ArcanistSymbolEngine.php +++ b/src/ref/symbol/ArcanistSymbolEngine.php @@ -48,6 +48,17 @@ final class ArcanistSymbolEngine $symbols); } + public function loadFileForSymbol($symbol) { + $refs = $this->loadFilesForSymbols(array($symbol)); + return head($refs)->getObject(); + } + + public function loadFilesForSymbols(array $symbols) { + return $this->loadRefsForSymbols( + new ArcanistFileSymbolRef(), + $symbols); + } + public function loadRefsForSymbols( ArcanistSymbolRef $template, array $symbols) { diff --git a/src/runtime/ArcanistRuntime.php b/src/runtime/ArcanistRuntime.php index d6e16780..79d665b1 100644 --- a/src/runtime/ArcanistRuntime.php +++ b/src/runtime/ArcanistRuntime.php @@ -113,7 +113,7 @@ final class ArcanistRuntime { $workflows = $this->newWorkflows($toolset); $this->workflows = $workflows; - $conduit_engine = $this->newConduitEngine($config); + $conduit_engine = $this->newConduitEngine($config, $args); $this->conduitEngine = $conduit_engine; $phutil_workflows = array(); @@ -698,16 +698,34 @@ final class ArcanistRuntime { return last($this->stack); } - private function newConduitEngine(ArcanistConfigurationSourceList $config) { + private function newConduitEngine( + ArcanistConfigurationSourceList $config, + PhutilArgumentParser $args) { - $conduit_uri = $config->getConfig('phabricator.uri'); - if ($conduit_uri === null) { - // For now, read this older config from raw storage. There is currently - // no definition of this option in the "toolsets" config list, and it - // would be nice to get rid of it. - $default_list = $config->getStorageValueList('default'); - if ($default_list) { - $conduit_uri = last($default_list)->getValue(); + try { + $force_uri = $args->getArg('conduit-uri'); + } catch (PhutilArgumentSpecificationException $ex) { + $force_uri = null; + } + + try { + $force_token = $args->getArg('conduit-token'); + } catch (PhutilArgumentSpecificationException $ex) { + $force_token = null; + } + + if ($force_uri !== null) { + $conduit_uri = $force_uri; + } else { + $conduit_uri = $config->getConfig('phabricator.uri'); + if ($conduit_uri === null) { + // For now, read this older config from raw storage. There is currently + // no definition of this option in the "toolsets" config list, and it + // would be nice to get rid of it. + $default_list = $config->getStorageValueList('default'); + if ($default_list) { + $conduit_uri = last($default_list)->getValue(); + } } } @@ -731,17 +749,20 @@ final class ArcanistRuntime { // TODO: This isn't using "getConfig()" because we aren't defining a // real config entry for the moment. - $hosts = array(); + if ($force_token !== null) { + $conduit_token = $force_token; + } else { + $hosts = array(); - $hosts_list = $config->getStorageValueList('hosts'); - foreach ($hosts_list as $hosts_config) { - $hosts += $hosts_config->getValue(); + $hosts_list = $config->getStorageValueList('hosts'); + foreach ($hosts_list as $hosts_config) { + $hosts += $hosts_config->getValue(); + } + + $host_config = idx($hosts, $conduit_uri, array()); + $conduit_token = idx($host_config, 'token'); } - $host_config = idx($hosts, $conduit_uri, array()); - $user_name = idx($host_config, 'user'); - $conduit_token = idx($host_config, 'token'); - if ($conduit_token !== null) { $engine->setConduitToken($conduit_token); } diff --git a/src/workflow/ArcanistDownloadWorkflow.php b/src/workflow/ArcanistDownloadWorkflow.php index 543b715e..3b1264b2 100644 --- a/src/workflow/ArcanistDownloadWorkflow.php +++ b/src/workflow/ArcanistDownloadWorkflow.php @@ -1,53 +1,31 @@ newWorkflowInformation() + ->setSynopsis(pht('Download a file to local disk.')) + ->addExample(pht('**download** [__options__] -- __file__')) + ->setHelp($help); } - public function getCommandHelp() { - return phutil_console_format(<< array( - 'conflicts' => array( - 'as' => pht( - 'Use %s to direct the file to stdout, or %s to direct '. - 'it to a named location.', - '--show', - '--as'), - ), - 'help' => pht('Write file to stdout instead of to disk.'), - ), - 'as' => array( - 'param' => 'name', - 'help' => pht( - 'Save the file with a specific name rather than the default.'), - ), - '*' => 'argv', + $this->newWorkflowArgument('as') + ->setParameter('path') + ->setHelp(pht('Save the file to a specific location.')), + $this->newWorkflowArgument('argv') + ->setWildcard(true), ); } @@ -72,207 +50,143 @@ EOTEXT $this->show = $this->getArgument('show'); } - public function requiresAuthentication() { - return true; - } - public function run() { - $conduit = $this->getConduit(); + public function runWorkflow() { + $file_symbols = $this->getArgument('argv'); - $id = $this->id; - $display_name = 'F'.$id; - $is_show = $this->show; - $save_as = $this->saveAs; + if (!$file_symbols) { + throw new PhutilArgumentUsageException( + pht( + 'Specify a file to download, like "F123".')); + } + + if (count($file_symbols) > 1) { + throw new PhutilArgumentUsageException( + pht( + 'Specify exactly one file to download.')); + } + + $file_symbol = head($file_symbols); + + $symbols = $this->getSymbolEngine(); + $file_ref = $symbols->loadFileForSymbol($file_symbol); + if (!$file_ref) { + throw new PhutilArgumentUsageException( + pht( + 'File "%s" does not exist, or you do not have permission to '. + 'view it.', + $file_symbol)); + } + + $is_stdout = false; $path = null; - try { - $file = $conduit->callMethodSynchronous( - 'file.search', - array( - 'constraints' => array( - 'ids' => array($id), - ), - )); + $save_as = $this->getArgument('as'); + if ($save_as === '-') { + $is_stdout = true; + } else if ($save_as === null) { + $path = $file_ref->getName(); + $path = basename($path); + $path = Filesystem::resolvePath($path); - $data = $file['data']; - if (!$data) { - throw new ArcanistUsageException( - pht( - 'File "%s" is not a valid file, or not visible.', - $display_name)); - } + $try_unique = true; + } else { + $path = Filesystem::resolvePath($save_as); - $file = head($data); - $data_uri = idxv($file, array('fields', 'dataURI')); + $try_unique = false; + } - if ($data_uri === null) { - throw new ArcanistUsageException( - pht( - 'File "%s" can not be downloaded.', - $display_name)); - } - - if ($is_show) { - // Skip all the file path stuff if we're just going to echo the - // file contents. + $file_handle = null; + if (!$is_stdout) { + if ($try_unique) { + $path = Filesystem::writeUniqueFile($path, ''); + Filesystem::remove($path); } else { - if ($save_as !== null) { - $path = Filesystem::resolvePath($save_as); - - $try_unique = false; - } else { - $path = idxv($file, array('fields', 'name'), $display_name); - $path = basename($path); - $path = Filesystem::resolvePath($path); - - $try_unique = true; - } - - if ($try_unique) { - $path = Filesystem::writeUniqueFile($path, ''); - } else { - if (Filesystem::pathExists($path)) { - throw new ArcanistUsageException( - pht( - 'File "%s" already exists.', - $save_as)); - } - - Filesystem::writeFile($path, ''); - } - - $display_path = Filesystem::readablePath($path); - } - - $size = idxv($file, array('fields', 'size'), 0); - - if ($is_show) { - $file_handle = null; - } else { - $file_handle = fopen($path, 'ab+'); - if ($file_handle === false) { - throw new Exception( + if (Filesystem::pathExists($path)) { + throw new PhutilArgumentUsageException( pht( - 'Failed to open file "%s" for writing.', + 'File "%s" already exists.', $path)); } - - $this->writeInfo( - pht('DATA'), - pht( - 'Downloading "%s" (%s byte(s))...', - $display_name, - new PhutilNumber($size))); } + } - $future = new HTTPSFuture($data_uri); + $display_path = Filesystem::readablePath($path); + $display_name = $file_ref->getName(); + if (!strlen($display_name)) { + $display_name = $file_ref->getMonogram(); + } + + $expected_bytes = $file_ref->getSize(); + $log = $this->getLogEngine(); + + if (!$is_stdout) { + $log->writeStatus( + pht('DATA'), + pht( + 'Downloading "%s" (%s byte(s)) to "%s"...', + $display_name, + new PhutilNumber($expected_bytes), + $display_path)); + } + + $data_uri = $file_ref->getDataURI(); + $future = new HTTPSFuture($data_uri); + + if (!$is_stdout) { // For small files, don't bother drawing a progress bar. $minimum_bar_bytes = (1024 * 1024 * 4); + if ($expected_bytes > $minimum_bar_bytes) { + $progress = id(new PhutilConsoleProgressSink()) + ->setTotalWork($expected_bytes); - if ($is_show || ($size < $minimum_bar_bytes)) { - $bar = null; - } else { - $bar = id(new PhutilConsoleProgressBar()) - ->setTotal($size); + $future->setProgressSink($progress); } - // TODO: We should stream responses to disk, but cURL gives us the raw - // HTTP response data and BaseHTTPFuture can not currently parse it in - // a stream-oriented way. Until this is resolved, buffer the file data - // in memory and write it to disk in one shot. + // Compute a timeout based on the expected filesize. + $transfer_rate = 32 * 1024; + $timeout = (int)(120 + ($expected_bytes / $transfer_rate)); - list($status, $data) = $future->resolve(); - if ($status->getStatusCode() !== 200) { - throw new Exception( - pht( - 'Got HTTP %d status response, expected HTTP 200.', - $status->getStatusCode())); - } + $future + ->setTimeout($timeout) + ->setDownloadPath($path); + } - if (strlen($data)) { - if ($is_show) { - echo $data; - } else { - $ok = fwrite($file_handle, $data); - if ($ok === false) { - throw new Exception( - pht( - 'Failed to write file data to "%s".', - $path)); - } - } - } - - if ($bar) { - $bar->update(strlen($data)); - } - - if ($bar) { - $bar->done(); - } - - if ($file_handle) { - $ok = fclose($file_handle); - if ($ok === false) { - throw new Exception( - pht( - 'Failed to close file handle for "%s".', - $path)); - } - } - - if (!$is_show) { - $this->writeOkay( - pht('DONE'), - pht( - 'Saved "%s" as "%s".', - $display_name, - $display_path)); - } - - return 0; + try { + list($data) = $future->resolvex(); } catch (Exception $ex) { - - // If we created an empty file, clean it up. - if (!$is_show) { - if ($path !== null) { - Filesystem::remove($path); - } - } - - // If we fail for any reason, fall back to the older mechanism using - // "file.info" and "file.download". + Filesystem::removePath($path); + throw $ex; } - $this->writeStatusMessage(pht('Getting file information...')."\n"); - $info = $conduit->callMethodSynchronous( - 'file.info', - array( - 'id' => $this->id, - )); - - $desc = pht('(%s bytes)', new PhutilNumber($info['byteSize'])); - if ($info['name']) { - $desc = "'".$info['name']."' ".$desc; + if ($is_stdout) { + $file_bytes = strlen($data); + } else { + // TODO: This has various potential problems with clearstatcache() and + // 32-bit systems, but just ignore them for now. + $file_bytes = filesize($path); } - $this->writeStatusMessage(pht('Downloading file %s...', $desc)."\n"); - $data = $conduit->callMethodSynchronous( - 'file.download', - array( - 'phid' => $info['phid'], - )); + if ($file_bytes !== $expected_bytes) { + throw new Exception( + pht( + 'Downloaded file size (%s bytes) does not match expected '. + 'file size (%s bytes). This download may be incomplete or '. + 'corrupt.', + new PhutilNumber($file_bytes), + new PhutilNumber($expected_bytes))); + } - $data = base64_decode($data); - - if ($this->show) { + if ($is_stdout) { echo $data; } else { - $path = Filesystem::writeUniqueFile( - nonempty($this->saveAs, $info['name'], 'file'), - $data); - $this->writeStatusMessage(pht("Saved file as '%s'.", $path)."\n"); + $log->writeStatus( + pht('DONE'), + pht( + 'Saved "%s" as "%s".', + $display_name, + $display_path)); } return 0;