Fix all query warnings in "arc unit --everything"

Summary:
Ref T13216. Ref T13217. Depends on D19800. This fixes all of the remaining query warnings that pop up when you run "arc unit --everything".

There's likely still quite a bit of stuff lurking around, but hopefully this covers a big set of the most common queries.

Test Plan: Ran `arc unit --everything`. Before change: lots of query warnings. After change: no query warnings.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13217, T13216

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

src/applications/config/schema/PhabricatorConfigSchemaQuery.php

@@ -115,8 +115,8 @@ final class PhabricatorConfigSchemaQuery extends Phobject {
         'SELECT TABLE_SCHEMA, TABLE_NAME, COLUMN_NAME, CHARACTER_SET_NAME,
             COLLATION_NAME, COLUMN_TYPE, IS_NULLABLE, EXTRA
           FROM INFORMATION_SCHEMA.COLUMNS
-          WHERE (%Q)',
+          WHERE %LO',
-        '('.implode(') OR (', $sql).')');
+        $sql);
       $column_info = igroup($column_info, 'TABLE_SCHEMA');
     } else {
       $column_info = array();

src/applications/diffusion/query/DiffusionCommitQuery.php

@@ -696,7 +696,7 @@ final class DiffusionCommitQuery
           pht('No commit identifiers.'));
       }
 
-      $where[] = '('.implode(' OR ', $sql).')';
+      $where[] = qsprintf($conn, '%LO', $sql);
     }
 
     if ($this->auditIDs !== null) {

src/applications/diffusion/query/DiffusionPathQuery.php

@@ -10,12 +10,12 @@ final class DiffusionPathQuery extends Phobject {
   }
 
   public function execute() {
-    $conn_r = id(new PhabricatorRepository())->establishConnection('r');
+    $conn = id(new PhabricatorRepository())->establishConnection('r');
 
-    $where = $this->buildWhereClause($conn_r);
+    $where = $this->buildWhereClause($conn);
 
     $results = queryfx_all(
-      $conn_r,
+      $conn,
       'SELECT * FROM %T %Q',
       PhabricatorRepository::TABLE_PATH,
       $where);
@@ -23,20 +23,20 @@ final class DiffusionPathQuery extends Phobject {
     return ipull($results, null, 'id');
   }
 
-  protected function buildWhereClause(AphrontDatabaseConnection $conn_r) {
+  protected function buildWhereClause(AphrontDatabaseConnection $conn) {
     $where = array();
 
     if ($this->pathIDs) {
       $where[] = qsprintf(
-        $conn_r,
+        $conn,
         'id IN (%Ld)',
         $this->pathIDs);
     }
 
     if ($where) {
-      return 'WHERE ('.implode(') AND (', $where).')';
+      return qsprintf($conn, 'WHERE %LA', $where);
     } else {
-      return '';
+      return qsprintf($conn, '');
     }
   }
 

src/applications/files/query/PhabricatorFileQuery.php

@@ -391,7 +391,7 @@ final class PhabricatorFileQuery
             $transform['transform']);
         }
       }
-      $where[] = qsprintf($conn, '(%Q)', implode(') OR (', $clauses));
+      $where[] = qsprintf($conn, '%LO', $clauses);
     }
 
     if ($this->dateCreatedAfter !== null) {

src/applications/people/storage/PhabricatorUser.php

@@ -458,11 +458,14 @@ final class PhabricatorUser
   }
 
   public function loadPrimaryEmail() {
+    $email = new PhabricatorUserEmail();
+    $conn = $email->establishConnection('r');
+
     return $this->loadOneRelative(
-      new PhabricatorUserEmail(),
+      $email,
       'userPHID',
       'getPHID',
-      '(isPrimary = 1)');
+      qsprintf($conn, '(isPrimary = 1)'));
   }
 
 

src/applications/phortune/query/PhortuneAccountQuery.php

@@ -42,20 +42,12 @@ final class PhortuneAccountQuery
     return $this;
   }
 
+  public function newResultObject() {
+    return new PhortuneAccount();
+  }
+
   protected function loadPage() {
-    $table = new PhortuneAccount();
+    return $this->loadStandardPage($this->newResultObject());
-    $conn = $table->establishConnection('r');
-
-    $rows = queryfx_all(
-      $conn,
-      'SELECT a.* FROM %T a %Q %Q %Q %Q',
-      $table->getTableName(),
-      $this->buildJoinClause($conn),
-      $this->buildWhereClause($conn),
-      $this->buildOrderClause($conn),
-      $this->buildLimitClause($conn));
-
-    return $table->loadAllFromArray($rows);
   }
 
   protected function willFilterPage(array $accounts) {
@@ -73,39 +65,37 @@ final class PhortuneAccountQuery
     return $accounts;
   }
 
