Make buildPagingClauseFromMultipleColumns() safer

Summary: Ref T7803. Reduce the amount of code we're trusting to build SQL queries.

Test Plan:
  - Paged through results in Maniphest, Differential and Diffusion.
  - Some of the NULLable groups in Maniphest are a bit funky but this was preexisting.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7803

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

src/applications/differential/query/DifferentialRevisionQuery.php

@@ -896,14 +896,16 @@ final class DifferentialRevisionQuery
         return $default;
       case self::ORDER_MODIFIED:
         $columns[] = array(
-          'name' => 'r.dateModified',
+          'table' => 'r',
+          'column' => 'dateModified',
           'value' => $cursor->getDateModified(),
           'type' => 'int',
         );
         break;
       case self::ORDER_PATH_MODIFIED:
         $columns[] = array(
-          'name' => 'p.epoch',
+          'table' => 'p',
+          'column' => 'epoch',
           'value' => $cursor->getDateCreated(),
           'type' => 'int',
         );
@@ -911,7 +913,8 @@ final class DifferentialRevisionQuery
     }
 
     $columns[] = array(
-      'name' => 'r.id',
+      'table' => 'r',
+      'column' => 'id',
       'value' => $cursor->getID(),
       'type' => 'int',
     );

src/applications/maniphest/query/ManiphestTaskQuery.php

@@ -1005,16 +1005,18 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
         break;
       case self::GROUP_PRIORITY:
         $columns[] = array(
-          'name' => 'task.priority',
+          'table' => 'task',
+          'column' => 'priority',
           'value' => (int)$group_id,
           'type' => 'int',
         );
         break;
       case self::GROUP_OWNER:
         $columns[] = array(
-          'name' => '(task.ownerOrdering IS NULL)',
+          'table' => 'task',
-          'value' => (int)(strlen($group_id) ? 0 : 1),
+          'column' => 'ownerOrdering',
-          'type' => 'int',
+          'value' => strlen($group_id),
+          'type' => 'null',
         );
         if ($group_id) {
           $paging_users = id(new PhabricatorPeopleQuery())
@@ -1025,7 +1027,8 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
             return null;
           }
           $columns[] = array(
-            'name' => 'task.ownerOrdering',
+            'table' => 'task',
+            'column' => 'ownerOrdering',
             'value' => head($paging_users)->getUsername(),
             'type' => 'string',
             'reverse' => true,
@@ -1034,16 +1037,18 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
         break;
       case self::GROUP_STATUS:
         $columns[] = array(
-          'name' => 'task.status',
+          'table' => 'task',
+          'column' => 'status',
           'value' => $group_id,
           'type' => 'string',
         );
         break;
       case self::GROUP_PROJECT:
         $columns[] = array(
-          'name' => '(projectGroupName.indexedObjectName IS NULL)',
+          'table' => 'projectGroupName',
-          'value' => (int)(strlen($group_id) ? 0 : 1),
+          'column' => 'indexedObjectName',
-          'type' => 'int',
+          'value' => strlen($group_id),
+          'type' => 'null',
         );
         if ($group_id) {
           $paging_projects = id(new PhabricatorProjectQuery())
@@ -1054,7 +1059,8 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
             return null;
           }
           $columns[] = array(
-            'name' => 'projectGroupName.indexedObjectName',
+            'table' => 'projectGroupName',
+            'column' => 'indexedObjectName',
             'value' => head($paging_projects)->getName(),
             'type' => 'string',
             'reverse' => true,
@@ -1073,18 +1079,21 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
         case self::ORDER_PRIORITY:
           if ($this->groupBy != self::GROUP_PRIORITY) {
             $columns[] = array(
-              'name' => 'task.priority',
+              'table' => 'task',
+              'column' => 'priority',
               'value' => (int)$cursor->getPriority(),
               'type' => 'int',
             );
           }
           $columns[] = array(
-            'name' => 'task.subpriority',
+            'table' => 'task',
+            'column' => 'subpriority',
             'value' => $cursor->getSubpriority(),
             'type' => 'float',
           );
           $columns[] = array(
-            'name' => 'task.dateModified',
+            'table' => 'task',
+            'column' => 'dateModified',
             'value' => (int)$cursor->getDateModified(),
             'type' => 'int',
           );
@@ -1094,14 +1103,16 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
           break;
         case self::ORDER_MODIFIED:
           $columns[] = array(
-            'name' => 'task.dateModified',
+            'table' => 'task',
+            'column' => 'dateModified',
             'value' => (int)$cursor->getDateModified(),
             'type' => 'int',
           );
           break;
         case self::ORDER_TITLE:
           $columns[] = array(
-            'name' => 'task.title',
+            'table' => 'task',
+            'column' => 'title',
             'value' => $cursor->getTitle(),
             'type' => 'string',
           );
@@ -1112,7 +1123,8 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
     }
 
     $columns[] = array(
-      'name' => 'task.id',
+      'table' => 'task',
+      'column' => 'id',
       'value' => $cursor->getID(),
       'type' => 'int',
     );

src/applications/repository/query/PhabricatorRepositoryQuery.php

@@ -383,7 +383,8 @@ final class PhabricatorRepositoryQuery
     }
 
     $id_column = array(
-      'name' => 'r.id',
+      'table' => 'r',
+      'column' => 'id',
       'type' => 'int',
       'value' => $cursor->getID(),
     );
