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

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)

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

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');

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);
 

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(

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'),

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');

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.
    *

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) {

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<string> 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();