Modernize ManiphestTask paging and ordering

Summary:
Ref T7803. The ApplicationSearch integration is still a little rough here, but it seems to have the correct behavior.

The rest of this is now at least relatively sane, cohesive, and properly behaved.

Test Plan:
  - Used all grouping and ordering queries in Maniphest. Pagingated results.
  - Used custom field ordering in Maniphest. Paginated results.
  - Paginated through the `null` section of "Assigned" and "Projects" group-by queries. Pagingation now works correctly (it does not work at HEAD).
  - Ran unit tests covering priority changes.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7803

Differential Revision: https://secure.phabricator.com/D12372

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -612,12 +612,13 @@ final class ManiphestTransactionEditor
 
     $query = id(new ManiphestTaskQuery())
       ->setViewer(PhabricatorUser::getOmnipotentUser())
-      ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY)
       ->withPriorities(array($priority))
       ->setLimit(1);
 
     if ($is_end) {
-      $query->setReversePaging(true);
+      $query->setOrderVector(array('-priority', '-subpriority', '-id'));
+    } else {
+      $query->setOrderVector(array('priority', 'subpriority', 'id'));
     }
 
     $result = $query->executeOne();
@@ -675,13 +676,18 @@ final class ManiphestTransactionEditor
         // Get all of the tasks with the same subpriority as the adjacent
         // task, including the adjacent task itself.
         $shift_base = $adjacent->getSubpriority();
-        $shift_all = id(new ManiphestTaskQuery())
+        $query = id(new ManiphestTaskQuery())
           ->setViewer(PhabricatorUser::getOmnipotentUser())
-          ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY)
           ->withPriorities(array($adjacent->getPriority()))
-          ->withSubpriorities(array($shift_base))
+          ->withSubpriorities(array($shift_base));
-          ->setReversePaging(!$is_after)
+
-          ->execute();
+        if (!$is_after) {
+          $query->setOrderVector(array('-priority', '-subpriority', '-id'));
+        } else {
+          $query->setOrderVector(array('priority', 'subpriority', 'id'));
+        }
+
+        $shift_all = $query->execute();
         $shift_last = last($shift_all);
 
         // Find the subpriority before or after the task at the end of the

src/applications/maniphest/query/ManiphestTaskQuery.php

@@ -21,7 +21,6 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
   private $dateCreatedBefore;
   private $dateModifiedAfter;
   private $dateModifiedBefore;
-  private $reversePaging;
 
   private $fullTextSearch   = '';
 
@@ -54,14 +53,10 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
 
   private $needSubscriberPHIDs;
   private $needProjectPHIDs;
-
   private $blockingTasks;
   private $blockedTasks;
-
   private $projectPolicyCheckFailed = false;
 
-  const DEFAULT_PAGE_SIZE   = 1000;
-
   public function withAuthors(array $authors) {
     $this->authorPHIDs = $authors;
     return $this;
@@ -231,6 +226,10 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
     return $this;
   }
 
+  protected function newResultObject() {
+    return new ManiphestTask();
+  }
+
   protected function willExecute() {
     // Make sure the user can see any projects specified in this
     // query FIRST.
@@ -252,6 +251,67 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
       }
       $this->projectPHIDs = array_values($this->projectPHIDs);
     }
+
+    // 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() {
@@ -268,7 +328,6 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
     $where[] = $this->buildTaskPHIDsWhereClause($conn);
     $where[] = $this->buildStatusWhereClause($conn);
     $where[] = $this->buildStatusesWhereClause($conn);
-    $where[] = $this->buildPrioritiesWhereClause($conn);
     $where[] = $this->buildDependenciesWhereClause($conn);
     $where[] = $this->buildAuthorWhereClause($conn);
     $where[] = $this->buildOwnerWhereClause($conn);
@@ -306,6 +365,20 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
         $this->dateModifiedBefore);
     }
 