@@ -396,7 +397,8 @@ final class PhabricatorRepositoryQuery
           return null;
         }
         $columns[] = array(
-          'name' => 's.epoch',
+          'table' => 's',
+          'column' => 'epoch',
           'type' => 'int',
           'value' => $commit->getEpoch(),
         );
@@ -404,7 +406,8 @@ final class PhabricatorRepositoryQuery
         break;
       case self::ORDER_CALLSIGN:
         $columns[] = array(
-          'name' => 'r.callsign',
+          'table' => 'r',
+          'column' => 'callsign',
           'type' => 'string',
           'value' => $cursor->getCallsign(),
           'reverse' => true,
@@ -412,7 +415,8 @@ final class PhabricatorRepositoryQuery
         break;
       case self::ORDER_NAME:
         $columns[] = array(
-          'name' => 'r.name',
+          'table' => 'r',
+          'column' => 'name',
           'type' => 'string',
           'value' => $cursor->getName(),
           'reverse' => true,
@@ -421,7 +425,8 @@ final class PhabricatorRepositoryQuery
         break;
       case self::ORDER_SIZE:
         $columns[] = array(
-          'name' => 's.size',
+          'table' => 's',
+          'column' => 'size',
           'type' => 'int',
           'value' => $cursor->getCommitCount(),
         );

src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php

@@ -181,13 +181,15 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
    *     $conn_r,
    *     array(
    *       array(
-   *         'name' => 'title',
+   *         'table' => 't',
+   *         'column' => 'title',
    *         'type' => 'string',
    *         'value' => $cursor->getTitle(),
    *         'reverse' => true,
    *       ),
    *       array(
-   *         'name' => 'id',
+   *         'table' => 't',
+   *         'column' => 'id',
    *         'type' => 'int',
    *         'value' => $cursor->getID(),
    *       ),
@@ -212,7 +214,8 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
       PhutilTypeSpec::checkMap(
         $column,
         array(
-          'name' => 'string',
+          'table' => 'optional string',
+          'column' => 'string',
           'value' => 'wild',
           'type' => 'string',
           'reverse' => 'optional bool',
@@ -231,10 +234,11 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
     $accumulated = array();
     $last_key = last_key($columns);
     foreach ($columns as $key => $column) {
-      $name = $column['name'];
-
       $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;
@@ -252,10 +256,23 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
       $reverse = ($is_query_reversed xor $is_column_reversed);
 
       $clause = $accumulated;
+
+      $table_name = idx($column, 'table');
+      $column_name = $column['column'];
+      if ($table_name !== null) {
+        $field = qsprintf($conn, '%T.%T', $table_name, $column_name);
+      } else {
+        $field = qsprintf($conn, '%T', $column_name);
+      }
+
+      if ($type == 'null') {
+        $field = qsprintf($conn, '(%Q IS NULL)', $field);
+      }
+
       $clause[] = qsprintf(
         $conn,
         '%Q %Q %Q',
-        $name,
+        $field,
         $reverse ? '>' : '<',
         $value);
       $clauses[] = '('.implode(') AND (', $clause).')';
@@ -263,7 +280,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
       $accumulated[] = qsprintf(
         $conn,
         '%Q = %Q',
-        $name,
+        $field,
         $value);
     }