Update PhabricatorLiskDAO::chunkSQL() for new %Q semantics

Summary:
Ref T13217. This method is slightly tricky:

  - We can't safely return a string: return an array instead.
  - It no longer makes sense to accept glue. All callers use `', '` as glue anyway, so hard-code that.

Then convert all callsites.

Test Plan: Browsed around, saw fewer "unsafe" errors in error log.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13217

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

resources/sql/autopatches/20140805.boardcol.2.php

@@ -45,7 +45,7 @@ foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) {
   queryfx(
     $conn_w,
     'INSERT INTO %T (boardPHID, columnPHID, objectPHID, sequence)
-      VALUES %Q',
+      VALUES %LQ',
     id(new PhabricatorProjectColumnPosition())->getTableName(),
     $chunk);
 }

resources/sql/patches/20130820.file-mailkey-populate.php

@@ -22,12 +22,12 @@ foreach (new LiskRawMigrationIterator($conn_w, 'file') as $row) {
 }
 
 if ($sql) {
-  foreach (PhabricatorLiskDAO::chunkSQL($sql, ', ') as $chunk) {
+  foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) {
     queryfx(
       $conn_w,
       'INSERT INTO %T
         (id, mailKey, phid, byteSize, storageEngine, storageFormat,
-          storageHandle, dateCreated, dateModified, metadata) VALUES %Q '.
+          storageHandle, dateCreated, dateModified, metadata) VALUES %LQ '.
       'ON DUPLICATE KEY UPDATE mailKey = VALUES(mailKey)',
       $table_name,
       $chunk);

resources/sql/patches/20131106.diffphid.2.mig.php

@@ -34,10 +34,10 @@ foreach ($chunk_iter as $chunk) {
     continue;
   }
 
-  foreach (PhabricatorLiskDAO::chunkSQL($sql, ', ') as $sql_chunk) {
+  foreach (PhabricatorLiskDAO::chunkSQL($sql) as $sql_chunk) {
     queryfx(
       $conn_w,
-      'INSERT IGNORE INTO %T (id, phid) VALUES %Q
+      'INSERT IGNORE INTO %T (id, phid) VALUES %LQ
         ON DUPLICATE KEY UPDATE phid = VALUES(phid)',
       $diff_table->getTableName(),
       $sql_chunk);

src/applications/cache/PhabricatorKeyValueDatabaseCache.php

@@ -38,7 +38,7 @@ final class PhabricatorKeyValueDatabaseCache
             $conn_w,
             'INSERT INTO %T
               (cacheKeyHash, cacheKey, cacheFormat, cacheData,
-                cacheCreated, cacheExpires) VALUES %Q
+                cacheCreated, cacheExpires) VALUES %LQ
               ON DUPLICATE KEY UPDATE
                 cacheKey = VALUES(cacheKey),
                 cacheFormat = VALUES(cacheFormat),

src/applications/calendar/notifications/PhabricatorCalendarNotificationEngine.php

@@ -245,7 +245,7 @@ final class PhabricatorCalendarNotificationEngine
         $conn,
         'INSERT IGNORE INTO %T
           (eventPHID, targetPHID, utcInitialEpoch, didNotifyEpoch)
-          VALUES %Q',
+          VALUES %LQ',
         $table->getTableName(),
         $chunk);
     }

src/applications/diffusion/conduit/DiffusionUpdateCoverageConduitAPIMethod.php

@@ -106,8 +106,8 @@ final class DiffusionUpdateCoverageConduitAPIMethod
       foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) {
         queryfx(
           $conn,
-          'INSERT INTO %T (branchID, pathID, commitID, coverage) VALUES %Q'.
+          'INSERT INTO %T (branchID, pathID, commitID, coverage) VALUES %LQ'.
-          ' ON DUPLICATE KEY UPDATE coverage=VALUES(coverage)',
+          ' ON DUPLICATE KEY UPDATE coverage = VALUES(coverage)',
           $table_name,
           $chunk);
       }

src/applications/diviner/publisher/DivinerLivePublisher.php