+    if ($this->priorities) {
+      $where[] = qsprintf(
+        $conn,
+        'task.priority IN (%Ld)',
+        $this->priorities);
+    }
+
+    if ($this->subpriorities) {
+      $where[] = qsprintf(
+        $conn,
+        'task.subpriority IN (%Lf)',
+        $this->subpriorities);
+    }
+
     $where[] = $this->buildPagingClause($conn);
 
     $where = $this->formatWhereClause($where);
@@ -325,13 +398,6 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
         count($this->projectPHIDs));
     }
 
-    $order = $this->buildCustomOrderClause($conn);
-
-    // TODO: Clean up this nonstandardness.
-    if (!$this->getLimit()) {
-      $this->setLimit(self::DEFAULT_PAGE_SIZE);
-    }
-
     $group_column = '';
     switch ($this->groupBy) {
       case self::GROUP_PROJECT:
@@ -351,7 +417,7 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
       $where,
       $this->buildGroupClause($conn),
       $having,
-      $order,
+      $this->buildOrderClause($conn),
       $this->buildLimitClause($conn));
 
     switch ($this->groupBy) {
@@ -504,24 +570,6 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
     return null;
   }
 
-  private function buildPrioritiesWhereClause(AphrontDatabaseConnection $conn) {
-    if ($this->priorities) {
-      return qsprintf(
-        $conn,
-        'task.priority IN (%Ld)',
-        $this->priorities);
-    }
-
-    if ($this->subpriorities) {
-      return qsprintf(
-        $conn,
-        'task.subpriority IN (%Lf)',
-        $this->subpriorities);
-    }
-
-    return null;
-  }
-
   private function buildAuthorWhereClause(AphrontDatabaseConnection $conn) {
     if (!$this->authorPHIDs) {
       return null;
@@ -689,107 +737,6 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
       'xproject.dst IS NULL');
   }
 
-  private function buildCustomOrderClause(AphrontDatabaseConnection $conn) {
-    $reverse = ($this->getBeforeID() xor $this->getReversePaging());
-
-    $order = array();
-
-    switch ($this->groupBy) {
-      case self::GROUP_NONE:
-        break;
-      case self::GROUP_PRIORITY:
-        $order[] = 'task.priority';
-        break;
-      case self::GROUP_OWNER:
-        $order[] = 'task.ownerOrdering';
-        break;
-      case self::GROUP_STATUS:
-        $order[] = 'task.status';
-        break;
-      case self::GROUP_PROJECT:
-        $order[] = '<group.project>';
-        break;
-      default:
-        throw new Exception("Unknown group query '{$this->groupBy}'!");
-    }
-
-    $app_order = $this->buildApplicationSearchOrders($conn, $reverse);
-
-    if (!$app_order) {
-      switch ($this->orderBy) {
-        case self::ORDER_PRIORITY:
-          $order[] = 'task.priority';
-          $order[] = 'task.subpriority';
-          $order[] = 'task.dateModified';
-          break;
-        case self::ORDER_CREATED:
-          $order[] = 'task.id';
-          break;
-        case self::ORDER_MODIFIED:
-          $order[] = 'task.dateModified';
-          break;
-        case self::ORDER_TITLE:
-          $order[] = 'task.title';
-          break;
-        default:
-          throw new Exception("Unknown order query '{$this->orderBy}'!");
-      }
-    }
-
-    $order = array_unique($order);
-
-    if (empty($order) && empty($app_order)) {
-      return null;
-    }
-
-    foreach ($order as $k => $column) {
-      switch ($column) {
-        case 'subpriority':
-        case 'ownerOrdering':
-        case 'title':
-          if ($reverse) {
-            $order[$k] = "{$column} DESC";
-          } else {
-            $order[$k] = "{$column} ASC";
-          }
-          break;
-        case '<group.project>':
-          // Put "No Project" at the end of the list.
-          if ($reverse) {
-            $order[$k] =
-              'projectGroupName.indexedObjectName IS NULL DESC, '.
-              'projectGroupName.indexedObjectName DESC';
-          } else {
-            $order[$k] =
-              'projectGroupName.indexedObjectName IS NULL ASC, '.
-              'projectGroupName.indexedObjectName ASC';
-          }
-          break;
-        default:
-          if ($reverse) {
-            $order[$k] = "{$column} ASC";
-          } else {
-            $order[$k] = "{$column} DESC";
-          }
-          break;
-      }
-    }
-
-    if ($app_order) {
-      foreach ($app_order as $order_by) {
-        $order[] = $order_by;
-      }
-    }
-
-    if ($reverse) {
-      $order[] = 'task.id ASC';
-    } else {
-      $order[] = 'task.id DESC';
-    }
-
-    return 'ORDER BY '.implode(', ', $order);
-  }
-
   private function buildJoinsClause(AphrontDatabaseConnection $conn_r) {
     $edge_table = PhabricatorEdgeConfig::TABLE_NAME_EDGE;
 
@@ -954,13 +901,10 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
 
     switch ($this->groupBy) {
       case self::GROUP_NONE:
-        return $id;
-      case self::GROUP_PRIORITY:
-        return $id.'.'.$result->getPriority();
-      case self::GROUP_OWNER:
-        return rtrim($id.'.'.$result->getOwnerPHID(), '.');
       case self::GROUP_STATUS:
-        return $id.'.'.$result->getStatus();
+      case self::GROUP_PRIORITY:
+      case self::GROUP_OWNER:
+        return $id;
       case self::GROUP_PROJECT:
         return rtrim($id.'.'.$result->getGroupByProjectPHID(), '.');
       default:
@@ -968,68 +912,72 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
     }
   }
 
