From da40f807410634ac8220bfe47649cdabfe6a65f4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 6 Nov 2018 16:57:55 -0800 Subject: [PATCH] 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 --- .../sql/autopatches/20140805.boardcol.2.php | 2 +- .../20130820.file-mailkey-populate.php | 4 +-- .../sql/patches/20131106.diffphid.2.mig.php | 4 +-- .../PhabricatorKeyValueDatabaseCache.php | 2 +- .../PhabricatorCalendarNotificationEngine.php | 2 +- ...iffusionUpdateCoverageConduitAPIMethod.php | 4 +-- .../publisher/DivinerLivePublisher.php | 4 +-- .../fact/daemon/PhabricatorFactDaemon.php | 2 +- .../fact/storage/PhabricatorFactDimension.php | 2 +- .../editor/ManiphestTransactionEditor.php | 2 +- .../people/storage/PhabricatorUserCache.php | 2 +- .../project/storage/PhabricatorProject.php | 2 +- ...torRepositoryManagementParentsWorkflow.php | 4 +-- ...atorRepositoryCommitChangeParserWorker.php | 2 +- ...abricatorFerretFulltextEngineExtension.php | 2 +- ...bricatorSearchManagementNgramsWorkflow.php | 2 +- .../engine/PhabricatorSystemActionEngine.php | 2 +- .../field/PhabricatorCustomFieldList.php | 2 +- .../__tests__/QueryFormattingTestCase.php | 24 +++++++------- .../storage/lisk/PhabricatorLiskDAO.php | 17 ++++------ .../lisk/__tests__/LiskChunkTestCase.php | 32 ++++++++++--------- 21 files changed, 59 insertions(+), 60 deletions(-) diff --git a/resources/sql/autopatches/20140805.boardcol.2.php b/resources/sql/autopatches/20140805.boardcol.2.php index 317de4e370..40d6c46ec2 100644 --- a/resources/sql/autopatches/20140805.boardcol.2.php +++ b/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); } diff --git a/resources/sql/patches/20130820.file-mailkey-populate.php b/resources/sql/patches/20130820.file-mailkey-populate.php index ba4d6d1606..0db10bef58 100644 --- a/resources/sql/patches/20130820.file-mailkey-populate.php +++ b/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); diff --git a/resources/sql/patches/20131106.diffphid.2.mig.php b/resources/sql/patches/20131106.diffphid.2.mig.php index 67fd14aad0..7976c910eb 100644 --- a/resources/sql/patches/20131106.diffphid.2.mig.php +++ b/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); diff --git a/src/applications/cache/PhabricatorKeyValueDatabaseCache.php b/src/applications/cache/PhabricatorKeyValueDatabaseCache.php index c6a52024fe..0b4609074a 100644 --- a/src/applications/cache/PhabricatorKeyValueDatabaseCache.php +++ b/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), diff --git a/src/applications/calendar/notifications/PhabricatorCalendarNotificationEngine.php b/src/applications/calendar/notifications/PhabricatorCalendarNotificationEngine.php index 59d8476c87..cf7fc30554 100644 --- a/src/applications/calendar/notifications/PhabricatorCalendarNotificationEngine.php +++ b/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); } diff --git a/src/applications/diffusion/conduit/DiffusionUpdateCoverageConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionUpdateCoverageConduitAPIMethod.php index be0d2c4faa..256d023345 100644 --- a/src/applications/diffusion/conduit/DiffusionUpdateCoverageConduitAPIMethod.php +++ b/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'. - ' ON DUPLICATE KEY UPDATE coverage=VALUES(coverage)', + 'INSERT INTO %T (branchID, pathID, commitID, coverage) VALUES %LQ'. + ' ON DUPLICATE KEY UPDATE coverage = VALUES(coverage)', $table_name, $chunk); } diff --git a/src/applications/diviner/publisher/DivinerLivePublisher.php b/src/applications/diviner/publisher/DivinerLivePublisher.php index 80f3bd7a1e..e9e1848d6e 100644 --- a/src/applications/diviner/publisher/DivinerLivePublisher.php +++ b/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); } diff --git a/src/applications/fact/daemon/PhabricatorFactDaemon.php b/src/applications/fact/daemon/PhabricatorFactDaemon.php index 3fa6c6f0ff..57813b3021 100644 --- a/src/applications/fact/daemon/PhabricatorFactDaemon.php +++ b/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); } diff --git a/src/applications/fact/storage/PhabricatorFactDimension.php b/src/applications/fact/storage/PhabricatorFactDimension.php index 9c05121c9c..5644da331e 100644 --- a/src/applications/fact/storage/PhabricatorFactDimension.php +++ b/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); diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index 66c523573f..19fd57b0d2 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/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)), diff --git a/src/applications/people/storage/PhabricatorUserCache.php b/src/applications/people/storage/PhabricatorUserCache.php index 2afb06a437..71b3e4816b 100644 --- a/src/applications/people/storage/PhabricatorUserCache.php +++ b/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)', diff --git a/src/applications/project/storage/PhabricatorProject.php b/src/applications/project/storage/PhabricatorProject.php index a049354ad7..f134b2c633 100644 --- a/src/applications/project/storage/PhabricatorProject.php +++ b/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); } diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementParentsWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementParentsWorkflow.php index 671287c3d6..e201ef40fc 100644 --- a/src/applications/repository/management/PhabricatorRepositoryManagementParentsWorkflow.php +++ b/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); } diff --git a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php index 6d79742a32..afbf42af97 100644 --- a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php +++ b/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); } diff --git a/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php b/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php index fe50518f3f..8647bc83eb 100644 --- a/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php +++ b/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); } diff --git a/src/applications/search/management/PhabricatorSearchManagementNgramsWorkflow.php b/src/applications/search/management/PhabricatorSearchManagementNgramsWorkflow.php index 1a1124b4d8..3d87378849 100644 --- a/src/applications/search/management/PhabricatorSearchManagementNgramsWorkflow.php +++ b/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); } diff --git a/src/applications/system/engine/PhabricatorSystemActionEngine.php b/src/applications/system/engine/PhabricatorSystemActionEngine.php index b6a8e26711..6b8352a29e 100644 --- a/src/applications/system/engine/PhabricatorSystemActionEngine.php +++ b/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); } diff --git a/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php b/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php index efe12e68b8..8ae5529f95 100644 --- a/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php +++ b/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); } diff --git a/src/infrastructure/storage/__tests__/QueryFormattingTestCase.php b/src/infrastructure/storage/__tests__/QueryFormattingTestCase.php index 53d16a7948..cff7390949 100644 --- a/src/infrastructure/storage/__tests__/QueryFormattingTestCase.php +++ b/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( "''", - qsprintf($conn, '%s', null)); + (string)qsprintf($conn, '%s', null)); $this->assertEqual( 'NULL', - qsprintf($conn, '%ns', null)); + (string)qsprintf($conn, '%ns', null)); $this->assertEqual( "'', ''", - qsprintf($conn, '%Ls', array('x', 'y'))); + (string)qsprintf($conn, '%Ls', array('x', 'y'))); $this->assertEqual( "''", - qsprintf($conn, '%B', null)); + (string)qsprintf($conn, '%B', null)); $this->assertEqual( 'NULL', - qsprintf($conn, '%nB', null)); + (string)qsprintf($conn, '%nB', null)); $this->assertEqual( "'', ''", - qsprintf($conn, '%LB', array('x', 'y'))); + (string)qsprintf($conn, '%LB', array('x', 'y'))); $this->assertEqual( '', - qsprintf($conn, '%T', 'x')); + (string)qsprintf($conn, '%T', 'x')); $this->assertEqual( '', - qsprintf($conn, '%C', 'y')); + (string)qsprintf($conn, '%C', 'y')); $this->assertEqual( '.', - qsprintf($conn, '%R', new AphrontDatabaseTableRef('x', 'y'))); + (string)qsprintf($conn, '%R', new AphrontDatabaseTableRef('x', 'y'))); } diff --git a/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php b/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php index b300efbf4b..e47d701df9 100644 --- a/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php +++ b/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; } diff --git a/src/infrastructure/storage/lisk/__tests__/LiskChunkTestCase.php b/src/infrastructure/storage/lisk/__tests__/LiskChunkTestCase.php index 8ce7608a8b..66ffdb17bc 100644 --- a/src/infrastructure/storage/lisk/__tests__/LiskChunkTestCase.php +++ b/src/infrastructure/storage/lisk/__tests__/LiskChunkTestCase.php @@ -15,13 +15,15 @@ final class LiskChunkTestCase extends PhabricatorTestCase { $this->assertEqual( array( - 'aa', - 'bb', - 'ccc', - 'dd', - 'e', + array('a'), + array('a'), + array('b'), + array('b'), + 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', - 'XX, a, a', - 'a, a', + array('a', 'a', 'a'), + array('XX', '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', - 'yyyyyyyyyy', - 'a, b, c', - 'zzzzzzzzzz', + array('xxxxxxxxxx'), + array('yyyyyyyyyy'), + array('a', 'b', 'c'), + array('zzzzzzzzzz'), ), - PhabricatorLiskDAO::chunkSQL($fragments, ', ', 8)); + PhabricatorLiskDAO::chunkSQL($fragments, 8)); } }