1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-22 14:52:41 +01:00

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
This commit is contained in:
epriestley 2018-10-26 07:56:15 -07:00
parent d2316e8025
commit 798a391e5a
5 changed files with 57 additions and 25 deletions

View file

@ -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',

View file

@ -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(
"'<S>'",
qsprintf($conn_r, '%s', null));
qsprintf($conn, '%s', null));
$this->assertEqual(
'NULL',
qsprintf($conn_r, '%ns', null));
qsprintf($conn, '%ns', null));
$this->assertEqual(
"'<S>', '<S>'",
qsprintf($conn_r, '%Ls', array('x', 'y')));
qsprintf($conn, '%Ls', array('x', 'y')));
$this->assertEqual(
"'<B>'",
qsprintf($conn_r, '%B', null));
qsprintf($conn, '%B', null));
$this->assertEqual(
'NULL',
qsprintf($conn_r, '%nB', null));
qsprintf($conn, '%nB', null));
$this->assertEqual(
"'<B>', '<B>'",
qsprintf($conn_r, '%LB', array('x', 'y')));
qsprintf($conn, '%LB', array('x', 'y')));
$this->assertEqual(
'<C>',
qsprintf($conn, '%T', 'x'));
$this->assertEqual(
'<C>',
qsprintf($conn, '%C', 'y'));
$this->assertEqual(
'<C>.<C>',
qsprintf($conn, '%R', new AphrontDatabaseTableRef('x', 'y')));
}

View file

@ -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();
}
}

View file

@ -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

View file

@ -22,7 +22,7 @@ final class LiskIsolationTestDAO extends LiskDAO {
'resource!'));
}
protected function getConnectionNamespace() {
protected function getDatabaseName() {
return 'test';
}