-  protected function buildPagingClause(AphrontDatabaseConnection $conn_r) {
+  public function getOrderableColumns() {
-    $before_id = $this->getBeforeID();
+    return parent::getOrderableColumns() + array(
-    $after_id = $this->getAfterID();
+      'priority' => array(
-
-    if (!$before_id && !$after_id) {
-      return '';
-    }
-
-    $cursor_id = nonempty($before_id, $after_id);
-    $cursor_parts = explode('.', $cursor_id, 2);
-    $task_id = $cursor_parts[0];
-    $group_id = idx($cursor_parts, 1);
-
-    $cursor = $this->loadCursorObject($task_id);
-    if (!$cursor) {
-      // We may loop if we have a cursor and don't build a paging clause; fail
-      // instead.
-      throw new PhabricatorEmptyQueryException();
-    }
-
-    $columns = array();
-
-    switch ($this->groupBy) {
-      case self::GROUP_NONE:
-        break;
-      case self::GROUP_PRIORITY:
-        $columns[] = array(
         'table' => 'task',
         'column' => 'priority',
-          'value' => (int)$group_id,
         'type' => 'int',
-        );
+      ),
-        break;
+      'owner' => array(
-      case self::GROUP_OWNER:
-        $value = null;
-        if ($group_id) {
-          $paging_users = id(new PhabricatorPeopleQuery())
-            ->setViewer($this->getViewer())
-            ->withPHIDs(array($group_id))
-            ->execute();
-          if ($paging_users) {
-            $value = head($paging_users)->getUsername();
-          }
-        }
-        $columns[] = array(
         'table' => 'task',
         'column' => 'ownerOrdering',
-          'value' => $value,
+        'null' => 'head',
+        'reverse' => true,
+        'type' => 'string',
+      ),
+      'status' => array(
+        'table' => 'task',
+        'column' => 'status',
+        'type' => 'string',
+        'reverse' => true,
+      ),
+      'project' => array(
+        'table' => 'projectGroupName',
+        'column' => 'indexedObjectName',
         'type' => 'string',
         'null' => 'head',
         'reverse' => true,
-        );
+      ),
-        break;
+      'title' => array(
-      case self::GROUP_STATUS:
-        $columns[] = array(
         'table' => 'task',
-          'column' => 'status',
+        'column' => 'title',
-          'value' => $group_id,
         'type' => 'string',
+        'reverse' => true,
+      ),
+      'subpriority' => array(
+        'table' => 'task',
+        'column' => 'subpriority',
+        'type' => 'float',
+      ),
+      'updated' => array(
+        'table' => 'task',
+        'column' => 'dateModified',
+        'type' => 'int',
+      ),
     );
-        break;
+  }
-      case self::GROUP_PROJECT:
+
+  protected function getPagingValueMap($cursor, array $keys) {
+    $cursor_parts = explode('.', $cursor, 2);
+    $task_id = $cursor_parts[0];
+    $group_id = idx($cursor_parts, 1);
+
+    $task = $this->loadCursorObject($task_id);
+
+    $map = array(
+      'id' => $task->getID(),
+      'priority' => $task->getPriority(),
+      'subpriority' => $task->getSubpriority(),
+      'owner' => $task->getOwnerOrdering(),
+      'status' => $task->getStatus(),
+      'title' => $task->getTitle(),
+      'updated' => $task->getDateModified(),
+    );
+
+    foreach ($keys as $key) {
+      switch ($key) {
+        case 'project':
           $value = null;
           if ($group_id) {
             $paging_projects = id(new PhabricatorProjectQuery())
@@ -1040,84 +988,19 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
               $value = head($paging_projects)->getName();
             }
           }
-
+          $map[$key] = $value;
-        $columns[] = array(
-          'table' => 'projectGroupName',
-          'column' => 'indexedObjectName',
-          'value' => $value,
-          'type' => 'string',
-          'null' => 'head',
-          'reverse' => true,
-        );
           break;
-      default:
-        throw new Exception("Unknown group query '{$this->groupBy}'!");
-    }
-
-    $app_columns = $this->buildApplicationSearchPagination($conn_r, $cursor);
-    if ($app_columns) {
-      $columns = array_merge($columns, $app_columns);
-    } else {
-      switch ($this->orderBy) {
-        case self::ORDER_PRIORITY:
-          if ($this->groupBy != self::GROUP_PRIORITY) {
-            $columns[] = array(
-              'table' => 'task',
-              'column' => 'priority',
-              'value' => (int)$cursor->getPriority(),
-              'type' => 'int',
-            );
-          }
-          $columns[] = array(
-            'table' => 'task',
-            'column' => 'subpriority',
-            'value' => $cursor->getSubpriority(),
-            'type' => 'float',
-          );
-          $columns[] = array(
-            'table' => 'task',
-            'column' => 'dateModified',
-            'value' => (int)$cursor->getDateModified(),
-            'type' => 'int',
-          );
-          break;
-        case self::ORDER_CREATED:
-          // This just uses the ID column, below.
-          break;
-        case self::ORDER_MODIFIED:
-          $columns[] = array(
-            'table' => 'task',
-            'column' => 'dateModified',
-            'value' => (int)$cursor->getDateModified(),
-            'type' => 'int',
-          );
-          break;
-        case self::ORDER_TITLE:
-          $columns[] = array(
-            'table' => 'task',
-            'column' => 'title',
-            'value' => $cursor->getTitle(),
-            'type' => 'string',
-          );
-          break;
-        default:
-          throw new Exception("Unknown order query '{$this->orderBy}'!");
       }
     }
 
-    $columns[] = array(
+    foreach ($keys as $key) {
-      'table' => 'task',
+      if ($this->isCustomFieldOrderKey($key)) {
-      'column' => 'id',
+        $map += $this->getPagingValueMapForCustomFields($task);
-      'value' => $cursor->getID(),
+        break;
-      'type' => 'int',
+      }
-    );
+    }
 
-    return $this->buildPagingClauseFromMultipleColumns(
+    return $map;
-      $conn_r,
-      $columns,
-      array(
-        'reversed' => (bool)($before_id xor $this->getReversePaging()),
-      ));
   }
 
   protected function getPrimaryTableAlias() {
@@ -1128,13 +1011,4 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
     return 'PhabricatorManiphestApplication';
   }
 
-  public function setReversePaging($reverse_paging) {
-    $this->reversePaging = $reverse_paging;
-    return $this;
-  }
-
-  protected function getReversePaging() {
-    return $this->reversePaging;
-  }
-
 }

src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php

@@ -14,7 +14,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
   private $afterID;
   private $beforeID;
   private $applicationSearchConstraints = array();
-  private $applicationSearchOrders = array();
+  protected $applicationSearchOrders = array();
   private $internalPaging;
   private $orderVector;
 
@@ -161,6 +161,10 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
     return null;
   }
 
+  protected function newResultObject() {
+    return null;
+  }
+
 
 /* -(  Paging  )------------------------------------------------------------- */
 
@@ -546,7 +550,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
       return array();
     }
 
