Modernize more paging/order queries

Summary:
Ref T7803. Removes some getReversePaging().

This also fixes `null` column handling, by adding an explicit `'null'` key with possible values "head" (put NULL before other values) or "tail" (put NULL after other values).

Maniphest has some glitchiness in paging through NULLs right now, but I believe it's all pre-existing and will be resolved when it fully converts. Diffusion is fully converted and pages through NULL correctly.

Test Plan:
  - Failed to identify any reason for ChangesetQuery to reverse paging.
  - Paged thorugh Diffusion.
  - Paged through Maniphest.
    - Maniphest has some issues when paging inside a NULL section, but these issues are preexisting and will be resolved later in this change sequence.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7803

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

src/applications/differential/query/DifferentialChangesetQuery.php

@@ -150,8 +150,4 @@ final class DifferentialChangesetQuery
     return 'PhabricatorDifferentialApplication';
   }
 
-  protected function getReversePaging() {
-    return true;
-  }
-
 }

src/applications/maniphest/query/ManiphestTaskQuery.php

@@ -969,13 +969,11 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
   }
 
   protected function buildPagingClause(AphrontDatabaseConnection $conn_r) {
-    $default = parent::buildPagingClause($conn_r);
-
     $before_id = $this->getBeforeID();
     $after_id = $this->getAfterID();
 
     if (!$before_id && !$after_id) {
-      return $default;
+      return '';
     }
 
     $cursor_id = nonempty($before_id, $after_id);
@@ -1004,28 +1002,24 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
         );
         break;
       case self::GROUP_OWNER:
-        $columns[] = array(
-          'table' => 'task',
-          'column' => 'ownerOrdering',
-          'value' => strlen($group_id),
-          'type' => 'null',
-        );
+        $value = null;
         if ($group_id) {
           $paging_users = id(new PhabricatorPeopleQuery())
             ->setViewer($this->getViewer())
             ->withPHIDs(array($group_id))
             ->execute();
-          if (!$paging_users) {
-            return null;
+          if ($paging_users) {
+            $value = head($paging_users)->getUsername();
           }
-          $columns[] = array(
-            'table' => 'task',
-            'column' => 'ownerOrdering',
-            'value' => head($paging_users)->getUsername(),
-            'type' => 'string',
-            'reverse' => true,
-          );
         }
+        $columns[] = array(
+          'table' => 'task',
+          'column' => 'ownerOrdering',
+          'value' => $value,
+          'type' => 'string',
+          'null' => 'head',
+          'reverse' => true,
+        );
         break;
       case self::GROUP_STATUS:
         $columns[] = array(
@@ -1036,28 +1030,25 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
         );
         break;
       case self::GROUP_PROJECT:
-        $columns[] = array(
-          'table' => 'projectGroupName',
-          'column' => 'indexedObjectName',
-          'value' => strlen($group_id),
-          'type' => 'null',
-        );
+        $value = null;
         if ($group_id) {
           $paging_projects = id(new PhabricatorProjectQuery())
             ->setViewer($this->getViewer())
             ->withPHIDs(array($group_id))
             ->execute();
-          if (!$paging_projects) {
-            return null;
+          if ($paging_projects) {
+            $value = head($paging_projects)->getName();
           }
-          $columns[] = array(
-            'table' => 'projectGroupName',
-            'column' => 'indexedObjectName',
-            'value' => head($paging_projects)->getName(),
-            'type' => 'string',
-            'reverse' => true,
-          );
         }
+
+        $columns[] = array(
+          'table' => 'projectGroupName',
+          'column' => 'indexedObjectName',
+          'value' => $value,
+          'type' => 'string',
+          'null' => 'head',
+          'reverse' => true,
+        );
         break;
       default:
         throw new Exception("Unknown group query '{$this->groupBy}'!");

src/applications/phrequent/query/PhrequentUserTimeQuery.php

@@ -56,10 +56,10 @@ final class PhrequentUserTimeQuery
         $this->setOrderVector(array('start', 'id'));
         break;
       case self::ORDER_ENDED_ASC:
-        $this->setOrderVector(array('-ongoing', '-end', '-id'));
+        $this->setOrderVector(array('-end', '-id'));
         break;
       case self::ORDER_ENDED_DESC:
-        $this->setOrderVector(array('ongoing', 'end', 'id'));
+        $this->setOrderVector(array('end', 'id'));
         break;
       default:
         throw new Exception(pht('Unknown order "%s".', $order));
@@ -125,13 +125,10 @@ final class PhrequentUserTimeQuery
         'column' => 'dateStarted',
         'type' => 'int',
       ),
-      'ongoing' => array(
-        'column' => 'dateEnded',
-        'type' => 'null',
-      ),
       'end' => array(
         'column' => 'dateEnded',
         'type' => 'int',
+        'null' => 'head',
       ),
     );
   }
