From 798a391e5a9031876c8e9fef471ad156428ae439 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 26 Oct 2018 07:56:15 -0700 Subject: [PATCH] Add test coverage for "%R" in qsprintf() and convert LiskDAO to support it Summary: Ref T13210. Ref T11908. Add some basic test coverage for the new "%R" introduced in D19764, then convert LiskDAO to implement the "Database + Table Ref" interface. To move forward, we need to convert all of these (where `%T` is not a table alias): ```counterexample qsprintf($conn, '... %T ...', $thing->getTableName()); ``` ...to these: ``` qsprintf($conn, '... %R ...', $thing); ``` The new code is a little simpler (no `->getTableName()` call) which is sort of nice. But we also have a //lot// of `%T` so this is probably going to take a while. (I'll hold this until after the release cut.) Test Plan: - Ran unit tests. - Browsed around and edited some objects without issues. This change causes a reasonably large percentage of our total queries to use the new code since the LiskDAO builtin queries are some of the most commonly-constructed queries, although there are still ~700 callsites which need to be examined for possible conversion. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13210, T11908 Differential Revision: https://secure.phabricator.com/D19765 --- src/__phutil_library_map__.php | 5 ++- .../__tests__/QueryFormattingTestCase.php | 35 ++++++++++++------ src/infrastructure/storage/lisk/LiskDAO.php | 37 ++++++++++++++----- .../storage/lisk/PhabricatorLiskDAO.php | 3 +- .../lisk/__tests__/LiskIsolationTestDAO.php | 2 +- 5 files changed, 57 insertions(+), 25 deletions(-) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 7a9ea1df70..11a939c489 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -7131,7 +7131,10 @@ phutil_register_library_map(array( 'LegalpadTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'LegalpadTransactionView' => 'PhabricatorApplicationTransactionView', 'LiskChunkTestCase' => 'PhabricatorTestCase', - 'LiskDAO' => 'Phobject', + 'LiskDAO' => array( + 'Phobject', + 'AphrontDatabaseTableRefInterface', + ), 'LiskDAOSet' => 'Phobject', 'LiskDAOTestCase' => 'PhabricatorTestCase', 'LiskEphemeralObjectException' => 'Exception', diff --git a/src/infrastructure/storage/__tests__/QueryFormattingTestCase.php b/src/infrastructure/storage/__tests__/QueryFormattingTestCase.php index 6bd0eeedaf..53d16a7948 100644 --- a/src/infrastructure/storage/__tests__/QueryFormattingTestCase.php +++ b/src/infrastructure/storage/__tests__/QueryFormattingTestCase.php @@ -3,23 +3,23 @@ final class QueryFormattingTestCase extends PhabricatorTestCase { public function testQueryFormatting() { - $conn_r = id(new PhabricatorUser())->establishConnection('r'); + $conn = id(new PhabricatorUser())->establishConnection('r'); $this->assertEqual( 'NULL', - qsprintf($conn_r, '%nd', null)); + qsprintf($conn, '%nd', null)); $this->assertEqual( '0', - qsprintf($conn_r, '%nd', 0)); + qsprintf($conn, '%nd', 0)); $this->assertEqual( '0', - qsprintf($conn_r, '%d', 0)); + qsprintf($conn, '%d', 0)); $raised = null; try { - qsprintf($conn_r, '%d', 'derp'); + qsprintf($conn, '%d', 'derp'); } catch (Exception $ex) { $raised = $ex; } @@ -29,27 +29,40 @@ final class QueryFormattingTestCase extends PhabricatorTestCase { $this->assertEqual( "''", - qsprintf($conn_r, '%s', null)); + qsprintf($conn, '%s', null)); $this->assertEqual( 'NULL', - qsprintf($conn_r, '%ns', null)); + qsprintf($conn, '%ns', null)); $this->assertEqual( "'', ''", - qsprintf($conn_r, '%Ls', array('x', 'y'))); + qsprintf($conn, '%Ls', array('x', 'y'))); $this->assertEqual( "''", - qsprintf($conn_r, '%B', null)); + qsprintf($conn, '%B', null)); $this->assertEqual( 'NULL', - qsprintf($conn_r, '%nB', null)); + qsprintf($conn, '%nB', null)); $this->assertEqual( "'', ''", - qsprintf($conn_r, '%LB', array('x', 'y'))); + qsprintf($conn, '%LB', array('x', 'y'))); + + $this->assertEqual( + '', + qsprintf($conn, '%T', 'x')); + + $this->assertEqual( + '', + qsprintf($conn, '%C', 'y')); + + $this->assertEqual( + '.', + qsprintf($conn, '%R', new AphrontDatabaseTableRef('x', 'y'))); + } diff --git a/src/infrastructure/storage/lisk/LiskDAO.php b/src/infrastructure/storage/lisk/LiskDAO.php index 0c940ef5f0..583d8226f6 100644 --- a/src/infrastructure/storage/lisk/LiskDAO.php +++ b/src/infrastructure/storage/lisk/LiskDAO.php @@ -162,7 +162,8 @@ * @task xaction Managing Transactions * @task isolate Isolation for Unit Testing */ -abstract class LiskDAO extends Phobject { +abstract class LiskDAO extends Phobject + implements AphrontDatabaseTableRefInterface { const CONFIG_IDS = 'id-mechanism'; const CONFIG_TIMESTAMPS = 'timestamps'; @@ -235,8 +236,11 @@ abstract class LiskDAO extends Phobject { * @return string Connection namespace for cache * @task conn */ - abstract protected function getConnectionNamespace(); + protected function getConnectionNamespace() { + return $this->getDatabaseName(); + } + abstract protected function getDatabaseName(); /** * Get an existing, cached connection for this object. @@ -525,8 +529,8 @@ abstract class LiskDAO extends Phobject { $args = func_get_args(); $args = array_slice($args, 1); - $pattern = 'SELECT * FROM %T WHERE '.$pattern.' %Q'; - array_unshift($args, $this->getTableName()); + $pattern = 'SELECT * FROM %R WHERE '.$pattern.' %Q'; + array_unshift($args, $this); array_push($args, $lock_clause); array_unshift($args, $pattern); @@ -1150,8 +1154,8 @@ abstract class LiskDAO extends Phobject { $id = $this->getID(); $conn->query( - 'UPDATE %T SET %Q WHERE %C = '.(is_int($id) ? '%d' : '%s'), - $this->getTableName(), + 'UPDATE %R SET %Q WHERE %C = '.(is_int($id) ? '%d' : '%s'), + $this, $map, $this->getIDKeyForUse(), $id); @@ -1178,8 +1182,8 @@ abstract class LiskDAO extends Phobject { $conn = $this->establishConnection('w'); $conn->query( - 'DELETE FROM %T WHERE %C = %d', - $this->getTableName(), + 'DELETE FROM %R WHERE %C = %d', + $this, $this->getIDKeyForUse(), $this->getID()); @@ -1255,9 +1259,9 @@ abstract class LiskDAO extends Phobject { $data = implode(', ', $data); $conn->query( - '%Q INTO %T (%LC) VALUES (%Q)', + '%Q INTO %R (%LC) VALUES (%Q)', $mode, - $this->getTableName(), + $this, $columns, $data); @@ -2019,4 +2023,17 @@ abstract class LiskDAO extends Phobject { ->getMaximumByteLengthForDataType($data_type); } + +/* -( AphrontDatabaseTableRefInterface )----------------------------------- */ + + + public function getAphrontRefDatabaseName() { + return $this->getDatabaseName(); + } + + public function getAphrontRefTableName() { + return $this->getTableName(); + } + + } diff --git a/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php b/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php index b3b324e951..b300efbf4b 100644 --- a/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php +++ b/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php @@ -187,11 +187,10 @@ abstract class PhabricatorLiskDAO extends LiskDAO { */ abstract public function getApplicationName(); - protected function getConnectionNamespace() { + protected function getDatabaseName() { return self::getStorageNamespace().'_'.$this->getApplicationName(); } - /** * Break a list of escaped SQL statement fragments (e.g., VALUES lists for * INSERT, previously built with @{function:qsprintf}) into chunks which will diff --git a/src/infrastructure/storage/lisk/__tests__/LiskIsolationTestDAO.php b/src/infrastructure/storage/lisk/__tests__/LiskIsolationTestDAO.php index d9070753f8..8008fb8e46 100644 --- a/src/infrastructure/storage/lisk/__tests__/LiskIsolationTestDAO.php +++ b/src/infrastructure/storage/lisk/__tests__/LiskIsolationTestDAO.php @@ -22,7 +22,7 @@ final class LiskIsolationTestDAO extends LiskDAO { 'resource!')); } - protected function getConnectionNamespace() { + protected function getDatabaseName() { return 'test'; }