From 86fd2041484fe600c6004170e101e8d1d1f17de2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 13 Nov 2018 09:32:52 -0800 Subject: [PATCH] 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 --- .../schema/PhabricatorConfigSchemaQuery.php | 4 +- .../diffusion/query/DiffusionCommitQuery.php | 2 +- .../diffusion/query/DiffusionPathQuery.php | 14 ++-- .../files/query/PhabricatorFileQuery.php | 2 +- .../people/storage/PhabricatorUser.php | 7 +- .../phortune/query/PhortuneAccountQuery.php | 44 +++++------- .../engine/PhabricatorBoardLayoutEngine.php | 4 +- .../project/query/PhabricatorProjectQuery.php | 2 +- .../query/PhabricatorRepositoryQuery.php | 6 +- ...atorRepositoryCommitChangeParserWorker.php | 4 +- ...rRepositorySvnCommitChangeParserWorker.php | 4 +- ...PhabricatorCursorPagedPolicyAwareQuery.php | 2 +- .../storage/lisk/LiskDAOSet.php | 11 ++- .../PhabricatorStorageManagementAPI.php | 4 +- .../PhabricatorStorageManagementWorkflow.php | 70 ++++++++++++++++--- 15 files changed, 121 insertions(+), 59 deletions(-) diff --git a/src/applications/config/schema/PhabricatorConfigSchemaQuery.php b/src/applications/config/schema/PhabricatorConfigSchemaQuery.php index 2cb7763110..4a93396671 100644 --- a/src/applications/config/schema/PhabricatorConfigSchemaQuery.php +++ b/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)', - '('.implode(') OR (', $sql).')'); + WHERE %LO', + $sql); $column_info = igroup($column_info, 'TABLE_SCHEMA'); } else { $column_info = array(); diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php index f2d0920b3a..cf8c25baf1 100644 --- a/src/applications/diffusion/query/DiffusionCommitQuery.php +++ b/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) { diff --git a/src/applications/diffusion/query/DiffusionPathQuery.php b/src/applications/diffusion/query/DiffusionPathQuery.php index 45dc978ec1..0783643dc0 100644 --- a/src/applications/diffusion/query/DiffusionPathQuery.php +++ b/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, ''); } } diff --git a/src/applications/files/query/PhabricatorFileQuery.php b/src/applications/files/query/PhabricatorFileQuery.php index 9f1659a7f3..4205ab5c5d 100644 --- a/src/applications/files/query/PhabricatorFileQuery.php +++ b/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) { diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 216e451517..651d1b75a6 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/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)')); } diff --git a/src/applications/phortune/query/PhortuneAccountQuery.php b/src/applications/phortune/query/PhortuneAccountQuery.php index 4ada4f2845..ee8291218c 100644 --- a/src/applications/phortune/query/PhortuneAccountQuery.php +++ b/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(); - $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); + return $this->loadStandardPage($this->newResultObject()); } protected function willFilterPage(array $accounts) { @@ -73,39 +65,37 @@ final class PhortuneAccountQuery return $accounts; } - protected function buildWhereClause(AphrontDatabaseConnection $conn) { - $where = array(); + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); - $where[] = $this->buildPagingClause($conn); - - if ($this->ids) { + if ($this->ids !== null) { $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) { - $joins = array(); + protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) { + $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'; + } + } diff --git a/src/applications/project/engine/PhabricatorBoardLayoutEngine.php b/src/applications/project/engine/PhabricatorBoardLayoutEngine.php index 248677fbec..863f8c0e8c 100644 --- a/src/applications/project/engine/PhabricatorBoardLayoutEngine.php +++ b/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) { diff --git a/src/applications/project/query/PhabricatorProjectQuery.php b/src/applications/project/query/PhabricatorProjectQuery.php index 98713078fb..e3a02edbd0 100644 --- a/src/applications/project/query/PhabricatorProjectQuery.php +++ b/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, diff --git a/src/applications/repository/query/PhabricatorRepositoryQuery.php b/src/applications/repository/query/PhabricatorRepositoryQuery.php index 4175ffc2d5..56c62cc8fb 100644 --- a/src/applications/repository/query/PhabricatorRepositoryQuery.php +++ b/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', - id(new PhabricatorRepositoryURIIndex())->getTableName()); + 'LEFT JOIN %R uri ON r.phid = uri.repositoryPHID', + 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) { diff --git a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php index afbf42af97..b544228561 100644 --- a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php +++ b/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); } diff --git a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositorySvnCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositorySvnCommitChangeParserWorker.php index 6725ddb1d1..6be2251f1e 100644 --- a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositorySvnCommitChangeParserWorker.php +++ b/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); } } diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index 162132d508..2d91f5e608 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/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, diff --git a/src/infrastructure/storage/lisk/LiskDAOSet.php b/src/infrastructure/storage/lisk/LiskDAOSet.php index e1bc1e50d1..90eba708ea 100644 --- a/src/infrastructure/storage/lisk/LiskDAOSet.php +++ b/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); } } diff --git a/src/infrastructure/storage/management/PhabricatorStorageManagementAPI.php b/src/infrastructure/storage/management/PhabricatorStorageManagementAPI.php index 19d7a98d42..e66ba784f7 100644 --- a/src/infrastructure/storage/management/PhabricatorStorageManagementAPI.php +++ b/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( diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php index 5b9459cfb8..5bc83972dd 100644 --- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php +++ b/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'], - $adjust['collation']); + $charset_value, + $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), - $adjust['nullable'] ? 'NULL' : 'NOT NULL'); + $parts, + $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: