Make it easier to construct multi-column paging clauses from Query classes

Summary:
We currently have two giant messes for paging across multiple columns (usually because one column is not unique), and I'm about to add a third for Maniphest.

Provide a more structured way to build these `A > a OR (A = a AND B > b)` clauses.

Test Plan: Set page size to `2` for Differential and Diffusion and paged forward and backward with a bunch of different orders set. Pages worked as expected.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2625

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

src/applications/differential/query/DifferentialRevisionQuery.php

@@ -731,7 +731,7 @@ final class DifferentialRevisionQuery
   private function loadCursorObject($id) {
     $results = id(new DifferentialRevisionQuery())
       ->setViewer($this->getViewer())
-      ->withIDs(array($id))
+      ->withIDs(array((int)$id))
       ->execute();
     return head($results);
   }
@@ -756,50 +756,39 @@ final class DifferentialRevisionQuery
       return null;
     }
 
+    $columns = array();
+
     switch ($this->order) {
       case self::ORDER_CREATED:
         return $default;
       case self::ORDER_MODIFIED:
-        if ($before_id) {
+        $columns[] = array(
-          return qsprintf(
+          'name' => 'r.dateModified',
-            $conn_r,
+          'value' => $cursor->getDateModified(),
-            '(r.dateModified %Q %d OR (r.dateModified = %d AND r.id %Q %d))',
+          'type' => 'int',
-            $this->getReversePaging() ? '<' : '>',
+        );
-            $cursor->getDateModified(),
+        break;
-            $cursor->getDateModified(),
-            $this->getReversePaging() ? '<' : '>',
-            $cursor->getID());
-        } else {
-          return qsprintf(
-            $conn_r,
-            '(r.dateModified %Q %d OR (r.dateModified = %d AND r.id %Q %d))',
-            $this->getReversePaging() ? '>' : '<',
-            $cursor->getDateModified(),
-            $cursor->getDateModified(),
-            $this->getReversePaging() ? '>' : '<',
-            $cursor->getID());
-        }
       case self::ORDER_PATH_MODIFIED:
-        if ($before_id) {
+        $columns[] = array(
-          return qsprintf(
+          'name' => 'p.epoch',
-            $conn_r,
+          'value' => $cursor->getDateCreated(),
-            '(p.epoch %Q %d OR (p.epoch = %d AND r.id %Q %d))',
+          'type' => 'int',
-            $this->getReversePaging() ? '<' : '>',
+        );
-            $cursor->getDateCreated(),
+        break;
-            $cursor->getDateCreated(),
-            $this->getReversePaging() ? '<' : '>',
-            $cursor->getID());
-        } else {
-          return qsprintf(
-            $conn_r,
-            '(p.epoch %Q %d OR (p.epoch = %d AND r.id %Q %d))',
-            $this->getReversePaging() ? '>' : '<',
-            $cursor->getDateCreated(),
-            $cursor->getDateCreated(),
-            $this->getReversePaging() ? '>' : '<',
-            $cursor->getID());
-        }
     }
+
+    $columns[] = array(
+      'name' => 'r.id',
+      'value' => $cursor->getID(),
+      'type' => 'int',
+    );
+
+    return $this->buildPagingClauseFromMultipleColumns(
+      $conn_r,
+      $columns,
+      array(
+        'reversed' => (bool)($before_id xor $this->getReversePaging()),
+      ));
   }
 
   protected function getPagingColumn() {

src/applications/repository/query/PhabricatorRepositoryQuery.php

@@ -162,7 +162,7 @@ final class PhabricatorRepositoryQuery
   private function loadCursorObject($id) {
     $results = id(new PhabricatorRepositoryQuery())
       ->setViewer($this->getViewer())
-      ->withIDs(array($id))
+      ->withIDs(array((int)$id))
       ->execute();
     return head($results);
   }
@@ -192,69 +192,55 @@ final class PhabricatorRepositoryQuery
       return null;
     }
 
