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:
1) 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:
1) 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(a)lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l