From 2f10d4adebf9519492f6b01001d8776ed8dc3850 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 7 Nov 2018 03:57:28 -0800
Subject: [PATCH] Continue making application fixes to Phabricator for changes
 to %Q semantics

Summary: Depends on D19789. Ref T13217. Continue updating things to use the new %Q-flavored conversions instead of smushing a bunch of strings together.

Test Plan: Browsed around, far fewer errors. These changes are largely mechanical in nature.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13217

Differential Revision: https://secure.phabricator.com/D19790
---
 .../engine/PhabricatorAuthSessionEngine.php   |  8 +--
 .../auth/query/PhabricatorAuthSSHKeyQuery.php |  2 +-
 .../query/PhabricatorCalendarEventQuery.php   |  1 -
 .../query/DifferentialRevisionQuery.php       | 32 ++++++++----
 .../editor/ManiphestTransactionEditor.php     | 49 ++++++++++---------
 .../maniphest/query/ManiphestTaskQuery.php    | 16 +++---
 .../people/storage/PhabricatorUser.php        |  4 +-
 .../cluster/PhabricatorDatabaseRef.php        | 10 +++-
 .../edges/editor/PhabricatorEdgeEditor.php    | 10 ++--
 .../edges/query/PhabricatorEdgeQuery.php      |  6 +--
 ...PhabricatorCursorPagedPolicyAwareQuery.php | 45 +++++++++--------
 src/infrastructure/storage/lisk/LiskDAO.php   | 22 +++++++--
 12 files changed, 120 insertions(+), 85 deletions(-)

diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
index 6e40cdde98..76c0b310a3 100644
--- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
+++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
@@ -809,15 +809,15 @@ final class PhabricatorAuthSessionEngine extends Phobject {
     }
 
     if ($cache_selects) {
-      $cache_selects = ', '.implode(', ', $cache_selects);
+      $cache_selects = qsprintf($conn, ', %LQ', $cache_selects);
     } else {
-      $cache_selects = '';
+      $cache_selects = qsprintf($conn, '');
     }
 
     if ($cache_joins) {
-      $cache_joins = implode(' ', $cache_joins);
+      $cache_joins = qsprintf($conn, '%LJ', $cache_joins);
     } else {
-      $cache_joins = '';
+      $cache_joins = qsprintf($conn, '');
     }
 
     return array($cache_selects, $cache_joins, $cache_map, $types_map);
diff --git a/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php b/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php
index d8474085b2..3a310ed173 100644
--- a/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php
+++ b/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php
@@ -111,7 +111,7 @@ final class PhabricatorAuthSSHKeyQuery
           $key->getType(),
           $key->getHash());
       }
-      $where[] = implode(' OR ', $sql);
+      $where[] = qsprintf($conn, '%LO', $sql);
     }
 
     if ($this->isActive !== null) {
diff --git a/src/applications/calendar/query/PhabricatorCalendarEventQuery.php b/src/applications/calendar/query/PhabricatorCalendarEventQuery.php
index 2be76a631f..c35622fc0e 100644
--- a/src/applications/calendar/query/PhabricatorCalendarEventQuery.php
+++ b/src/applications/calendar/query/PhabricatorCalendarEventQuery.php
@@ -513,7 +513,6 @@ final class PhabricatorCalendarEventQuery
     return 'PhabricatorCalendarApplication';
   }
 
-
   protected function willFilterPage(array $events) {
     $instance_of_event_phids = array();
     $recurring_events = array();
diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php
index d24106328b..fdd4904bee 100644
--- a/src/applications/differential/query/DifferentialRevisionQuery.php
+++ b/src/applications/differential/query/DifferentialRevisionQuery.php
@@ -453,7 +453,7 @@ final class DifferentialRevisionQuery
 
   private function loadData() {
     $table = $this->newResultObject();
-    $conn_r = $table->establishConnection('r');
+    $conn = $table->establishConnection('r');
 
     $selects = array();
 
@@ -469,13 +469,13 @@ final class DifferentialRevisionQuery
         $this->authors = array_merge($basic_authors, $this->responsibles);
 
         $this->reviewers = $basic_reviewers;
-        $selects[] = $this->buildSelectStatement($conn_r);
+        $selects[] = $this->buildSelectStatement($conn);
 
         // Build the query where the responsible users are reviewers, or
         // projects they are members of are reviewers.
         $this->authors = $basic_authors;
         $this->reviewers = array_merge($basic_reviewers, $this->responsibles);
-        $selects[] = $this->buildSelectStatement($conn_r);
+        $selects[] = $this->buildSelectStatement($conn);
 
         // Put everything back like it was.
         $this->authors = $basic_authors;
@@ -486,21 +486,35 @@ final class DifferentialRevisionQuery
         throw $ex;
       }
     } else {
-      $selects[] = $this->buildSelectStatement($conn_r);
+      $selects[] = $this->buildSelectStatement($conn);
     }
 
     if (count($selects) > 1) {
+      $unions = null;
+      foreach ($selects as $select) {
+        if (!$unions) {
+          $unions = $select;
+          continue;
+        }
+
+        $unions = qsprintf(
+          $conn,
+          '%Q UNION DISTINCT %Q',
+          $unions,
+          $select);
+      }
+
       $query = qsprintf(
-        $conn_r,
+        $conn,
         '%Q %Q %Q',
-        implode(' UNION DISTINCT ', $selects),
-        $this->buildOrderClause($conn_r, true),
-        $this->buildLimitClause($conn_r));
+        $unions,
+        $this->buildOrderClause($conn, true),
+        $this->buildLimitClause($conn));
     } else {
       $query = head($selects);
     }
 
-    return queryfx_all($conn_r, '%Q', $query);
+    return queryfx_all($conn, '%Q', $query);
   }
 
   private function buildSelectStatement(AphrontDatabaseConnection $conn_r) {
diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php
index 19fd57b0d2..4b6c7707f6 100644
--- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php
+++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php
@@ -471,27 +471,28 @@ final class ManiphestTransactionEditor
     // be worth evaluating is to use "CASE". Another approach is to disable
     // strict mode for this query.
 
-    $extra_columns = array(
-      'phid' => '""',
-      'authorPHID' => '""',
-      'status' => '""',
-      'priority' => 0,
-      'title' => '""',
-      'description' => '""',
-      'dateCreated' => 0,
-      'dateModified' => 0,
-      'mailKey' => '""',
-      'viewPolicy' => '""',
-      'editPolicy' => '""',
-      'ownerOrdering' => '""',
-      'spacePHID' => '""',
-      'bridgedObjectPHID' => '""',
-      'properties' => '""',
-      'points' => 0,
-      'subtype' => '""',
-    );
+    $default_str = qsprintf($conn, '%s', '');
+    $default_int = qsprintf($conn, '%d', 0);
 
-    $defaults = implode(', ', $extra_columns);
+    $extra_columns = array(
+      'phid' => $default_str,
+      'authorPHID' => $default_str,
+      'status' => $default_str,
+      'priority' => $default_int,
+      'title' => $default_str,
+      'description' => $default_str,
+      'dateCreated' => $default_int,
+      'dateModified' => $default_int,
+      'mailKey' => $default_str,
+      'viewPolicy' => $default_str,
+      'editPolicy' => $default_str,
+      'ownerOrdering' => $default_str,
+      'spacePHID' => $default_str,
+      'bridgedObjectPHID' => $default_str,
+      'properties' => $default_str,
+      'points' => $default_int,
+      'subtype' => $default_str,
+    );
 
     $sql = array();
     $offset = 0;
@@ -520,9 +521,9 @@ final class ManiphestTransactionEditor
 
       $sql[] = qsprintf(
         $conn,
-        '(%d, %Q, %f)',
+        '(%d, %LQ, %f)',
         $id,
-        $defaults,
+        $extra_columns,
         $subpriority);
 
       $offset++;
@@ -531,10 +532,10 @@ final class ManiphestTransactionEditor
     foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) {
       queryfx(
         $conn,
-        'INSERT INTO %T (id, %Q, subpriority) VALUES %LQ
+        'INSERT INTO %T (id, %LC, subpriority) VALUES %LQ
           ON DUPLICATE KEY UPDATE subpriority = VALUES(subpriority)',
         $task->getTableName(),
-        implode(', ', array_keys($extra_columns)),
+        array_keys($extra_columns),
         $chunk);
     }
 
diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php
index ccf93c4a2d..d1932a7597 100644
--- a/src/applications/maniphest/query/ManiphestTaskQuery.php
+++ b/src/applications/maniphest/query/ManiphestTaskQuery.php
@@ -237,7 +237,7 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
 
     $where = $this->buildWhereClause($conn);
 
-    $group_column = '';
+    $group_column = qsprintf($conn, '');
     switch ($this->groupBy) {
       case self::GROUP_PROJECT:
         $group_column = qsprintf(
@@ -601,10 +601,10 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
     }
 
     if (!$subclause) {
-      return '';
+      return qsprintf($conn, '');
     }
 
-    return '('.implode(') OR (', $subclause).')';
+    return qsprintf($conn, '%LO', $subclause);
   }
 
   protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) {
@@ -736,7 +736,7 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
     return $joins;
   }
 
-  protected function buildGroupClause(AphrontDatabaseConnection $conn_r) {
+  protected function buildGroupClause(AphrontDatabaseConnection $conn) {
     $joined_multiple_rows =
       ($this->hasOpenParents !== null) ||
       ($this->hasOpenSubtasks !== null) ||
@@ -750,13 +750,13 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
     // task IDs.
     if ($joined_multiple_rows) {
       if ($joined_project_name) {
-        return 'GROUP BY task.phid, projectGroup.dst';
+        return qsprintf($conn, 'GROUP BY task.phid, projectGroup.dst');
       } else {
-        return 'GROUP BY task.phid';
+        return qsprintf($conn, 'GROUP BY task.phid');
       }
-    } else {
-      return '';
     }
+
+    return qsprintf($conn, '');
   }
 
 
diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php
index e2a0f9a2a2..216e451517 100644
--- a/src/applications/people/storage/PhabricatorUser.php
+++ b/src/applications/people/storage/PhabricatorUser.php
@@ -663,9 +663,9 @@ final class PhabricatorUser
     if ($sql) {
       queryfx(
         $conn_w,
-        'INSERT INTO %T (userID, token) VALUES %Q',
+        'INSERT INTO %T (userID, token) VALUES %LQ',
         $table,
-        implode(', ', $sql));
+        $sql);
     }
   }
 
diff --git a/src/infrastructure/cluster/PhabricatorDatabaseRef.php b/src/infrastructure/cluster/PhabricatorDatabaseRef.php
index 7dc55427ca..89435b5869 100644
--- a/src/infrastructure/cluster/PhabricatorDatabaseRef.php
+++ b/src/infrastructure/cluster/PhabricatorDatabaseRef.php
@@ -557,8 +557,14 @@ final class PhabricatorDatabaseRef
     $conn = $this->newManagementConnection();
 
     try {
-      $value = queryfx_one($conn, 'SELECT @@%Q', $key);
-      $value = $value['@@'.$key];
+      $value = queryfx_one($conn, 'SELECT @@%C', $key);
+
+      // NOTE: Although MySQL allows us to escape configuration values as if
+      // they are column names, the escaping is included in the column name
+      // of the return value: if we select "@@`x`", we get back a column named
+      // "@@`x`", not "@@x" as we might expect.
+      $value = head($value);
+
     } catch (AphrontQueryException $ex) {
       $value = null;
     }
