Page MenuHomePhabricator

Database::selectSQLText should prefix 'table_name.*'
Open, LowPublic

Description

Using Database::select or any similar function which calls selectSQLText with 'table_name.*' in the $vars argument will fail if a table prefix is set. This, for instance, makes such code untestable with PHPUnit (T217487). While selecting '*' is seldom the right thing to do, we should nevertheless keep it working.

Event Timeline

Daimona created this task.Mar 6 2019, 10:27 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 6 2019, 10:27 AM

Change 494040 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] [WIP] Add table prefix when using 'table_name.*' in SELECT

https://gerrit.wikimedia.org/r/494040

Daimona claimed this task.Mar 6 2019, 10:29 AM
Krinkle triaged this task as Low priority.Jul 24 2019, 2:40 PM
Restricted Application added a project: Core Platform Team. · View Herald TranscriptJul 24 2019, 2:40 PM
Anomie moved this task from Inbox to Triage Meeting Inbox on the Core Platform Team board.EditedJul 24 2019, 4:54 PM
Anomie added a subscriber: Anomie.

The solution here is probably to either use a table alias (so the prefix doesn't matter) or to use $db->tableName( $table ) . '.*' (so it gets the prefix) rather than hard-coding the table name and expecting Database to parse it.

The solution here is probably to either use a table alias (so the prefix doesn't matter) or to use $db->tableName( $table ) . '.*' (so it gets the prefix) rather than hard-coding the table name and expecting Database to parse it.

Yeah, I get the point, but IMHO it's not clear that Database won't even try to parse it. I think a compromise could be to:

  1. Write a line somewhere in the Database docs
  2. Add a shortcut, something like:
public function allFieldsInTable( $tname ) {
  return $this->tableName( $table ) . '.*';
}

This way, people would still have to add the prefix themselves, but we'd provide a specific API for that. Which should also emphasize the need to call it.

Yeah, I get the point, but IMHO it's not clear that Database won't even try to parse it. I think a compromise could be to:

  1. Write a line somewhere in the Database docs

I'm not sure why anyone would expect that Database would try to parse it, but feel free to propose the doc change.

  1. Add a shortcut, something like:
public function allFieldsInTable( $tname ) {
  return $this->tableName( $table ) . '.*';
}

This way, people would still have to add the prefix themselves, but we'd provide a specific API for that. Which should also emphasize the need to call it.

Given the trouble that "*" queries have caused recently with the actor and comment migrations (making it harder to find code using certain database fields), I'm not sure we should encourage their use by providing such a method.

I'm not sure why anyone would expect that Database would try to parse it, but feel free to propose the doc change.

Because I don't think there's any indication that it won't try to. Or is there?

Given the trouble that "*" queries have caused recently with the actor and comment migrations (making it harder to find code using certain database fields), I'm not sure we should encourage their use by providing such a method.

Fair, but then maybe we should add some debug/info/deprecation logging?

Right now, it sounds to me that this feature lies in a sort of limbo, where we do allow it, although we implicitly discourage it, and only support it partially. Shouldn't we take a clearer position?