SQLite update queries using expressions (like UPDATE ... SET count = count + 1) are failing, because of the way SQLite is rewriting the queries. This patch may or may not be the correct solution, but that's for chx to figure out. I think $data['expression'] is a db-safe value, but I don't know for sue

CommentFileSizeAuthor
#18 sqlite_update_query_expression.patch2.64 KBdmitrig01
#17 sqlite_update_query_expression.patch2.64 KBdmitrig01
#16 sqlite_update_query_expression.patch2.64 KBdmitrig01
#15 sqlite_update_query_expression.patch3.39 KBdmitrig01
#14 sqlite_update_query_expression.patch3.39 KBdmitrig01
#11 sqlite_update_query_expression.patch2.07 KBdmitrig01
#10 sqlite_update_query_expression.patch2.07 KBdmitrig01
#9 sqlite_update_query_expression.patch1.67 KBdmitrig01
#8 test_only.patch944 bytesdmitrig01
#8 sqlite_update_query_expression.patch1.66 KBdmitrig01
#7 test_only.patch2.91 KBdmitrig01
#7 sqlite_update_query_expression_test.patch2.91 KBdmitrig01
#4 Patch + test2.45 KBdmitrig01
#4 Only test1.68 KBdmitrig01
#1 sqlite_update_query_expression.patch1.17 KBdmitrig01

Comments

dmitrig01’s picture

dmitrig01 commented
StatusFileSize
new sqlite_update_query_expression.patch1.17 KB
dmitrig01’s picture

dmitrig01 commented

Also, whether or not it's the right solution, it fixes two of the five failing SQLite tests.

chx’s picture

chx commented
Status: Needs review » Needs work
Issue tags: +Needs tests

While the patch is correct (I surely had a brain damaged moment when I wrote this if it was me who wrote this :) )...

Edit: yes the expression is safe, see how arguments can be passed into an expression.

dmitrig01’s picture

dmitrig01 commented
Status: Needs work » Needs review
StatusFileSize
new Only test1.68 KB
new Patch + test2.45 KB

Test will pass in MySQL but fail in SQLite w/o patch.

dmitrig01’s picture

dmitrig01 commented

The test is very ugly. Basically, I initially discovered this bug because of the fact that mergequery was using the same condition twice, with two separate queries. Here's what happens in MergeQuery:

  1. A DatabaseCondition is created
  2. A condition with a placeholder is added.
  3. The condition is compiled, with the query object #1. Query object #1 increments its internal placeholder counter, and gives the placeholder added in #1 the id of 0.
  4. The condition is added to query #2.
  5. An expression using an unnamed placeholder is added to query #2.
  6. Query #2 is run. This is where it fails: the internal counter for placeholders is still at 0, so the expression is assigned placeholder #0. Now, there are two things using placeholder #0.

So, this is an ugly bug, without an obvious solution. I think it would require an API change, and chx told me that it was already quite hard getting the placeholder incrementing correct.

HOWEVER, this also uncovered another bug (specific to SQLite), addressed by this patch: placeholders are used for expressions where the expression should just be inserted straight into SQL, and that's what this patch addresses. Thus, the test for this patch uses the fact that the above case fails to test it's own functionality: if the expression is inserted into as a placeholder, the placeholder will be a duplicate, and that will fail. If the expression is inserted as SQL, the placeholder won't be inserted, and it won't duplicate (and fail). I haven't thought of a better way to test this, as, so far as I've thought it through, it would have to look at SQL directly.

chx’s picture

chx commented

Let me say this another way.

        $update = $this->connection->update($this->table)
          ->fields($this->updateFields)
          ->condition($this->condition); // This one has db_placeholder_0, db_placeholder_1, ...
        if ($this->expressionFields) {
          foreach ($this->expressionFields as $field => $data) {
            $update->expression($field, $data['expression'], $data['arguments']); // in SQLite, this breaks because when it inverts the expression HEAD reuses db_placeholder_0 when it should not use any numbered placeholder.
dmitrig01’s picture

dmitrig01 commented
StatusFileSize
new sqlite_update_query_expression_test.patch2.91 KB
new test_only.patch2.91 KB

Damien is brilliant. 1*1 = 1, everything else is different. New tests which don't depend on the other bug. patch for that bug coming shortly

dmitrig01’s picture

dmitrig01 commented
StatusFileSize
new sqlite_update_query_expression.patch1.66 KB
new test_only.patch944 bytes

wrong patch (solves the other problem :P)

dmitrig01’s picture

dmitrig01 commented
StatusFileSize
new sqlite_update_query_expression.patch1.67 KB

Now without a literal update

dmitrig01’s picture

dmitrig01 commented
StatusFileSize
new sqlite_update_query_expression.patch2.07 KB

Now with comments:

There are 4 rows total. We update one of them to have age = 1. Then, they're squared. This should update three of them, as 1^2 doesn't change. This only actually fails in SQLite, as SQLite inverts the expression (age <> age * age) to only match rows it will change, and age <> "age * age" will match all 4 rows, even though only 3 will be updated.

dmitrig01’s picture

dmitrig01 commented
StatusFileSize
new sqlite_update_query_expression.patch2.07 KB

Now with comments:

There are 4 rows total. We update one of them to have age = 1. Then, they're squared. This should update three of them, as 1^2 doesn't change. This only actually fails in SQLite, as SQLite inverts the expression (age <> age * age) to only match rows it will change, and age <> "age * age" will match all 4 rows, even though only 3 will be updated.

dmitrig01’s picture

dmitrig01 commented
Issue tags: -Needs tests

.

dmitrig01’s picture

dmitrig01 commented

The other issue, btw, has been split out into #1004794: Adding a DatabaseCondition object to two queries fails

dmitrig01’s picture

dmitrig01 commented
StatusFileSize
new sqlite_update_query_expression.patch3.39 KB

Better comment, from Crell

dmitrig01’s picture

dmitrig01 commented
StatusFileSize
new sqlite_update_query_expression.patch3.39 KB

Better comment, from Crell

dmitrig01’s picture

dmitrig01 commented
StatusFileSize
new sqlite_update_query_expression.patch2.64 KB

ARGH i don't like managing multiple patches from a single drupal install

dmitrig01’s picture

dmitrig01 commented
StatusFileSize
new sqlite_update_query_expression.patch2.64 KB

better assertion, from damz

dmitrig01’s picture

dmitrig01 commented
StatusFileSize
new sqlite_update_query_expression.patch2.64 KB

gah

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community

I like it this way. And I like progress toward making SQLite 100% pass.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Since #18 is just #8 with comments (which somehow took 11,000 follow-ups to get right ;)), committed to HEAD.

Yay SQLite passing!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.