diff --git a/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php b/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php
index 700a11ea36..588b5267b4 100644
--- a/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php
+++ b/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php
@@ -275,13 +275,13 @@ final class PhabricatorEdgeEditor extends Phobject {
       $conn_w->openTransaction();
       $this->openTransactions[] = $conn_w;
 
-      foreach (array_chunk($sql, 256) as $chunk) {
+      foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) {
         queryfx(
           $conn_w,
           'INSERT INTO %T (src, type, dst, dateCreated, seq, dataID)
-            VALUES %Q ON DUPLICATE KEY UPDATE dataID = VALUES(dataID)',
+            VALUES %LQ ON DUPLICATE KEY UPDATE dataID = VALUES(dataID)',
           PhabricatorEdgeConfig::TABLE_NAME_EDGE,
-          implode(', ', $chunk));
+          $chunk);
       }
     }
   }
@@ -320,9 +320,9 @@ final class PhabricatorEdgeEditor extends Phobject {
       foreach (array_chunk($sql, 256) as $chunk) {
         queryfx(
           $conn_w,
-          'DELETE FROM %T WHERE (%Q)',
+          'DELETE FROM %T WHERE %LO',
           PhabricatorEdgeConfig::TABLE_NAME_EDGE,
-          implode(' OR ', $chunk));
+          $chunk);
       }
     }
   }
diff --git a/src/infrastructure/edges/query/PhabricatorEdgeQuery.php b/src/infrastructure/edges/query/PhabricatorEdgeQuery.php
index edd78c868e..6519c47724 100644
--- a/src/infrastructure/edges/query/PhabricatorEdgeQuery.php
+++ b/src/infrastructure/edges/query/PhabricatorEdgeQuery.php
@@ -322,11 +322,11 @@ final class PhabricatorEdgeQuery extends PhabricatorQuery {
   /**
    * @task internal
    */
-  private function buildOrderClause($conn_r) {
+  private function buildOrderClause(AphrontDatabaseConnection $conn) {
     if ($this->order == self::ORDER_NEWEST_FIRST) {
-      return 'ORDER BY edge.dateCreated DESC, edge.seq DESC';
+      return qsprintf($conn, 'ORDER BY edge.dateCreated DESC, edge.seq DESC');
     } else {
-      return 'ORDER BY edge.dateCreated ASC, edge.seq ASC';
+      return qsprintf($conn, 'ORDER BY edge.dateCreated ASC, edge.seq ASC');
     }
   }
 
diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
index 49ab55ef4f..162132d508 100644
--- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
+++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
@@ -123,12 +123,19 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
     AphrontDatabaseConnection $conn,
     $table_name) {
 
+    $table_alias = $this->getPrimaryTableAlias();
+    if ($table_alias === null) {
+      $table_alias = qsprintf($conn, '');
+    } else {
+      $table_alias = qsprintf($conn, '%T', $table_alias);
+    }
+
     return qsprintf(
       $conn,
       '%Q FROM %T %Q %Q %Q %Q %Q %Q %Q',
       $this->buildSelectClause($conn),
       $table_name,
-      (string)$this->getPrimaryTableAlias(),
+      $table_alias,
       $this->buildJoinClause($conn),
       $this->buildWhereClause($conn),
       $this->buildGroupClause($conn),
@@ -425,7 +432,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
     } else {
       // No paging is being applied to this query so we do not need to
       // construct a paging clause.
-      return '';
+      return qsprintf($conn, '');
     }
 
     $keys = array();
@@ -655,24 +662,16 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
           $conn,
           '%Q %Q %Q',
           $field,
-          $reverse ? '>' : '<',
+          $reverse ? qsprintf($conn, '>') : qsprintf($conn, '<'),
           $value);
       }
 
       if ($parts) {
-        if (count($parts) > 1) {
-          $clause[] = '('.implode(') OR (', $parts).')';
-        } else {
-          $clause[] = head($parts);
-        }
+        $clause[] = qsprintf($conn, '%LO', $parts);
       }
 
       if ($clause) {
-        if (count($clause) > 1) {
-          $clauses[] = '('.implode(') AND (', $clause).')';
-        } else {
-          $clauses[] = head($clause);
-        }
+        $clauses[] = qsprintf($conn, '%LA', $clause);
       }
 
       if ($value === null) {
@@ -689,7 +688,11 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
       }
     }
 