@@ -141,7 +138,6 @@ final class PhrequentUserTimeQuery
     return array(
       'id' => $usertime->getID(),
       'start' => $usertime->getDateStarted(),
-      'ongoing' => $usertime->getDateEnded(),
       'end' => $usertime->getDateEnded(),
     );
   }

src/applications/repository/query/PhabricatorRepositoryQuery.php

@@ -28,7 +28,6 @@ final class PhabricatorRepositoryQuery
   const ORDER_CALLSIGN = 'order-callsign';
   const ORDER_NAME = 'order-name';
   const ORDER_SIZE = 'order-size';
-  private $order = self::ORDER_CREATED;
 
   const HOSTED_PHABRICATOR = 'hosted-phab';
   const HOSTED_REMOTE = 'hosted-remote';
@@ -126,7 +125,25 @@ final class PhabricatorRepositoryQuery
   }
 
   public function setOrder($order) {
-    $this->order = $order;
+    switch ($order) {
+      case self::ORDER_CREATED:
+        $this->setOrderVector(array('id'));
+        break;
+      case self::ORDER_COMMITTED:
+        $this->setOrderVector(array('committed', 'id'));
+        break;
+      case self::ORDER_CALLSIGN:
+        $this->setOrderVector(array('callsign'));
+        break;
+      case self::ORDER_NAME:
+        $this->setOrderVector(array('name', 'id'));
+        break;
+      case self::ORDER_SIZE:
+        $this->setOrderVector(array('size', 'id'));
+        break;
+      default:
+        throw new Exception(pht('Unknown order "%s".', $order));
+    }
     return $this;
   }
 
@@ -152,7 +169,7 @@ final class PhabricatorRepositoryQuery
       $table->getTableName(),
       $this->buildJoinsClause($conn_r),
       $this->buildWhereClause($conn_r),
-      $this->buildCustomOrderClause($conn_r),
+      $this->buildOrderClause($conn_r),
       $this->buildLimitClause($conn_r));
 
     $repositories = $table->loadAllFromArray($data);
@@ -295,162 +312,97 @@ final class PhabricatorRepositoryQuery
     return $repositories;
   }
 
-  protected function buildCustomOrderClause(AphrontDatabaseConnection $conn) {
-    $parts = array();
-
-    $order = $this->order;
-    switch ($order) {
-      case self::ORDER_CREATED:
-        break;
-      case self::ORDER_COMMITTED:
-        $parts[] = array(
-          'table' => 's',
-          'column' => 'epoch',
-        );
-        break;
-      case self::ORDER_CALLSIGN:
-        $parts[] = array(
-          'table' => 'r',
-          'column' => 'callsign',
-          'reverse' => true,
-        );
-        break;
-      case self::ORDER_NAME:
-        $parts[] = array(
-          'table' => 'r',
-          'column' => 'name',
-          'reverse' => true,
-        );
-        break;
-      case self::ORDER_SIZE:
-        $parts[] = array(
-          'table' => 's',
-          'column' => 'size',
-        );
-        break;
-      default:
-        throw new Exception("Unknown order '{$order}!'");
-    }
-
-    $parts[] = array(
-      'table' => 'r',
-      'column' => 'id',
-    );
-
-    return $this->formatOrderClause($conn, $parts);
+  public function getPrimaryTableAlias() {
+    return 'r';
   }
 
-  protected function loadCursorObject($id) {
-    $query = id(new PhabricatorRepositoryQuery())
-      ->setViewer($this->getPagingViewer())
-      ->withIDs(array((int)$id));
+  public function getOrderableColumns() {
+    return parent::getOrderableColumns() + array(
+      'committed' => array(
+        'table' => 's',
+        'column' => 'epoch',
+        'type' => 'int',
+        'null' => 'tail',
+      ),
+      'callsign' => array(
+        'table' => 'r',
+        'column' => 'callsign',
+        'type' => 'string',
+        'unique' => true,
+        'reverse' => true,
+      ),
+      'name' => array(
+        'table' => 'r',
+        'column' => 'name',
+        'type' => 'string',
+        'reverse' => true,
+      ),
+      'size' => array(
+        'table' => 's',
+        'column' => 'size',
+        'type' => 'int',
+        'null' => 'tail',
+      ),
+    );
+  }
 
-    if ($this->order == self::ORDER_COMMITTED) {
+  protected function willExecuteCursorQuery(
+    PhabricatorCursorPagedPolicyAwareQuery $query) {
+    $vector = $this->getOrderVector();
+
+    if ($vector->containsKey('committed')) {
       $query->needMostRecentCommits(true);
     }
 
-    if ($this->order == self::ORDER_SIZE) {
+    if ($vector->containsKey('size')) {
       $query->needCommitCounts(true);
     }
-
-    $results = $query->execute();
-    return head($results);
   }
 
-  protected function buildPagingClause(AphrontDatabaseConnection $conn_r) {
-    $default = parent::buildPagingClause($conn_r);
+  protected function getPagingValueMap($cursor, array $keys) {
+    $repository = $this->loadCursorObject($cursor);
 
-    $before_id = $this->getBeforeID();
-    $after_id = $this->getAfterID();
-
-    if (!$before_id && !$after_id) {
-      return $default;
-    }
-
-    $order = $this->order;
-    if ($order == self::ORDER_CREATED) {
-      return $default;
-    }
-
-    if ($before_id) {
-      $cursor = $this->loadCursorObject($before_id);
-    } else {
-      $cursor = $this->loadCursorObject($after_id);
-    }
-
-    if (!$cursor) {
-      return null;
-    }
-
-    $id_column = array(
-      'table' => 'r',
-      'column' => 'id',
-      'type' => 'int',
-      'value' => $cursor->getID(),
+    $map = array(
+      'id' => $repository->getID(),
+      'callsign' => $repository->getCallsign(),
+      'name' => $repository->getName(),
     );
 
-    $columns = array();
-    switch ($order) {
-      case self::ORDER_COMMITTED:
-        $commit = $cursor->getMostRecentCommit();
-        if (!$commit) {
-          return null;
-        }
-        $columns[] = array(
-          'table' => 's',
-          'column' => 'epoch',
-          'type' => 'int',
-          'value' => $commit->getEpoch(),
-        );
-        $columns[] = $id_column;
-        break;
-      case self::ORDER_CALLSIGN:
-        $columns[] = array(
-          'table' => 'r',
-          'column' => 'callsign',
-          'type' => 'string',
-          'value' => $cursor->getCallsign(),
-          'reverse' => true,
-        );
-        break;
-      case self::ORDER_NAME:
-        $columns[] = array(
-          'table' => 'r',
-          'column' => 'name',
-          'type' => 'string',
-          'value' => $cursor->getName(),
-          'reverse' => true,
-        );
-        $columns[] = $id_column;
-        break;
-      case self::ORDER_SIZE:
-        $columns[] = array(
-          'table' => 's',
-          'column' => 'size',
-          'type' => 'int',
-          'value' => $cursor->getCommitCount(),
-        );
-        $columns[] = $id_column;
-        break;
-      default:
-        throw new Exception("Unknown order '{$order}'!");
+    foreach ($keys as $key) {
+      switch ($key) {
+        case 'committed':
+          $commit = $repository->getMostRecentCommit();
+          if ($commit) {
+            $map[$key] = $commit->getEpoch();
+          } else {
+            $map[$key] = null;
+          }
+          break;
+        case 'size':
+          $count = $repository->getCommitCount();
+          if ($count) {
+            $map[$key] = $count;
+          } else {
+            $map[$key] = null;
+          }
+          break;
+      }
     }
 
-    return $this->buildPagingClauseFromMultipleColumns(
-      $conn_r,
-      $columns,
-      array(
-        'reversed' => ($this->getReversePaging() xor (bool)($before_id)),
-      ));
+    return $map;
   }
 
   private function buildJoinsClause(AphrontDatabaseConnection $conn_r) {
     $joins = array();
 
     $join_summary_table = $this->needCommitCounts ||
-                          $this->needMostRecentCommits ||
-                          ($this->order == self::ORDER_COMMITTED) ||
-                          ($this->order == self::ORDER_SIZE);
+                          $this->needMostRecentCommits;
+
+    $vector = $this->getOrderVector();
+    if ($vector->containsKey('committed') ||
+        $vector->containsKey('size')) {
+      $join_summary_table = true;
+    }
 
     if ($join_summary_table) {
       $joins[] = qsprintf(
@@ -554,6 +506,8 @@ final class PhabricatorRepositoryQuery
     return $this->formatWhereClause($where);
   }
 
+
+
   public function getQueryApplicationClass() {
     return 'PhabricatorDiffusionApplication';
   }

src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php

@@ -320,6 +320,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
           'type' => 'string',
           'reverse' => 'optional bool',
           'unique' => 'optional bool',
+          'null' => 'optional string|null',
         ));
     }
 
@@ -336,21 +337,36 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
     $last_key = last_key($columns);
     foreach ($columns as $key => $column) {
       $type = $column['type'];
-      switch ($type) {
-        case 'null':
-          $value = qsprintf($conn, '%d', ($column['value'] ? 0 : 1));
-          break;
-        case 'int':
-          $value = qsprintf($conn, '%d', $column['value']);
-          break;
-        case 'float':
-          $value = qsprintf($conn, '%f', $column['value']);
-          break;
-        case 'string':
-          $value = qsprintf($conn, '%s', $column['value']);
-          break;
-        default:
-          throw new Exception("Unknown column type '{$type}'!");
+
+      $null = idx($column, 'null');
+      if ($column['value'] === null) {
+        if ($null) {
+          $value = null;
+        } else {
+          throw new Exception(
+            pht(
+              'Column "%s" has null value, but does not specify a null '.
+              'behavior.',
+              $key));
+        }
+      } else {
+        switch ($type) {
+          case 'int':
+            $value = qsprintf($conn, '%d', $column['value']);
+            break;
+          case 'float':
+            $value = qsprintf($conn, '%f', $column['value']);
+            break;
+          case 'string':
+            $value = qsprintf($conn, '%s', $column['value']);
+            break;
+          default:
+            throw new Exception(
+              pht(
+                'Column "%s" has unknown column type "%s".',
+                $column['column'],
+                $type));
+        }
       }
 
       $is_column_reversed = idx($column, 'reverse', false);
@@ -366,23 +382,67 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
         $field = qsprintf($conn, '%T', $column_name);
       }
 
-      if ($type == 'null') {
-        $field = qsprintf($conn, '(%Q IS NULL)', $field);
+      $parts = array();
+      if ($null) {
+        $can_page_if_null = ($null === 'head');
+        $can_page_if_nonnull = ($null === 'tail');
+
+        if ($reverse) {
+          $can_page_if_null = !$can_page_if_null;
+          $can_page_if_nonnull = !$can_page_if_nonnull;
+        }
+
+        $subclause = null;
+        if ($can_page_if_null && $value === null) {
+          $parts[] = qsprintf(
+            $conn,
+            '(%Q IS NOT NULL)',
+            $field);
+        } else if ($can_page_if_nonnull && $value !== null) {
+          $parts[] = qsprintf(
+            $conn,
+            '(%Q IS NULL)',
+            $field);
+        }
       }
 
-      $clause[] = qsprintf(
-        $conn,
-        '%Q %Q %Q',
-        $field,
-        $reverse ? '>' : '<',
-        $value);
-      $clauses[] = '('.implode(') AND (', $clause).')';
+      if ($value !== null) {
+        $parts[] = qsprintf(
+          $conn,
+          '%Q %Q %Q',
+          $field,
+          $reverse ? '>' : '<',
+          $value);
+      }
 
-      $accumulated[] = qsprintf(
-        $conn,
-        '%Q = %Q',
-        $field,
-        $value);
+      if ($parts) {
+        if (count($parts) > 1) {
+          $clause[] = '('.implode(') OR (', $parts).')';
+        } else {
+          $clause[] = head($parts);
+        }
+      }
+
+      if ($clause) {
+        if (count($clause) > 1) {
+          $clauses[] = '('.implode(') AND (', $clause).')';
+        } else {
+          $clauses[] = head($clause);
+        }
+      }
+
+      if ($value === null) {
+        $accumulated[] = qsprintf(
+          $conn,
+          '%Q IS NULL',
+          $field);
+      } else {
+        $accumulated[] = qsprintf(
+          $conn,
+          '%Q = %Q',
+          $field,
+          $value);
+      }
     }
 
     return '('.implode(') OR (', $clauses).')';
@@ -576,8 +636,28 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
         $field = qsprintf($conn, '%T', $column);
       }
 
-      if (idx($part, 'type') === 'null') {
-        $field = qsprintf($conn, '(%Q IS NULL)', $field);
+      $null = idx($part, 'null');
+      if ($null) {
+        switch ($null) {
+          case 'head':
+            $null_field = qsprintf($conn, '(%Q IS NULL)', $field);
+            break;
+          case 'tail':
+            $null_field = qsprintf($conn, '(%Q IS NOT NULL)', $field);
+            break;
+          default:
+            throw new Exception(
+              pht(
+                'NULL value "%s" is invalid. Valid values are "head" and '.
+                '"tail".',
+                $null));
+        }
+
+        if ($descending) {
+          $sql[] = qsprintf($conn, '%Q DESC', $null_field);
+        } else {
+          $sql[] = qsprintf($conn, '%Q ASC', $null_field);
+        }
       }
 
       if ($descending) {