@@ -101,11 +101,11 @@ final class DivinerLivePublisher extends DivinerPublisher {
       $strings[] = qsprintf($conn_w, '%s', $hash);
     }
 
-    foreach (PhabricatorLiskDAO::chunkSQL($strings, ', ') as $chunk) {
+    foreach (PhabricatorLiskDAO::chunkSQL($strings) as $chunk) {
       queryfx(
         $conn_w,
         'UPDATE %T SET graphHash = NULL, nodeHash = NULL
-          WHERE graphHash IN (%Q)',
+          WHERE graphHash IN (%LQ)',
         $symbol_table->getTableName(),
         $chunk);
     }

src/applications/fact/daemon/PhabricatorFactDaemon.php

@@ -189,7 +189,7 @@ final class PhabricatorFactDaemon extends PhabricatorDaemon {
             $conn,
             'INSERT INTO %T
               (keyID, objectID, dimensionID, value, epoch)
-              VALUES %Q',
+              VALUES %LQ',
             $table_name,
             $chunk);
         }

src/applications/fact/storage/PhabricatorFactDimension.php

@@ -75,7 +75,7 @@ abstract class PhabricatorFactDimension extends PhabricatorFactDAO {
       foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) {
         queryfx(
           $conn,
-          'INSERT IGNORE INTO %T (%C) VALUES %Q',
+          'INSERT IGNORE INTO %T (%C) VALUES %LQ',
           $this->getTableName(),
           $column,
           $chunk);

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -531,7 +531,7 @@ final class ManiphestTransactionEditor
     foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) {
       queryfx(
         $conn,
-        'INSERT INTO %T (id, %Q, subpriority) VALUES %Q
+        'INSERT INTO %T (id, %Q, subpriority) VALUES %LQ
           ON DUPLICATE KEY UPDATE subpriority = VALUES(subpriority)',
         $task->getTableName(),
         implode(', ', array_keys($extra_columns)),

src/applications/people/storage/PhabricatorUserCache.php

@@ -85,7 +85,7 @@ final class PhabricatorUserCache extends PhabricatorUserDAO {
       queryfx(
         $conn_w,
         'INSERT INTO %T (userPHID, cacheIndex, cacheKey, cacheData, cacheType)
-          VALUES %Q
+          VALUES %LQ
           ON DUPLICATE KEY UPDATE
             cacheData = VALUES(cacheData),
             cacheType = VALUES(cacheType)',

src/applications/project/storage/PhabricatorProject.php

@@ -467,7 +467,7 @@ final class PhabricatorProject extends PhabricatorProjectDAO
       foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) {
         queryfx(
           $conn_w,
-          'INSERT INTO %T (projectID, token) VALUES %Q',
+          'INSERT INTO %T (projectID, token) VALUES %LQ',
           $table,
           $chunk);
       }

src/applications/repository/management/PhabricatorRepositoryManagementParentsWorkflow.php

@@ -161,7 +161,7 @@ final class PhabricatorRepositoryManagementParentsWorkflow
     foreach (PhabricatorLiskDAO::chunkSQL($delete_sql) as $chunk) {
       queryfx(
         $conn_w,
-        'DELETE FROM %T WHERE childCommitID IN (%Q)',
+        'DELETE FROM %T WHERE childCommitID IN (%LQ)',
         PhabricatorRepository::TABLE_PARENTS,
         $chunk);
     }
@@ -169,7 +169,7 @@ final class PhabricatorRepositoryManagementParentsWorkflow
     foreach (PhabricatorLiskDAO::chunkSQL($insert_sql) as $chunk) {
       queryfx(
         $conn_w,
-        'INSERT INTO %T (childCommitID, parentCommitID) VALUES %Q',
+        'INSERT INTO %T (childCommitID, parentCommitID) VALUES %LQ',
         PhabricatorRepository::TABLE_PARENTS,
         $chunk);
     }

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

@@ -151,7 +151,7 @@ abstract class PhabricatorRepositoryCommitChangeParserWorker
         'INSERT INTO %T
           (repositoryID, pathID, commitID, targetPathID, targetCommitID,
             changeType, fileType, isDirect, commitSequence)
-          VALUES %Q',
+          VALUES %LQ',
         PhabricatorRepository::TABLE_PATHCHANGE,
         $chunk);
     }

src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php

@@ -201,7 +201,7 @@ final class PhabricatorFerretFulltextEngineExtension
         foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) {
           queryfx(
             $conn,
-            'INSERT INTO %T (documentID, ngram) VALUES %Q',
+            'INSERT INTO %T (documentID, ngram) VALUES %LQ',
             $engine->getNgramsTableName(),
             $chunk);
         }