-    return '('.implode(') OR (', $clauses).')';
+    if ($clauses) {
+      return qsprintf($conn, '%LO', $clauses);
+    }
+
+    return qsprintf($conn, '');
   }
 
 
@@ -1315,7 +1318,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
       return qsprintf(
         $conn,
         'GROUP BY %Q',
-        $this->getApplicationSearchObjectPHIDColumn());
+        $this->getApplicationSearchObjectPHIDColumn($conn));
     } else {
       return qsprintf($conn, '');
     }
@@ -1339,16 +1342,16 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
       $alias = $constraint['alias'];
       $index = $constraint['index'];
       $cond = $constraint['cond'];
-      $phid_column = $this->getApplicationSearchObjectPHIDColumn();
+      $phid_column = $this->getApplicationSearchObjectPHIDColumn($conn);
       switch ($cond) {
         case '=':
           // Figure out whether we need to do a LEFT JOIN or not. We need to
           // LEFT JOIN if we're going to select "IS NULL" rows.
-          $join_type = 'JOIN';
+          $join_type = qsprintf($conn, 'JOIN');
           foreach ($constraint['constraints'] as $query_constraint) {
             $op = $query_constraint->getOperator();
             if ($op === PhabricatorQueryConstraint::OPERATOR_NULL) {
-              $join_type = 'LEFT JOIN';
+              $join_type = qsprintf($conn, 'LEFT JOIN');
               break;
             }
           }
@@ -2437,9 +2440,9 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
             // this to a LEFT join. We'll use WHERE to select matching rows
             // later.
             if ($has_null) {
-              $join_type = 'LEFT';
+              $join_type = qsprintf($conn, 'LEFT');
             } else {
-              $join_type = '';
+              $join_type = qsprintf($conn, '');
             }
 
             $joins[] = qsprintf(
@@ -2912,7 +2915,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
     if ($alias) {
       $col = qsprintf($conn, '%T.spacePHID', $alias);
     } else {
-      $col = 'spacePHID';
+      $col = qsprintf($conn, 'spacePHID');
     }
 
     if ($space_phids && $include_null) {
diff --git a/src/infrastructure/storage/lisk/LiskDAO.php b/src/infrastructure/storage/lisk/LiskDAO.php
index 03dbf51961..1095aadf59 100644
--- a/src/infrastructure/storage/lisk/LiskDAO.php
+++ b/src/infrastructure/storage/lisk/LiskDAO.php
@@ -1149,11 +1149,10 @@ abstract class LiskDAO extends Phobject
         $map[$key] = qsprintf($conn, '%C = %ns', $key, $value);
       }
     }
-    $map = implode(', ', $map);
 
     $id = $this->getID();
     $conn->query(
-      'UPDATE %R SET %Q WHERE %C = '.(is_int($id) ? '%d' : '%s'),
+      'UPDATE %R SET %LQ WHERE %C = '.(is_int($id) ? '%d' : '%s'),
       $this,
       $map,
       $this->getIDKeyForUse(),
@@ -1255,11 +1254,24 @@ abstract class LiskDAO extends Phobject
           $parameter_exception);
       }
     }
-    $data = implode(', ', $data);
+
+    switch ($mode) {
+      case 'INSERT':
+        $verb = qsprintf($conn, 'INSERT');
+        break;
+      case 'REPLACE':
+        $verb = qsprintf($conn, 'REPLACE');
+        break;
+      default:
+        throw new Exception(
+          pht(
+            'Insert mode verb "%s" is not recognized, use INSERT or REPLACE.',
+            $mode));
+    }
 
     $conn->query(
-      '%Q INTO %R (%LC) VALUES (%Q)',
-      $mode,
+      '%Q INTO %R (%LC) VALUES (%LQ)',
+      $verb,
       $this,
       $columns,
       $data);