Make it easier to add multiple fields in a dynamic SELECT
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | database system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Crell |
| Status: | closed |
webchick was giving me a hard time for not updating the hook_file patch to DBTNG so I took a swing at it. Most of it was easy enough but the bit that stumped me was file_load()'s dynamic query based off of node_load(). The problem I ran into is that it doesn't seem like there's an easy way to have a SELECT * query with a dynamic WHERE clause.
The code I'm working to replace looks roughly like this:
<?php
$cond = array();
$arguments = array();
foreach ($param as $key => $value) {
$cond[] = 'f.' . db_escape_table($key) . " = '%s'";
$arguments[] = $value;
}
$result = db_query('SELECT f.* FROM {files} f WHERE ' . implode(' AND ', $cond), $arguments);
$file = $result->fetch(PDO::FETCH_OBJ);
?>My first effort at converting this looked like:
<?php
$query = db_select('files', 'f');
$query->addField('f', '*');
foreach ($param as $key => $value) {
$query->condition($key, $value);
}
$result = $query->execute();
$file = $result->fetch(PDO::FETCH_OBJ);
?>Fails since you can't alias a *:
Uncaught exception 'PDOException' with message 'SELECT f.* AS f_* FROM {files} AS f WHERE (f.fid = :db_condition_placeholder_1) - Array ( [:db_condition_placeholder_1] => 1 ) SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '* FROM files AS f WHERE (f.fid = '1')' at line 1' in /Users/amorton/Sites/dh/includes/database/database.inc:365I tried omitting
SelectQuery::addField() with no luck and substituting SelectQuery::addExpression() but it seems to do the same alias generation.
I'm expecting some push back that I ought to just specify all the fields but after you try it on a small table you'll see that it's both tedious and repetitive:
<?php
$query = db_select('files', 'f');
$query->addField('f', 'fid', 'fid');
$query->addField('f', 'filename', 'filename');
$query->addField('f', 'filename', 'filename');
$query->addField('f', 'filepath', 'filepath');
$query->addField('f', 'filemime', 'filemime');
$query->addField('f', 'filesize', 'filesize');
$query->addField('f', 'status', 'status');
$query->addField('f', 'timestamp', 'timestamp');
foreach ($param as $key => $value) {
$query->condition('f.' . $key, $value);
}
$result = $query->execute();
$file = $result->fetch(PDO::FETCH_OBJ);
?>I'm not being intentionally verbose either, you can't even chain the
SelectQuery::addField() calls because they return the field's alias rather than the query. And if you don't manually specify an alias you end up with a class full of f_* members.
Another options is to use the SchemaAPI to save some typing:
We could use the schema API to get the fields:
<?php
$query = db_select('files', 'f');
foreach (drupal_schema_fields_sql('files') as $field) {
$query->addField('f', $field, $field);
}
foreach ($param as $key => $value) {
$query->condition('f.' . $key, $value);
}
$result = $query->execute();
$file = $result->fetch(PDO::FETCH_OBJ);
?>This is better but still way more typing that you ought to have to do for a simple query.
As part of the whole DX initiative we should make it easy to build dynamic queries without resorting to manually building a dynamic query without either falling back to db_query() or requiring the developer to type out all the field names.
I see two obvious solutions:
- Add a function that adds in the * for a given, e.g.
tableSelectQuery::addAllFields($table_alias) - If fields have been specified when the query is finally executed then just assume they wanted table_alias.* for each table.
The attached patch implements the later approach in a very simplistic manner. It may be preferable to to use the schema to explicitly list out the fields.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| db_select_star.patch | 759 bytes | Ignored | None | None |
#1
This started out as a support request but then the patch came about.
#2
You can use
addExpression('*'). But the true answer is that you should specify the column names one by one: even if it's "tedious and repetitive", it makes code more readable and less error-prone.Also, note there is a feature request that would make this a little less repetitive (#316868: Default SelectQuery::addField() alias to $field, not $table_$field).
#3
I like this: tableSelectQuery::addAllFields($table_alias). Internally, the method would do roughly as drewish suggests
<?phpforeach (drupal_schema_fields_sql('files') as $field) {
$query->addField('f', $field, $field);
}
?>
I agree that there is a DX issue here.
#4
#3 would get the cleaner SQL, but would still not be at all self-documenting. I can't tell from the code what fields I'm going to end up with. I also want to minimize schema usage wherever possible for performance reasons.
#5
On IRC Crell indicated that $query->addExpression('*'); should work but that results in:
Fatal error: Uncaught exception 'PDOException' with message 'SELECT * AS expression FROM {files} AS f WHERE (status = :db_condition_placeholder_1) - Array ( [:db_condition_placeholder_1] => 1 ) SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'AS expression FROM files AS f WHERE (status = '1')' at line 1' in /Users/amorton/Sites/dh/includes/database/database.inc:365 [...]Since he indicated that this is a bug I'll change the focus of this issue.
#6
As discussed in IRC, IMO the solution is not to make SELECT * work but to make it easier to add multiple fields in one shot. SELECT * is bad for SQL performance, with addFields() or similar.
#7
@Crell - your critique in #4 does not make much sense to me. You don't know what fields you will get back in exactly the way with SELECT *. Thats a feature, not a bug. Sometimes you do not care. I agree that listing the fields is preferable to SELECT *, and the call to schema gets you that. Note that schema is in static cache so only the first call costs anything. If that first call is still bothersome, consider allowing SELECT * :)
I think this is just one of those conveniences like fetchAllAsssoc that you use when you need it and when you know what the tradeoffs are.
#8
SELECT * is the Enemy of Decent Code. This is a documentation problem (ie I need to google together the articles on this) and not a code one. That said, I think menu has a SELECT * but that's rather easy to fix.
#9
i did a quick check:
amorton@paycheck:~/Sites/dh% grep -i "select \*" * */* */*/* */*/*/* */*/*/*/* | wc -l98
and it looks like there's plenty of instances in core where we're doing select *. so feel free to roll the mega patch to kill it but if you're going to end up adding a ton of code for what i'd guess is a very minor performance gain. the downside is that there will be a lot more code to keep synched up when you add/remove/rename a field.
#10
How many of those would actually require a dynamic query? :-)
#11
crell, !@#$%#$%^$%^& if SELECT * is such a threat to all that is good and holy then you should be leading the charge to fix those queries
toso that they specify all the fields... if it's not then DBTNG should let you run those queries.#12
Hey, I'm leading enough charges as is. :-)
However, attached patch adds an addFields() method, as discussed in IRC, that makes it much easier to queue up multiple fields from the same table at the same time. (Vis, it's one line.) It returns an array of aliases. (We probably do want to get #316868: Default SelectQuery::addField() alias to $field, not $table_$field in soon.)
If we're going to add "SELECT *" support to the dynamic query builder, my recommendation would be to allow NULL as the second parameter to addFields() which would result in an expression of "tablename.*", with no alias, and then no alias returned. (I'm sure I can come up with some way to make that work.) I defer to D&A on that one.
#13
Could we consolidate addFields method and addField method? Seems like the one method could just behave differently if an array is passed. Seems like a gotcha to have both.
#14
@13 something like the folllowing?
<?phppublic function addField($table_alias, $field, $alias = NULL) {
if (is_array($field)) {
foreach ($field as $a_field) {
$this->addField($table_alias, $_afield);
}
}
....
?>
#15
I'm wary about consolidating too many things into the same method, especially when they will then be returning different things. (A string alias, an array of aliases, or nothing if we're doing a SELECT *.) Most of the rest of the DB API is atomic methods, at least on the return side.
#16
Per discussion with webchick in IRC tonight, the following is the game plan (patch still to come):
- addField() remains as it is now.
- we add addFields(), which takes either addFields($table, $array_of_fields), which returns the query so it's chainable or addFields($table), which turns into $table.*. In this case, if you need to know the alias that was used you have to use getFields() to access the data structure directly and figure it out yourself.
- The aliasing logic will remain the same, modulo #316868: Default SelectQuery::addField() alias to $field, not $table_$field which we agreed was a good thing.
Revised patch for the changes to point 2 forthcoming.
#17
Here we are again, as described in #16 with one change. Since we're now dealing with multiple fields, I went ahead and called the method fields() instead of addFields(). That also makes it not quite so annoyingly similar to addField(). A unit test with a fully chained query is also included. (Note that join() still cannot be chained, but we'll deal with that later.)
Note that this patch is dependent on #316868: Default SelectQuery::addField() alias to $field, not $table_$field, since you only get the default alias back. Since we know that one is going in I didn't bother to write the test for the old aliases. That means the unit test will not pass unless you apply the other patch first, or that gets committed. Committers, commit it, please! :-)
#18
Oops. I forgot about SELECT *. :-)
Attached patch supports ->fields($tablename) to convert to SELECT $tablename.*. It does so by flagging the $tables array that the query should select everything from that table. That keeps the changes minimal and also keeps it exposed to alter hooks.
Of note, we are now iterating $tables twice in __toString(). While we could optimize that into a single loop, it would make the code less compact. It would also conflict with #299178: Add support for subqueries in FROM and JOIN clauses in dynamic query, which also mucks with that loop. For now let's keep the separate loops. Once all relevant patches are in we can decide if it's worth merging them in a follow-up.
The SELECT * unit test is *not* dependent on the field aliasing patch, as there is no aliasing at all for SELECT *. The one for explicit fields will still fail without the aliasing patch.
#19
Are these test results right: http://testing.drupal.org/pifr/file/1/3 ?
8 fails, 8 exceptions with Select tests.
Checking testing.drupal.org.
#20
That's correct, due to the fact that testing.drupal.org doesn't have the other dependent patch applied to it yet. :-) See comments in #16, 17, and 18.
#21
Thanks.
#22
The other patch just landed, so this one should pass all tests now. Can someone RTBC this so we can get back to converting core? :-)
#23
humm... it looks good but for some reason i can't run the tests with or without this patch... i'm getting:
# Notice: Undefined index: test_id in _simpletest_batch_finished() (line 403 of /Users/amorton/Sites/dh/modules/simpletest/simpletest.module).# The tests did not successfully finish
i'll try to figure that out then come back to this.
#24
I suspect it's your system. I just did a cvs update, applied this patch, and ran the entire suite of DB tests and everything passed perfectly. (Modulo a little offset.)
#25
Code looks good. Confirmed that the tests pass. RTBC.
This is one of the prerequisites for progressing with #299176: Replace db_rewrite_sql() with hook_query_alter()..
#26
OK looked this over, and apart from some double spacing in the comments which I already gave him crap about (and core is too inconsistent to rule one way or the other -- must make a crusade for that ;)), this looks good. I noticed that testSimpleSelectConditional() could use some minor love now after recent DBTNG commits, but Crell opted to handle this in a separate issue.
Committed to HEAD. Thanks! Marking CNW until the docs are updated.
#27
Docs have been updated: http://drupal.org/node/310075
And there was much rejoicing!
#28
Automatically closed -- issue fixed for two weeks with no activity.