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.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 17, 3:38 AM
Unknown Object (File)
Thu, Apr 11, 9:06 AM
Unknown Object (File)
Wed, Apr 10, 10:05 AM
Unknown Object (File)
Sun, Mar 31, 8:37 AM
Unknown Object (File)
Sat, Mar 30, 6:36 PM
Unknown Object (File)
Mar 20 2024, 10:31 PM
Unknown Object (File)
Mar 20 2024, 10:31 PM
Unknown Object (File)
Mar 20 2024, 10:31 PM
Subscribers
None

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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!

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

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.