From: Patrick Hayes <pfhayes@gmail.com>
Date: Sat, 13 Jun 2015 18:11:16 +0000 (-0400)
Subject: Fix primary key behaviour in bulk_update
X-Git-Tag: rel_1_0_6~25
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=09485d733131b667813f44eb0b6807b698668ee7;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

Fix primary key behaviour in bulk_update

Suppose you have a model class with a primary key.

Base = declarative_base()
class User(Base):
  id = Column(BigInteger, primary_key=True)
  name = Column(String)

Previously, running
`bulk_update_mappings(User, {'id': 1, 'name': 'hello'})`
would emit the following:

```UPDATE users SET id=1, name='hello' WHERE id=1```

This is contrary to the stated behaviour, where primary keys are omitted
from the SET clause. Furthermore, this behaviour is harmful, as it
can cause the db engine to lock over-aggresively (at least in Postgres).

With this change, the emitted SQL is:

```UPDATE users SET name='hello' WHERE id=1```
---

diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py
index b429aa4c17..254d3bf09a 100644
--- a/lib/sqlalchemy/orm/persistence.py
+++ b/lib/sqlalchemy/orm/persistence.py
@@ -472,12 +472,11 @@ def _collect_update_commands(
             continue
 
         if bulk:
-            pk_params = dict(
-                (propkey_to_col[propkey]._label, state_dict.get(propkey))
-                for propkey in
-                set(propkey_to_col).
-                intersection(mapper._pk_keys_by_table[table])
-            )
+            pk_params = {}
+            for propkey in set(propkey_to_col).intersection(mapper._pk_keys_by_table[table]):
+              col = propkey_to_col[propkey]
+              pk_params[col._label] = state_dict.get(propkey)
+              params.pop(col.key, None)
         else:
             pk_params = {}
             for col in pks:
diff --git a/test/orm/test_bulk.py b/test/orm/test_bulk.py
index e27d3b73c8..1e0a735c76 100644
--- a/test/orm/test_bulk.py
+++ b/test/orm/test_bulk.py
@@ -96,11 +96,41 @@ class BulkInsertUpdateTest(BulkTest, _fixtures.FixtureTest):
 
         asserter.assert_(
             CompiledSQL(
-                "UPDATE users SET id=:id, name=:name WHERE "
+                "UPDATE users SET name=:name WHERE "
                 "users.id = :users_id",
-                [{'users_id': 1, 'id': 1, 'name': 'u1new'},
-                 {'users_id': 2, 'id': 2, 'name': 'u2'},
-                 {'users_id': 3, 'id': 3, 'name': 'u3new'}]
+                [{'users_id': 1, 'name': 'u1new'},
+                 {'users_id': 2, 'name': 'u2'},
+                 {'users_id': 3, 'name': 'u3new'}]
+            )
+        )
+
+    def test_bulk_update(self):
+        User, = self.classes("User",)
+
+        s = Session(expire_on_commit=False)
+        objects = [
+            User(name="u1"),
+            User(name="u2"),
+            User(name="u3")
+        ]
+        s.add_all(objects)
+        s.commit()
+
+        s = Session()
+        with self.sql_execution_asserter() as asserter:
+            s.bulk_update_mappings(
+                User,
+                [{'id': 1, 'name': 'u1new'},
+                 {'id': 2, 'name': 'u2'},
+                 {'id': 3, 'name': 'u3new'}]
+            )
+
+        asserter.assert_(
+            CompiledSQL(
+                "UPDATE users SET name=:name WHERE users.id = :users_id",
+                [{'users_id': 1, 'name': 'u1new'},
+                 {'users_id': 2, 'name': 'u2'},
+                 {'users_id': 3, 'name': 'u3new'}]
             )
         )