Issue: Correct format for PostgreSQL UPDATE statement with FROM clause.
Author: nilp0inter
State: closed
Created: 2018-11-03 12:38:32

Hi,

I tried to use **UPDATE ... FROM** clause using peewee+PostgreSQL without luck. After seeing #1321  I made this patch to fix it.

I'll explain the problem and my solution.


**The Problem**

Consider the following model:

```python
class Race(Model):
    name = CharField()
    gold_bonus = IntegerField()

    class Meta:
        database = db


class Player(Model):
    name = CharField()
    race = ForeignKeyField(Race, backref='players')
    gold = IntegerField(default=0)

    class Meta:
        database = db

if __name__ == '__main__':
    db.connect()
    db.create_tables([Race, Player])

    r1=Race.create(name='Human', gold_bonus=3)
    r2=Race.create(name='Elf', gold_bonus=5)
    p1=Player.create(name='player1', race=r1, gold=0)
    p2=Player.create(name='player2', race=r2, gold=0)
```

I need to perform a query to increment all players *gold* value by the amount of *gold_bonus* of their race plus one.

So I tried:
```python
>>> Player.update(gold=Race.gold_bonus + Player.gold + 1).from_(Race).where(Player.race==Race.id).execute()
Traceback (most recent call last):
  File "/home/nil/.local/share/virtualenvs/miopia-UO75lLMN/lib/python3.6/site-packages/peewee.py", line 2683, in execute_sql
    cursor.execute(sql, params or ())
psycopg2.ProgrammingError: column reference "id" is ambiguous
LINE 1: ... + "gold") + 1) FROM "race" AS "t1" WHERE ("race_id" = "id")
                                                                  ^


During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/nil/.local/share/virtualenvs/miopia-UO75lLMN/lib/python3.6/site-packages/peewee.py", line 1604, in inner
    return method(self, database, *args, **kwargs)
  File "/home/nil/.local/share/virtualenvs/miopia-UO75lLMN/lib/python3.6/site-packages/peewee.py", line 1675, in execute
    return self._execute(database)
  File "/home/nil/.local/share/virtualenvs/miopia-UO75lLMN/lib/python3.6/site-packages/peewee.py", line 2097, in _execute
    cursor = database.execute(self)
  File "/home/nil/.local/share/virtualenvs/miopia-UO75lLMN/lib/python3.6/site-packages/peewee.py", line 2696, in execute
    return self.execute_sql(sql, params, commit=commit)
  File "/home/nil/.local/share/virtualenvs/miopia-UO75lLMN/lib/python3.6/site-packages/peewee.py", line 2690, in execute_sql
    self.commit()
  File "/home/nil/.local/share/virtualenvs/miopia-UO75lLMN/lib/python3.6/site-packages/peewee.py", line 2481, in __exit__
    reraise(new_type, new_type(*exc_args), traceback)
  File "/home/nil/.local/share/virtualenvs/miopia-UO75lLMN/lib/python3.6/site-packages/peewee.py", line 178, in reraise
    raise value.with_traceback(tb)
  File "/home/nil/.local/share/virtualenvs/miopia-UO75lLMN/lib/python3.6/site-packages/peewee.py", line 2683, in execute_sql
    cursor.execute(sql, params or ())
peewee.ProgrammingError: column reference "id" is ambiguous
LINE 1: ... + "gold") + 1) FROM "race" AS "t1" WHERE ("race_id" = "id")
                                                                  ^
```

The problem is with the generated query:

```sql
UPDATE
    "player"
SET
    "gold" = (("gold_bonus" + "gold") + 1)
FROM
    "race" AS "t1"
WHERE 
    ("race_id" = "id")
```

As you can see, the term **id** is ambiguous as it appears in both *player* and *race* tables.

A correct version of the query should look something like:

```sql
UPDATE
    "player"
SET
    "gold" = (("t1"."gold_bonus" + "player"."gold") + 1)
FROM
    "race" AS "t1"
WHERE
    ("player"."race_id" = "t1"."id")
```

Note in the *SET* portion of the query, the left hand side of the assignment is not fully-qualified (because you can only assign to fields in the *UPDATE* table) while the right hand side of the assignment is fully-qualified.


**The Patch**

1. I modified the for loop in the \_\_sql\_\_ method of the Update class to use *QualifiedNames()* just for the right hand side of the assignments.
2. In the same method, I modified the part where the **WHERE** is added surrounding it with *ctx.scope_normal*. I don't know if I am using the correct scope, but it yields the desired behavior.
3. I rewrote the part of the tests where static *UPDATE* queries are checked against the implementation, using the new rules for the SET and WHERE part.

I run the test suit against:

- Sqlite: No errors
- Postgres: Some warnings because my installation lacks hstore.
- MySQL: Some errors and failed tests but appears unrelated to this patch.

Please let me know what you think about the patch or any changes/improvements you want me to include.

Cheers!
 

The build is passing, which builds against python 2.x/3.x across postgres/mysql/sqlite, so that's fantastic.

I'm happy to merge this patch and will probably add some regression tests in `tests/regressions.py`. I'm not sure if there are any weird, unintended effects that will crop up from wrapping the WHERE clause in a `scope_normal()` but I hope to clarify that with some additional regression/integration tests.

Found one regression, where the right-hand-side of the assignment is a subquery. e.g. to append a user's tweet count to their username:

```python
subq = Tweet.select(fn.COUNT(Tweet.id)).where(Tweet.user == User.id)
query = User.update(username=User.username + '-' + subq)
```

I have reverted this patch, as it contains a pretty serious regression (which unfortunately was not covered by a test). You can see the test-cases that illustrate the issue:

bc4f04886253d81219a5b2e3a7af48bdce22160a

If you can find a workaround I'd be happy to merge.

Alternatively, you can always wrap your own values in `QualifiedNames` before using them on the right-hand-side of the UPDATE?

I've merged a patch that implements half of what this patch changed: the "normal scope" for the WHERE clause. I've also extended that to apply to the WHERE clause of DELETE queries. However, the QualifiedNames change is still absent due to problems described in my previous comment.

Nice catch! I'll give it a go tomorrow using the new regression tests.

Also, I am not sure, but I think the RETURNING clause would need `QualifiedNames` too.

Thank you!

I'm pretty sure you cannot use qualified names in the returning clause, but feel free to try it out.