src/applications/search/management/PhabricatorSearchManagementNgramsWorkflow.php

@@ -123,7 +123,7 @@ final class PhabricatorSearchManagementNgramsWorkflow
         queryfx(
           $conn,
           'INSERT IGNORE INTO %T (ngram, needsCollection)
-            VALUES %Q',
+            VALUES %LQ',
           $engine->getCommonNgramsTableName(),
           $chunk);
       }

src/applications/system/engine/PhabricatorSystemActionEngine.php

@@ -153,7 +153,7 @@ final class PhabricatorSystemActionEngine extends Phobject {
       queryfx(
         $conn_w,
         'INSERT INTO %T (actorHash, actorIdentity, action, score, epoch)
-          VALUES %Q',
+          VALUES %LQ',
         $log->getTableName(),
         $chunk);
     }

src/infrastructure/customfield/field/PhabricatorCustomFieldList.php

@@ -303,7 +303,7 @@ final class PhabricatorCustomFieldList extends Phobject {
         foreach (PhabricatorLiskDAO::chunkSQL($sql_list) as $chunk) {
           queryfx(
             $conn_w,
-            'INSERT INTO %T (objectPHID, indexKey, indexValue) VALUES %Q',
+            'INSERT INTO %T (objectPHID, indexKey, indexValue) VALUES %LQ',
             $table,
             $chunk);
         }

src/infrastructure/storage/__tests__/QueryFormattingTestCase.php

@@ -7,15 +7,15 @@ final class QueryFormattingTestCase extends PhabricatorTestCase {
 
     $this->assertEqual(
       'NULL',
-      qsprintf($conn, '%nd', null));
+      (string)qsprintf($conn, '%nd', null));
 
     $this->assertEqual(
       '0',
-      qsprintf($conn, '%nd', 0));
+      (string)qsprintf($conn, '%nd', 0));
 
     $this->assertEqual(
       '0',
-      qsprintf($conn, '%d', 0));
+      (string)qsprintf($conn, '%d', 0));
 
     $raised = null;
     try {
@@ -29,39 +29,39 @@ final class QueryFormattingTestCase extends PhabricatorTestCase {
 
     $this->assertEqual(
       "'<S>'",
-      qsprintf($conn, '%s', null));
+      (string)qsprintf($conn, '%s', null));
 
     $this->assertEqual(
       'NULL',
-      qsprintf($conn, '%ns', null));
+      (string)qsprintf($conn, '%ns', null));
 
     $this->assertEqual(
       "'<S>', '<S>'",
-      qsprintf($conn, '%Ls', array('x', 'y')));
+      (string)qsprintf($conn, '%Ls', array('x', 'y')));
 
     $this->assertEqual(
       "'<B>'",
-      qsprintf($conn, '%B', null));
+      (string)qsprintf($conn, '%B', null));
 
     $this->assertEqual(
       'NULL',
-      qsprintf($conn, '%nB', null));
+      (string)qsprintf($conn, '%nB', null));
 
     $this->assertEqual(
       "'<B>', '<B>'",
-      qsprintf($conn, '%LB', array('x', 'y')));
+      (string)qsprintf($conn, '%LB', array('x', 'y')));
 
     $this->assertEqual(
       '<C>',
-      qsprintf($conn, '%T', 'x'));
+      (string)qsprintf($conn, '%T', 'x'));
 
     $this->assertEqual(
       '<C>',
-      qsprintf($conn, '%C', 'y'));
+      (string)qsprintf($conn, '%C', 'y'));
 
     $this->assertEqual(
       '<C>.<C>',
-      qsprintf($conn, '%R', new AphrontDatabaseTableRef('x', 'y')));
+      (string)qsprintf($conn, '%R', new AphrontDatabaseTableRef('x', 'y')));
 
   }
 

