Posted by Damien Tournoud on November 9, 2008 at 6:21am
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | database system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Crell |
| Status: | closed (fixed) |
Issue Summary
The aggregator module has a query like this one:
<?php
db_merge('aggregator_category_feed')
->fields(array(
'fid' => $edit['fid'],
'cid' => $cid,
))
->execute();
?>This is very wrong: that query has no "key" fields, so it tells you: "if any field exist in the table, replace with the fields I indicate".
We should make sure MergeQuery (and descendant) throws a PDOException in that case.
Comments
#1
We should probably handle this the same way we handle it for inserts: #321100: Empty insert statements should fail gracefully
#2
I vote for "throw an exception at the face of the caller".
#3
So after sleeping on it, I think there are no good cases where a merge query should be missing key fields, so that should throw an exception, but there is a valid use case for no-op insert statements, so those should be handled gracefully.
- The exception text should end in a period.
- Let's go ahead and make an InvalidMergeQueryException class that we can throw. It doesn't have to have any body, just extend off of PDOException the same way TransactionsNotSupportedException does. That way we can catch exceptions by type if we need to for better OOPness and flexibility.
- Can we have a unit test here to confirm that the exception is thrown?
#4
Follow-up: After a brief chat with webchick, DO make a new exception class but extend it from Exception, NOT PDOException. The error here isn't related to PDO itself but to our database layer, so it shouldn't extend PDOException. We can fix the transaction exception class later, or go ahead and do it here if you don't mind mildly singeing a kitten. :-)
#5
OK, so let's try this again. This does what is described in #3 and #4, and includes a new unit test accordingly.
#6
This patch looks good to me. I assume the queries in statistics.module and aggregator.module result in two failures when running the tests without the fixes? If that is not the case, we should probably consider adding a couple more tests.
#7
Possibly, but I don't know those modules well enough to write good tests for them.
#8
This patch needs a quick re-roll because of the aggregation changes.
#9
Looks like the missing key() call in aggregator isn't an issue anymore, so this is the same patch but without that block (I think :-) ).
#10
I'd rather have the merge queries fall back on the primary key if no key is specified. Still, we'd need an exception if you neither specify values for the primary key columns nor the key columns you want to use.
#11
They can't fall back on the primary key, as then they still have no value for the primary key column. The resulting query would still be invalid.
#12
@Crell I said, "Still, we'd need an exception if you neither specify values for the primary key columns nor the key columns you want to use." You would always have to specify the values for the key. I'm just asking that you not always have to specify the key columns themselves because most of my merge-style queries just use the PK column(s).
#13
That's true, but then we'd have to modify key() to accept a primitive as well as an array (remember, the key could be multi-column) and add a schema lookup for the primary key. I'd prefer to keep the API and implementation simple, and especially avoid hitting the schema if we don't need to.
#14
@Crell That makes sense. In any case, requiring key specification now does not tie our hands later.
#15
Looks good, all tests pass. Committed to CVS HEAD. Thanks for the tests!
#16
Automatically closed -- issue fixed for two weeks with no activity.