From 45c2152988136b9476b12f805643b3e07da18552 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 22 Nov 2016 15:55:18 -0800 Subject: [PATCH] Begin modernizing the Arcanist interaction with Conduit Summary: Ref T10895. NOTE: I'm going to land this and other changes to a new `experimental` branch until `arc` is more substantially rebuilt, since everything I touch feels like it requires me to rebuild 30 other things first. Currently, many `arc` workflows are unnecessarily slow because they call `conduit.connect` on startup. There's no need to do this with the modern way the API works, and we've generally moved away from explicit version testing to more granular capability testing on specific workflows. Additionally, some workflows like `arc patch` are huge messes (see T11434) because they're trying to run in anonymous mode but it doesn't really work with all the upfront stuff Conduit does now. It's not possible, in the general case, for a workflow to know upfront if it needs Conduit or not. And: - `ArcanistWorkflow` has piles of Conduit logic, but should not. - Pooling Conduit requests isn't very easy. - There's a lot of general cruft around the workflow. - We should drop certificate support. This pulls out Conduit into a separate on-demand class with modern support, future pooling, less cruft, inline handling of login issues, and generally less garbage. Also adds an `--anonymous` flag, mostly to make testing easier. Test Plan: Ran `arc browse`, used `--anonymous` and `--trace`, fiddled with credentials, got approximatley the same behavior that mainline `arc` has. Reviewers: chad, avivey Reviewed By: avivey Subscribers: avivey Maniphest Tasks: T10895 Differential Revision: https://secure.phabricator.com/D16921 --- .gitignore | 1 + scripts/arcanist.php | 28 +++- src/__phutil_library_map__.php | 4 + src/conduit/ArcanistConduitCall.php | 155 +++++++++++++++++++++++ src/conduit/ArcanistConduitEngine.php | 162 ++++++++++++++++++++++++ src/workflow/ArcanistBrowseWorkflow.php | 37 +++--- src/workflow/ArcanistWorkflow.php | 16 ++- 7 files changed, 380 insertions(+), 23 deletions(-) create mode 100644 src/conduit/ArcanistConduitCall.php create mode 100644 src/conduit/ArcanistConduitEngine.php diff --git a/.gitignore b/.gitignore index 824a9e89..427d945b 100644 --- a/.gitignore +++ b/.gitignore @@ -7,6 +7,7 @@ # libphutil /src/.phutil_module_cache +/src/.cache # User extensions /externals/includes/* diff --git a/scripts/arcanist.php b/scripts/arcanist.php index dc2f07b7..c90b084a 100755 --- a/scripts/arcanist.php +++ b/scripts/arcanist.php @@ -35,6 +35,10 @@ $base_args->parsePartial( 'param' => 'token', 'help' => pht('Use a specific authentication token.'), ), + array( + 'name' => 'anonymous', + 'help' => pht('Run workflow as a public user, without authenticating.'), + ), array( 'name' => 'conduit-version', 'param' => 'version', @@ -64,6 +68,7 @@ $force_conduit_version = $base_args->getArg('conduit-version'); $conduit_timeout = $base_args->getArg('conduit-timeout'); $skip_arcconfig = $base_args->getArg('skip-arcconfig'); $custom_arcrc = $base_args->getArg('arcrc-file'); +$is_anonymous = $base_args->getArg('anonymous'); $load = $base_args->getArg('load-phutil-library'); $help = $base_args->getArg('help'); $args = array_values($base_args->getUnconsumedArgumentVector()); @@ -324,6 +329,10 @@ try { $conduit_token = $force_token; } + if ($is_anonymous) { + $conduit_token = null; + } + $description = implode(' ', $original_argv); $credentials = array( 'user' => $user_name, @@ -333,6 +342,23 @@ try { ); $workflow->setConduitCredentials($credentials); + $basic_user = $configuration_manager->getConfigFromAnySource( + 'http.basicauth.user'); + $basic_pass = $configuration_manager->getConfigFromAnySource( + 'http.basicauth.pass'); + + $engine = id(new ArcanistConduitEngine()) + ->setConduitURI($conduit_uri) + ->setConduitToken($conduit_token) + ->setBasicAuthUser($basic_user) + ->setBasicAuthPass($basic_pass); + + if ($conduit_timeout) { + $engine->setConduitTimeout($conduit_timeout); + } + + $workflow->setConduitEngine($engine); + if ($need_auth) { if ((!$user_name || !$certificate) && (!$conduit_token)) { $arc = 'arc'; @@ -409,7 +435,7 @@ try { fwrite(STDERR, phutil_console_format( "**%s** %s\n", pht('Usage Exception:'), - $ex->getMessage())); + rtrim($ex->getMessage()))); } if ($config) { diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d8f10dd4..813a31ca 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -82,6 +82,8 @@ phutil_register_library_map(array( 'ArcanistComprehensiveLintEngine' => 'lint/engine/ArcanistComprehensiveLintEngine.php', 'ArcanistConcatenationOperatorXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistConcatenationOperatorXHPASTLinterRule.php', 'ArcanistConcatenationOperatorXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistConcatenationOperatorXHPASTLinterRuleTestCase.php', + 'ArcanistConduitCall' => 'conduit/ArcanistConduitCall.php', + 'ArcanistConduitEngine' => 'conduit/ArcanistConduitEngine.php', 'ArcanistConfiguration' => 'configuration/ArcanistConfiguration.php', 'ArcanistConfigurationDrivenLintEngine' => 'lint/engine/ArcanistConfigurationDrivenLintEngine.php', 'ArcanistConfigurationDrivenUnitTestEngine' => 'unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php', @@ -496,6 +498,8 @@ phutil_register_library_map(array( 'ArcanistComprehensiveLintEngine' => 'ArcanistLintEngine', 'ArcanistConcatenationOperatorXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistConcatenationOperatorXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', + 'ArcanistConduitCall' => 'Phobject', + 'ArcanistConduitEngine' => 'Phobject', 'ArcanistConfiguration' => 'Phobject', 'ArcanistConfigurationDrivenLintEngine' => 'ArcanistLintEngine', 'ArcanistConfigurationDrivenUnitTestEngine' => 'ArcanistUnitTestEngine', diff --git a/src/conduit/ArcanistConduitCall.php b/src/conduit/ArcanistConduitCall.php new file mode 100644 index 00000000..29e67762 --- /dev/null +++ b/src/conduit/ArcanistConduitCall.php @@ -0,0 +1,155 @@ +key = $key; + return $this; + } + + public function getKey() { + return $this->key; + } + + public function setEngine(ArcanistConduitEngine $engine) { + $this->engine = $engine; + return $this; + } + + public function getEngine() { + return $this->engine; + } + + public function setMethod($method) { + $this->method = $method; + return $this; + } + + public function getMethod() { + return $this->method; + } + + public function setParameters(array $parameters) { + $this->parameters = $parameters; + return $this; + } + + public function getParameters() { + return $this->parameters; + } + + private function newFuture() { + if ($this->future) { + throw new Exception( + pht( + 'Call has previously generated a future. Create a '. + 'new call object for each API method invocation.')); + } + + $method = $this->getMethod(); + $parameters = $this->getParameters(); + $future = $this->getEngine()->newFuture($this); + $this->future = $future; + + return $this->future; + } + + public function resolve() { + if (!$this->future) { + $this->newFuture(); + } + + $this->getEngine()->resolveFuture($this->getKey()); + + return $this->resolveFuture(); + } + + private function resolveFuture() { + $future = $this->future; + + try { + $result = $future->resolve(); + } catch (ConduitClientException $ex) { + switch ($ex->getErrorCode()) { + case 'ERR-INVALID-SESSION': + if (!$this->getEngine()->getConduitToken()) { + $this->raiseLoginRequired(); + } + break; + case 'ERR-INVALID-AUTH': + $this->raiseInvalidAuth(); + break; + } + + throw $ex; + } + + return $result; + } + + private function raiseLoginRequired() { + $conduit_uri = $this->getEngine()->getConduitURI(); + $conduit_uri = new PhutilURI($conduit_uri); + $conduit_uri->setPath('/'); + + $conduit_domain = $conduit_uri->getDomain(); + + $block = id(new PhutilConsoleBlock()) + ->addParagraph( + tsprintf( + '** %s **', + pht('LOGIN REQUIRED'))) + ->addParagraph( + pht( + 'You are trying to connect to a server ("%s") that you do not '. + 'have any stored credentials for, but the command you are '. + 'running requires authentication.', + $conduit_domain)) + ->addParagraph( + pht( + 'To login and save credentials for this server, run this '. + 'command:')) + ->addParagraph( + tsprintf( + " $ arc install-certificate %s\n", + $conduit_uri)); + + throw new ArcanistUsageException($block->drawConsoleString()); + } + + private function raiseInvalidAuth() { + $conduit_uri = $this->getEngine()->getConduitURI(); + $conduit_uri = new PhutilURI($conduit_uri); + $conduit_uri->setPath('/'); + + $conduit_domain = $conduit_uri->getDomain(); + + $block = id(new PhutilConsoleBlock()) + ->addParagraph( + tsprintf( + '** %s **', + pht('INVALID CREDENTIALS'))) + ->addParagraph( + pht( + 'Your stored credentials for this server ("%s") are not valid.', + $conduit_domain)) + ->addParagraph( + pht( + 'To login and save valid credentials for this server, run this '. + 'command:')) + ->addParagraph( + tsprintf( + " $ arc install-certificate %s\n", + $conduit_uri)); + + throw new ArcanistUsageException($block->drawConsoleString()); + } + +} diff --git a/src/conduit/ArcanistConduitEngine.php b/src/conduit/ArcanistConduitEngine.php new file mode 100644 index 00000000..2bdd0789 --- /dev/null +++ b/src/conduit/ArcanistConduitEngine.php @@ -0,0 +1,162 @@ +conduitURI !== null); + } + + public function setConduitURI($conduit_uri) { + $this->conduitURI = $conduit_uri; + return $this; + } + + public function getConduitURI() { + return $this->conduitURI; + } + + public function setConduitToken($conduit_token) { + $this->conduitToken = $conduit_token; + return $this; + } + + public function getConduitToken() { + return $this->conduitToken; + } + + public function setConduitTimeout($conduit_timeout) { + $this->conduitTimeout = $conduit_timeout; + return $this; + } + + public function getConduitTimeout() { + return $this->conduitTimeout; + } + + public function setBasicAuthUser($basic_auth_user) { + $this->basicAuthUser = $basic_auth_user; + return $this; + } + + public function getBasicAuthUser() { + return $this->basicAuthUser; + } + + public function setBasicAuthPass($basic_auth_pass) { + $this->basicAuthPass = $basic_auth_pass; + return $this; + } + + public function getBasicAuthPass() { + return $this->basicAuthPass; + } + + public function newCall($method, array $parameters) { + if ($this->conduitURI == null) { + $this->raiseURIException(); + } + + $next_key = ++$this->callKey; + + return id(new ArcanistConduitCall()) + ->setKey($next_key) + ->setEngine($this) + ->setMethod($method) + ->setParameters($parameters); + } + + public function newFuture(ArcanistConduitCall $call) { + $method = $call->getMethod(); + $parameters = $call->getParameters(); + + $future = $this->getClient()->callMethod($method, $parameters); + $this->activeFutures[$call->getKey()] = $future; + return $future; + } + + private function getClient() { + if (!$this->client) { + $conduit_uri = $this->getConduitURI(); + + $client = new ConduitClient($conduit_uri); + + $timeout = $this->getConduitTimeout(); + if ($timeout) { + $client->setTimeout($timeout); + } + + $basic_user = $this->getBasicAuthUser(); + $basic_pass = $this->getBasicAuthPass(); + if ($basic_user !== null || $basic_pass !== null) { + $client->setBasicAuthCredentials($basic_user, $basic_pass); + } + + $token = $this->getConduitToken(); + if ($token) { + $client->setConduitToken($this->getConduitToken()); + } + } + + return $client; + } + + public function resolveFuture($key) { + if (isset($this->resolvedFutures[$key])) { + return; + } + + if (!isset($this->activeFutures[$key])) { + throw new Exception( + pht( + 'No future with key "%s" is present in pool.', + $key)); + } + + $iterator = new FutureIterator($this->activeFutures); + foreach ($iterator as $future_key => $future) { + $this->resolvedFutures[$future_key] = $future; + unset($this->activeFutures[$future_key]); + if ($future_key == $key) { + break; + } + } + + return; + } + + private function raiseURIException() { + $list = id(new PhutilConsoleList()) + ->addItem( + pht( + 'Run in a working copy with "phabricator.uri" set in ".arcconfig".')) + ->addItem( + pht( + 'Set a default URI with `arc set-config default `.')) + ->addItem( + pht( + 'Specify a URI explicitly with `--conduit-uri=`.')); + + $block = id(new PhutilConsoleBlock()) + ->addParagraph( + pht( + 'This command needs to communicate with Phabricator, but no '. + 'Phabricator URI is configured.')) + ->addList($list); + + throw new ArcanistUsageException($block->drawConsoleString()); + } + +} diff --git a/src/workflow/ArcanistBrowseWorkflow.php b/src/workflow/ArcanistBrowseWorkflow.php index 64d8f84a..f31d27bf 100644 --- a/src/workflow/ArcanistBrowseWorkflow.php +++ b/src/workflow/ArcanistBrowseWorkflow.php @@ -53,19 +53,13 @@ EOTEXT return true; } - public function requiresConduit() { - return true; - } - - public function requiresAuthentication() { - return true; - } - public function desiresRepositoryAPI() { return true; } public function run() { + $conduit = $this->getConduitEngine(); + $console = PhutilConsole::getConsole(); $is_force = $this->getArgument('force'); @@ -80,11 +74,13 @@ EOTEXT } $things = array_fuse($things); - $objects = $this->getConduit()->callMethodSynchronous( - 'phid.lookup', - array( - 'names' => array_keys($things), - )); + $method = 'phid.lookup'; + $params = array( + 'names' => array_keys($things), + ); + + $objects = $conduit->newCall($method, $params) + ->resolve(); $uris = array(); foreach ($objects as $name => $object) { @@ -123,12 +119,15 @@ EOTEXT } if ($commits) { - $commit_info = $this->getConduit()->callMethodSynchronous( - 'diffusion.querycommits', - array( - 'repositoryPHID' => $this->getRepositoryPHID(), - 'names' => array_keys($commits), - )); + $method = 'diffusion.querycommits'; + + $params = array( + 'repositoryPHID' => $this->getRepositoryPHID(), + 'names' => array_keys($commits), + ); + + $commit_info = $conduit->newCall($method, $params) + ->resolve(); foreach ($commit_info['identifierMap'] as $ckey => $cphid) { $thing = $commits[$ckey]; diff --git a/src/workflow/ArcanistWorkflow.php b/src/workflow/ArcanistWorkflow.php index b4b105d6..12b9816c 100644 --- a/src/workflow/ArcanistWorkflow.php +++ b/src/workflow/ArcanistWorkflow.php @@ -69,6 +69,7 @@ abstract class ArcanistWorkflow extends Phobject { private $repositoryVersion; private $changeCache = array(); + private $conduitEngine; public function __construct() {} @@ -1767,9 +1768,9 @@ abstract class ArcanistWorkflow extends Phobject { } try { - $results = $this->getConduit()->callMethodSynchronous( - 'repository.query', - $query); + $method = 'repository.query'; + $results = $this->getConduitEngine()->newCall($method, $query) + ->resolve(); } catch (ConduitClientException $ex) { if ($ex->getErrorCode() == 'ERR-CONDUIT-CALL') { $reasons[] = pht( @@ -2062,5 +2063,14 @@ abstract class ArcanistWorkflow extends Phobject { return $map; } + final public function setConduitEngine( + ArcanistConduitEngine $conduit_engine) { + $this->conduitEngine = $conduit_engine; + return $this; + } + + final public function getConduitEngine() { + return $this->conduitEngine; + } }