Oh... I forgot one: 5) Have tableName test for words like ON, JOIN, etc... which signal this is a sql query, not a table.
~Daniel Friesen(Dantman) of: -The Gaiapedia (http://gaia.wikia.com) -Wikia ACG on Wikia.com (http://wikia.com/wiki/Wikia_ACG) -and Wiki-Tools.com (http://wiki-tools.com)
DanTMan wrote:
Ugh... this looks more like bad usage of Database functions on the API's part rather than a breakage in the Database functions. When I rewrote tableName talking to brion for feedback on what various inputs should be defined as, we cleaned up and strictly defined what inputting something into tableName meant:
- Strings in the form 'table' were loose input which were expected to
have a shared DB assigned if they were shared, prefixed with the right database, and quoted.
- Strings in the form '`table`' or '`table.with.dots`' were literal,
when one is seen with quotes on both sides, it should be left alone.
- Strings in the form 'database.table', '`database`.table',
'database.`table`' are a database.table pair, no prefixes should be added, and the shared database should not override it. These should only be quoted.
But it looks like tableName is getting input it shouldn't be. The API is basically inputting: "`database`.`table` dbt ON dbt.column=value" Which, honestly is not a table. That's a full blown SQL JOIN query.
This is a result of the chain of calls:
- ApiQueryUsers.php calls:
$this->addTables($tables); With the input: "$userTable AS u1 LEFT JOIN $ug ON ug_user=u1.user_id LEFT JOIN $ipb ON ipb_user=u1.user_id LEFT JOIN $userTable AS u2 ON ipb_by=u2.user_id"; 2) ApiQueryBase.php's addTables adds those to $this->tables; 3) ApiQueryUsers.php calls: $this->select(__METHOD__); 3) ApiQueryBase.php's select calls: $db->select($this->tables, $this->fields, $this->where, $method, $this->options); 4) Database's select takes $this->tables and calls tableName for each one: 5) tableName receives the input "`user` AS u1 LEFT JOIN `user_groups` ON ug_user=u1.user_id LEFT JOIN `ipblocks` ON ipb_user=u1.user_id LEFT JOIN `user` AS u2 ON ipb_by=u2.user_id" 6) Because there is a ` at the start, but no ` at the end, and tableName is only supposed to handle table names it splits into "`user` AS u1 LEFT JOIN `user_groups` ON ug_user=u1.user_id LEFT JOIN `ipblocks` ON ipb_user=u1" and "user_id LEFT JOIN `ipblocks` ON ipb_user=u1.user_id LEFT JOIN `user` AS u2 ON ipb_by=u2.user_id" and quotes the second one because there is no ` at the start.
Additionally if you haven't caught it ApiQueryBase::addTables also calls $tables = $this->getDB()->tableName($tables) . ' ' . $alias; just a heads up, but that $alias normally has no `'s to quote it, right now it should probably check if it should quote the $alias just so that doesn't end up as an error while we sort out what to do to make tableName handle input it's not supposed to handle correctly.
We've got multiple options on how to fix this:
- Find a better way to make complex queries using the Database class
that doesn't involve building complex queries, and then trying to shove them into the simple handlers. This would greatly improve cross-db compatibility if done. 2) Have the API properly quote things it sends to simple database methods (using clean functions we could put into the Database class, such as tableQuote). This should be fine for working, as the MySQL handler is the only one which tests for ` the other DB tableName's don't do this. So with tableQuote always outputting the correct type of quoting it shouldn't break in other databases. That improves cross-db compatibility, but not as good as (1). 3) Don't use the Simple Database handlers. The API is already constructing near full SQL queries, calling simple DB methods to build the last portion, and expecting them to handle complex data without messing it up is kludge work and is bound to fail over and over. The select call inside of ApiQueryBase is only a light step away from simply dropping the data it has into a sql query and sending that directly to Query. Database::select is actually completely database generic, none of the other classes override it. It uses other functions inside of the Database class to handle the portions of data which are different with different database platforms. Api's select could easily call those same database methods to handle it's data in a cross-database compatible way then send the finalized data to query. 4) We could use some sort of code character, like the parser's \x07 which is never supposed to show up anywhere and have either tableName or select test for it and consider it as a signal of "I've fully handled all database stuff inside this string, don't touch it one bit, or else." And have query, select, or tableName strip it out when done with it.
4 is the quickest and easiest to do. Though of course, it's still a hack.... Then again, the Parser is a pretty big one to. 1 is fairly complex to do, but it would aid anyone doing joins and such and provide a new simple method to abstract the complex stuff. 2 may take some handling around the Api, though I'm not for it that much because of the reasons in 3 for not using simple functions to handle complex data. IMHO 3 is the best method for both cross-db compatibility and simplicity.
Input?
~Daniel Friesen(Dantman) of: -The Gaiapedia (http://gaia.wikia.com) -Wikia ACG on Wikia.com (http://wikia.com/wiki/Wikia_ACG) -and Wiki-Tools.com (http://wiki-tools.com)
Roan Kattouw wrote:
Revision 34353 [1] (by you) caused some breakage in Database::select(), which I worked around in in r34388 [2], a workaround Greg didn't like (see wikitech-l). Could you look into this piece of code again and see if you can get it to behave properly for complex things?
Roan Kattouw (Catrope)
[1] http://svn.wikimedia.org/viewvc/mediawiki?view=rev&revision=34353 [2] http://svn.wikimedia.org/viewvc/mediawiki?view=rev&revision=34388
Roan Kattouw wrote:
Greg Sabino Mullane schreef:
Revision: 34388 `ug1`.`ug_user`=`user_id`";
Ugh. Please don't hardcode backticks: not only are they unneeded here, but they horribly break any chance we have of cross-database compatibility.
They're far from unneeded. Database::select() is broken in that it adds the backticks wrongly, generating invalid SQL [1]. I'll poke dantman to fix this, but in the meantime we should have a working API.
Roan Kattouw (Catrope)
[1] http://www.mediawiki.org/wiki/User_talk:Catrope#API_probleem
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l