-  protected function buildWhereClause(AphrontDatabaseConnection $conn) {
+  protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
-    $where = array();
+    $where = parent::buildWhereClauseParts($conn);
 
-    $where[] = $this->buildPagingClause($conn);
+    if ($this->ids !== null) {
-
-    if ($this->ids) {
       $where[] = qsprintf(
         $conn,
         'a.id IN (%Ld)',
         $this->ids);
     }
 
-    if ($this->phids) {
+    if ($this->phids !== null) {
       $where[] = qsprintf(
         $conn,
         'a.phid IN (%Ls)',
         $this->phids);
     }
 
-    if ($this->memberPHIDs) {
+    if ($this->memberPHIDs !== null) {
       $where[] = qsprintf(
         $conn,
         'm.dst IN (%Ls)',
         $this->memberPHIDs);
     }
 
-    return $this->formatWhereClause($conn, $where);
+    return $where;
   }
 
-  protected function buildJoinClause(AphrontDatabaseConnection $conn) {
+  protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) {
-    $joins = array();
+    $joins = parent::buildJoinClauseParts($conn);
 
-    if ($this->memberPHIDs) {
+    if ($this->memberPHIDs !== null) {
       $joins[] = qsprintf(
         $conn,
         'LEFT JOIN %T m ON a.phid = m.src AND m.type = %d',
@@ -113,11 +103,15 @@ final class PhortuneAccountQuery
         PhortuneAccountHasMemberEdgeType::EDGECONST);
     }
 
-    return implode(' ', $joins);
+    return $joins;
   }
 
   public function getQueryApplicationClass() {
     return 'PhabricatorPhortuneApplication';
   }
 
+  protected function getPrimaryTableAlias() {
+    return 'a';
+  }
+
 }

src/applications/project/engine/PhabricatorBoardLayoutEngine.php

@@ -297,9 +297,9 @@ final class PhabricatorBoardLayoutEngine extends Phobject {
       queryfx(
         $conn_w,
         'INSERT INTO %T (id, sequence, boardPHID, columnPHID, objectPHID)
-          VALUES %Q ON DUPLICATE KEY UPDATE sequence = VALUES(sequence)',
+          VALUES %LQ ON DUPLICATE KEY UPDATE sequence = VALUES(sequence)',
         $table->getTableName(),
-        implode(', ', $pairs));
+        $pairs);
     }
 
     foreach ($adds as $position) {

src/applications/project/query/PhabricatorProjectQuery.php

@@ -557,7 +557,7 @@ final class PhabricatorProjectQuery
           $ancestor_path['projectDepth']);
       }
 
-      $where[] = '('.implode(' OR ', $sql).')';
+      $where[] = qsprintf($conn, '%LO', $sql);
 
       $where[] = qsprintf(
         $conn,

src/applications/repository/query/PhabricatorRepositoryQuery.php

@@ -513,8 +513,8 @@ final class PhabricatorRepositoryQuery
     if ($this->shouldJoinURITable()) {
       $joins[] = qsprintf(
         $conn,
-        'LEFT JOIN %T uri ON r.phid = uri.repositoryPHID',
+        'LEFT JOIN %R uri ON r.phid = uri.repositoryPHID',
-        id(new PhabricatorRepositoryURIIndex())->getTableName());
+        new PhabricatorRepositoryURIIndex());
     }
 
     return $joins;
@@ -639,7 +639,7 @@ final class PhabricatorRepositoryQuery
           $this->slugIdentifiers);
       }
 
-      $where = array('('.implode(' OR ', $identifier_clause).')');
+      $where[] = qsprintf($conn, '%LO', $identifier_clause);
     }
 
     if ($this->types) {

src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php

@@ -70,9 +70,9 @@ abstract class PhabricatorRepositoryCommitChangeParserWorker
         }
         queryfx(
           $conn_w,
-          'INSERT IGNORE INTO %T (path, pathHash) VALUES %Q',
+          'INSERT IGNORE INTO %T (path, pathHash) VALUES %LQ',
           PhabricatorRepository::TABLE_PATH,
-          implode(', ', $sql));
+          $sql);
       }
       $result_map += self::lookupPaths($missing_paths);
     }

src/applications/repository/worker/commitchangeparser/PhabricatorRepositorySvnCommitChangeParserWorker.php

@@ -463,9 +463,9 @@ final class PhabricatorRepositorySvnCommitChangeParserWorker
         $conn_w,
         'INSERT INTO %T
           (repositoryID, parentID, svnCommit, pathID, existed, fileType)
-          VALUES %Q',
+          VALUES %LQ',
         PhabricatorRepository::TABLE_FILESYSTEM,
-        implode(', ', $sql_chunk));
+        $sql_chunk);
     }
 
   }

src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php

@@ -2353,7 +2353,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
                   (array)$constraint->getValue(),
                   $idx++);
               }