src/infrastructure/storage/lisk/PhabricatorLiskDAO.php

@@ -196,14 +196,11 @@ abstract class PhabricatorLiskDAO extends LiskDAO {
    * INSERT, previously built with @{function:qsprintf}) into chunks which will
    * fit under the MySQL 'max_allowed_packet' limit.
    *
-   * Chunks are glued together with `$glue`, by default ", ".
-   *
    * If a statement is too large to fit within the limit, it is broken into
    * its own chunk (but might fail when the query executes).
    */
   public static function chunkSQL(
     array $fragments,
-    $glue = ', ',
     $limit = null) {
 
     if ($limit === null) {
@@ -216,9 +213,13 @@ abstract class PhabricatorLiskDAO extends LiskDAO {
 
     $chunk = array();
     $len = 0;
-    $glue_len = strlen($glue);
+    $glue_len = strlen(', ');
     foreach ($fragments as $fragment) {
-      $this_len = strlen($fragment);
+      if ($fragment instanceof PhutilQueryString) {
+        $this_len = strlen($fragment->getUnmaskedString());
+      } else {
+        $this_len = strlen($fragment);
+      }
 
       if ($chunk) {
         // Chunks after the first also imply glue.
@@ -232,7 +233,7 @@ abstract class PhabricatorLiskDAO extends LiskDAO {
         if ($chunk) {
           $result[] = $chunk;
         }
-        $len = strlen($fragment);
+        $len = ($this_len - $glue_len);
         $chunk = array($fragment);
       }
     }
@@ -241,10 +242,6 @@ abstract class PhabricatorLiskDAO extends LiskDAO {
       $result[] = $chunk;
     }
 
-    foreach ($result as $key => $fragment_list) {
-      $result[$key] = implode($glue, $fragment_list);
-    }
-
     return $result;
   }
 

src/infrastructure/storage/lisk/__tests__/LiskChunkTestCase.php

@@ -15,13 +15,15 @@ final class LiskChunkTestCase extends PhabricatorTestCase {
 
     $this->assertEqual(
       array(
-        'aa',
+        array('a'),
-        'bb',
+        array('a'),
-        'ccc',
+        array('b'),
-        'dd',
+        array('b'),
-        'e',
+        array('ccc'),
+        array('dd'),
+        array('e'),
       ),
-      PhabricatorLiskDAO::chunkSQL($fragments, '', 2));
+      PhabricatorLiskDAO::chunkSQL($fragments, 2));
 
 
     $fragments = array(
@@ -37,11 +39,11 @@ final class LiskChunkTestCase extends PhabricatorTestCase {
 
     $this->assertEqual(
       array(
-        'a, a, a',
+        array('a', 'a', 'a'),
-        'XX, a, a',
+        array('XX', 'a', 'a'),
-        'a, a',
+        array('a', 'a'),
       ),
-      PhabricatorLiskDAO::chunkSQL($fragments, ', ', 8));
+      PhabricatorLiskDAO::chunkSQL($fragments, 8));
 
 
     $fragments = array(
@@ -55,12 +57,12 @@ final class LiskChunkTestCase extends PhabricatorTestCase {
 
     $this->assertEqual(
       array(
-        'xxxxxxxxxx',
+        array('xxxxxxxxxx'),
-        'yyyyyyyyyy',
+        array('yyyyyyyyyy'),
-        'a, b, c',
+        array('a', 'b', 'c'),
-        'zzzzzzzzzz',
+        array('zzzzzzzzzz'),
       ),
-      PhabricatorLiskDAO::chunkSQL($fragments, ', ', 8));
+      PhabricatorLiskDAO::chunkSQL($fragments, 8));
   }
 
 }