From 4928c34d00c2fd1e162331c49b0bd48c7663fc5c Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 8 Oct 2018 10:16:25 -0700 Subject: [PATCH 1/5] Allow "bin/bulk export" to merge multiple queries and accept more flexible flags Summary: Ref T13210. Minor usability improvements to "bin/bulk export": - Allow `--class task` to work (previously, only `--class ManiphestTaskSearchEngine` worked). - If you run `--query jXIlzQyOYHPU`, don't require `--class`, since the query identifies the class on its own. - Allow users to call `--query A --query B --query C` and get a union of all results. Test Plan: - Ran `--class task`, `--query A --query B`, `--query X` (with no `--class`), got good results. - Ran various flavors of bad combinations (queries from different engines, invalid engines, query and class differing, ambiguous/invalid `--class` name) and got sensible errors. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13210 Differential Revision: https://secure.phabricator.com/D19738 --- ...habricatorBulkManagementExportWorkflow.php | 238 ++++++++++++++---- .../export/engine/PhabricatorExportEngine.php | 2 +- 2 files changed, 188 insertions(+), 52 deletions(-) diff --git a/src/applications/transactions/bulk/management/PhabricatorBulkManagementExportWorkflow.php b/src/applications/transactions/bulk/management/PhabricatorBulkManagementExportWorkflow.php index cd0ed346fc..154c266da2 100644 --- a/src/applications/transactions/bulk/management/PhabricatorBulkManagementExportWorkflow.php +++ b/src/applications/transactions/bulk/management/PhabricatorBulkManagementExportWorkflow.php @@ -26,7 +26,8 @@ final class PhabricatorBulkManagementExportWorkflow 'name' => 'query', 'param' => 'key', 'help' => pht( - 'Export the data selected by this query.'), + 'Export the data selected by one or more queries.'), + 'repeat' => true, ), array( 'name' => 'output', @@ -47,56 +48,7 @@ final class PhabricatorBulkManagementExportWorkflow public function execute(PhutilArgumentParser $args) { $viewer = $this->getViewer(); - $class = $args->getArg('class'); - - if (!strlen($class)) { - throw new PhutilArgumentUsageException( - pht( - 'Specify a search engine class to export data from with '. - '"--class".')); - } - - if (!is_subclass_of($class, 'PhabricatorApplicationSearchEngine')) { - throw new PhutilArgumentUsageException( - pht( - 'SearchEngine class ("%s") is unknown.', - $class)); - } - - $engine = newv($class, array()) - ->setViewer($viewer); - - if (!$engine->canExport()) { - throw new PhutilArgumentUsageException( - pht( - 'SearchEngine class ("%s") does not support data export.', - $class)); - } - - $query_key = $args->getArg('query'); - if (!strlen($query_key)) { - throw new PhutilArgumentUsageException( - pht( - 'Specify a query to export with "--query".')); - } - - if ($engine->isBuiltinQuery($query_key)) { - $saved_query = $engine->buildSavedQueryFromBuiltin($query_key); - } else if ($query_key) { - $saved_query = id(new PhabricatorSavedQueryQuery()) - ->setViewer($viewer) - ->withQueryKeys(array($query_key)) - ->executeOne(); - } else { - $saved_query = null; - } - - if (!$saved_query) { - throw new PhutilArgumentUsageException( - pht( - 'Failed to load saved query ("%s").', - $query_key)); - } + list($engine, $queries) = $this->newQueries($args); $format_key = $args->getArg('format'); if (!strlen($format_key)) { @@ -140,6 +92,15 @@ final class PhabricatorBulkManagementExportWorkflow } } + // If we have more than one query, execute the queries to figure out which + // results they hit, then build a synthetic query for all those results + // using the IDs. + if (count($queries) > 1) { + $saved_query = $this->newUnionQuery($engine, $queries); + } else { + $saved_query = head($queries); + } + $export_engine = id(new PhabricatorExportEngine()) ->setViewer($viewer) ->setTitle(pht('Export')) @@ -165,4 +126,179 @@ final class PhabricatorBulkManagementExportWorkflow return 0; } + private function newQueries(PhutilArgumentParser $args) { + $viewer = $this->getViewer(); + + $query_keys = $args->getArg('query'); + if (!$query_keys) { + throw new PhutilArgumentUsageException( + pht( + 'Specify one or more queries to export with "--query".')); + } + + $engine_classes = id(new PhutilClassMapQuery()) + ->setAncestorClass('PhabricatorApplicationSearchEngine') + ->execute(); + + $class = $args->getArg('class'); + if (strlen($class)) { + + $class_list = array(); + foreach ($engine_classes as $class_name => $engine_object) { + $can_export = id(clone $engine_object) + ->setViewer($viewer) + ->canExport(); + if ($can_export) { + $class_list[] = $class_name; + } + } + + sort($class_list); + $class_list = implode(', ', $class_list); + + $matches = array(); + foreach ($engine_classes as $class_name => $engine_object) { + if (stripos($class_name, $class) !== false) { + if (strtolower($class_name) == strtolower($class)) { + $matches = array($class_name); + break; + } else { + $matches[] = $class_name; + } + } + } + + if (!$matches) { + throw new PhutilArgumentUsageException( + pht( + 'No search engines match "%s". Available engines which support '. + 'data export are: %s.', + $class, + $class_list)); + } else if (count($matches) > 1) { + throw new PhutilArgumentUsageException( + pht( + 'Multiple search engines match "%s": %s.', + $class, + implode(', ', $matches))); + } else { + $class = head($matches); + } + + $engine = newv($class, array()) + ->setViewer($viewer); + } else { + $engine = null; + } + + $queries = array(); + foreach ($query_keys as $query_key) { + if ($engine) { + if ($engine->isBuiltinQuery($query_key)) { + $queries[$query_key] = $engine->buildSavedQueryFromBuiltin( + $query_key); + continue; + } + } + + $saved_query = id(new PhabricatorSavedQueryQuery()) + ->setViewer($viewer) + ->withQueryKeys(array($query_key)) + ->executeOne(); + if (!$saved_query) { + if (!$engine) { + throw new PhutilArgumentUsageException( + pht( + 'Query "%s" is unknown. To run a builtin query like "all" or '. + '"active", also specify the search engine with "--class".', + $query_key)); + } else { + throw new PhutilArgumentUsageException( + pht( + 'Query "%s" is not a recognized query for class "%s".', + $query_key, + get_class($engine))); + } + } + + $queries[$query_key] = $saved_query; + } + + // If we don't have an engine from "--class", fill it in by looking at the + // class of the first query. + if (!$engine) { + foreach ($queries as $query) { + $engine = newv($query->getEngineClassName(), array()) + ->setViewer($viewer); + break; + } + } + + $engine_class = get_class($engine); + + foreach ($queries as $query) { + $query_class = $query->getEngineClassName(); + if ($query_class !== $engine_class) { + throw new PhutilArgumentUsageException( + pht( + 'Specified queries use different engines: query "%s" uses '. + 'engine "%s", not "%s". All queries must run on the same '. + 'engine.', + $query->getQueryKey(), + $query_class, + $engine_class)); + } + } + + if (!$engine->canExport()) { + throw new PhutilArgumentUsageException( + pht( + 'SearchEngine class ("%s") does not support data export.', + $engine_class)); + } + + return array($engine, $queries); + } + + private function newUnionQuery( + PhabricatorApplicationSearchEngine $engine, + array $queries) { + + assert_instances_of($queries, 'PhabricatorSavedQuery'); + + $engine = clone $engine; + + $ids = array(); + foreach ($queries as $saved_query) { + $page_size = 1000; + $page_cursor = null; + do { + $query = $engine->buildQueryFromSavedQuery($saved_query); + $pager = $engine->newPagerForSavedQuery($saved_query); + $pager->setPageSize($page_size); + + if ($page_cursor !== null) { + $pager->setAfterID($page_cursor); + } + + $objects = $engine->executeQuery($query, $pager); + $page_cursor = $pager->getNextPageID(); + + foreach ($objects as $object) { + $ids[] = $object->getID(); + } + } while ($pager->getHasMoreResults()); + } + + // When we're merging multiple different queries, override any query order + // and just put the combined result list in ID order. At time of writing, + // we can't merge the result sets together while retaining the overall sort + // order even if they all used the same order, and it's meaningless to try + // to retain orders if the queries had different orders in the first place. + rsort($ids); + + return id($engine->newSavedQuery()) + ->setParameter('ids', $ids); + } + } diff --git a/src/infrastructure/export/engine/PhabricatorExportEngine.php b/src/infrastructure/export/engine/PhabricatorExportEngine.php index f2c6a1270b..f29eaf0f60 100644 --- a/src/infrastructure/export/engine/PhabricatorExportEngine.php +++ b/src/infrastructure/export/engine/PhabricatorExportEngine.php @@ -125,7 +125,7 @@ final class PhabricatorExportEngine $field_list = mpull($field_list, null, 'getKey'); $format->addHeaders($field_list); - // Iterate over the query results in large page so we don't have to hold + // Iterate over the query results in large pages so we don't have to hold // too much stuff in memory. $page_size = 1000; $page_cursor = null; From 4f557ff075ad115c598061777f98cb3c7a93d08b Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 8 Oct 2018 11:20:58 -0700 Subject: [PATCH 2/5] When using "bin/bulk export --overwrite", actually overwrite the file Summary: Depends on D19738. Ref T13210. Currently, when you use "--overwrite", we just //append// the new content. Instead, actually overwrite the file. Test Plan: Used `--overwrite`, saw an actual clean overwrite instead of an append. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13210 Differential Revision: https://secure.phabricator.com/D19739 --- .../management/PhabricatorBulkManagementExportWorkflow.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/applications/transactions/bulk/management/PhabricatorBulkManagementExportWorkflow.php b/src/applications/transactions/bulk/management/PhabricatorBulkManagementExportWorkflow.php index 154c266da2..53d8441f91 100644 --- a/src/applications/transactions/bulk/management/PhabricatorBulkManagementExportWorkflow.php +++ b/src/applications/transactions/bulk/management/PhabricatorBulkManagementExportWorkflow.php @@ -114,6 +114,10 @@ final class PhabricatorBulkManagementExportWorkflow $iterator = $file->getFileDataIterator(); if (strlen($output_path)) { + // Empty the file before we start writing to it. Otherwise, "--overwrite" + // will really mean "--append". + Filesystem::writeFile($output_path, ''); + foreach ($iterator as $chunk) { Filesystem::appendFile($output_path, $chunk); } From 4f54d483d5bfbbf915ae8586cff18f6093422bec Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 11 Oct 2018 08:20:13 -0700 Subject: [PATCH 3/5] Support export of revisions to Excel/CSV/JSON/etc Summary: Ref T13210. See PHI806. This enables basic export of revisions into flat data formats. This isn't too fancy, but just covers the basics since the driving use case isn't especially concerned about getting all the fields and details. Test Plan: Exported some revisions into JSON, got sensible output. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13210 Differential Revision: https://secure.phabricator.com/D19743 --- .../PhabricatorDifferentialApplication.php | 6 +- .../DifferentialRevisionSearchEngine.php | 73 +++++++++++++++++++ 2 files changed, 76 insertions(+), 3 deletions(-) diff --git a/src/applications/differential/application/PhabricatorDifferentialApplication.php b/src/applications/differential/application/PhabricatorDifferentialApplication.php index 52b222301a..c27d0da820 100644 --- a/src/applications/differential/application/PhabricatorDifferentialApplication.php +++ b/src/applications/differential/application/PhabricatorDifferentialApplication.php @@ -1,6 +1,7 @@ new)/' => 'DifferentialRevisionViewController', ), '/differential/' => array( - '(?:query/(?P[^/]+)/)?' - => 'DifferentialRevisionListController', + $this->getQueryRoutePattern() => 'DifferentialRevisionListController', 'diff/' => array( '(?P[1-9]\d*)/' => array( '' => 'DifferentialDiffViewController', diff --git a/src/applications/differential/query/DifferentialRevisionSearchEngine.php b/src/applications/differential/query/DifferentialRevisionSearchEngine.php index 557ca23078..be753c5e17 100644 --- a/src/applications/differential/query/DifferentialRevisionSearchEngine.php +++ b/src/applications/differential/query/DifferentialRevisionSearchEngine.php @@ -289,4 +289,77 @@ final class DifferentialRevisionSearchEngine return $result; } + protected function newExportFields() { + $fields = array( + id(new PhabricatorStringExportField()) + ->setKey('monogram') + ->setLabel(pht('Monogram')), + id(new PhabricatorPHIDExportField()) + ->setKey('authorPHID') + ->setLabel(pht('Author PHID')), + id(new PhabricatorStringExportField()) + ->setKey('author') + ->setLabel(pht('Author')), + id(new PhabricatorStringExportField()) + ->setKey('status') + ->setLabel(pht('Status')), + id(new PhabricatorStringExportField()) + ->setKey('statusName') + ->setLabel(pht('Status Name')), + id(new PhabricatorURIExportField()) + ->setKey('uri') + ->setLabel(pht('URI')), + id(new PhabricatorStringExportField()) + ->setKey('title') + ->setLabel(pht('Title')), + id(new PhabricatorStringExportField()) + ->setKey('summary') + ->setLabel(pht('Summary')), + id(new PhabricatorStringExportField()) + ->setKey('testPlan') + ->setLabel(pht('Test Plan')), + ); + + return $fields; + } + + protected function newExportData(array $revisions) { + $viewer = $this->requireViewer(); + + $phids = array(); + foreach ($revisions as $revision) { + $phids[] = $revision->getAuthorPHID(); + } + $handles = $viewer->loadHandles($phids); + + $export = array(); + foreach ($revisions as $revision) { + + $author_phid = $revision->getAuthorPHID(); + if ($author_phid) { + $author_name = $handles[$author_phid]->getName(); + } else { + $author_name = null; + } + + $status = $revision->getStatusObject(); + $status_name = $status->getDisplayName(); + $status_value = $status->getKey(); + + $export[] = array( + 'monogram' => $revision->getMonogram(), + 'authorPHID' => $author_phid, + 'author' => $author_name, + 'status' => $status_value, + 'statusName' => $status_name, + 'uri' => PhabricatorEnv::getProductionURI($revision->getURI()), + 'title' => (string)$revision->getTitle(), + 'summary' => (string)$revision->getSummary(), + 'testPlan' => (string)$revision->getTestPlan(), + ); + } + + return $export; + } + } From 8bffc9ea0efe2237ae08be6ecaa54c3bed6f7fa0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 11 Oct 2018 12:35:11 -0700 Subject: [PATCH 4/5] In "bin/bulk export", require "--output " by default Summary: Depends on D19743. Ref T13210. Since this command can easily dump a bunch of binary data (or just a huge long blob of nonsense) to stdout, default to requiring "--output ". Using `--output -` will print to stdout. Test Plan: Ran with: no `--output`, `--output file`, `--output -`, `--output - --overwrite`. Got sensible results or errors in all cases. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13210 Differential Revision: https://secure.phabricator.com/D19744 --- ...habricatorBulkManagementExportWorkflow.php | 27 ++++++++++++++++--- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/src/applications/transactions/bulk/management/PhabricatorBulkManagementExportWorkflow.php b/src/applications/transactions/bulk/management/PhabricatorBulkManagementExportWorkflow.php index 53d8441f91..7f10a28d4b 100644 --- a/src/applications/transactions/bulk/management/PhabricatorBulkManagementExportWorkflow.php +++ b/src/applications/transactions/bulk/management/PhabricatorBulkManagementExportWorkflow.php @@ -77,14 +77,27 @@ final class PhabricatorBulkManagementExportWorkflow $is_overwrite = $args->getArg('overwrite'); $output_path = $args->getArg('output'); - if (!strlen($output_path) && $is_overwrite) { + if (!strlen($output_path)) { throw new PhutilArgumentUsageException( pht( - 'Flag "--overwrite" has no effect without "--output".')); + 'Use "--output " to specify an output file, or "--output -" '. + 'to print to stdout.')); + } + + if ($output_path === '-') { + $is_stdout = true; + } else { + $is_stdout = false; + } + + if ($is_stdout && $is_overwrite) { + throw new PhutilArgumentUsageException( + pht( + 'Flag "--overwrite" has no effect when outputting to stdout.')); } if (!$is_overwrite) { - if (Filesystem::pathExists($output_path)) { + if (!$is_stdout && Filesystem::pathExists($output_path)) { throw new PhutilArgumentUsageException( pht( 'Output path already exists. Use "--overwrite" to overwrite '. @@ -113,7 +126,7 @@ final class PhabricatorBulkManagementExportWorkflow $iterator = $file->getFileDataIterator(); - if (strlen($output_path)) { + if (!$is_stdout) { // Empty the file before we start writing to it. Otherwise, "--overwrite" // will really mean "--append". Filesystem::writeFile($output_path, ''); @@ -121,6 +134,12 @@ final class PhabricatorBulkManagementExportWorkflow foreach ($iterator as $chunk) { Filesystem::appendFile($output_path, $chunk); } + + echo tsprintf( + "%s\n", + pht( + 'Exported data to "%s".', + Filesystem::readablePath($output_path))); } else { foreach ($iterator as $chunk) { echo $chunk; From 0a51bc4f05bf5704e3608a839cc199fe69e88834 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 10 Oct 2018 12:40:47 -0700 Subject: [PATCH 5/5] Add a space after "View Inline" in mail to prevent double-click on the filename from selecting "Inline" Summary: See PHI920. Ref T13210. Since the HTML is just: ``` View Inlinefilename.txt ``` ..double-clicking "filename.txt" in email selects "Inlinefilename.txt". Add a space to stop this. At least in Safari, a space between the tags is not sufficient (perhaps because the parent is a `
`?). I couldn't find an authoritative-seeming source on what the rules for this actually are and adding a space here fixes the issue without changing the visual rendering, so just put it here. Test Plan: - Made an inline. - Used `bin/mail show-outbound --id ... --dump-html` to dump the HTML. - Double-clicked the filename. {F5929186} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13210 Differential Revision: https://secure.phabricator.com/D19742 --- .../mail/DifferentialInlineCommentMailView.php | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/applications/differential/mail/DifferentialInlineCommentMailView.php b/src/applications/differential/mail/DifferentialInlineCommentMailView.php index d88effa46c..0bc914914b 100644 --- a/src/applications/differential/mail/DifferentialInlineCommentMailView.php +++ b/src/applications/differential/mail/DifferentialInlineCommentMailView.php @@ -446,7 +446,18 @@ final class DifferentialInlineCommentMailView 'style' => implode(' ', $link_style), 'href' => $link_href, ), - pht('View Inline')); + array( + pht('View Inline'), + + // See PHI920. Add a space after the link so we render this into + // the document: + // + // View Inline filename.txt + // + // Otherwise, we render "Inlinefilename.txt" and double-clicking + // the file name selects the word "Inline" as well. + ' ', + )); } else { $link = null; }