Trying to run:
db_merge('role_permission')
->key(array(
'rid' => 20,
'permission' => 'example permission',
))
->execute();
This causes the following SQL/error: PDOException: INSERT INTO {role_permission} (rid, permission) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1) ON DUPLICATE KEY UPDATE - Array ( [:db_insert_placeholder_0] => 20 [:db_insert_placeholder_1] => example permission ) 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 '' at line 1 in user_role_set_permissions() (line 2887 of /home/davereid/Projects/drupal-head/modules/user/user.module).
Since the role_permission table only has two fields, and they are both the primary key, there are not fields to 'update' on a duplicate key. Talked with chx in #drupal and found that this query should not use the "ON DUPLICATE KEY UPDATE ..." syntax, but instead a "INSERT IGNORE INTO ..." syntax when there are no fields to update on a duplicate key.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 374940-merge-update-keys-only-D7.patch | 5.26 KB | dave reid |
| #7 | 374940-mysql-merge-keys-only-D7.patch | 3.15 KB | dave reid |
| #5 | 374940-mysql-merge-keys-only-D7.patch | 3.13 KB | dave reid |
| #1 | 374940-mysql-merge-keys-only-D7.patch | 5.09 KB | dave reid |
Comments
Comment #1
dave reidPatch with tests for review.
Comment #3
dave reidJust ran the full test suite and was not able to duplicate the exception. I can haz a second opinion?
Comment #4
david straussI'm giving a "no" on this because IGNORE's effects extend to errors beyond duplicate keys. For example, IGNORE will cause not finding a suitable partition to issue a warning instead of an error.
Can we just remove the ON DUPLICATE KEY and trap the exception or error?
Comment #5
dave reidPDO unfortunately provide a reliable exception for a duplicate key insertion, so chx came up with an alternate solution that I've implemented. If there are no updateFields or expresssionFields, the first keyField will be added as an updateField.
Comment #7
dave reidWell, that's what I get for not trying to re-install head. This one works since it checks if insertFields is empty as well.
Comment #8
Crell commentedThis looks like it's just modifying the base class. Will that even have an effect on the MySQL or Postgres drivers since they override execute()?
Something I've run into in the handlers patch is the need to run a merge query that contains more than key fields but should still do nothing if all the key fields are present. Normally I'd use updateExcept(), but if I updateExcept() all insert/update fields so that there are no fields to set then I get a syntax error. I've worked around that for now by allowing one int field to be updated to itself, but apropos of this patch if there were a way to cleanly short-circuit the query to do nothing at that point without that workaround, it would be great. I'm not sure if that's appropriate to deal with in this patch, but I am throwing it out in case it is.
Comment #9
dave reid#7 actually only modifies the MergeQuery_mysql class so sorry it wasn't more specific in title. I tried to run the new MergeQuery tests on PostgreSQL and found the failure that you mentioned Crell, so I have fixed the genereal MergeQuery class as well so that if there are no fields to update, then don't run the update query. I've also added an extra test for a merge query with ->fields(array('age' => 31))->updateExcept(array('age')). Let's see how the test bot likes this one.
Comment #10
boombatower commentedI'm not DBTNG expert, but code looks clean and tests seem to make sense..
Comment #11
Crell commentedAnd to me, too. Thanks, Dave!
Comment #12
dries commentedLooks good to me. Committed to CVS HEAD. Thanks!