From 1a091e526047af72d14d115cf168277f1707785c Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 8 Jun 2015 12:23:13 -0700 Subject: [PATCH] Drive Maniphest grouping and ordering through standard infrastructure Summary: Ref T8441. Ref T7715. Ref T7909. Clean up all the ordering and grouping hacks in Maniphest so we can drive it through normal infrastructure, move it to SearchField, introduce Spaces, and eventually modernize the Conduit API. Test Plan: - Executed all grouping/ordering queries, including custom queries. - Forced execution with old aliases; got modern results. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T7909, T7715, T8441 Differential Revision: https://secure.phabricator.com/D13197 --- .../autopatches/20150504.symbolsproject.1.php | 4 + .../__tests__/ManiphestTaskTestCase.php | 2 +- .../ManiphestQueryConduitAPIMethod.php | 2 +- .../editor/ManiphestTransactionEditor.php | 2 +- .../maniphest/query/ManiphestTaskQuery.php | 123 ++++++++---------- .../query/ManiphestTaskSearchEngine.php | 42 ++---- .../PhabricatorProjectBoardViewController.php | 2 +- .../PhabricatorApplicationSearchEngine.php | 62 +-------- .../order/PhabricatorQueryOrderVector.php | 16 +++ ...PhabricatorCursorPagedPolicyAwareQuery.php | 112 +++++++++------- 10 files changed, 155 insertions(+), 212 deletions(-) diff --git a/resources/sql/autopatches/20150504.symbolsproject.1.php b/resources/sql/autopatches/20150504.symbolsproject.1.php index 8ab77bbef4..037397998b 100644 --- a/resources/sql/autopatches/20150504.symbolsproject.1.php +++ b/resources/sql/autopatches/20150504.symbolsproject.1.php @@ -11,6 +11,10 @@ $raw_projects_data = queryfx_all($conn_r, 'SELECT * FROM %T', $projects_table); $raw_projects_data = ipull($raw_projects_data, null, 'id'); $repository_ids = ipull($raw_projects_data, 'repositoryID'); +if (!$repository_ids) { + return; +} + $repositories = id(new PhabricatorRepositoryQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withIDs($repository_ids) diff --git a/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php b/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php index b7fafa83ff..18f433fe52 100644 --- a/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php +++ b/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php @@ -145,7 +145,7 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase { private function loadTasks(PhabricatorUser $viewer, $auto_base) { $tasks = id(new ManiphestTaskQuery()) ->setViewer($viewer) - ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY) + ->setOrder(ManiphestTaskQuery::ORDER_PRIORITY) ->execute(); // NOTE: AUTO_INCREMENT changes survive ROLLBACK, and we can't throw them diff --git a/src/applications/maniphest/conduit/ManiphestQueryConduitAPIMethod.php b/src/applications/maniphest/conduit/ManiphestQueryConduitAPIMethod.php index 8a2321b2d3..ea8aeb95ed 100644 --- a/src/applications/maniphest/conduit/ManiphestQueryConduitAPIMethod.php +++ b/src/applications/maniphest/conduit/ManiphestQueryConduitAPIMethod.php @@ -102,7 +102,7 @@ final class ManiphestQueryConduitAPIMethod extends ManiphestConduitAPIMethod { $order = $request->getValue('order'); if ($order) { - $query->setOrderBy($order); + $query->setOrder($order); } $limit = $request->getValue('limit'); diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index 76a8c7f6f1..39111644cb 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -632,7 +632,7 @@ final class ManiphestTransactionEditor $query = id(new ManiphestTaskQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY) + ->setOrder(ManiphestTaskQuery::ORDER_PRIORITY) ->withPriorities(array($dst->getPriority())) ->setLimit(1); diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php index 636fea218d..956810524d 100644 --- a/src/applications/maniphest/query/ManiphestTaskQuery.php +++ b/src/applications/maniphest/query/ManiphestTaskQuery.php @@ -43,7 +43,6 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { const GROUP_STATUS = 'group-status'; const GROUP_PROJECT = 'group-project'; - private $orderBy = 'order-modified'; const ORDER_PRIORITY = 'order-priority'; const ORDER_CREATED = 'order-created'; const ORDER_MODIFIED = 'order-modified'; @@ -127,11 +126,27 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { public function setGroupBy($group) { $this->groupBy = $group; - return $this; - } - public function setOrderBy($order) { - $this->orderBy = $order; + switch ($this->groupBy) { + case self::GROUP_NONE: + $vector = array(); + break; + case self::GROUP_PRIORITY: + $vector = array('priority'); + break; + case self::GROUP_OWNER: + $vector = array('owner'); + break; + case self::GROUP_STATUS: + $vector = array('status'); + break; + case self::GROUP_PROJECT: + $vector = array('project'); + break; + } + + $this->setGroupVector($vector); + return $this; } @@ -197,69 +212,6 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { return new ManiphestTask(); } - protected function willExecute() { - // If we already have an order vector, use it as provided. - // TODO: This is a messy hack to make setOrderVector() stronger than - // setPriority(). - $vector = $this->getOrderVector(); - $keys = mpull(iterator_to_array($vector), 'getOrderKey'); - if (array_values($keys) !== array('id')) { - return; - } - - $parts = array(); - switch ($this->groupBy) { - case self::GROUP_NONE: - break; - case self::GROUP_PRIORITY: - $parts[] = array('priority'); - break; - case self::GROUP_OWNER: - $parts[] = array('owner'); - break; - case self::GROUP_STATUS: - $parts[] = array('status'); - break; - case self::GROUP_PROJECT: - $parts[] = array('project'); - break; - } - - if ($this->applicationSearchOrders) { - $columns = array(); - foreach ($this->applicationSearchOrders as $order) { - $part = 'custom:'.$order['key']; - if ($order['ascending']) { - $part = '-'.$part; - } - $columns[] = $part; - } - $columns[] = 'id'; - $parts[] = $columns; - } else { - switch ($this->orderBy) { - case self::ORDER_PRIORITY: - $parts[] = array('priority', 'subpriority', 'id'); - break; - case self::ORDER_CREATED: - $parts[] = array('id'); - break; - case self::ORDER_MODIFIED: - $parts[] = array('updated', 'id'); - break; - case self::ORDER_TITLE: - $parts[] = array('title', 'id'); - break; - } - } - - $parts = array_mergev($parts); - // We may have a duplicate column if we are both ordering and grouping - // by priority. - $parts = array_unique($parts); - $this->setOrderVector($parts); - } - protected function loadPage() { $task_dao = new ManiphestTask(); $conn = $task_dao->establishConnection('r'); @@ -760,6 +712,41 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { return $id; } + public function getBuiltinOrders() { + $orders = array( + 'priority' => array( + 'vector' => array('priority', 'subpriority', 'id'), + 'name' => pht('Priority'), + 'aliases' => array(self::ORDER_PRIORITY), + ), + 'updated' => array( + 'vector' => array('updated', 'id'), + 'name' => pht('Date Updated'), + 'aliases' => array(self::ORDER_MODIFIED), + ), + 'title' => array( + 'vector' => array('title', 'id'), + 'name' => pht('Title'), + 'aliases' => array(self::ORDER_TITLE), + ), + ) + parent::getBuiltinOrders(); + + // Alias the "newest" builtin to the historical key for it. + $orders['newest']['aliases'][] = self::ORDER_CREATED; + + $orders = array_select_keys( + $orders, + array( + 'priority', + 'updated', + 'newest', + 'oldest', + 'title', + )) + $orders; + + return $orders; + } + public function getOrderableColumns() { return parent::getOrderableColumns() + array( 'priority' => array( diff --git a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php index c587de5c10..1cdb71994d 100644 --- a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php +++ b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php @@ -38,8 +38,9 @@ final class ManiphestTaskSearchEngine return 'PhabricatorManiphestApplication'; } - public function getCustomFieldObject() { - return new ManiphestTask(); + public function newQuery() { + return id(new ManiphestTaskQuery()) + ->needProjectPHIDs(true); } public function buildSavedQueryFromRequest(AphrontRequest $request) { @@ -108,8 +109,7 @@ final class ManiphestTaskSearchEngine } public function buildQueryFromSavedQuery(PhabricatorSavedQuery $saved) { - $query = id(new ManiphestTaskQuery()) - ->needProjectPHIDs(true); + $query = $this->newQuery(); $viewer = $this->requireViewer(); @@ -156,10 +156,14 @@ final class ManiphestTaskSearchEngine $query->withBlockingTasks($saved->getParameter('blocking')); $query->withBlockedTasks($saved->getParameter('blocked')); - $this->applyOrderByToQuery( - $query, - $this->getOrderValues(), - $saved->getParameter('order')); + // TODO: This is glue that will be obsolete soon. + $order = $saved->getParameter('order'); + $builtin = $query->getBuiltinOrderAliasMap(); + if (strlen($order) && isset($builtin[$order])) { + $query->setOrder($order); + } else { + $query->setOrder(head_key($builtin)); + } $group = $saved->getParameter('group'); $group = idx($this->getGroupValues(), $group); @@ -246,9 +250,7 @@ final class ManiphestTaskSearchEngine $ids = $saved->getParameter('ids', array()); - $builtin_orders = $this->getOrderOptions(); - $custom_orders = $this->getCustomFieldOrderOptions(); - $all_orders = $builtin_orders + $custom_orders; + $all_orders = ipull($this->newQuery()->getBuiltinOrders(), 'name'); $form ->appendControl( @@ -405,24 +407,6 @@ final class ManiphestTaskSearchEngine return parent::buildSavedQueryFromBuiltin($query_key); } - private function getOrderOptions() { - return array( - 'priority' => pht('Priority'), - 'updated' => pht('Date Updated'), - 'created' => pht('Date Created'), - 'title' => pht('Title'), - ); - } - - private function getOrderValues() { - return array( - 'priority' => ManiphestTaskQuery::ORDER_PRIORITY, - 'updated' => ManiphestTaskQuery::ORDER_MODIFIED, - 'created' => ManiphestTaskQuery::ORDER_CREATED, - 'title' => ManiphestTaskQuery::ORDER_TITLE, - ); - } - private function getGroupOptions() { return array( 'priority' => pht('Priority'), diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index b9c366fecc..bcb2b281ad 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -163,7 +163,7 @@ final class PhabricatorProjectBoardViewController PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, PhabricatorQueryConstraint::OPERATOR_AND, array($project->getPHID())) - ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY) + ->setOrder(ManiphestTaskQuery::ORDER_PRIORITY) ->setViewer($viewer) ->execute(); $tasks = mpull($tasks, null, 'getPHID'); diff --git a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php index 419447497c..6204a37c35 100644 --- a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php +++ b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php @@ -157,7 +157,7 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject { } $order = $saved->getParameter('order'); - $builtin = $query->getBuiltinOrders(); + $builtin = $query->getBuiltinOrderAliasMap(); if (strlen($order) && isset($builtin[$order])) { $query->setOrder($order); } else { @@ -1113,66 +1113,6 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject { } } - protected function applyOrderByToQuery( - PhabricatorCursorPagedPolicyAwareQuery $query, - array $standard_values, - $order) { - - if (substr($order, 0, 7) === 'custom:') { - $list = $this->getCustomFieldList(); - if (!$list) { - $query->setOrderBy(head($standard_values)); - return; - } - - foreach ($list->getFields() as $field) { - $key = $this->getKeyForCustomField($field); - - if ($key === $order) { - $index = $field->buildOrderIndex(); - - if ($index === null) { - $query->setOrderBy(head($standard_values)); - return; - } - - $query->withApplicationSearchOrder( - $field, - $index, - false); - break; - } - } - } else { - $order = idx($standard_values, $order); - if ($order) { - $query->setOrderBy($order); - } else { - $query->setOrderBy(head($standard_values)); - } - } - } - - - protected function getCustomFieldOrderOptions() { - $list = $this->getCustomFieldList(); - if (!$list) { - return; - } - - $custom_order = array(); - foreach ($list->getFields() as $field) { - if ($field->shouldAppearInApplicationSearch()) { - if ($field->buildOrderIndex() !== null) { - $key = $this->getKeyForCustomField($field); - $custom_order[$key] = $field->getFieldName(); - } - } - } - - return $custom_order; - } - /** * Get a unique key identifying a field. * diff --git a/src/infrastructure/query/order/PhabricatorQueryOrderVector.php b/src/infrastructure/query/order/PhabricatorQueryOrderVector.php index 37c0fda440..d4b100437b 100644 --- a/src/infrastructure/query/order/PhabricatorQueryOrderVector.php +++ b/src/infrastructure/query/order/PhabricatorQueryOrderVector.php @@ -84,6 +84,22 @@ final class PhabricatorQueryOrderVector return $obj; } + public function appendVector($vector) { + $vector = self::newFromVector($vector); + + // When combining vectors (like "group by" and "order by" vectors), there + // may be redundant columns. We only want to append unique columns which + // aren't already present in the vector. + foreach ($vector->items as $key => $item) { + if (empty($this->items[$key])) { + $this->items[$key] = $item; + $this->keys[] = $key; + } + } + + return $this; + } + public function getAsString() { $scalars = array(); foreach ($this->items as $item) { diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index aace1c728c..b48e1e5c46 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -18,9 +18,9 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery private $afterID; private $beforeID; private $applicationSearchConstraints = array(); - protected $applicationSearchOrders = array(); private $internalPaging; private $orderVector; + private $groupVector; private $builtinOrder; private $edgeLogicConstraints = array(); private $edgeLogicConstraintsAreValid = false; @@ -628,19 +628,40 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery * @task order */ public function setOrder($order) { - $orders = $this->getBuiltinOrders(); + $aliases = $this->getBuiltinOrderAliasMap(); - if (empty($orders[$order])) { + if (empty($aliases[$order])) { throw new Exception( pht( 'Query "%s" does not support a builtin order "%s". Supported orders '. 'are: %s.', get_class($this), $order, - implode(', ', array_keys($orders)))); + implode(', ', array_keys($aliases)))); } - $this->builtinOrder = $order; + $this->builtinOrder = $aliases[$order]; + $this->orderVector = null; + + return $this; + } + + + /** + * Set a grouping order to apply before primary result ordering. + * + * This allows you to preface the query order vector with additional orders, + * so you can effect "group by" queries while still respecting "order by". + * + * This is a high-level method which works alongside @{method:setOrder}. For + * lower-level control over order vectors, use @{method:setOrderVector}. + * + * @param PhabricatorQueryOrderVector|list List of order keys. + * @return this + * @task order + */ + public function setGroupVector($vector) { + $this->groupVector = $vector; $this->orderVector = null; return $this; @@ -707,6 +728,35 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery return $orders; } + public function getBuiltinOrderAliasMap() { + $orders = $this->getBuiltinOrders(); + + $map = array(); + foreach ($orders as $key => $order) { + $keys = array(); + $keys[] = $key; + foreach (idx($order, 'aliases', array()) as $alias) { + $keys[] = $alias; + } + + foreach ($keys as $alias) { + if (isset($map[$alias])) { + throw new Exception( + pht( + 'Two builtin orders ("%s" and "%s") define the same key or '. + 'alias ("%s"). Each order alias and key must be unique and '. + 'identify a single order.', + $key, + $map[$alias], + $alias)); + } + $map[$alias] = $key; + } + } + + return $map; + } + /** * Set a low-level column ordering. @@ -791,6 +841,13 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } else { $vector = $this->getDefaultOrderVector(); } + + if ($this->groupVector) { + $group = PhabricatorQueryOrderVector::newFromVector($this->groupVector); + $group->appendVector($vector); + $vector = $group; + } + $vector = PhabricatorQueryOrderVector::newFromVector($vector); // We call setOrderVector() here to apply checks to the default vector. @@ -1022,32 +1079,6 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } - /** - * Order the results by an ApplicationSearch index. - * - * @param PhabricatorCustomField Field to which the index belongs. - * @param PhabricatorCustomFieldIndexStorage Table where the index is stored. - * @param bool True to sort ascending. - * @return this - * @task appsearch - */ - public function withApplicationSearchOrder( - PhabricatorCustomField $field, - PhabricatorCustomFieldIndexStorage $index, - $ascending) { - - $this->applicationSearchOrders[] = array( - 'key' => $field->getFieldKey(), - 'type' => $index->getIndexValueType(), - 'table' => $index->getTableName(), - 'index' => $index->getIndexKey(), - 'ascending' => $ascending, - ); - - return $this; - } - - /** * Get the name of the query's primary object PHID column, for constructing * JOIN clauses. Normally (and by default) this is just `"phid"`, but it may @@ -1232,25 +1263,6 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } } - // TODO: Get rid of this. - foreach ($this->applicationSearchOrders as $key => $order) { - $table = $order['table']; - $index = $order['index']; - $alias = 'appsearch_order_'.$index; - $phid_column = $this->getApplicationSearchObjectPHIDColumn(); - - $joins[] = qsprintf( - $conn_r, - 'LEFT JOIN %T %T ON %T.objectPHID = %Q - AND %T.indexKey = %s', - $table, - $alias, - $alias, - $phid_column, - $alias, - $index); - } - $phid_column = $this->getApplicationSearchObjectPHIDColumn(); $orderable = $this->getOrderableColumns();