-    return array(
+    $columns = array(
       'id' => array(
         'table' => $this->getPrimaryTableAlias(),
         'column' => 'id',
@@ -555,6 +559,32 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
         'unique' => true,
       ),
     );
+
+    $object = $this->newResultObject();
+    if ($object instanceof PhabricatorCustomFieldInterface) {
+      $list = PhabricatorCustomField::getObjectFields(
+        $object,
+        PhabricatorCustomField::ROLE_APPLICATIONSEARCH);
+      foreach ($list->getFields() as $field) {
+        $index = $field->buildOrderIndex();
+        if (!$index) {
+          continue;
+        }
+
+        $key = $field->getFieldKey();
+        $digest = $field->getFieldIndex();
+
+        $full_key = 'custom:'.$key;
+        $columns[$full_key] = array(
+          'table' => 'appsearch_order_'.$digest,
+          'column' => 'indexValue',
+          'type' => $index->getIndexValueType(),
+          'null' => 'tail',
+        );
+      }
+    }
+
+    return $columns;
   }
 
 
@@ -961,8 +991,8 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
 
     foreach ($this->applicationSearchOrders as $key => $order) {
       $table = $order['table'];
-      $alias = 'appsearch_order_'.$key;
       $index = $order['index'];
+      $alias = 'appsearch_order_'.$index;
       $phid_column = $this->getApplicationSearchObjectPHIDColumn();
 
       $joins[] = qsprintf(
@@ -980,51 +1010,27 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
     return implode(' ', $joins);
   }
 
-  protected function buildApplicationSearchOrders(
+  protected function getPagingValueMapForCustomFields(
-    AphrontDatabaseConnection $conn_r,
+    PhabricatorCustomFieldInterface $object) {
-    $reverse) {
-
-    $orders = array();
-    foreach ($this->applicationSearchOrders as $key => $order) {
-      $alias = 'appsearch_order_'.$key;
-
-      if ($order['ascending'] xor $reverse) {
-        $orders[] = qsprintf($conn_r, '%T.indexValue ASC', $alias);
-      } else {
-        $orders[] = qsprintf($conn_r, '%T.indexValue DESC', $alias);
-      }
-    }
-
-    return $orders;
-  }
-
-  protected function buildApplicationSearchPagination(
-    AphrontDatabaseConnection $conn_r,
-    $cursor) {
 
     // We have to get the current field values on the cursor object.
     $fields = PhabricatorCustomField::getObjectFields(
-      $cursor,
+      $object,
       PhabricatorCustomField::ROLE_APPLICATIONSEARCH);
     $fields->setViewer($this->getViewer());
-    $fields->readFieldsFromStorage($cursor);
+    $fields->readFieldsFromStorage($object);
 
-    $fields = mpull($fields->getFields(), null, 'getFieldKey');
+    $map = array();
-
+    foreach ($fields->getFields() as $field) {
-    $columns = array();
+      $map['custom:'.$field->getFieldKey()] = $field->getValueForStorage();
-    foreach ($this->applicationSearchOrders as $key => $order) {
-      $alias = 'appsearch_order_'.$key;
-
-      $field = idx($fields, $order['key']);
-
-      $columns[] = array(
-        'name' => $alias.'.indexValue',
-        'value' => $field->getValueForStorage(),
-        'type' => $order['type'],
-      );
     }
 
-    return $columns;
+    return $map;
+  }
+
+  protected function isCustomFieldOrderKey($key) {
+    $prefix = 'custom:';
+    return !strncmp($key, $prefix, strlen($prefix));
   }
 
 }