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
Branch
query2
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 21055
Build 28610: Run Core Tests
Build 28609: arc lint + arc unit

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.