Page MenuHomePhabricator

Add test coverage for "%R" in qsprintf() and convert LiskDAO to support it
ClosedPublic

Authored by epriestley on Oct 26 2018, 3:14 PM.

Details

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):

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.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Oct 26 2018, 3:14 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 26 2018, 3:14 PM
Harbormaster failed remote builds in B21055: Diff 47210!
epriestley requested review of this revision.Oct 26 2018, 3:15 PM

Test failure is because this depends on D19764, which is in libphutil/.

amckinley accepted this revision.Nov 2 2018, 6:26 PM
amckinley added inline comments.
src/infrastructure/storage/__tests__/QueryFormattingTestCase.php
53–64

I'm not sure how to read these test definitions. I was looking around for QueryFormattingTestCase to redefine assertEqual, since it doesn't make a lot of sense to me that qsprintf would actually return strings like '<C>.<C>'. Can you give me some insight?

This revision is now accepted and ready to land.Nov 2 2018, 6:26 PM

Yeah, this is a bit unusual. The magic is that $conn is an AphrontIsolatedDatabaseConnection ("Isolated"), not a normal AphrontMySQLiDatabaseConnection.php ("MySQLi"). "Isolated" is the default type of connection when running in unit tests, unless you opt the test into a real database connection.

Since an isolated database connection doesn't actually connect to any kind of database server, it never needs to actually build a real query string. We just implement all the escape functions in a trivial way to make unit testing and some aspects of connection/query isolation easier -- and the escaping itself, since we don't have a real $conn and thus can't call mysql_real_escape_string($string, $conn) to get real escaping. We could fake it with ad-hoc addcslashes()-based escaping or something, but since we don't actually need to escape things and nothing will ever evaluate these queries, these trivial implementations are sufficient:

AphrontIsolatedDatabaseConnection.php
...

  public function escapeUTF8String($string) {
    return '<S>';
  }

  public function escapeBinaryString($string) {
    return '<B>';
  }

  public function escapeColumnName($name) {
    return '<C>';
  }

  public function escapeMultilineComment($comment) {
    return '<K>';
  }

  public function escapeStringForLikeClause($value) {
    return '<L>';
  }

...

So qsprintf(<IsolatedConn>, ...) really is returning literally <S> and <C> and such, and assertEqual() is doing normal check (that the strings are exactly identical).

This revision was automatically updated to reflect the committed changes.