+    $id_column = array(
+      'name' => 'r.id',
+      'type' => 'int',
+      'value' => $cursor->getID(),
+    );
+
+    $columns = array();
     switch ($order) {
       case self::ORDER_COMMITTED:
         $commit = $cursor->getMostRecentCommit();
         if (!$commit) {
           return null;
         }
-        $epoch = $commit->getEpoch();
+        $columns[] = array(
-        if ($before_id) {
+          'name' => 's.epoch',
-          return qsprintf(
+          'type' => 'int',
-            $conn_r,
+          'value' => $commit->getEpoch(),
-            '(s.epoch %Q %d OR (s.epoch = %d AND r.id %Q %d))',
+        );
-            $this->getReversePaging() ? '<' : '>',
+        $columns[] = $id_column;
-            $epoch,
+        break;
-            $epoch,
-            $this->getReversePaging() ? '<' : '>',
-            $cursor->getID());
-        } else {
-          return qsprintf(
-            $conn_r,
-            '(s.epoch %Q %d OR (s.epoch = %d AND r.id %Q %d))',
-            $this->getReversePaging() ? '>' : '<',
-            $epoch,
-            $epoch,
-            $this->getReversePaging() ? '>' : '<',
-            $cursor->getID());
-        }
       case self::ORDER_CALLSIGN:
-        if ($before_id) {
+        $columns[] = array(
-          return qsprintf(
+          'name' => 'r.callsign',
-            $conn_r,
+          'type' => 'string',
-            '(r.callsign %Q %s)',
+          'value' => $cursor->getCallsign(),
-            $this->getReversePaging() ? '<' : '>',
+          'reverse' => true,
-            $cursor->getCallsign());
+        );
-        } else {
+        break;
-          return qsprintf(
-            $conn_r,
-            '(r.callsign %Q %s)',
-            $this->getReversePaging() ? '>' : '<',
-            $cursor->getCallsign());
-        }
       case self::ORDER_NAME:
-        if ($before_id) {
+        $columns[] = array(
-          return qsprintf(
+          'name' => 'r.name',
-            $conn_r,
+          'type' => 'string',
-            '(r.name %Q %s OR (r.name = %s AND r.id %Q %d))',
+          'value' => $cursor->getName(),
-            $this->getReversePaging() ? '<' : '>',
+          'reverse' => true,
-            $cursor->getName(),
+        );
-            $cursor->getName(),
+        $columns[] = $id_column;
-            $this->getReversePaging() ? '<' : '>',
+        break;
-            $cursor->getID());
-        } else {
-          return qsprintf(
-            $conn_r,
-            '(r.name %Q %s OR (r.name = %s AND r.id %Q %d))',
-            $this->getReversePaging() ? '>' : '<',
-            $cursor->getName(),
-            $cursor->getName(),
-            $this->getReversePaging() ? '>' : '<',
-            $cursor->getID());
-        }
       default:
         throw new Exception("Unknown order '{$order}'!");
     }
+
+    return $this->buildPagingClauseFromMultipleColumns(
+      $conn_r,
+      $columns,
+      array(
+        // TODO: Clean up the column ordering stuff and then make this
+        // depend on getReversePaging().
+        'reversed' => (bool)($before_id),
+      ));
   }
 
   private function buildJoinsClause(AphrontDatabaseConnection $conn_r) {

src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php

@@ -126,4 +126,104 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
     return $sliced_results;
   }
 
+
+  /**
+   * Simplifies the task of constructing a paging clause across multiple
+   * columns. In the general case, this looks like:
+   *
+   *   A > a OR (A = a AND B > b) OR (A = a AND B = b AND C > c)
+   *
+   * To build a clause, specify the name, type, and value of each column
+   * to include:
+   *
+   *   $this->buildPagingClauseFromMultipleColumns(
+   *     $conn_r,
+   *     array(
+   *       array(
+   *         'name' => 'title',
+   *         'type' => 'string',
+   *         'value' => $cursor->getTitle(),
+   *         'reverse' => true,
+   *       ),
+   *       array(
+   *         'name' => 'id',
+   *         'type' => 'int',
+   *         'value' => $cursor->getID(),
+   *       ),
+   *     ),
+   *     array(
+   *       'reversed' => $is_reversed,
+   *     ));
+   *
+   * This method will then return a composable clause for inclusion in WHERE.
+   *
+   * @param AphrontDatabaseConnection Connection query will execute on.
+   * @param list<map> Column description dictionaries.
+   * @param map Additional constuction options.
+   * @return string Query clause.
+   */
+  final protected function buildPagingClauseFromMultipleColumns(
+    AphrontDatabaseConnection $conn,
+    array $columns,
+    array $options) {
+
+    foreach ($columns as $column) {
+      PhutilTypeSpec::checkMap(
+        $column,
+        array(
+          'name' => 'string',
+          'value' => 'wild',
+          'type' => 'string',
+          'reverse' => 'optional bool',
+        ));
+    }
+
+    PhutilTypeSpec::checkMap(
+      $options,
+      array(
+        'reversed' => 'optional bool',
+      ));
+
+    $is_query_reversed = idx($options, 'reversed', false);
+
+    $clauses = array();
+    $accumulated = array();
+    $last_key = last_key($columns);
+    foreach ($columns as $key => $column) {
+      $name = $column['name'];
+
+      $type = $column['type'];
+      switch ($type) {
+        case 'int':
+          $value = qsprintf($conn, '%d', $column['value']);
+          break;
+        case 'string':
+          $value = qsprintf($conn, '%s', $column['value']);
+          break;
+        default:
+          throw new Exception("Unknown column type '{$type}'!");
+      }
+
+      $is_column_reversed = idx($column, 'reverse', false);
+      $reverse = ($is_query_reversed xor $is_column_reversed);
+
+      $clause = $accumulated;
+      $clause[] = qsprintf(
+        $conn,
+        '%Q %Q %Q',
+        $name,
+        $reverse ? '>' : '<',
+        $value);
+      $clauses[] = '('.implode(') AND (', $clause).')';
+
+      $accumulated[] = qsprintf(
+        $conn,
+        '%Q = %Q',
+        $name,
+        $value);
+    }
+
+    return '('.implode(') OR (', $clauses).')';
+  }
+
 }