-              $parts = implode(', ', $parts);
+              $parts = qsprintf($conn, '%LQ', $parts);
 
               $select[] = qsprintf(
                 $conn,

src/infrastructure/storage/lisk/LiskDAOSet.php

@@ -77,11 +77,20 @@ final class LiskDAOSet extends Phobject {
       } else {
         $set = new LiskDAOSet();
         $this->subsets[] = $set;
+
+        $conn = $object->establishConnection('r');
+
+        if (strlen($where)) {
+          $where_clause = qsprintf($conn, 'AND %Q', $where);
+        } else {
+          $where_clause = qsprintf($conn, '');
+        }
+
         $relatives = $object->putInSet($set)->loadAllWhere(
           '%C IN (%Ls) %Q',
           $foreign_column,
           $ids,
-          ($where != '' ? 'AND '.$where : ''));
+          $where_clause);
         $relatives = mgroup($relatives, 'get'.$foreign_column);
       }
     }

src/infrastructure/storage/management/PhabricatorStorageManagementAPI.php

@@ -265,7 +265,9 @@ final class PhabricatorStorageManagementAPI extends Phobject {
       }
 
       try {
-        queryfx($conn, '%Q', $query);
+        // NOTE: We're using the unsafe "%Z" conversion here. There's no
+        // avoiding it since we're executing raw text files full of SQL.
+        queryfx($conn, '%Z', $query);
       } catch (AphrontAccessDeniedQueryException $ex) {
         throw new PhutilProxyException(
           pht(

src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php

@@ -357,22 +357,76 @@ abstract class PhabricatorStorageManagementWorkflow
                 }
 
                 if ($adjust['charset']) {
+                  switch ($adjust['charset']) {
+                    case 'binary':
+                      $charset_value = qsprintf($conn, 'binary');
+                      break;
+                    case 'utf8':
+                      $charset_value = qsprintf($conn, 'utf8');
+                      break;
+                    case 'utf8mb4':
+                      $charset_value = qsprintf($conn, 'utf8mb4');
+                      break;
+                    default:
+                      throw new Exception(
+                        pht(
+                          'Unsupported character set "%s".',
+                          $adjust['charset']));
+                  }
+
+                  switch ($adjust['collation']) {
+                    case 'binary':
+                      $collation_value = qsprintf($conn, 'binary');
+                      break;
+                    case 'utf8_general_ci':
+                      $collation_value = qsprintf($conn, 'utf8_general_ci');
+                      break;
+                    case 'utf8mb4_bin':
+                      $collation_value = qsprintf($conn, 'utf8mb4_bin');
+                      break;
+                    case 'utf8mb4_unicode_ci':
+                      $collation_value = qsprintf($conn, 'utf8mb4_unicode_ci');
+                      break;
+                    default:
+                      throw new Exception(
+                        pht(
+                          'Unsupported collation set "%s".',
+                          $adjust['collation']));
+                  }
+
                   $parts[] = qsprintf(
                     $conn,
                     'CHARACTER SET %Q COLLATE %Q',
-                    $adjust['charset'],
+                    $charset_value,
-                    $adjust['collation']);
+                    $collation_value);
                 }
 
+                if ($parts) {
+                  $parts = qsprintf($conn, '%LJ', $parts);
+                } else {
+                  $parts = qsprintf($conn, '');
+                }
+
+                if ($adjust['nullable']) {
+                  $nullable = qsprintf($conn, 'NULL');
+                } else {
+                  $nullable = qsprintf($conn, 'NOT NULL');
+                }
+
+                // TODO: We're using "%Z" here for the column type, which is
+                // technically unsafe. It would be nice to be able to use "%Q"
+                // instead, but this requires a fair amount of legwork to
+                // enumerate all column types.
+
                 queryfx(
                   $conn,
-                  'ALTER TABLE %T.%T MODIFY %T %Q %Q %Q',
+                  'ALTER TABLE %T.%T MODIFY %T %Z %Q %Q',
                   $adjust['database'],
                   $adjust['table'],
                   $adjust['name'],
                   $adjust['type'],
-                  implode(' ', $parts),
+                  $parts,
-                  $adjust['nullable'] ? 'NULL' : 'NOT NULL');
+                  $nullable);
               }
               break;
             case 'key':
@@ -395,7 +449,7 @@ abstract class PhabricatorStorageManagementWorkflow
                 // Different keys need different creation syntax. Notable
                 // special cases are primary keys and fulltext keys.
                 if ($adjust['name'] == 'PRIMARY') {
-                  $key_name = 'PRIMARY KEY';
+                  $key_name = qsprintf($conn, 'PRIMARY KEY');
                 } else if ($adjust['indexType'] == 'FULLTEXT') {
                   $key_name = qsprintf($conn, 'FULLTEXT %T', $adjust['name']);
                 } else {
@@ -414,11 +468,11 @@ abstract class PhabricatorStorageManagementWorkflow
 
                 queryfx(
                   $conn,
-                  'ALTER TABLE %T.%T ADD %Q (%Q)',
+                  'ALTER TABLE %T.%T ADD %Q (%LK)',
                   $adjust['database'],
                   $adjust['table'],
                   $key_name,
-                  implode(', ', $adjust['columns']));
+                  $adjust['columns']);
               }
               break;
             default: